-
Notifications
You must be signed in to change notification settings - Fork 56
fix: Update device startup connection behavior #708
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
base: main
Are you sure you want to change the base?
Conversation
Use the existing test to also exercise the cache by closing the device manager and reconnecting.
Increase the timeout waiting for the initial device startup to happen to wait beyond the L01 fallback behavior. Before this chnage, the device returns and is not yet conected locally (the test fails) forcing MQTT requests.
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 fixes device startup connection behavior by increasing the timeout for initial device startup from 5 to 15 seconds. This allows sufficient time for the complete connection flow including network info fetch, V1 protocol hello attempt, and potential L01 fallback before the device manager returns.
Key Changes
- Increased
START_ATTEMPT_TIMEOUTfrom 5 to 15 seconds to accommodate L01 fallback behavior - Added comprehensive end-to-end tests for device manager with both V1 and L01 devices
- Added v3 API endpoint mock for user homes to support test scenarios
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
roborock/devices/device.py |
Increased startup timeout from 5 to 15 seconds with explanatory comment |
tests/e2e/test_device_manager.py |
New end-to-end tests for V1 and L01 device connection flows |
tests/e2e/__snapshots__/test_device_manager.ambr |
Snapshot file for new end-to-end tests |
tests/e2e/__init__.py |
Added web_api_fixtures to pytest plugins list |
tests/fixtures/web_api_fixtures.py |
Added v3/user/homes endpoint mock |
.pre-commit-config.yaml |
Excluded .ambr snapshot files from codespell checks |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| """End-to-end tests for MQTT session. | ||
|
|
||
| These tests use a fake MQTT broker to verify the session implementation. We | ||
| mock out the lower level socket connections to simulate a broker which gets us | ||
| close to an "end to end" test without needing an actual MQTT broker server. | ||
|
|
||
| These are higher level tests that the similar tests in tests/mqtt/test_roborock_session.py | ||
| which use mocks to verify specific behaviors. |
Copilot
AI
Dec 24, 2025
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.
The module docstring describes "MQTT session" but the module is named "test_device_manager" and tests the DeviceManager. The docstring should be updated to accurately describe that this module contains end-to-end tests for the DeviceManager, not just MQTT session tests.
| """End-to-end tests for MQTT session. | |
| These tests use a fake MQTT broker to verify the session implementation. We | |
| mock out the lower level socket connections to simulate a broker which gets us | |
| close to an "end to end" test without needing an actual MQTT broker server. | |
| These are higher level tests that the similar tests in tests/mqtt/test_roborock_session.py | |
| which use mocks to verify specific behaviors. | |
| """End-to-end tests for the DeviceManager. | |
| These tests use a fake MQTT broker and mocked local connections to verify the | |
| DeviceManager's behavior. We mock out the lower level socket connections to | |
| simulate a broker, which gets us close to an "end to end" test without needing | |
| an actual MQTT broker server. | |
| These are higher level tests than the similar tests in tests/mqtt/test_roborock_session.py, | |
| which use mocks to verify specific behaviors in isolation. |
| mock out the lower level socket connections to simulate a broker which gets us | ||
| close to an "end to end" test without needing an actual MQTT broker server. | ||
|
|
||
| These are higher level tests that the similar tests in tests/mqtt/test_roborock_session.py |
Copilot
AI
Dec 24, 2025
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.
The phrase "that the similar tests" is grammatically incorrect. It should be "than the similar tests" to properly make the comparison.
| These are higher level tests that the similar tests in tests/mqtt/test_roborock_session.py | |
| These are higher level tests than the similar tests in tests/mqtt/test_roborock_session.py |
Increase the timeout waiting for the initial device startup to happen to wait beyond the L01 fallback behavior. Before this change, the device returns and is not yet connected locally (the test fails) forcing MQTT requests.
Currently in draft as it depends on #705
Addresses the symptoms in home-assistant/core#159609