-
Notifications
You must be signed in to change notification settings - Fork 56
feat: Small tweaks to test fixtures #704
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
These are improvements factored out of a large change to add more e2e tests for device manager.
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 improves test fixture reliability and maintainability by cleaning up test setup code and making test behavior more predictable. The changes focus on preventing test side effects, improving test output readability, and adding better warnings for test hygiene issues.
Key changes:
- Extracted
get_running_loop()function in production code to enable more precise mocking without affecting global event loop state - Modified MQTT fixture to only populate response buffers after client connection, ensuring request/response ordering in test logs
- Added proper task cancellation handling and warnings for unconsumed test responses
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| roborock/devices/local_channel.py | Extracted get_running_loop() helper function to enable targeted mocking |
| tests/devices/test_local_channel.py | Updated patch target to use the new get_running_loop() function instead of global asyncio.get_running_loop |
| tests/fixtures/local_async_fixtures.py | Updated patch target to use the new get_running_loop() function instead of global asyncio.get_running_loop |
| tests/fixtures/mqtt.py | Added client_connected flag and gated response population to ensure proper ordering in test logs |
| tests/fixtures/pahomqtt_fixtures.py | Converted assertions to warnings for unconsumed responses and changed fixture to generator for cleanup |
| tests/fixtures/aiomqtt_fixtures.py | Added proper handling of CancelledError in polling task and ensured task is awaited after cancellation |
| tests/mqtt/test_roborock_session.py | Removed unconsumed MQTT responses that would now trigger warnings |
| tests/e2e/snapshots/test_mqtt_session.ambr | Updated snapshots to reflect reordered MQTT request/response logging (connack now appears after connect) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
977e181 to
82c648e
Compare
|
This now contains a feat commit since I want to build a new release. |
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.
looks good - just make sure that when you squash you change hte commit title, i don't think the descriptions will cause a bump
Update the test fixtures with some cleanups pulled out of a larger PR: