Adding OpenWeatherMap Minute forecast action #128799
Conversation
![home-assistant[bot]](https://avatars.githubusercontent.com/in/97978?s=60&v=4){kind=small}
home-assistant
bot
added
cla-needed
Hacktoberfest
integration: openweathermap
new-feature
labels
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
|
Hey there @fabaff, @freekode, @nzapponi, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
home-assistant
bot
added
cla-recheck
cla-signed
and removed
cla-recheck
cla-needed
labels
| @@ -104,6 +111,14 @@ def _convert_weather_response(self, weather_report: WeatherReport): | |||
| ], | |||
| } | |||
|
|
|||
| def _get_minutely_weather_data(self, minutely_forecast): | |||
There was a problem hiding this comment.
Can we add typing to the function?
| for item in minutely_forecast | ||
| ] | ||
|
|
||
| return {f"{Platform.WEATHER}.{DOMAIN}": {"forecast": forecasts}} |
There was a problem hiding this comment.
Not sure how much this adds if its static
There was a problem hiding this comment.
Largely rewritten, based on the requirement for an OpenWeatherMap weather entity as action target.
| return round(precipitation["3h"], 2) | ||
| if "1h" in precipitation: | ||
| return round(precipitation["1h"], 2) | ||
| if precipitation is not None: |
There was a problem hiding this comment.
The pyopenweathermap library >0.2.0 seems to have introduced the possibility of None values being included for rain and snow: wind_gust=14.92, wind_bearing=140, rain=None, snow=None.
I'll raise, and possibly fix, the issue on pyopenweathermap.
Edit: rain and snow only appear in the response from the API if they are due / above zero, which appears to cause the None. Previously, they were empty dictionaries.
There was a problem hiding this comment.
Split into separate PR
| @@ -6,5 +6,5 @@ | |||
| "documentation": "https://www.home-assistant.io/integrations/openweathermap", | |||
| "iot_class": "cloud_polling", | |||
| "loggers": ["pyopenweathermap"], | |||
| "requirements": ["pyopenweathermap==0.1.1"] | |||
| "requirements": ["pyopenweathermap==0.2.1"] | |||
There was a problem hiding this comment.
Can we bump this in a separate preliminary PR?
There was a problem hiding this comment.
Will do, pending resolution of the comment above (None values) as it may require a new version.
There was a problem hiding this comment.
Split into separate PR
| if mode == OWM_MODE_V30: | ||
| hass.services.async_register( | ||
| domain=DOMAIN, | ||
| service=SERVICE_GET_MINUTELY_FORECAST, | ||
| service_func=handle_get_minutely_forecasts, | ||
| supports_response=SupportsResponse.ONLY, | ||
| ) | ||
| else: | ||
| hass.services.async_remove( | ||
| domain=DOMAIN, | ||
| service=SERVICE_GET_MINUTELY_FORECAST, | ||
| ) |
There was a problem hiding this comment.
Services should be registered in async_setup and should not be removed on runtime
There was a problem hiding this comment.
This is only called when the user makes a change to the integration configuration (Settings -> Devices & Services -> OpenWeatherMap -> Configure) and changes the forecast mode of the integration to a mode which does not support Minute forecasting. As such, this isn't really on runtime: it's only when the user reconfigures the integration without first deleting it.
If the service isn't removed when using an unsupported mode, the user will simply be returned an empty forecast - with no feedback as to the reason. I would argue that is worse UX.
There was a problem hiding this comment.
If the mode of the user is incorrect, you can raise a ServiceValidationError, explaining why it failed. More info about this can be found for now in https://github.com/home-assistant/developers.home-assistant/pull/2400/files
There was a problem hiding this comment.
Changed. Mode now checked on a per-call, not per registration, basis. ServiceValidationError raised if mode is not v3.0
| @@ -0,0 +1 @@ | |||
| get_minutely_forecasts: | |||
There was a problem hiding this comment.
Don't we need to add a location or anything? What if I have OWM setup twice?
There was a problem hiding this comment.
Very good point. Action now requires an OpenWeatherMap weather entity as a target, and location is derived from there.
Agreed. I had thought that it fitted neatly with "daily" and "hourly", but you're right: it does not mean "minute-by-minute". Will change. |
10100011
changed the title
| @@ -100,6 +115,17 @@ def __init__( | |||
| elif mode == OWM_MODE_FREE_FORECAST: | |||
| self._attr_supported_features = WeatherEntityFeature.FORECAST_HOURLY | |||
|
|
|||
| async def async_get_minute_forecast(self) -> None: | |||
There was a problem hiding this comment.
return typing is incorrect, we do return something
There was a problem hiding this comment.
Return typing corrected
| @pytest.fixture | ||
| def mock_hass(): | ||
| """Create a mock HomeAssistant instance.""" | ||
| return MagicMock(spec=HomeAssistant) | ||
|
|
||
|
|
||
| @pytest.fixture | ||
| def mock_coordinator(): | ||
| """Create a mock WeatherUpdateCoordinator.""" | ||
| coordinator = MagicMock(spec=WeatherUpdateCoordinator) | ||
| coordinator.data = MOCK_WEATHER_DATA | ||
| return coordinator | ||
|
|
||
|
|
||
| @pytest.fixture | ||
| def mock_config_entry(): | ||
| """Create a mock OpenweathermapConfigEntry.""" | ||
| config_entry = MagicMock() | ||
| config_entry.unique_id = MOCK_UNIQUE_ID | ||
| config_entry.runtime_data = MagicMock() | ||
| config_entry.runtime_data.name = MOCK_NAME | ||
| config_entry.runtime_data.mode = OWM_MODE_V30 | ||
| config_entry.runtime_data.coordinator = MagicMock(spec=WeatherUpdateCoordinator) | ||
| config_entry.runtime_data.coordinator.data = MOCK_WEATHER_DATA | ||
| return config_entry | ||
|
|
||
|
|
||
| @pytest.fixture | ||
| def mock_add_entities(): | ||
| """Create a mock AddEntitiesCallback.""" | ||
| return MagicMock(spec=AddEntitiesCallback) | ||
|
|
||
|
|
||
| async def test_async_get_minute_forecast(mock_coordinator) -> None: | ||
| """Test the async_get_minute_forecast method.""" | ||
| weather = OpenWeatherMapWeather( | ||
| name=MOCK_NAME, | ||
| unique_id=MOCK_UNIQUE_ID, | ||
| mode=OWM_MODE_V30, | ||
| weather_coordinator=mock_coordinator, | ||
| ) | ||
|
|
||
| # Test successful minute forecast retrieval | ||
| result = await weather.async_get_minute_forecast() | ||
| assert result == mock_coordinator.data[ATTR_API_MINUTE_FORECAST] | ||
|
|
||
| # Test exception when mode is not OWM_MODE_V30 | ||
| weather.mode = OWM_MODE_V25 | ||
| with pytest.raises(ServiceValidationError): | ||
| await weather.async_get_minute_forecast() |
There was a problem hiding this comment.
On testing: We don't want to make our own hass and stuff. We want to create a MockConfigEntry, and have Home Assistant setup the integration naturally. For that to work we need to patch the library so it returns what we want.
There was a problem hiding this comment.
I've rewritten this testing to make better use of MockConfigEntry, getting rid of MagicMock and so not creating our own hass.
As a result of other comments, I have also introduced end-to-end testing based on a fixture.
| def mock_weather_update(): | ||
| """Mock the WeatherUpdateCoordinator to prevent API calls.""" | ||
| with patch( | ||
| "homeassistant.components.openweathermap.coordinator.WeatherUpdateCoordinator._async_update_data", |
There was a problem hiding this comment.
We should not patch internals, we should only patch the external library.
There was a problem hiding this comment.
Good point. Patching is now limited to the HTTP call with the data provided by a static JSON file. This has had the added benefit of increasing the test coverage.
| # Test the async_get_minute_forecast service | ||
| # We can call the service and assert the result | ||
| with patch( | ||
| "homeassistant.components.openweathermap.weather.OpenWeatherMapWeather.async_get_minute_forecast", |
There was a problem hiding this comment.
Same reply as above. :)
frenck
added
the
smash
| # Test exception when mode is not OWM_MODE_V30 | ||
| hass.config_entries.async_update_entry( | ||
| mock_config_entry, | ||
| options={**mock_config_entry.options, CONF_MODE: OWM_MODE_V25}, | ||
| ) | ||
| await hass.config_entries.async_reload(mock_config_entry.entry_id) | ||
| await hass.async_block_till_done() | ||
|
|
||
| # Expect a ServiceValidationError when mode is not OWM_MODE_V30 | ||
| with pytest.raises(ServiceValidationError): | ||
| await hass.services.async_call( | ||
| DOMAIN, | ||
| SERVICE_GET_MINUTE_FORECAST, | ||
| {"entity_id": ENTITY_ID}, | ||
| blocking=True, | ||
| return_response=True, | ||
| ) | ||
|
|
||
| # Cleanup | ||
| await hass.config_entries.async_unload(mock_config_entry.entry_id) | ||
| await hass.async_block_till_done() |
There was a problem hiding this comment.
Let's have this be a separate test with its own mock config entry
| blocking=True, | ||
| return_response=True, | ||
| ) | ||
| assert result == MINUTE_FORECAST |
There was a problem hiding this comment.
You can use snapshot here
| assert result == MINUTE_FORECAST | |
| assert result == snapshot |
If you then execute pytest ./tests/components/openweathermap --snapshot-update it will generate a .ambr file which is all the data you asserted but then serialized
| entity_registry = er.async_get(hass) | ||
| entry = entity_registry.async_get(ENTITY_ID) | ||
| assert entry is not None |
There was a problem hiding this comment.
You can also check this by just doing assert hass.states.get(ENTITY_ID) is not None
| @patch( | ||
| "pyopenweathermap.http_client.HttpClient.request", | ||
| new_callable=AsyncMock, | ||
| return_value=json.loads(load_fixture("openweathermap.json", "openweathermap")), |
There was a problem hiding this comment.
Can we patch this at the place where the library is used? So when I look in the coordinator, self._owm_client.get_weather is called. So we want to patch that at homeassistant.components.openweathermap.OWMClient.get_weather and return what that function would return. This way we don't rely on the internals of the library which decreases coupling and increases code quality
| return_value=json.loads(load_fixture("openweathermap.json", "openweathermap")), | ||
| ) | ||
| async def test_minute_forecast( | ||
| mock_api_response, hass: HomeAssistant, mock_config_entry |
There was a problem hiding this comment.
please have hass as the first parameter
| ) | ||
|
|
||
|
|
||
| @pytest.mark.asyncio |
There was a problem hiding this comment.
| @pytest.mark.asyncio |
Not needed
Proposed change
This PR adds functionality to the OpenWeatherMap integration to expose Minute foreasts as an Action. Minute forecasts compliment Daily and Hourly forecasts by providing precipitation (rain or snow) levels for each minute of the next hour.
Instead of creating 60 separate sensors, or even 60 state attributes of a single sensor, this PR creates a new Service accessible as an OpenWeatherMap Action. It returns a response in the same format as
weather.get_forecasts, with 60 objects containing adatetimein UTC (starting now) andprecipitationin mm/h.Key changes:
async_get_minute_forecastinweather.pyminute_forecastdata incoordinator.py(no additional API data is requested)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: