fix: Failing service and subscription tests#1147
Merged
Merged
Conversation
bjsowa
changed the title
Contributor
There was a problem hiding this comment.
Pull request overview
Updates rosbridge_library tests and service-advertisement handling to align with recent rclpy coroutine/exception propagation behavior, fixing previously failing service and subscription-related tests.
Changes:
- Replace fixed sleeps in subscriber tests with polling assertions for subscribed/unsubscribed state.
- Update
test_unadvertise_with_live_requestto assert timeout behavior for in-flight requests and immediate rejection for subsequent calls. - Add error logging for exceptions while awaiting advertised-service responses and adjust service teardown to use
node_handle.destroy_service(...).
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| rosbridge_library/test/internal/subscribers/test_subscriber_manager.py | Adds polling-based subscription assertions and updates tests to use them instead of direct checks. |
| rosbridge_library/test/internal/subscribers/test_multi_subscriber.py | Adds the same polling-based assertions and updates multi-subscriber tests accordingly. |
| rosbridge_library/test/capabilities/test_service_capabilities.py | Adjusts service timeout behavior and updates unadvertise-with-inflight-request expectations. |
| rosbridge_library/src/rosbridge_library/capabilities/advertise_service.py | Logs exceptions while awaiting responses and changes service destruction mechanism during shutdown. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Member
Author
|
I've rerun the workflow 10 times, no more randomly failing tests... I hope. |
Member
Author
|
@MatthijsBurgh Could you approve the changes? |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
MatthijsBurgh
approved these changes
Feb 19, 2026
bjsowa
added a commit
that referenced
this pull request
Feb 19, 2026
* Add error logging when service is unadvertised during in-flight request * Fix service destruction method in AdvertisedServiceHandler * Fix unadvertise service test * Try to fix failing topic subscription assertions by adding a timeout * Remove redundant sleeps * Remove extra call to destroy service after graceful shutdown * Re-raise original exception in handle_request
MatthijsBurgh
pushed a commit
that referenced
this pull request
Feb 19, 2026
* Add error logging when service is unadvertised during in-flight request * Fix service destruction method in AdvertisedServiceHandler * Fix unadvertise service test * Try to fix failing topic subscription assertions by adding a timeout * Remove redundant sleeps * Remove extra call to destroy service after graceful shutdown * Re-raise original exception in handle_request
bjsowa
added a commit
that referenced
this pull request
Feb 21, 2026
bjsowa
added a commit
that referenced
this pull request
Feb 22, 2026
* Add error logging when service is unadvertised during in-flight request * Fix service destruction method in AdvertisedServiceHandler * Fix unadvertise service test * Try to fix failing topic subscription assertions by adding a timeout * Remove redundant sleeps * Remove extra call to destroy service after graceful shutdown * Re-raise original exception in handle_request
bjsowa
added a commit
that referenced
this pull request
Feb 22, 2026
* fix: Failing service and subscription tests (backport #1147) * Add error logging when service is unadvertised during in-flight request * Fix service destruction method in AdvertisedServiceHandler * Fix unadvertise service test * Try to fix failing topic subscription assertions by adding a timeout * Remove redundant sleeps * Remove extra call to destroy service after graceful shutdown * Re-raise original exception in handle_request * fix: Suppress RuntimeError during rclpy spin in service capabilities tests
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Public API Changes
None
Description
Recent changes related to coroutines in rclpy (ros2/rclpy#1469) fixed (at least partially) exception propagation. Executor no longer throws an exception when the service handler awaits a future that resulted in exception (this is a good thing). This, however, uncovered another problem with the test as it falsy expects to receive a message after sending
unadvertise_service.This PR fixes the test and adds a comment explaining current limitations of the implementation.
EDIT: Also modified the subscription asserts to add a timeout. This should hopefully fix the flaky tests.