Yardian: Add sensors #153020
Yardian: Add sensors #153020
Conversation
|
Hey there @h3l1o5, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
|
Putting to draft as the comments in the binary sensor PR also apply here. But feel free to just do one at a time if you're not sure if you understood what I was saying, otherwise you'd waste too much time trying to get both PRs ready only to update them even more later :) |
Thanks! I've made similar changes here - let me know what you think |
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
|
Double checking, you seem to have resolved the unique id suffix discussions but didn't change any code related to that, mind elaborating so I understand your opinion on that suggestion? |
|
Be sure to take a look at the binary sensor tests and the switch ones, I think you can make great tests for this :) |
barneyonline
force-pushed
the
yardian-sensors-expansion
branch
from
7d47885 to
13ceff0
Compare
Apologies I overrode the change - have now updated the sensor unique IDs to use the entity key suffix (yid_) instead of custom strings |
|
|
||
| try: | ||
| return max(0, int(val)) | ||
| except (TypeError, ValueError): |
There was a problem hiding this comment.
why do we catch type errors or value errors? as in, that would only be the case if val is not a value that is a stringified int. But do we actually expect this? Or are these just safeguards for the sake of it? Because in case of the latter I would just remove them as it's obsolete
There was a problem hiding this comment.
This wasn't answered. As in, I am not saying it's wrong, I am trying to understand why we do this :)
| try: | ||
| delay = int(val) | ||
| except (TypeError, ValueError): | ||
| return None |
| from homeassistant.config_entries import ConfigEntry | ||
| from homeassistant.core import HomeAssistant | ||
| from homeassistant.exceptions import ConfigEntryError | ||
| from homeassistant.exceptions import ConfigEntryAuthFailed |
There was a problem hiding this comment.
| from homeassistant.exceptions import ConfigEntryAuthFailed | |
| from homeassistant.exceptions import ConfigEntryError |
| raise UpdateFailed("Timeout communicating with device") from e | ||
| except NotAuthorizedException as e: | ||
| raise ConfigEntryError("Invalid access token") from e | ||
| raise ConfigEntryAuthFailed("Invalid access token") from e |
There was a problem hiding this comment.
| raise ConfigEntryAuthFailed("Invalid access token") from e | |
| raise ConfigEntryError("Invalid access token") from e |
The yardian integration doesn't have a reauth flow yet, so we should keep raising error (next to the fact that this isn't in the scope of this PR)
|
|
||
| try: | ||
| return max(0, int(val)) | ||
| except (TypeError, ValueError): |
There was a problem hiding this comment.
This wasn't answered. As in, I am not saying it's wrong, I am trying to understand why we do this :)
| from .const import DOMAIN | ||
| from .coordinator import YardianUpdateCoordinator | ||
|
|
||
| _OPER_INFO_ABSOLUTE_THRESHOLD = 365 * 24 * 3600 |
There was a problem hiding this comment.
I guess we can add a comment mentioning what this value is
| client.start_irrigation = AsyncMock() | ||
| client.stop_irrigation = AsyncMock() |
There was a problem hiding this comment.
With autospec set to True, we don't have to explicitly set this
| mock_yardian_client: object, | ||
| ) -> None: | ||
| """Diagnostic sensors are disabled by default but report values when enabled.""" | ||
|
|
||
| with patch("homeassistant.components.yardian.PLATFORMS", [Platform.SENSOR]): | ||
| await setup_integration(hass, mock_config_entry) | ||
|
|
||
| ent_reg = er.async_get(hass) |
There was a problem hiding this comment.
| mock_yardian_client: object, | |
| ) -> None: | |
| """Diagnostic sensors are disabled by default but report values when enabled.""" | |
| with patch("homeassistant.components.yardian.PLATFORMS", [Platform.SENSOR]): | |
| await setup_integration(hass, mock_config_entry) | |
| ent_reg = er.async_get(hass) | |
| mock_yardian_client: object, | |
| entity_registry: er.EntityRegistry, | |
| ) -> None: | |
| """Diagnostic sensors are disabled by default but report values when enabled.""" | |
| with patch("homeassistant.components.yardian.PLATFORMS", [Platform.SENSOR]): | |
| await setup_integration(hass, mock_config_entry) |
It can just be loaded as test fixture
| def _set_oper_info( | ||
| coordinator: YardianUpdateCoordinator, updates: Mapping[str, object] | ||
| ) -> None: | ||
| """Update coordinator data with new oper_info values.""" | ||
| device_state = coordinator.data | ||
| assert device_state is not None | ||
| oper_info_source = getattr(device_state, "oper_info", {}) | ||
| try: | ||
| oper_info = dict(oper_info_source) | ||
| except TypeError: | ||
| oper_info = {} | ||
| oper_info.update(updates) | ||
| coordinator.async_set_updated_data( | ||
| YardianCombinedState( | ||
| zones=device_state.zones, | ||
| active_zones=device_state.active_zones, | ||
| oper_info=oper_info, | ||
| ) | ||
| ) |
There was a problem hiding this comment.
Not quite. We should not touch internals like the coordinator in hass.data or entry.runtime_data.
Instead we should just update the return value of the mock, and then use freezer to skip time (which would then trigger the update "normally" as it would in real life as time passed by
| async def test_rain_delay_value_handles_invalid_data( | ||
| hass: HomeAssistant, | ||
| mock_config_entry: MockConfigEntry, | ||
| mock_yardian_client: object, | ||
| ) -> None: | ||
| """Rain delay sensor converts integers and ignores invalid data.""" | ||
| coordinator = await _prepare_coordinator(hass, mock_config_entry) | ||
|
|
||
| assert _rain_delay_value(coordinator) == 3600 | ||
|
|
||
| _set_oper_info(coordinator, {"iRainDelay": "bad-value"}) | ||
| assert _rain_delay_value(coordinator) is None | ||
|
|
||
| _set_oper_info(coordinator, {"iRainDelay": None}) | ||
| assert _rain_delay_value(coordinator) is None |
There was a problem hiding this comment.
And for this test, to continue on my last comment, I don't really care that _rain_delay_value return a certain value, I want to see how that value is used. afaik it's a state, so have HA setup the integration and then just look at the state of the sensor instead.
| async def test_zone_delay_value_interprets_timestamp_and_errors( | ||
| hass: HomeAssistant, | ||
| mock_config_entry: MockConfigEntry, | ||
| mock_yardian_client: object, | ||
| ) -> None: | ||
| """Zone delay interprets timestamps and guards against invalid values.""" | ||
| coordinator = await _prepare_coordinator(hass, mock_config_entry) | ||
|
|
||
| assert _zone_delay_value(coordinator) == 5 | ||
|
|
||
| frozen_now = datetime(2024, 1, 1, tzinfo=UTC) | ||
| with patch( | ||
| "homeassistant.components.yardian.sensor.dt_util.utcnow", | ||
| return_value=frozen_now, | ||
| ): | ||
| absolute_delay = int(frozen_now.timestamp()) + 120 | ||
| _set_oper_info(coordinator, {"iSensorDelay": absolute_delay}) | ||
| assert _zone_delay_value(coordinator) == 120 | ||
|
|
||
| _set_oper_info(coordinator, {"iSensorDelay": "invalid"}) | ||
| assert _zone_delay_value(coordinator) is None | ||
|
|
||
| _set_oper_info(coordinator, {"iSensorDelay": None}) | ||
| assert _zone_delay_value(coordinator) is None |
There was a problem hiding this comment.
You want to use the freezer as well here
| async def test_water_hammer_duration_handles_invalid_data( | ||
| hass: HomeAssistant, | ||
| mock_config_entry: MockConfigEntry, | ||
| mock_yardian_client: object, | ||
| ) -> None: | ||
| """Water hammer duration converts integers and ignores invalid data.""" | ||
| coordinator = await _prepare_coordinator(hass, mock_config_entry) | ||
|
|
||
| assert _water_hammer_duration_value(coordinator) == 2 | ||
|
|
||
| _set_oper_info(coordinator, {"iWaterHammerDuration": "oops"}) | ||
| assert _water_hammer_duration_value(coordinator) is None | ||
|
|
||
| _set_oper_info(coordinator, {"iWaterHammerDuration": None}) | ||
| assert _water_hammer_duration_value(coordinator) is None |
There was a problem hiding this comment.
So to repeat my previous question, is this a real life scenario or is this just being cautious?
3 participants
Proposed change
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: