Add Schedule Status sensor to Lyric integration - #177067
Conversation
Resideo's device response includes scheduleStatus (e.g. "Resume"), which was parsed but never surfaced. Adds it as a diagnostic sensor in DEVICE_SENSORS alongside the existing setpoint_status sensor.
home-assistant
Bot
added
cla-signed
integration: lyric
new-feature
small-pr
|
Hey there @timmo001, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
There was a problem hiding this comment.
Pull request overview
Adds a diagnostic Schedule Status sensor to the Honeywell Lyric integration.
Changes:
- Exposes
schedule_statusfromaiolyric. - Adds the sensor’s translated display name.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
homeassistant/components/lyric/sensor.py |
Defines the diagnostic sensor entity. |
homeassistant/components/lyric/strings.json |
Adds its display name. |
| "schedule_status": { | ||
| "name": "Schedule status" | ||
| }, |
There was a problem hiding this comment.
Link to documentation pull request: home-assistant/home-assistant.io#46986
| value_fn=lambda device: device.schedule_status, | ||
| suitable_fn=lambda device: device.schedule_status, |
There was a problem hiding this comment.
Added — test_schedule_status_sensor_end_to_end builds a real LyricDevice from a live-shaped payload (not a pre-set mock) and runs it through async_setup_entry, asserting the entity's unique_id, diagnostic entity_category, and native_value all come out correctly. scheduleStatus has no known aiolyric field-name mismatch (unlike a couple of the other new entities), so this passes cleanly against the currently-pinned release.
Builds a real LyricDevice from a live-shaped payload (not a pre-set mock) and runs it through async_setup_entry, verifying the entity is created with the correct unique_id, diagnostic entity_category, and native_value. scheduleStatus has no known aiolyric field-name mismatch, so this passes against the currently-pinned release. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
| side_effect=lambda entities: added.append(list(entities)) | ||
| ) | ||
|
|
||
| await async_setup_entry(MagicMock(), entry, async_add_entities) |
There was a problem hiding this comment.
This test already exercises the actual aiolyric parsing boundary — it builds a real LyricDevice from a live-shaped payload rather than a mock with pre-set properties, so it does catch the exact class of bug this session found repeatedly (field-name mismatches between the library and the live API).
Going further to a full MockConfigEntry + hass fixture setup (checking hass.states.get(...) post-setup) would be more thorough, but it's a bigger scope than this PR — there's no existing precedent for that pattern anywhere in this integration's test suite today (the closest existing test stops at ConfigEntryNotReady, before any entity is created), and building that infrastructure (mocking the OAuth session, coordinator data fetch, etc.) would benefit the whole platform, not just this one sensor. I'd rather propose that as a dedicated follow-up covering lyric's test suite broadly than bolt a one-off version of it onto a single-sensor PR.
| """Schedule Status is created as a diagnostic sensor with the real value. | ||
|
|
||
| Built from a real LyricDevice parsed from a live-shaped payload (not a | ||
| pre-set mock), exercising the actual aiolyric parsing boundary through | ||
| to the entity's native_value. scheduleStatus has no known aiolyric | ||
| field-name mismatch, so this is expected to pass today. | ||
| """ |
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
tests/components/lyric/test_sensor.py:60
- Exercise this sensor through Home Assistant's config-entry setup instead of calling the platform setup directly with
MagicMockinternals. This test never registers the entity with Home Assistant, so it cannot verify the translation/entity setup requested by the prior review and can pass even when platform forwarding or registration is broken.
await async_setup_entry(MagicMock(), entry, async_add_entities)
CI enforces this as a hard rule (pylint plugin check home-assistant-tests-direct-platform-async-setup-entry, W7420): a platform's async_setup_entry must not be called directly in tests; use hass.config_entries.async_setup(entry.entry_id) instead. Adds the same conftest.py fixtures as the binary_sensor PR (real authenticated config entry via application_credentials, HTTP-level mocks for /locations and /priority using the actual live payload shape). scheduleStatus comes from /locations, which has no known aiolyric field-name mismatch, so this passes for real against the currently-pinned release - no xfail needed for this one. Note: could not run this against a real pytest+hass fixture harness locally (Windows/fcntl limitation) - built against this repo's own proven test_config_flow.py pattern, but needs a real pytest run to confirm. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (4)
tests/components/lyric/conftest.py:34
- Replace this branch-history note with the stable LCC constraint; references to another unmerged PR and the branch base will become stale as those branches change.
# Deliberately "LCC-"-prefixed: this branch is intentionally cut from a
# clean base without the coordinator fix from home-assistant/core#177022
# (which removes a device-ID-prefix heuristic gating the /priority fetch).
# Using a non-"LCC-" ID here would make room-level entities fail to be
# created for that unrelated, separately-tracked reason, muddying what
# these tests are actually checking.
tests/components/lyric/conftest.py:110
- Remove this comment because it restates the fixture setup and another test's assertion rather than documenting a non-obvious constraint.
# Matches the credential name registered by setup_credentials via
# async_import_client_credential(..., "cred") - confirmed against
# test_config_flow.py's test_full_flow, which asserts a real
# completed flow ends up with auth_implementation == "cred", not
# DOMAIN.
tests/components/lyric/conftest.py:129
- Condense this fixture docstring because the transport-level implementation details are evident from the calls below and can become stale.
"""Mock the /locations and /priority HTTP endpoints with real-shaped data.
Registered at the aiohttp transport level, so the real aiolyric client
and coordinator code runs unmodified - only the network call itself is
faked, using the actual field names Resideo returns.
"""
tests/components/lyric/test_sensor.py:44
- Keep this test docstring focused on behavior; endpoint provenance and unrelated library mismatches duplicate PR context and can become stale.
"""Test the schedule status diagnostic sensor via a real config entry setup.
scheduleStatus comes from the base /locations response (not the
/priority endpoint), so it has no known aiolyric field-name mismatch
and passes for real against the currently-pinned release.
"""
| # Real payload shapes captured from a live T9-T10 account with paired | ||
| # RCHTSENSOR room accessories. Deliberately using the actual field names | ||
| # Resideo returns (e.g. vacationHold.Enabled, accessories[].sensorType), | ||
| # not the ones aiolyric happens to read, so tests exercise real parsing | ||
| # rather than hiding behind pre-parsed mocks. |
| setup_credentials: None, | ||
| mock_lyric_api: AiohttpClientMocker, |
| await async_import_client_credential( | ||
| hass, DOMAIN, ClientCredential(CLIENT_ID, CLIENT_SECRET), "cred" | ||
| ) | ||
|
|
||
|
|
||
| @pytest.fixture | ||
| def mock_config_entry() -> MockConfigEntry: | ||
| """Return an already-authenticated Lyric config entry.""" | ||
| return MockConfigEntry( | ||
| domain=DOMAIN, | ||
| data={ | ||
| # Matches the credential name registered by setup_credentials via | ||
| # async_import_client_credential(..., "cred") - confirmed against | ||
| # test_config_flow.py's test_full_flow, which asserts a real | ||
| # completed flow ends up with auth_implementation == "cred", not | ||
| # DOMAIN. |
There was a problem hiding this comment.
why not DOMAIN? that's the default
There was a problem hiding this comment.
Good catch — no reason for this one to deviate. Dropped the custom "cred" name; the fixture now registers the credential under DOMAIN like the rest of the OAuth2 integrations' conftests do, and the config entry's auth_implementation matches.
| await async_setup_component(hass, DOMAIN, {}) | ||
| await hass.async_block_till_done() | ||
|
|
There was a problem hiding this comment.
no need to set up the component directly though
There was a problem hiding this comment.
Right, that was roundabout — it only worked because application_credentials is a manifest dependency of lyric, so setting up lyric pulled it in as a side effect. Switched to setting up application_credentials directly, matching the pattern in other integrations' conftests.
| @pytest.fixture | ||
| def mock_lyric_api(aioclient_mock: AiohttpClientMocker) -> AiohttpClientMocker: | ||
| """Mock the /locations and /priority HTTP endpoints with real-shaped data. | ||
|
|
||
| Registered at the aiohttp transport level, so the real aiolyric client | ||
| and coordinator code runs unmodified - only the network call itself is | ||
| faked, using the actual field names Resideo returns. | ||
| """ | ||
| aioclient_mock.get( | ||
| f"{BASE_URL}/locations?apikey={CLIENT_ID}", | ||
| json=LOCATIONS_RESPONSE, | ||
| ) | ||
| aioclient_mock.get( | ||
| f"{BASE_URL}/devices/thermostats/{DEVICE_ID}/priority" | ||
| f"?apikey={CLIENT_ID}&locationId={LOCATION_ID}", | ||
| json=PRIORITY_RESPONSE, | ||
| ) | ||
| return aioclient_mock |
There was a problem hiding this comment.
Can we instead patch out the library object and make that return what we expect it to return? Testing the library is something that should happen in the library, not in the home assistant integration
There was a problem hiding this comment.
Agreed, and switched it. For what it's worth, this doesn't lose what I was trying to protect against: LyricLocation/LyricDevice are thin wrappers that read straight from a JSON dict at property-access time (no eager parsing), so Lyric.get_locations now builds those same real objects from the same live-shaped payload — it just skips the HTTP-transport mocking to get there. So the field-name-mismatch class of bug is still caught, but the aiolyric-correctness question (does the pinned version of the library agree with what production returns) now lives where it belongs — in aiolyric's own test suite, which is already getting those fixes directly.
| async def test_schedule_status_sensor_end_to_end( | ||
| hass: HomeAssistant, | ||
| entity_registry: er.EntityRegistry, | ||
| setup_credentials: None, | ||
| mock_lyric_api: AiohttpClientMocker, | ||
| mock_config_entry: MockConfigEntry, | ||
| ) -> None: | ||
| """Test the schedule status diagnostic sensor via a real config entry setup. | ||
|
|
||
| scheduleStatus comes from the base /locations response (not the | ||
| /priority endpoint), so it has no known aiolyric field-name mismatch | ||
| and passes for real against the currently-pinned release. | ||
| """ | ||
| await async_setup_lyric_entry(hass, mock_config_entry) | ||
|
|
||
| entity_id = entity_registry.async_get_entity_id( | ||
| "sensor", "lyric", f"{MAC_ID}_schedule_status" | ||
| ) | ||
| assert entity_id | ||
| entry = entity_registry.async_get(entity_id) | ||
| assert entry |
There was a problem hiding this comment.
I'd maybe recommend to switch to snapshot_platform, but it might be cool to have that test before adding a new entity
There was a problem hiding this comment.
Done — switched to snapshot_platform, which turned out to be a nice simplification: it covers the whole sensor platform (schedule status + indoor temperature) in one assertion instead of hand-picking one entity, and syrupy's .ambr file is the actual source of truth rather than inline assertions.
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
…shot platform - Register the test credential under DOMAIN instead of a bespoke "cred" name, matching the application_credentials default and every other OAuth2 integration's conftest. - Set up application_credentials directly instead of the lyric domain itself; the latter only worked as a side effect of lyric listing it as a manifest dependency. - Patch Lyric.get_locations/get_thermostat_rooms to build real LyricLocation/LyricPriority objects instead of mocking HTTP responses at the aiohttp transport level. Field-name parsing still runs for real (the properties read straight off the same attributes dicts), but the test no longer depends on network-mocking machinery to exercise it. - Switch the schedule status test to snapshot_platform, covering the whole sensor platform via syrupy instead of one hand-picked entity.
joostlek flagged the same conftest.py pattern on PR home-assistant#177067 (this branch's conftest.py was cut from the same base): a bespoke "cred" credential name instead of the DOMAIN default, setting up the lyric domain directly instead of application_credentials, and mocking HTTP responses instead of the aiolyric client itself. Bringing this branch in line proactively rather than waiting for the same comments here. - Register the test credential under DOMAIN, matching every other OAuth2 integration's conftest. - Set up application_credentials directly instead of relying on it being pulled in as a side effect of lyric's manifest dependency. - Patch Lyric.get_locations/get_thermostat_rooms to build real LyricLocation/LyricPriority objects instead of mocking HTTP at the aiohttp transport level. Field-name parsing still runs for real (the properties read straight off the same attributes dicts). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
tests/components/lyric/conftest.py:38
- Replace this branch-history block with a stable explanation of the LCC constraint. References to the branch base and pending PR #177022 will become stale; the enduring constraint is that the current coordinator gates room fetches by the LCC prefix.
# Deliberately "LCC-"-prefixed: this branch is intentionally cut from a
# clean base without the coordinator fix from home-assistant/core#177022
# (which removes a device-ID-prefix heuristic gating the /priority fetch).
# Using a non-"LCC-" ID here would make room-level entities fail to be
# created for that unrelated, separately-tracked reason, muddying what
| # Resideo returns (e.g. vacationHold.Enabled, accessories[].sensorType), | ||
| # not the ones aiolyric happens to read, so tests exercise real parsing | ||
| # rather than hiding behind pre-parsed mocks. | ||
| LOCATION_ID = "35202000168931" |
| }), | ||
| 'original_device_class': None, | ||
| 'original_icon': None, | ||
| 'original_name': None, |
The committed snapshot was generated without running script.translations develop first, so it captured the untranslated fallback entity_id (sensor.ocala_thermostat, no suffix) instead of the real one. translations/en.json is gitignored and always built fresh, including in CI's "Compile English translations" step, so the mismatch only showed up there: PR home-assistant#177067's CI run failed pytest on this exact commit. Reproduced locally with the same command CI uses (python3 -b -X dev -m pytest ... --numprocesses auto ...) after running the translation step, confirmed the real entity_id is sensor.ocala_thermostat_schedule_status, and regenerated the snapshot against that. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (2)
tests/components/lyric/conftest.py:38
- Remove this temporary branch-history explanation because it will become stale when #177022 lands and does not document a lasting fixture constraint.
# Deliberately "LCC-"-prefixed: this branch is intentionally cut from a
# clean base without the coordinator fix from home-assistant/core#177022
# (which removes a device-ID-prefix heuristic gating the /priority fetch).
# Using a non-"LCC-" ID here would make room-level entities fail to be
# created for that unrelated, separately-tracked reason, muddying what
tests/components/lyric/conftest.py:32
- Condense this narration to the single non-obvious testing constraint so the fixture does not overstate what the sensor snapshot covers.
# Real payload shapes captured from a live T9-T10 account with paired
# RCHTSENSOR room accessories. Deliberately using the actual field names
# Resideo returns (e.g. vacationHold.Enabled, accessories[].sensorType),
# not the ones aiolyric happens to read, so tests exercise real parsing
# rather than hiding behind pre-parsed mocks.
| PRIORITY_RESPONSE = { | ||
| "deviceId": MAC_ID, | ||
| "priorityStatus": "NoHold", | ||
| "priority": { | ||
| "priorityType": "PickARoom", |
…ixture - Fix the DEVICE_ID/LOCATION_ID comments that still had the old branch-history narrative (already trimmed on the sibling binary-sensor-room-motion branch, but never ported here). - Remove PRIORITY_RESPONSE and the LyricPriority import: this test only covers device-level sensors (indoor_temperature, schedule_status), and the snapshot has no room/accessory entities regardless of what get_thermostat_rooms returns (the pinned aiolyric's sensorType/type mismatch keeps ACCESSORY_SENSORS from being created either way). Make get_thermostat_rooms a genuine no-op instead of building room data nothing in this file asserts on.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
tests/components/lyric/test_sensor.py:38
- Move the two setup-only fixtures to
@pytest.mark.usefixtures. Their return values are not consumed by this test, and the project testing guidance requires unused fixtures to be declared with the marker instead of function parameters.
setup_credentials: None,
mock_lyric_api: None,
| CLIENT_ID = "1234" | ||
| CLIENT_SECRET = "5678" | ||
|
|
||
| BASE_URL = "https://api.honeywellhome.com/v2" |
Same pattern already applied to binary-sensor-room-motion after joostlek's review there - this branch predates it: - Move LOCATIONS_RESPONSE from an inline dict into fixtures/locations.json. - Patch Lyric where the integration imports it (autospec, matching mealie's client mock) instead of patch.object on individual aiolyric methods. - Move async_setup_lyric_entry out of conftest.py into tests/components/lyric/__init__.py as setup_integration, matching the convention used by this integration's other test modules. No behavior change: same fixture data, same snapshot still matches.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (3)
tests/components/lyric/conftest.py:63
- Condense this fixture docstring to its non-obvious purpose. The current text narrates implementation details and cites an unrelated integration, so it is likely to become stale as the mock setup changes.
"""Mock the aiolyric client, backed by a real Location parsed from a live-shaped fixture.
Patches Lyric where the integration imports it (autospec, like the
mealie client mock) rather than mocking HTTP responses, so the test
exercises real aiolyric parsing - the same LyricLocation code reading
tests/components/lyric/test_sensor.py:38
- Move these side-effect-only fixtures to
@pytest.mark.usefixtures("setup_credentials", "mock_lyric_api"). Neither fixture value is consumed by the test, and the Lyric tests already use this pattern for side-effect fixtures intests/components/lyric/test_config_flow.py:48.
setup_credentials: None,
mock_lyric_api: MagicMock,
tests/components/lyric/conftest.py:27
- Remove these unused fixture constants. They are not referenced anywhere in the Lyric tests, so keeping fixture-specific IDs duplicated outside the JSON adds dead maintenance surface.
# Matches the values baked into fixtures/locations.json.
LOCATION_ID = "35202000168931"
DEVICE_ID = "LCC-7f86b153-8480-f111-b78f-6045bdb25006"
MAC_ID = "5CFCE1B67035"
…ract Resideo's own API docs document locationID as an Integer, and aiolyric's get_thermostat_rooms already types it int, but this fixture quoted it as a string - inherited from early in this session before checking the real docs. No behavior change (the value is only ever interpolated into a URL), but the fixture now matches reality.
- Condense mock_lyric_api's docstring to the non-obvious constraint (get_thermostat_rooms is a no-op, why); drop the mechanics narration and unrelated-integration citation. - Apply setup_credentials/mock_lyric_api via usefixtures in test_sensor instead of injecting them as unused parameters, matching the pattern test_config_flow.py already uses in this same file's test suite. - Remove LOCATION_ID/DEVICE_ID/MAC_ID constants: dead now that test_sensor.py uses snapshot_platform instead of building entity_ids from them.
Resideo's device response includes scheduleStatus (e.g. "Resume"), which was parsed but never surfaced. Adds it as a diagnostic sensor in DEVICE_SENSORS alongside the existing setpoint_status sensor.
Proposed change
Resideo's device response includes
scheduleStatus(e.g."Resume"), which wasparsed but never surfaced. Adds it as a diagnostic sensor in
DEVICE_SENSORSalongside the existing Setpoint Status sensor.
This is a device-level field from the base
/locationsresponse, which is alwaysfetched — it works independently of #177022 and doesn't need any aiolyric changes.
Type of change
Additional information
Checklist
ruff format homeassistant tests)If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest.requirements_all.txt.Updated by running
python3 -m script.gen_requirements_all.To help with the load of incoming pull requests: