diff --git a/custom_components/sws12500/pocasti_cz.py b/custom_components/sws12500/pocasti_cz.py index c975d88..d5344fc 100644 --- a/custom_components/sws12500/pocasti_cz.py +++ b/custom_components/sws12500/pocasti_cz.py @@ -10,6 +10,7 @@ from typing import Any, Literal from aiohttp import ClientError, ClientTimeout from py_typecheck.core import checked_or +from homeassistant.components import persistent_notification from homeassistant.config_entries import ConfigEntry from homeassistant.core import HomeAssistant from homeassistant.helpers.aiohttp_client import async_get_clientsession @@ -91,13 +92,20 @@ class PocasiPush: return "unexpected_response" async def _disable_pocasi(self, reason: str) -> None: - """Turn resending off and persist it, so it survives a restart.""" + """Turn resending off and persist it, so it survives a restart. + + Notifies the user as well - as `WindyPush._disable_windy` does. Forwarding + switching itself off is otherwise invisible until someone notices that data + stopped arriving upstream, since the only other trace is a log line. + """ self.last_error = reason if not await update_options(self.hass, self.config, POCASI_CZ_ENABLED, False): _LOGGER.debug("Failed to set Pocasi Meteo options to false.") + persistent_notification.async_create(self.hass, reason, "Pocasi Meteo resending disabled.") + async def push_data_to_server(self, data: dict[str, Any], mode: PocasiMode): """Forward a station payload to Pocasi Meteo. @@ -219,7 +227,20 @@ class PocasiPush: # TimeoutError is not a ClientError: an `async_timeout`/`asyncio` timeout would # otherwise escape into the webhook handler and answer the station with HTTP 500, # even though the measured data was already stored. - except (ClientError, TimeoutError) as ex: + # + # It gets its own branch because it must not spend the retry budget: a slow + # upstream is transient and self-healing, so the next push just tries again, + # while `invalid_response_count` is meant for faults that will not fix + # themselves. Caught first on purpose - aiohttp's ServerTimeoutError is both a + # ClientError and a TimeoutError, and it belongs here. + except TimeoutError as ex: + self.last_status = "timeout" + self.last_error = type(ex).__name__ + _LOGGER.warning( + "Pocasi Meteo did not answer within %s seconds. Will try again on the next push.", + FORWARD_TIMEOUT, + ) + except ClientError as ex: self.last_status = "client_error" # Store only the exception class - last_error is surfaced via entity # attributes; str(ex) could embed the request URL. diff --git a/custom_components/sws12500/windy_func.py b/custom_components/sws12500/windy_func.py index 46978bc..cae9055 100644 --- a/custom_components/sws12500/windy_func.py +++ b/custom_components/sws12500/windy_func.py @@ -175,15 +175,13 @@ class WindyPush: indata["uv"] = indata.pop("t1uvi") if "t1solrad" in indata: indata["solarradiation"] = indata.pop("t1solrad") - # Indoor readings mean nothing to Windy. Renaming them to their PWS spelling - # (rather than dropping them here) leaves PURGE_DATA - which lists PWS names - - # as the single place that decides what Windy never receives, exactly as on the - # Ecowitt path. The values stay Celsius/percent, which is fine because they are - # purged before the request is built. - if "intem" in indata: - indata["indoortempf"] = indata.pop("intem") - if "inhum" in indata: - indata["indoorhumidity"] = indata.pop("inhum") + + # Indoor readings mean nothing to Windy, and there is no PWS name to put them + # under: `indoortempf` is Fahrenheit while WSLink `intem` is Celsius. Dropping + # them outright keeps the payload unit-correct on its own terms, rather than + # relying on PURGE_DATA to discard a mislabelled value afterwards. + indata.pop("intem", None) + indata.pop("inhum", None) return indata @@ -349,7 +347,20 @@ class WindyPush: # TimeoutError is not a ClientError: an `async_timeout`/`asyncio` timeout would # otherwise escape into the webhook handler and answer the station with HTTP 500, # even though the measured data was already stored. - except (ClientError, TimeoutError) as ex: + # + # It gets its own branch because it must not spend the retry budget: a slow + # upstream is transient and self-healing, so the next push just tries again, + # while `invalid_response_count` is meant for faults that will not fix + # themselves. Caught first on purpose - aiohttp's ServerTimeoutError is both a + # ClientError and a TimeoutError, and it belongs here. + except TimeoutError as ex: + self.last_status = "timeout" + self.last_error = type(ex).__name__ + _LOGGER.warning( + "Windy did not answer within %s seconds. Will try again on the next push.", + FORWARD_TIMEOUT, + ) + except ClientError as ex: self.last_status = "client_error" # Store only the exception class - last_error is surfaced via entity # attributes; str(ex) could embed the request URL. diff --git a/tests/test_pocasi_ecowitt.py b/tests/test_pocasi_ecowitt.py index 022abf3..b59b41c 100644 --- a/tests/test_pocasi_ecowitt.py +++ b/tests/test_pocasi_ecowitt.py @@ -121,6 +121,16 @@ def hass(): return SimpleNamespace() +@pytest.fixture(autouse=True) +def notify(monkeypatch): + """Capture the disable notification; the stub `hass` cannot serve the real one.""" + created = MagicMock() + monkeypatch.setattr( + "custom_components.sws12500.pocasti_cz.persistent_notification.async_create", created + ) + return created + + @pytest.fixture def pusher(monkeypatch, hass): """A PocasiPush wired to a recording session and allowed to send immediately.""" diff --git a/tests/test_pocasi_push.py b/tests/test_pocasi_push.py index a6925ea..274f386 100644 --- a/tests/test_pocasi_push.py +++ b/tests/test_pocasi_push.py @@ -101,6 +101,16 @@ def hass(): return SimpleNamespace() +@pytest.fixture(autouse=True) +def notify(monkeypatch): + """Capture the disable notification; the stub `hass` cannot serve the real one.""" + created = MagicMock() + monkeypatch.setattr( + "custom_components.sws12500.pocasti_cz.persistent_notification.async_create", created + ) + return created + + @pytest.mark.asyncio async def test_push_data_to_server_missing_api_id_returns_early(monkeypatch, hass): entry = _make_entry(api_id=None, api_key="key") @@ -339,14 +349,15 @@ async def test_push_data_to_server_client_error_increments_and_disables_after_th @pytest.mark.asyncio -async def test_push_data_to_server_timeout_is_handled_like_client_error( - monkeypatch, hass -): +async def test_push_data_to_server_timeout_is_caught_and_not_counted(monkeypatch, hass): """A network timeout must not escape into the webhook handler. `TimeoutError` is not a subclass of `ClientError`, so it used to propagate out of `push_data_to_server`, through the awaiting coordinator, and answer the station with HTTP 500 - even though the measured data was already stored. + + It must not spend the retry budget either: a slow upstream is transient, so the + next push simply tries again. """ entry = _make_entry() pp = PocasiPush(hass, entry) @@ -366,12 +377,94 @@ async def test_push_data_to_server_timeout_is_handled_like_client_error( pp.next_update = dt_util.utcnow() - timedelta(seconds=1) await pp.push_data_to_server({"x": 1}, "WU") - assert pp.last_status == "client_error" + assert pp.last_status == "timeout" assert pp.last_error == "TimeoutError" - assert pp.invalid_response_count == 1 + assert pp.invalid_response_count == 0 assert pp.enabled is True +@pytest.mark.asyncio +async def test_repeated_timeouts_never_disable_pocasi(monkeypatch, hass, notify): + """Two minutes of a merely slow server must not turn forwarding off for good. + + With a 30 second send interval, counting timeouts would spend the whole retry + budget on a slowdown that recovers by itself. + """ + entry = _make_entry() + pp = PocasiPush(hass, entry) + + update_options = _write_through_update_options(entry) + monkeypatch.setattr( + "custom_components.sws12500.pocasti_cz.update_options", update_options + ) + + session = _FakeSession(exc=TimeoutError("timed out")) + monkeypatch.setattr( + "custom_components.sws12500.pocasti_cz.async_get_clientsession", + lambda _h: session, + ) + + for _ in range(POCASI_CZ_MAX_RETRIES + 2): + pp.next_update = dt_util.utcnow() - timedelta(seconds=1) + await pp.push_data_to_server({"x": 1}, "WU") + + assert pp.invalid_response_count == 0 + assert pp.enabled is True + update_options.assert_not_awaited() + notify.assert_not_called() + + +@pytest.mark.asyncio +async def test_timeout_does_not_reset_an_existing_client_error_budget(monkeypatch, hass): + """A timeout is ignored by the counter, not a substitute for a successful send.""" + entry = _make_entry() + pp = PocasiPush(hass, entry) + pp.invalid_response_count = 2 + pp.next_update = dt_util.utcnow() - timedelta(seconds=1) + + session = _FakeSession(exc=TimeoutError("timed out")) + monkeypatch.setattr( + "custom_components.sws12500.pocasti_cz.async_get_clientsession", + lambda _h: session, + ) + + await pp.push_data_to_server({"x": 1}, "WU") + + assert pp.invalid_response_count == 2 + + +@pytest.mark.asyncio +async def test_disabling_pocasi_notifies_the_user(monkeypatch, hass, notify): + """Forwarding switching itself off must be visible, not just a log line. + + Windy already raises a persistent notification; without one here the user only + finds out when data stops arriving upstream. + """ + entry = _make_entry() + pp = PocasiPush(hass, entry) + pp.next_update = dt_util.utcnow() - timedelta(seconds=1) + + session = _FakeSession(response=_FakeResponse("", status=401)) + monkeypatch.setattr( + "custom_components.sws12500.pocasti_cz.async_get_clientsession", + lambda _h: session, + ) + monkeypatch.setattr("custom_components.sws12500.pocasti_cz.anonymize", lambda d: d) + monkeypatch.setattr( + "custom_components.sws12500.pocasti_cz.update_options", + _write_through_update_options(entry), + ) + monkeypatch.setattr("custom_components.sws12500.pocasti_cz._LOGGER.critical", MagicMock()) + + await pp.push_data_to_server({"x": 1}, "WU") + + assert pp.enabled is False + notify.assert_called_once() + args = notify.call_args.args + assert args[0] is hass + assert POCASI_INVALID_KEY in args[1] + + @pytest.mark.asyncio async def test_push_data_to_server_bounds_the_request(monkeypatch, hass): """The send must carry an explicit timeout. diff --git a/tests/test_windy_push.py b/tests/test_windy_push.py index 103a61c..cacd86a 100644 --- a/tests/test_windy_push.py +++ b/tests/test_windy_push.py @@ -13,6 +13,7 @@ from custom_components.sws12500.const import ( PURGE_DATA, WINDY_ENABLED, WINDY_LOGGER_ENABLED, + WINDY_MAX_RETRIES, WINDY_STATION_ID, WINDY_STATION_PW, WINDY_UNEXPECTED, @@ -130,8 +131,6 @@ def test_covert_wslink_to_pws_maps_keys(hass): assert out["precip"] == "8" assert out["uv"] == "9" assert out["solarradiation"] == "10" - assert out["indoortempf"] == "21.5" - assert out["indoorhumidity"] == "48" assert out["other"] == "keep" for k in ( "t1ws", @@ -149,6 +148,11 @@ def test_covert_wslink_to_pws_maps_keys(hass): ): assert k not in out + # Indoor readings are dropped, not renamed: WSLink sends Celsius, while the PWS + # `indoortempf` spelling promises Fahrenheit. + assert "indoortempf" not in out + assert "indoorhumidity" not in out + @pytest.mark.asyncio async def test_push_data_to_windy_respects_initial_next_update(monkeypatch, hass): @@ -462,14 +466,15 @@ async def test_push_data_to_windy_client_error_increments_and_disables_after_thr @pytest.mark.asyncio -async def test_push_data_to_windy_timeout_is_handled_like_client_error( - monkeypatch, hass -): +async def test_push_data_to_windy_timeout_is_caught_and_not_counted(monkeypatch, hass): """A network timeout must not escape into the webhook handler. `TimeoutError` is not a subclass of `ClientError`, so it used to propagate out of `push_data_to_windy`, through the awaiting coordinator, and answer the station with HTTP 500 - even though the measured data was already stored. + + It must not spend the retry budget either: a slow upstream is transient, so the + next push simply tries again. """ entry = _make_entry() wp = WindyPush(hass, entry) @@ -493,9 +498,61 @@ async def test_push_data_to_windy_timeout_is_handled_like_client_error( ok = await wp.push_data_to_windy({"a": "b"}) assert ok is True - assert wp.last_status == "client_error" + assert wp.last_status == "timeout" assert wp.last_error == "TimeoutError" - assert wp.invalid_response_count == 1 + assert wp.invalid_response_count == 0 + + +@pytest.mark.asyncio +async def test_repeated_timeouts_never_disable_windy(monkeypatch, hass): + """15 minutes of a merely slow Windy must not turn forwarding off for good. + + Bounding the request made this branch reachable; counting it would let a + transient slowdown burn the whole retry budget with nothing to recover it. + """ + entry = _make_entry() + wp = WindyPush(hass, entry) + + update_options = AsyncMock(return_value=True) + monkeypatch.setattr("custom_components.sws12500.windy_func.update_options", update_options) + notify = MagicMock() + monkeypatch.setattr( + "custom_components.sws12500.windy_func.persistent_notification.async_create", notify + ) + + session = _FakeSession(exc=TimeoutError("timed out")) + monkeypatch.setattr( + "custom_components.sws12500.windy_func.async_get_clientsession", + lambda _h: session, + ) + + for _ in range(WINDY_MAX_RETRIES + 2): + wp.next_update = dt_util.utcnow() - timedelta(seconds=1) + assert await wp.push_data_to_windy({"a": "b"}) is True + + assert wp.invalid_response_count == 0 + assert wp.enabled is True + update_options.assert_not_awaited() + notify.assert_not_called() + + +@pytest.mark.asyncio +async def test_timeout_does_not_reset_an_existing_client_error_budget(monkeypatch, hass): + """A timeout is ignored by the counter, not a substitute for a successful send.""" + entry = _make_entry() + wp = WindyPush(hass, entry) + wp.invalid_response_count = 2 + wp.next_update = dt_util.utcnow() - timedelta(seconds=1) + + session = _FakeSession(exc=TimeoutError("timed out")) + monkeypatch.setattr( + "custom_components.sws12500.windy_func.async_get_clientsession", + lambda _h: session, + ) + + await wp.push_data_to_windy({"a": "b"}) + + assert wp.invalid_response_count == 2 @pytest.mark.asyncio @@ -608,8 +665,9 @@ async def test_push_data_to_windy_purges_after_converting(monkeypatch, hass): async def test_push_data_to_windy_drops_wslink_indoor_readings(monkeypatch, hass): """WSLink `intem`/`inhum` must not reach Windy under either spelling. - PURGE_DATA lists the PWS names, so before these two were mapped in the converter - they slipped through untouched and Windy received a raw `intem`/`inhum` pair. + PURGE_DATA lists the PWS names, so these two used to slip through untouched and + Windy received a raw `intem`/`inhum` pair. The converter drops them instead, which + holds regardless of what PURGE_DATA contains. """ entry = _make_entry() wp = WindyPush(hass, entry)