From 3c2bd51e8522b6afb842ee61ab92dc57402b5a71 Mon Sep 17 00:00:00 2001 From: Jules Dejaeghere Date: Fri, 10 May 2024 14:58:21 +0200 Subject: [PATCH 1/5] First steps for better timeout handling --- custom_components/irm_kmi/api.py | 2 +- custom_components/irm_kmi/coordinator.py | 15 +++++++++------ 2 files changed, 10 insertions(+), 7 deletions(-) diff --git a/custom_components/irm_kmi/api.py b/custom_components/irm_kmi/api.py index 2f0a326..a106fd7 100644 --- a/custom_components/irm_kmi/api.py +++ b/custom_components/irm_kmi/api.py @@ -71,7 +71,7 @@ class IrmKmiApiClient: """Get information from the API.""" try: - async with async_timeout.timeout(10): + async with async_timeout.timeout(60): response = await self._session.request( method=method, url=f"{self._base_url if base_url is None else base_url}{path}", diff --git a/custom_components/irm_kmi/coordinator.py b/custom_components/irm_kmi/coordinator.py index da847ae..f868e20 100644 --- a/custom_components/irm_kmi/coordinator.py +++ b/custom_components/irm_kmi/coordinator.py @@ -30,7 +30,8 @@ from .utils import disable_from_config, get_config_value _LOGGER = logging.getLogger(__name__) - +# TODO inherit from TimestampDataUpdateCoordinator and use the last update time to avoid having +# entities unavailable if API is down for a few minutes only (like 3x the update interval) class IrmKmiCoordinator(DataUpdateCoordinator): """Coordinator to update data from IRM KMI""" @@ -67,7 +68,7 @@ class IrmKmiCoordinator(DataUpdateCoordinator): try: # Note: asyncio.TimeoutError and aiohttp.ClientError are already # handled by the data update coordinator. - async with async_timeout.timeout(10): + async with async_timeout.timeout(60): api_data = await self._api_client.get_forecasts_coord( {'lat': zone.attributes[ATTR_LATITUDE], 'long': zone.attributes[ATTR_LONGITUDE]} @@ -114,8 +115,10 @@ class IrmKmiCoordinator(DataUpdateCoordinator): try: images_from_api = await self.download_images_from_api(animation_data, country, localisation_layer_url) - except IrmKmiApiError: - _LOGGER.warning(f"Could not get images for weather radar") + except IrmKmiApiError as err: + _LOGGER.warning(f"Could not get images for weather radar: {err}") + # TODO idea return self.data.get('animation', RadarAnimationData()) + # this way, when we cannot update, we keep the old data return RadarAnimationData() localisation = images_from_api[0] @@ -148,8 +151,8 @@ class IrmKmiCoordinator(DataUpdateCoordinator): try: _LOGGER.debug(f"Requesting pollen SVG at url {svg_url}") pollen_svg: str = await self._api_client.get_svg(svg_url) - except IrmKmiApiError: - _LOGGER.warning(f"Could not get pollen data from the API") + except IrmKmiApiError as err: + _LOGGER.warning(f"Could not get pollen data from the API: {err}") return PollenParser.get_default_data() return PollenParser(pollen_svg).get_pollen_data() From ac0fe07f4f5b65bd5247d93e7f38ed52cff6498d Mon Sep 17 00:00:00 2001 From: Jules Dejaeghere Date: Fri, 10 May 2024 18:43:54 +0200 Subject: [PATCH 2/5] Keep the previous value for pollen and radar when the API fails only for those items but not for the main forecast --- custom_components/irm_kmi/coordinator.py | 11 +++++------ custom_components/irm_kmi/pollen.py | 12 +++++++++++- tests/test_pollen.py | 5 +++-- 3 files changed, 19 insertions(+), 9 deletions(-) diff --git a/custom_components/irm_kmi/coordinator.py b/custom_components/irm_kmi/coordinator.py index f868e20..a63d30d 100644 --- a/custom_components/irm_kmi/coordinator.py +++ b/custom_components/irm_kmi/coordinator.py @@ -116,10 +116,8 @@ class IrmKmiCoordinator(DataUpdateCoordinator): try: images_from_api = await self.download_images_from_api(animation_data, country, localisation_layer_url) except IrmKmiApiError as err: - _LOGGER.warning(f"Could not get images for weather radar: {err}") - # TODO idea return self.data.get('animation', RadarAnimationData()) - # this way, when we cannot update, we keep the old data - return RadarAnimationData() + _LOGGER.warning(f"Could not get images for weather radar: {err}. Keep the existing radar data.") + return self.data.get('animation', RadarAnimationData()) if self.data is not None else RadarAnimationData() localisation = images_from_api[0] images_from_api = images_from_api[1:] @@ -152,8 +150,9 @@ class IrmKmiCoordinator(DataUpdateCoordinator): _LOGGER.debug(f"Requesting pollen SVG at url {svg_url}") pollen_svg: str = await self._api_client.get_svg(svg_url) except IrmKmiApiError as err: - _LOGGER.warning(f"Could not get pollen data from the API: {err}") - return PollenParser.get_default_data() + _LOGGER.warning(f"Could not get pollen data from the API: {err}. Keeping the same data.") + return self.data.get('pollen', PollenParser.get_unavailable_data()) \ + if self.data is not None else PollenParser.get_unavailable_data() return PollenParser(pollen_svg).get_pollen_data() diff --git a/custom_components/irm_kmi/pollen.py b/custom_components/irm_kmi/pollen.py index 09b880b..e29c729 100644 --- a/custom_components/irm_kmi/pollen.py +++ b/custom_components/irm_kmi/pollen.py @@ -8,6 +8,11 @@ from custom_components.irm_kmi.const import POLLEN_NAMES _LOGGER = logging.getLogger(__name__) +def get_unavailable_data() -> dict: + """Return all the known pollen with 'none' value""" + return {k.lower(): 'none' for k in POLLEN_NAMES} + + class PollenParser: """ The SVG looks as follows (see test fixture for a real example) @@ -25,6 +30,7 @@ class PollenParser: For the color scale, look for a white dot (nearly) at the same level as the pollen name. From the white dot horizontal position, determine the level """ + def __init__( self, xml_string: str @@ -53,6 +59,11 @@ class PollenParser: """Return all the known pollen with 'none' value""" return {k.lower(): 'none' for k in POLLEN_NAMES} + @staticmethod + def get_unavailable_data() -> dict: + """Return all the known pollen with 'none' value""" + return {k.lower(): None for k in POLLEN_NAMES} + @staticmethod def get_option_values() -> List[str]: """List all the values that the pollen can have""" @@ -128,4 +139,3 @@ class PollenParser: _LOGGER.debug(f"Pollen data: {pollen_data}") return pollen_data - diff --git a/tests/test_pollen.py b/tests/test_pollen.py index 667ca1c..b868e8c 100644 --- a/tests/test_pollen.py +++ b/tests/test_pollen.py @@ -27,6 +27,7 @@ def test_svg_pollen_parsing(): assert data == {'birch': 'none', 'oak': 'active', 'hazel': 'none', 'mugwort': 'none', 'alder': 'active', 'grasses': 'none', 'ash': 'active'} + def test_pollen_options(): assert PollenParser.get_option_values() == ['active', 'green', 'yellow', 'orange', 'red', 'purple', 'none'] @@ -50,7 +51,7 @@ async def test_pollen_data_from_api( assert result == expected -async def test_pollen_error_leads_to_default_values( +async def test_pollen_error_leads_to_unavailable_on_first_call( hass: HomeAssistant, mock_config_entry: MockConfigEntry, mock_exception_irm_kmi_api_svg_pollen: AsyncMock @@ -59,5 +60,5 @@ async def test_pollen_error_leads_to_default_values( api_data = get_api_data("be_forecast_warning.json") result = await coordinator._async_pollen_data(api_data) - expected = PollenParser.get_default_data() + expected = PollenParser.get_unavailable_data() assert result == expected From d5d400563426f02d7d26f0b2ee1a292c8ea069d7 Mon Sep 17 00:00:00 2001 From: Jules Dejaeghere Date: Fri, 10 May 2024 20:36:03 +0200 Subject: [PATCH 3/5] Weather data will become unavailable at the third failed update in a row --- custom_components/irm_kmi/coordinator.py | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/custom_components/irm_kmi/coordinator.py b/custom_components/irm_kmi/coordinator.py index a63d30d..d0ab859 100644 --- a/custom_components/irm_kmi/coordinator.py +++ b/custom_components/irm_kmi/coordinator.py @@ -13,8 +13,9 @@ from homeassistant.core import HomeAssistant from homeassistant.helpers import issue_registry from homeassistant.helpers.aiohttp_client import async_get_clientsession from homeassistant.helpers.device_registry import DeviceEntryType, DeviceInfo -from homeassistant.helpers.update_coordinator import (DataUpdateCoordinator, +from homeassistant.helpers.update_coordinator import (TimestampDataUpdateCoordinator, UpdateFailed) +from homeassistant.util.dt import utcnow from .api import IrmKmiApiClient, IrmKmiApiError from .const import CONF_DARK_MODE, CONF_STYLE, DOMAIN @@ -30,9 +31,8 @@ from .utils import disable_from_config, get_config_value _LOGGER = logging.getLogger(__name__) -# TODO inherit from TimestampDataUpdateCoordinator and use the last update time to avoid having -# entities unavailable if API is down for a few minutes only (like 3x the update interval) -class IrmKmiCoordinator(DataUpdateCoordinator): + +class IrmKmiCoordinator(TimestampDataUpdateCoordinator): """Coordinator to update data from IRM KMI""" def __init__(self, hass: HomeAssistant, entry: ConfigEntry): @@ -77,7 +77,13 @@ class IrmKmiCoordinator(DataUpdateCoordinator): _LOGGER.debug(f"Full data: {api_data}") except IrmKmiApiError as err: - raise UpdateFailed(f"Error communicating with API: {err}") + if self.last_update_success_time is not None \ + and self.last_update_success_time - utcnow() < 2.5 * self.update_interval: + _LOGGER.warning(f"Error communicating with API for general forecast: {err}. Keeping the old data.") + return self.data + else: + raise UpdateFailed(f"Error communicating with API for general forecast: {err}. " + f"Last success time is: {self.last_update_success_time}") if api_data.get('cityName', None) in OUT_OF_BENELUX: _LOGGER.error(f"The zone {self._zone} is now out of Benelux and forecast is only available in Benelux." From 68491fc7dad495ad2af89d3526850042fcd2a749 Mon Sep 17 00:00:00 2001 From: Jules Dejaeghere Date: Sun, 12 May 2024 14:33:24 +0200 Subject: [PATCH 4/5] Increaste timeout to 60s --- custom_components/irm_kmi/coordinator.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/custom_components/irm_kmi/coordinator.py b/custom_components/irm_kmi/coordinator.py index d0ab859..1be0cf3 100644 --- a/custom_components/irm_kmi/coordinator.py +++ b/custom_components/irm_kmi/coordinator.py @@ -187,7 +187,7 @@ class IrmKmiCoordinator(TimestampDataUpdateCoordinator): if frame.get('uri', None) is not None: coroutines.append( self._api_client.get_image(frame.get('uri'), params={'rs': STYLE_TO_PARAM_MAP[self._style]})) - async with async_timeout.timeout(20): + async with async_timeout.timeout(60): images_from_api = await asyncio.gather(*coroutines) _LOGGER.debug(f"Just downloaded {len(images_from_api)} images") From 4e8f1faebc46146b318827d125a6d46da283be6c Mon Sep 17 00:00:00 2001 From: Jules Dejaeghere Date: Sun, 12 May 2024 14:33:45 +0200 Subject: [PATCH 5/5] Add test --- tests/conftest.py | 17 +++++++++++++++ tests/test_coordinator.py | 45 ++++++++++++++++++++++++++++++++++++++- 2 files changed, 61 insertions(+), 1 deletion(-) diff --git a/tests/conftest.py b/tests/conftest.py index 3efeef0..40db775 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -262,3 +262,20 @@ def mock_coordinator(request: pytest.FixtureRequest) -> Generator[None, MagicMoc coord = coordinator_mock.return_value coord._async_animation_data.return_value = {'animation': None} yield coord + + +@pytest.fixture() +def mock_irm_kmi_api_works_but_pollen_and_radar_fail(request: pytest.FixtureRequest) -> Generator[ + None, MagicMock, None]: + """Return a mocked IrmKmi api client.""" + fixture: str = "forecast.json" + + forecast = json.loads(load_fixture(fixture)) + with patch( + "custom_components.irm_kmi.coordinator.IrmKmiApiClient", autospec=True + ) as irm_kmi_api_mock: + irm_kmi = irm_kmi_api_mock.return_value + irm_kmi.get_forecasts_coord.return_value = forecast + irm_kmi.get_svg.side_effect = IrmKmiApiError + irm_kmi.get_image.side_effect = IrmKmiApiError + yield irm_kmi diff --git a/tests/test_coordinator.py b/tests/test_coordinator.py index 86d7ac0..bd06d3b 100644 --- a/tests/test_coordinator.py +++ b/tests/test_coordinator.py @@ -8,7 +8,9 @@ from homeassistant.core import HomeAssistant from pytest_homeassistant_custom_component.common import MockConfigEntry from custom_components.irm_kmi.coordinator import IrmKmiCoordinator -from custom_components.irm_kmi.data import CurrentWeatherData, IrmKmiForecast +from custom_components.irm_kmi.data import CurrentWeatherData, IrmKmiForecast, ProcessedCoordinatorData, \ + RadarAnimationData +from custom_components.irm_kmi.pollen import PollenParser from tests.conftest import get_api_data @@ -134,3 +136,44 @@ def test_hourly_forecast() -> None: ) assert result[8] == expected + + +async def test_refresh_succeed_even_when_pollen_and_radar_fail( + hass: HomeAssistant, + mock_config_entry: MockConfigEntry, + mock_irm_kmi_api_works_but_pollen_and_radar_fail +): + hass.states.async_set( + "zone.home", + 0, + {"latitude": 50.738681639, "longitude": 4.054077148}, + ) + + mock_config_entry.add_to_hass(hass) + + coordinator = IrmKmiCoordinator(hass, mock_config_entry) + + result = await coordinator._async_update_data() + + assert result.get('current_weather').get('condition') == ATTR_CONDITION_CLOUDY + + assert result.get('animation') == dict() + + assert result.get('pollen') == PollenParser.get_unavailable_data() + + existing_data = ProcessedCoordinatorData( + current_weather=CurrentWeatherData(), + daily_forecast=[], + hourly_forecast=[], + animation=RadarAnimationData(hint="This will remain unchanged"), + warnings=[], + pollen={'foo': 'bar'} + ) + coordinator.data = existing_data + result = await coordinator._async_update_data() + + assert result.get('current_weather').get('condition') == ATTR_CONDITION_CLOUDY + + assert result.get('animation').get('hint') == "This will remain unchanged" + + assert result.get('pollen') == {'foo': 'bar'}