Fix shelly device_trigger tests#169305
Conversation
|
Hey there @bieniu, @thecode, @chemelli74, @bdraco, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
| and hasattr(entry, "runtime_data") | ||
| and isinstance(entry.runtime_data, ShellyEntryData) |
There was a problem hiding this comment.
I could not find ANY path in which the entry would be loaded without the runtime_data being present.
There was a problem hiding this comment.
Pull request overview
This PR removes defensive runtime_data checks in Shelly coordinator lookup helpers and drops tests that exercised missing runtime_data scenarios.
Changes:
- Remove
hasattr(entry, "runtime_data")/isinstance(..., ShellyEntryData)guards when resolving Shelly coordinators bydevice_id. - Remove tests that monkeypatched Shelly config entries to delete
runtime_data.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
homeassistant/components/shelly/coordinator.py |
Simplifies coordinator lookup by directly accessing entry.runtime_data when entries are loaded. |
tests/components/shelly/test_device_trigger.py |
Removes tests covering triggers when runtime_data is forcibly removed from the config entry. |
Co-authored-by: Copilot <copilot@github.com>
| async def test_rpc_no_runtime_data( | ||
| hass: HomeAssistant, | ||
| device_registry: dr.DeviceRegistry, | ||
| service_calls: list[ServiceCall], | ||
| mock_rpc_device: Mock, | ||
| monkeypatch: pytest.MonkeyPatch, | ||
| ) -> None: | ||
| """Test the device trigger for the RPC device when there is no runtime_data in the entry.""" |
There was a problem hiding this comment.
PR marked as draft to let code-owners describe why these tests were added...
|
I am worried that we recreate the bug it fixed if we remove it, see details at #118805 |
I have read it now - but it doesn't explain how that is possible :( and I still don't understand how it can occur. I think it points to a bigger underlying issue. Anyway, I have reverted the code changes and only updated the tests to clean up after themselves. |
|
I don't understand why yet, but the changes in this PR remove 2 lines from the coverage After: Missing: core/homeassistant/components/shelly/coordinator.py Lines 210 to 211 in 6663717 We didn't have any specific test to cover it, but I am just wondering why this change remove the coverage for these lines. |
I wonder if maybe:
|
Since the changes in this PR make sense (and your explanation) lets proceed with it, I will look into adding another test to cover |
Proposed change
Linked to https://github.com/home-assistant/core/actions/runs/24982630534/job/73149424430?pr=162263
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: