New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix some handle leaks in rainforest_raven #113035
Conversation
There were leaks when * The component was shutdown * There was a timeout during the initial device opening Additionally, the device was not closed/reopened when there was a timeout reading regular data.
I'd like this bugfix to be considered for release as part of the next 2024.3 patch. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please address the comments in a new PR. Thanks!
async def test_coordinator_device_timeout_update(hass: HomeAssistant, mock_device): | ||
"""Test handling of a device timeout during an update.""" | ||
entry = create_mock_entry() | ||
coordinator = RAVEnDataCoordinator(hass, entry) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Set up the integration instead. We shouldn't interact with the coordinator directly as that's an integration detail.
https://developers.home-assistant.io/docs/development_testing#writing-tests-for-integrations
await coordinator.async_config_entry_first_refresh() | ||
assert coordinator.last_update_success is True | ||
|
||
mock_device.get_network_info.side_effect = functools.partial(asyncio.sleep, 10) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Besides this, I'd make the timeout time a constant so we can easily patch it to 0. We don't want slow tests.
assert coordinator.last_update_success is True | ||
|
||
mock_device.get_network_info.side_effect = functools.partial(asyncio.sleep, 10) | ||
await coordinator.async_refresh() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move time forward instead to trigger a refresh. See other integrations that use a coordinator with tests for examples.
|
||
mock_device.get_network_info.side_effect = functools.partial(asyncio.sleep, 10) | ||
await coordinator.async_refresh() | ||
assert coordinator.last_update_success is False |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should assert that the mock device is closed. We can also assert that a coordinator entity have its state set to unavailable.
There were leaks when * The component was shutdown * There was a timeout during the initial device opening Additionally, the device was not closed/reopened when there was a timeout reading regular data.
There were leaks when * The component was shutdown * There was a timeout during the initial device opening Additionally, the device was not closed/reopened when there was a timeout reading regular data.
Proposed change
There were file handle leaks when:
Additionally, the device was not closed/reopened when there was a timeout reading regular data.
A test was added to exercise the timeout code. Note that this test actually does wait for the timeout to trigger normally, and therefore takes 5 seconds to complete. If this is not acceptable, I can update the test to explicitly raise a
TimeoutError
immediately.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
..coveragerc
.To help with the load of incoming pull requests: