Reduce flakiness in rosbag2 recorder end-to-end tests#2370
Reduce flakiness in rosbag2 recorder end-to-end tests#2370MichaelOrlov wants to merge 1 commit intorollingfrom
Conversation
Reduced flakiness in rosbag2 recorder tests by strengthening PublicationManager::wait_for_matched(). The helper `PublicationManager::wait_for_matched()` now waits on ROS graph events instead of relying only on fixed sleep-polling. It still preserves the existing API and boolean return behavior, but uses a hybrid loop: • check the publisher’s current subscription count • wait for a graph change • re-check periodically with a short bounded wait slice This improves readiness detection for recorder subscriptions that appear asynchronously during topic discovery, while still handling cases where graph notifications are delayed or not sufficient on their own. No test call sites were changed. Existing recorder tests continue to use pub_manager.wait_for_matched(...), but they now benefit from the stronger synchronization automatically. Signed-off-by: Michael Orlov <morlovmr@gmail.com>
|
Pulls: #2370
|
fujitatomoya
left a comment
fujitatomoya
left a comment
There was a problem hiding this comment.
lgtm, just a minor comment which does not block this PR.
| } while ((clock::now() - start) < timeout); | ||
|
|
||
| return false; | ||
| auto remaining = std::chrono::duration_cast<std::chrono::nanoseconds>(deadline - |
There was a problem hiding this comment.
Between the while (clock::now() < deadline) check at the top of the loop and the deadline - clock::now() computation a few lines later, time passes. If the deadline is hit in that narrow window, remaining becomes negative, which when cast to std::chrono::nanoseconds (a signed type) would be a negative duration.
probably we would want to add if (remaining <= std::chrono::nanoseconds::zero()) break;?
3 participants
Description
Reduced flakiness in rosbag2 recorder tests by strengthening PublicationManager::wait_for_matched().
Please see RCA in the relevant issue #2369 (comment)
The helper
PublicationManager::wait_for_matched()now waits on ROS graph events rather than relying solely on fixed sleep polling. It still preserves the existing API and boolean return behavior, but uses a hybrid loop:• check the publisher’s current subscription count
• wait for a graph change
• re-check periodically with a short bounded wait slice
This improves readiness detection for recorder subscriptions that appear asynchronously during topic discovery, while still handling cases where graph notifications are delayed or not sufficient on their own.
No test call sites were changed. Existing recorder tests continue to use pub_manager.wait_for_matched(...), but they now benefit from the stronger synchronization automatically.
Is this user-facing behavior change?
No.
Did you use Generative AI?
Yes. Codex gpt-5.4
Additional Information
Can be backported.