-
-
Notifications
You must be signed in to change notification settings - Fork 36.1k
Ensure Roborock disconnects mqtt on unload/stop #158144
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
Conversation
This simplifies the shutdown logic relying on the device manager to close everything. This adds tests that the device manager is closed on unload and stop.
|
Hey there @Lash-L, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
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.
Pull request overview
This PR simplifies the Roborock integration's shutdown logic by centralizing cleanup in the device manager's close() method, which handles both local device connections and shared MQTT sessions. Previously, the integration manually managed device closing and coordinator shutdown separately.
Key changes:
- Centralized shutdown logic using
device_manager.close()instead of individual device cleanup - Removed redundant coordinator shutdown code (coordinators auto-register cleanup via their base class)
- Added comprehensive test coverage for both config entry unload and Home Assistant stop events
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
homeassistant/components/roborock/__init__.py |
Simplified shutdown by registering shutdown_roborock() to call device_manager.close() and cache.flush() on both unload and HA stop events; removed redundant per-device and per-coordinator cleanup code |
tests/components/roborock/test_init.py |
Added two new tests verifying device manager closure on config entry unload and Home Assistant stop; added device_manager fixture parameter to test functions |
tests/components/roborock/conftest.py |
Replaced custom FakeDeviceManager class with proper AsyncMock of DeviceManager; created dedicated device_manager fixture that properly mocks get_devices() and close() methods |
Co-authored-by: Copilot <[email protected]>
Lash-L
left a comment
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.
Tested and confirmed working
|
Code seems to follow previous patterns and seems to simplify a lot of the previous code. Tests have passed. Good work. |
This simplifies the shutdown logic relying on the device manager to close everything. This adds tests that the device manager is closed on unload and stop.
Breaking change
Rewrite the roborock shutdown code so that it register unload right after the device manager is created. the
closemethod will close all the device local connections and the shared MQTT sessions. Add tests to cover both the unload and shutdown cases with a single function.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: