Skip to content

Conversation

@allenporter
Copy link
Contributor

Update the test fixtures with some cleanups pulled out of a larger PR:

  • Be more careful when patching the event loop to avoid side effects in other tests given the running loop is a global variable
  • Update the mqtt fixture to wait to populate the response buffer until a client is connected. This is more correct, but also helps make the test buffer output more readable so requests and responses are in order
  • Add warnings for unconsumed responses and remove some unconsumed responses

These are improvements factored out of a large change to add more e2e tests for device manager.
Copilot AI review requested due to automatic review settings December 23, 2025 19:53
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 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.

@allenporter
Copy link
Contributor Author

This now contains a feat commit since I want to build a new release.

Copy link
Collaborator

@Lash-L Lash-L left a 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

@allenporter allenporter changed the title chore: Small tweaks to test fixtures fix: Small tweaks to test fixtures Dec 23, 2025
@allenporter allenporter changed the title fix: Small tweaks to test fixtures feat: Small tweaks to test fixtures Dec 23, 2025
@allenporter allenporter merged commit b9a241c into Python-roborock:main Dec 23, 2025
7 checks passed
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.

2 participants