diff --git a/custom_components/nestquest/__init__.py b/custom_components/nestquest/__init__.py index c05e392..bbcb3ad 100644 --- a/custom_components/nestquest/__init__.py +++ b/custom_components/nestquest/__init__.py @@ -16,7 +16,6 @@ from homeassistant.core import HomeAssistant from homeassistant.exceptions import ConfigEntryNotReady from homeassistant.helpers.event import async_track_time_change -from .api_client import NestQuestApiError from .const import ( CONF_ADMIN_USER_IDS, CONF_UPDATE_INTERVAL, @@ -526,10 +525,16 @@ async def async_setup_entry(hass: HomeAssistant, entry: ConfigEntry) -> bool: # (unconfigured token, API service down) does NOT crash setup: # the coordinator reports last_update_success=False, entities # come up unavailable, and HA's standard retry cycle applies - # once the API is reachable. Any non-API error still raises. + # once the API is reachable. HA's real + # ``async_config_entry_first_refresh`` wraps ANY failed update + # pass in ConfigEntryNotReady — the original error survives + # only as ``__cause__``, never as the raised exception — so the + # catch keys on ConfigEntryNotReady, NOT on the typed API + # error. Everything else (a platform-forward failure, + # cancellation) still unwinds through the except block below. try: await coordinator.async_config_entry_first_refresh() - except NestQuestApiError: + except ConfigEntryNotReady: pass await hass.config_entries.async_forward_entry_setups(entry, PLATFORMS) except BaseException: @@ -564,6 +569,12 @@ async def async_setup_entry(hass: HomeAssistant, entry: ConfigEntry) -> bool: # closes the already-closed database through the stale record. if getattr(entry, "runtime_data", None) is not None: entry.runtime_data = None + # The runtime record was ALSO registered in hass.data before + # the first refresh: remove it here on ANY failure, or a later + # unload picks up the stale, already-torn-down record + # (hass.data wins over entry.runtime_data there) and + # re-invokes the already-consumed update-listener remover. + hass.data.get(DOMAIN, {}).pop(entry.entry_id, None) await database.close() raise assert remove_update_listener is not None diff --git a/custom_components/nestquest/coordinator.py b/custom_components/nestquest/coordinator.py index bc7596a..1d634e1 100644 --- a/custom_components/nestquest/coordinator.py +++ b/custom_components/nestquest/coordinator.py @@ -32,11 +32,17 @@ Failure handling: constructing the client fails fast (ValueError) when the entry has no panel token configured, and a fetch failure raises the typed :class:`~.api_client.NestQuestApiError`. Neither crashes integration setup: the constructor logs a clear message and hands back -a client-shaped stub whose refresh raises the same typed error, so the -coordinator's refresh pass marks the entities unavailable -(``last_update_success = False``, ``data = None``) — exactly how a HA -``DataUpdateCoordinator`` surfaces a failed update. The typed error is -never swallowed silently: it is logged with its message and chained. +a client-shaped stub whose refresh raises the same typed error, and +``_async_update_data`` converts every typed error into +:class:`~homeassistant.helpers.update_coordinator.UpdateFailed` — the +exception a HA ``DataUpdateCoordinator`` records as a failed pass +(``last_update_success = False``, ``data = None``), so the entities +come up unavailable. The typed error is never swallowed silently: it +is logged with its message and chained into the UpdateFailed. (The +conversion matters at setup: HA's real +``async_config_entry_first_refresh`` wraps any failed pass in +``ConfigEntryNotReady``, so a setup that keys on the original +exception type would never match it.) """ from __future__ import annotations @@ -45,7 +51,10 @@ import datetime from typing import Any from homeassistant.core import HomeAssistant -from homeassistant.helpers.update_coordinator import DataUpdateCoordinator +from homeassistant.helpers.update_coordinator import ( + DataUpdateCoordinator, + UpdateFailed, +) from .api_client import NestQuestApiClient, NestQuestApiError, resolve_api_config from .const import ( @@ -150,9 +159,11 @@ class _UnconfiguredApiClient: yet must not crash integration setup, so the coordinator swaps in this stub: every refresh raises the SAME typed :class:`~.api_client.NestQuestApiError` a transport failure would, - carrying a clear "API not configured" message, so the coordinator - marks the entities unavailable and the HA standard retry cycle - applies. The error is logged, never silent. + carrying a clear "API not configured" message; + :meth:`NestQuestCoordinator._async_update_data` converts it to + :class:`~homeassistant.helpers.update_coordinator.UpdateFailed`, so + the coordinator marks the entities unavailable and the HA standard + retry cycle applies. The error is logged, never silent. """ __slots__ = () @@ -171,10 +182,15 @@ class NestQuestCoordinator(DataUpdateCoordinator): One refresh pass = one ``GET /api/v1/panel/snapshot`` through the API client, reconstructed into the core snapshot dataclasses every entity reads. A failed pass (unconfigured API, transport failure, - non-2xx response) raises the typed error out of - ``_async_update_data``, so the coordinator's ``data`` keeps its last - good value while ``last_update_success`` goes False — HA marks the - entities unavailable until a later pass succeeds. + non-2xx response) raises the typed error out of the client; + ``_async_update_data`` converts it to + :class:`~homeassistant.helpers.update_coordinator.UpdateFailed`, the + exception HA's coordinator machinery records as a failed pass: the + coordinator's ``data`` keeps its last good value while + ``last_update_success`` goes False — HA marks the entities + unavailable until a later pass succeeds. An unexpected + non-typed error still propagates out of ``_async_update_data`` + and is logged (with its traceback) by HA's machinery, the same way. Forced refreshes (the service paths pushing an immediate update after a completion) are SERIALIZED per entry: overlapping @@ -218,13 +234,24 @@ class NestQuestCoordinator(DataUpdateCoordinator): await super().async_refresh() async def _async_update_data(self) -> NestQuestSnapshot: + """One refresh pass: fetch the panel snapshot and rebuild it. + + A typed API failure (unconfigured token stub, transport + failure, non-2xx response) is converted to + :class:`~homeassistant.helpers.update_coordinator.UpdateFailed` + — the exception HA's coordinator machinery records as a failed + pass (``last_update_success = False``, ``data`` unchanged) so + entities go unavailable and the setup-time first refresh + arrives as ``ConfigEntryNotReady`` — with the typed error + logged and chained. Anything else still propagates. + """ try: payload = await self.api_client.get_snapshot() except NestQuestApiError as err: LOGGER.error( "NestQuest coordinator refresh failed: %s", err ) - raise + raise UpdateFailed(str(err)) from err return snapshot_from_api_payload(payload) @@ -247,8 +274,6 @@ def coordinator_client_from_entry( inject a stub transport without importing aiohttp — the mock-only test harness has no ``homeassistant.helpers.aiohttp_client``. """ - from custom_components.nestquest.api_client import NestQuestApiClient - base_url, panel_token = resolve_api_config(entry) if not isinstance(panel_token, str) or not panel_token.strip(): LOGGER.warning( diff --git a/tests/conftest.py b/tests/conftest.py index 86b9b28..d7e836e 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -367,6 +367,15 @@ class BinarySensorEntity(Entity): return self._attr_device_class +class UpdateFailed(HomeAssistantError): + """Stand-in mirroring homeassistant.helpers.update_coordinator.UpdateFailed. + + The exception an update pass raises to report a failed fetch: the + coordinator records ``last_update_success = False`` instead of + propagating it out of the refresh. + """ + + class DataUpdateCoordinator: """Functional stand-in for HA's DataUpdateCoordinator. @@ -375,8 +384,20 @@ class DataUpdateCoordinator: ``async_add_listener``/``async_update_listeners`` for entity subscription, ``async_refresh``/``async_config_entry_first_refresh`` driving the subclass's ``_async_update_data``, ``last_update_success``, - and ``data``. Interval scheduling is NOT simulated (no real clock - ticks in tests) — tests drive refreshes directly. + ``last_exception``, and ``data``. Interval scheduling is NOT + simulated (no real clock ticks in tests) — tests drive refreshes + directly. + + The failure contract mirrors real HA exactly (it is the trap this + harness once hid): an update failure NEVER propagates out of + ``async_refresh`` — the coordinator records + ``last_update_success = False``, keeps its last good ``data``, and + still notifies listeners so entities re-render unavailable — and + ``async_config_entry_first_refresh`` wraps ANY failed pass in + :class:`ConfigEntryNotReady`, with the original error only + surviving as ``__cause__``. A stand-in that re-raised the raw + update error would let setup code key on the original exception + type — a clause real HA can never reach. """ def __init__(self, hass, logger, *, name=None, update_interval=None): @@ -386,6 +407,7 @@ class DataUpdateCoordinator: self.update_interval = update_interval self.data = None self.last_update_success = False + self.last_exception = None self._listeners = [] def async_add_listener(self, update_callback, context=None): @@ -405,18 +427,35 @@ class DataUpdateCoordinator: callback() async def async_refresh(self): - """Run one update pass and notify listeners.""" + """Run one update pass and notify listeners. + + Mirrors HA: a failed update pass is RECORDED, never raised — + ``last_update_success`` goes False, the exception lands on + ``last_exception``, and listeners still fire; only cancellation + propagates. + """ try: self.data = await self._async_update_data() self.last_update_success = True - except BaseException: + except Exception as err: self.last_update_success = False - raise + self.last_exception = err self.async_update_listeners() async def async_config_entry_first_refresh(self): - """First refresh at config-entry setup (alias of refresh).""" + """First refresh at config-entry setup, per HA's real contract. + + HA's real ``async_config_entry_first_refresh`` wraps ANY failed + update pass in ``ConfigEntryNotReady`` — the original error + survives only as ``__cause__`` — so setup code that must + tolerate a failed first refresh keys on ConfigEntryNotReady, + never on the original exception type. + """ await self.async_refresh() + if not self.last_update_success: + raise ConfigEntryNotReady( + f"error fetching {self.name} data: {self.last_exception}" + ) from self.last_exception async def async_shutdown(self): """Release listeners on teardown.""" @@ -426,6 +465,7 @@ class DataUpdateCoordinator: _update_coordinator_mock = _ha_mock("homeassistant.helpers.update_coordinator") _update_coordinator_mock.DataUpdateCoordinator = DataUpdateCoordinator _update_coordinator_mock.CoordinatorEntity = CoordinatorEntity +_update_coordinator_mock.UpdateFailed = UpdateFailed _entity_mock = _ha_mock("homeassistant.helpers.entity") _entity_mock.DeviceInfo = DeviceInfo @@ -787,9 +827,6 @@ def coordinator_serves_local_snapshot(): """ import custom_components.nestquest as nq_mod import custom_components.nestquest.coordinator as coordinator_module - from custom_components.nestquest.coordinator import ( - coordinator_client_from_entry as production_factory, - ) _COORDINATOR_CLIENT_OVERRIDES = ( coordinator_module._COORDINATOR_CLIENT_OVERRIDES diff --git a/tests/test_coordinator.py b/tests/test_coordinator.py index 5b9b818..25dc0a0 100644 --- a/tests/test_coordinator.py +++ b/tests/test_coordinator.py @@ -33,6 +33,7 @@ from custom_components.nestquest.coordinator import ( snapshot_from_api_payload, ) from custom_components.nestquest.core.snapshot import instance_payload +from homeassistant.helpers.update_coordinator import UpdateFailed def _today_iso() -> str: @@ -340,10 +341,12 @@ async def test_coordinator_entities_read_the_api_snapshot(hass, make_entry) -> N async def test_coordinator_failed_fetch_marks_unavailable(hass, make_entry) -> None: - """A failed fetch (transport failure or non-2xx — both typed) raises - out of the refresh pass: last_update_success goes False (entities - unavailable), data keeps its last good value, and the typed error is - logged, never swallowed.""" + """A failed fetch (transport failure or non-2xx — both typed) is a + FAILED refresh pass: the update pass converts the typed error to + UpdateFailed (the exception HA's coordinator machinery records as a + failed pass, chaining the typed error as ``__cause__``), the refresh + records the failure instead of raising, and data keeps its last + good value.""" error = NestQuestApiError( "NestQuest API GET /api/v1/panel/snapshot failed with HTTP 503", status=503, @@ -356,13 +359,61 @@ async def test_coordinator_failed_fetch_marks_unavailable(hass, make_entry) -> N assert coordinator.data is not None assert coordinator.last_update_success is True - # The next pass hits the scripted failure: it raises out of the - # refresh, marks the entities unavailable, and keeps the last GOOD - # snapshot as data. - with pytest.raises(NestQuestApiError): - await coordinator.async_refresh() + # The next pass hits the scripted failure: the update pass converts + # the typed error to UpdateFailed, with the typed error chained. + with pytest.raises(UpdateFailed, match="HTTP 503") as excinfo: + await coordinator._async_update_data() + assert isinstance(excinfo.value.__cause__, NestQuestApiError) + + # The refresh pass records the failure the HA way — it does NOT + # raise: last_update_success goes False, data keeps the last GOOD + # snapshot, and the failed pass lands on last_exception as the + # converted UpdateFailed with the typed error chained. + await coordinator.async_refresh() assert coordinator.last_update_success is False assert coordinator.data.children[0].child_name == "Ada" + assert isinstance(coordinator.last_exception, UpdateFailed) + assert isinstance(coordinator.last_exception.__cause__, NestQuestApiError) + + +async def test_failed_first_refresh_does_not_crash_setup( + hass, make_entry +) -> None: + """A failed fetch at the FIRST refresh (at setup) does not crash + setup: the coordinator reports last_update_success=False, the setup + catches the ConfigEntryNotReady that HA's real first refresh raises + for a failed pass, and the platforms still forward so the entities + come up unavailable.""" + # The stub fails every snapshot fetch (HTTP 503), including setup's + # first refresh. + client = StubSnapshotClient( + NestQuestApiError( + "NestQuest API GET /api/v1/panel/snapshot failed with HTTP 503", + status=503, + ) + ) + entry = wire_entry_to_registry( + make_entry( + data={"api_base_url": "http://api.test:8000", "panel_token": "tok"} + ), + hass.registry, + ) + set_coordinator_client(entry, client) + assert await async_setup_entry(hass, entry) is True + coordinator = entry.runtime_data.coordinator + assert coordinator.api_client is client + # The failed first pass: the entities' surface stays unavailable. + assert coordinator.last_update_success is False + assert coordinator.data is None + # Setup survived the failed first refresh — the platforms were + # forwarded (the regression the review caught: a first-refresh + # failure used to abort setup before any entity was created). + assert hass.config_entries.forwarded_platforms == [ + "sensor", + "binary_sensor", + ] + # The unload after the failed-first-refresh setup stays clean. + assert await async_unload_entry(hass, entry) is True async def test_coordinator_unconfigured_api_does_not_crash_setup( @@ -386,10 +437,19 @@ async def test_coordinator_unconfigured_api_does_not_crash_setup( assert isinstance(coordinator.api_client, _UnconfiguredApiClient) # A real NestQuestApiClient was never built (it would ValueError). assert not isinstance(coordinator.api_client, NestQuestApiClient) + # The first refresh failed the HA way: the update pass converted + # the stub's typed error to UpdateFailed, the first refresh raised + # ConfigEntryNotReady, and setup tolerated it — platforms forwarded + # so the entities come up unavailable. assert coordinator.last_update_success is False assert coordinator.data is None - with pytest.raises(NestQuestApiError, match="not configured"): - await coordinator.async_refresh() + with pytest.raises(UpdateFailed, match="not configured"): + await coordinator._async_update_data() + assert hass.config_entries.forwarded_platforms == [ + "sensor", + "binary_sensor", + ] + assert await async_unload_entry(hass, entry) is True async def test_coordinator_configured_token_builds_real_client( @@ -484,11 +544,23 @@ async def test_unload_shuts_coordinator_down(hass, make_entry) -> None: assert torn_down == [coordinator] -async def test_failed_first_refresh_shuts_coordinator_down( +async def test_setup_failure_after_registration_shuts_coordinator_down( hass, make_entry, monkeypatch ) -> None: """A setup failure after coordinator creation must not leak its - listeners.""" + listeners or leave a stale runtime record behind. + + The runtime record registers in ``hass.data`` BEFORE the platform + forward, so a forward failure reaches the unwind with the record + already registered. The unwind must shut the coordinator down, + clear ``entry.runtime_data``, AND remove the stale ``hass.data`` + entry — otherwise a later unload picks up the stale, already-torn- + down record and re-invokes the already-consumed update-listener + remover, which raises. (A failed first refresh is NOT such a + failure: HA's real first refresh wraps it in ConfigEntryNotReady, + which setup deliberately tolerates so entities come up + unavailable.) + """ from custom_components.nestquest import async_setup_entry import custom_components.nestquest.coordinator as coordinator_module @@ -503,20 +575,29 @@ async def test_failed_first_refresh_shuts_coordinator_down( _record_shutdown, ) - async def _boom(self): - raise RuntimeError("refresh failed") + async def _boom(entry, platforms): + raise RuntimeError("platform forward failed") monkeypatch.setattr( - coordinator_module.NestQuestCoordinator, - "_async_update_data", + hass.config_entries, + "async_forward_entry_setups", _boom, ) entry = wire_entry_to_registry(make_entry(), hass.registry) - with pytest.raises(RuntimeError, match="refresh failed"): + with pytest.raises(RuntimeError, match="platform forward failed"): await async_setup_entry(hass, entry) assert shutdown_calls == [entry.entry_id] assert getattr(entry, "runtime_data", None) is None + # The stale hass.data registration was removed with the runtime + # record: the unload below finds nothing stale, must not raise, + # and must leave no residue behind. + assert await async_unload_entry(hass, entry) is True + assert DOMAIN not in hass.data + assert hass.data == {} + assert hass.registry.size == 0 + assert hass.time_change.size == 0 + assert entry.runtime_data is None async def test_snapshot_from_api_payload_empty_household() -> None: diff --git a/tests/test_sensors.py b/tests/test_sensors.py index c451db4..fa29adb 100644 --- a/tests/test_sensors.py +++ b/tests/test_sensors.py @@ -264,6 +264,11 @@ async def test_due_today_payload_omits_missed_admin_equals_panel( "the API payload omits missed rows, so the admin payload " "carries none either (the PWA is the admin surface now)" ) + # Counts still roll the missed instance up as not-completed: the + # counts arrive precomputed from the route's own build, which rolls + # the FULL day (the missed row included) into the numbers. + assert _sensor(hass, bo.id, "quests_completed_today").native_value == 0 + assert _sensor(hass, bo.id, "quests_remaining_today").native_value == 1 @pytest.mark.parametrize("kind", SENSOR_KINDS)