Skip to content

Conversation

@allenporter
Copy link
Contributor

@allenporter allenporter commented Dec 24, 2025

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

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.
Copilot AI review requested due to automatic review settings December 24, 2025 17:24
@allenporter allenporter marked this pull request as draft December 24, 2025 17:24
Copy link
Contributor

Copilot AI left a 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_TIMEOUT from 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.

Comment on lines +1 to +8
"""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.
Copy link

Copilot AI Dec 24, 2025

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.

Suggested change
"""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.

Copilot uses AI. Check for mistakes.
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
Copy link

Copilot AI Dec 24, 2025

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.

Suggested change
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

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant