test: isolate every integration launch test on its own DDS domain#551
test: isolate every integration launch test on its own DDS domain#551bburda wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes nondeterministic failures in parallel CTest runs by ensuring every launch_testing integration test executes on a unique DDS domain (ROS_DOMAIN_ID), preventing cross-test ROS graph discovery and message/fault contamination.
Changes:
- Assign per-test
ROS_DOMAIN_IDto alladd_launch_test()targets in affected packages viamedkit_set_test_domain(...). - Reallocate and tighten DDS domain ranges to remove overlaps between
ros2_medkit_integration_testsand previously carved-out package test ranges. - Update the central domain allocation table and integration-test docs to reflect the new bounds.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/ros2_medkit_fault_manager/CMakeLists.txt | Applies medkit_set_test_domain to all fault_manager launch tests to avoid parallel DDS collisions. |
| src/ros2_medkit_diagnostic_bridge/CMakeLists.txt | Ensures the diagnostic_bridge integration launch test runs with an allocated ROS_DOMAIN_ID. |
| src/ros2_medkit_fault_reporter/CMakeLists.txt | Introduces a dedicated domain range and assigns a domain to the fault_reporter launch test. |
| src/ros2_medkit_log_bridge/CMakeLists.txt | Moves the package’s test domain pool into the new 130–139 block and assigns a domain to the launch test. |
| src/ros2_medkit_action_status_bridge/CMakeLists.txt | Moves the package’s test domain pool into the new 130–139 block and assigns a domain to the launch test. |
| src/ros2_medkit_integration_tests/CMakeLists.txt | Caps the per-test domain pool at 140–219 to avoid overlapping other package allocations. |
| src/ros2_medkit_integration_tests/ros2_medkit_test_utils/constants.py | Updates documentation for the primary per-test DDS domain range to 140–219. |
| src/ros2_medkit_cmake/cmake/ROS2MedkitTestDomain.cmake | Updates the global DDS domain allocation table and clarifies the requirement to isolate add_launch_test() via medkit_set_test_domain. |
3859ed8 to
b9686b1
Compare
| # ros2_medkit_opcua: 220 - 229 (10 slots) | ||
| # multi-domain tests (secondary): 230 - 232 (3 slots, reserved for peer_aggregation etc.) | ||
| # | ||
| # Every launch_testing test (add_launch_test) must consume a domain via |
There was a problem hiding this comment.
This states the invariant as a MUST, but nothing enforces it - and it fails open. If someone adds an add_launch_test and forgets the matching medkit_set_test_domain, the test keeps ROS_DOMAIN_ID unset; constants.py resolves int(os.environ.get('ROS_DOMAIN_ID', '0')) to the shared default domain 0, and it silently cross-contaminates with every other domain-0 test - exactly the failure this PR removes, reintroduced with no configure-time or runtime signal. Worth making it fail closed: a CMake check that errors when a launch test has no assigned domain, or a constants.py fallback that raises instead of defaulting to 0 in the isolated-test path. As-is the whole scheme rests on author discipline.
| # 230-232 reserved for the multi-gateway secondary pool (see | ||
| # get_test_domain_id below). One domain is consumed per feature + scenario | ||
| # test. | ||
| medkit_init_test_domains(START 140 END 219) |
There was a problem hiding this comment.
Heads-up on headroom: this pool is at 74/80, and it's the only one that grows by glob (file(GLOB test/features/*.test.py)) - a new feature-test file consumes a domain with no CMake change. The whole 1-232 space is now carved (220 = opcua), so ~6 more feature files hit the final FATAL_ERROR and block the entire package build, with no contiguous range left to extend into. Meanwhile the fixed 10-slot pools (param_beacon, topic_beacon, graph_provider) each use 1 of 10. The tightest headroom is on the fastest-growing pool; opcua is also tight at 9/10. Consider reclaiming from the underused fixed pools and handing that slack to this one, or at least noting in the comment that integration_tests/opcua are near capacity so the next person knows a re-carve is coming.
The add_launch_test integration tests in fault_manager, diagnostic_bridge, fault_reporter, action_status_bridge and log_bridge did not set ROS_DOMAIN_ID, so they all ran on the default domain 0. Under parallel ctest they discovered each other's nodes: the log_bridge fault manager picked up a fault from the action_status_bridge test node, and the timing shift made test_01_error_log_creates_attributed_fault fail intermittently. Only ros2_medkit_integration_tests isolated its launch tests. Add a medkit_add_launch_test wrapper that registers a launch test and assigns it a domain in one call, and route every launch test through it, so a launch test can no longer be added without isolation. fault_reporter had no range at all, so give it one. Reallocate the domain table so the pools are disjoint and the glob-grown integration_tests pool gets headroom: reclaim the slack in the fixed beacon pools (param_beacon and topic_beacon each used 1 of 10, now 5 each), move fault_reporter/log_bridge/action_status_bridge into 110-119, and grow integration_tests to 130-219 (90 slots, 74 used). graph_provider (120-129) and opcua (220-229) are unchanged. Three tests also hardcoded a domain that overrode the injected one and bypassed the table. The fault_manager launch tests test_rosbag_integration and test_rosbag_entity_scope forced ROS_DOMAIN_ID=42/43, which collided with the gateway gtests assigned 42 and 43. The gateway test_fault_manager gtest forced 99 as a manual workaround for the fault_manager launch tests running on domain 0. Drop all three overrides so each test inherits its CMake-assigned domain; keep the ROS_LOCALHOST_ONLY settings. Closes #550
b9686b1 to
7c610cf
Compare
The integration test used a fixed 1s sleep to let the bridge connect, then published a single ERROR log in test_01. On a slow or loaded machine (seen on the lyrical CI job) the bridge had not matched the test's /rosout publisher within 1s, so the first log was dropped before the subscription existed and its fault never appeared - test_01 failed while the later tests passed because discovery had completed by then. Replace the fixed sleep with an active wait on the matched subscription count, the same pattern the diagnostic_bridge and action_status_bridge integration tests already use.
Pull Request
Summary
Give every
launch_testingintegration test a uniqueROS_DOMAIN_IDso parallel ctest runs cannot cross-contaminate, and make that invariant hard to break.Before this change, only
ros2_medkit_integration_testsisolated its launch tests. Theadd_launch_testtargets inros2_medkit_fault_manager,ros2_medkit_diagnostic_bridge,ros2_medkit_fault_reporter,ros2_medkit_action_status_bridgeandros2_medkit_log_bridgeset no domain, so they all ran on the default domain 0. Under parallel ctest they discovered each other's nodes. That is howtest_01_error_log_creates_attributed_faultinros2_medkit_log_bridgefailed: its fault manager picked upACTION_TEST_ACTION_ABORTEDfrom theros2_medkit_action_status_bridgetest node.Changes (commit 1):
medkit_add_launch_testwrapper that registers a launch test and assigns its domain in one call, and route every launch test through it, so a launch test can no longer be added without isolation.fault_reporterhad no range, so it gets one.integration_testspool gets headroom: reclaim the slack in the fixed beacon pools (param_beacon,topic_beaconeach used 1 of 10, now 5 each), movefault_reporter/log_bridge/action_status_bridgeinto 110-119, and growintegration_teststo 130-219 (90 slots, 74 used).graph_provider(120-129) andopcua(220-229) are unchanged.ROS_DOMAIN_IDoverrides that bypassed the table: thefault_managerrosbag launch tests forced 42/43 (which collided with the gateway gtests assigned 42/43), and the gatewaytest_fault_managergtest forced 99 as a manual workaround for the launch tests running on domain 0. Each test now inherits its assigned domain.Commit 2 fixes a separate flake in the same test that CI surfaced: the
log_bridgeintegration test used a fixed 1s sleep to let the bridge connect, then published a single ERROR log. On a slow machine the bridge had not matched the/rosoutpublisher within 1s, so the first log was dropped and its fault never appeared. It now waits on the matched subscription count instead, the same pattern the other bridge tests already use.Issue
Type
Testing
Built the affected packages and confirmed each launch test's ctest
ENVIRONMENTcarries a uniqueROS_DOMAIN_ID(fault_manager 15-18, diagnostic_bridge 91, fault_reporter 110, log_bridge 114, action_status_bridge 117, integration_tests 130-203).medkit_add_launch_testverified to assign the domain (fault_reporter test -> 110).Ran the two tests that were cross-contaminating (
log_bridgeandaction_status_bridge) at the same time; both pass together now. Ran the two rosbag tests and the gatewaytest_fault_manageron their assigned domains, concurrently with a realfault_manager_node, to confirm the removed hardcodes were not needed.CMake configure passes with no domain-range exhaustion.
Checklist