test: add regression test for KEEP_ALL QoS with transient-local caching#3152
test: add regression test for KEEP_ALL QoS with transient-local caching#3152tonynajjar wants to merge 2 commits into
Conversation
Signed-off-by: Tony Najjar <tony.najjar@dexory.com>
|
@jmachowinski 👋 could you run CI on this PR to confirm that this test currently fails? The fix is here: ros2/rmw_cyclonedds#584 - after the first CI fail, is there a way to run CI with a specific commit of rmw_cyclonedds so we can confirm that it passes? If not then I'll just have to trust my local setup experimentation and merge the rmw_cyclonedds fix - then we run CI again to confirm that it passes |
fujitatomoya
left a comment
fujitatomoya
left a comment
There was a problem hiding this comment.
lgtm with green CI.
| // This test publishes before the subscriber exists (transient-local caching), | ||
| // then subscribes with KEEP_ALL + transient-local via EventsExecutor and | ||
| // verifies the cached message is delivered. | ||
| TEST_F(TestEventsExecutor, keep_all_transient_local_receives_cached_message) |
There was a problem hiding this comment.
(i think) although this test only runs on the default rnw implementation (or rmw implementation in user's configuration), nightly CI matrix (ci.ros2.org) does run per-RMW jobs to catch this failure.
|
Pulls: #3152, ros2/rmw_cyclonedds#584
|
|
Thanks @fujitatomoya. And one more detail, this was tested on Kilted and I know some work has been put since then on the EventsExecutor so there is a chance that we don't get a CI failure - but let's see |
|
|
CC: @skyegalaxy for expanding this to cover the CBGExecutor as well. |
3 participants
Description
Add a regression test for
EventsExecutorthat verifies transient-local cached messages are correctly delivered to subscribers using KEEP_ALL history QoS.This test covers the scenario where a publisher with
transient_local+reliable+depth=1publishes before the subscriber exists, and the subscriber (withKEEP_ALL+transient_local+reliableQoS) joins later usingEventsExecutor. The DDS cache should deliver the message to the late-joining subscriber.This pattern is exercised in production by rosbag2's
adapt_request_to_offers, which creates KEEP_ALL subscriptions when the publisher has depth=1 (e.g./tf_static).The test catches a bug in
rmw_cyclonedds_cppwherermw_subscription_set_on_new_message_callbackclips the unread count todepthviastd::min(unread_count, depth). For KEEP_ALL, depth is reported as 0, so the callback fires with 0 events and the executor never processes the cached messages. See ros2/rmw_cyclonedds#584Fixes # (issue)
Is this user-facing behavior change?
No — test-only change.
Did you use Generative AI?
Yes — GitHub Copilot (Claude Opus 4.6) was used to write the regression test.
Additional Information
rmw_cyclonedds_cppfix Fix KEEP_ALL history handling in subscription callback rmw_cyclonedds#584 — without that fix this test will fail whenRMW_IMPLEMENTATION=rmw_cyclonedds_cpp.rmw_zenoh_cpp(which does not have the depth-clipping bug).