Refactor the gateway integration test suite into a dedicated ros2_medkit_integration_tests package#227
Refactor the gateway integration test suite into a dedicated ros2_medkit_integration_tests package#227
ros2_medkit_integration_tests package#227Conversation
There was a problem hiding this comment.
Pull request overview
This pull request extracts integration tests and demo nodes from ros2_medkit_gateway into a new dedicated package ros2_medkit_integration_tests. The refactoring improves test organization by separating integration tests into two categories: scenario tests (end-to-end stories) and feature tests (atomic, independent tests). A new ros2_medkit_test_utils Python package provides shared utilities including launch helpers, base test classes, and constants.
Changes:
- Created new
ros2_medkit_integration_testspackage with demo nodes, test utilities, and reorganized tests - Moved 9 demo node C++ files from
ros2_medkit_gateway/test/demo_nodes/to the new package - Split monolithic integration tests into 23 smaller test files organized by scenario vs. feature
- Removed integration test dependencies from
ros2_medkit_gatewaypackage.xml and CMakeLists.txt - Updated launch files and test references to use new package names
Reviewed changes
Copilot reviewed 39 out of 49 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/ros2_medkit_integration_tests/package.xml | New package metadata with test and demo node dependencies |
| src/ros2_medkit_integration_tests/CMakeLists.txt | Build config for demo nodes and test registration with proper timeouts |
| src/ros2_medkit_integration_tests/setup.cfg | Python package setup for test utilities |
| src/ros2_medkit_integration_tests/ros2_medkit_test_utils/* | Shared test utilities: constants, coverage helpers, launch factories, base test class |
| src/ros2_medkit_integration_tests/test/scenarios/* | 9 scenario test files (end-to-end stories, 300s timeout) |
| src/ros2_medkit_integration_tests/test/features/* | 14 feature test files (atomic tests, 120s timeout) |
| src/ros2_medkit_integration_tests/demo_nodes/* | 9 demo node C++ files moved from gateway package |
| src/ros2_medkit_integration_tests/launch/demo_nodes.launch.py | Launch file updated to reference new package |
| src/ros2_medkit_gateway/package.xml | Removed test dependencies (pytest, launch_testing, etc.) |
| src/ros2_medkit_gateway/CMakeLists.txt | Removed integration tests and demo node build targets |
bburda
changed the title
ros2_medkit_integration_tests package
bburda
force-pushed
the
feat/improve-integration-tests
branch
from
ae470cf to
8050710
Compare
bburda
force-pushed
the
feat/improve-integration-tests
branch
from
8050710 to
86cd8f4
Compare
Four modules replacing ~1200 lines of duplicated code: - constants.py: API paths, ports, timeouts - coverage.py: unified get_coverage_env() - launch_helpers.py: factory for gateway/demo/fault_manager nodes - gateway_test_case.py: base class with health polling, HTTP helpers, wait helpers, and discovery assertions Refs #139, #222
Replaces test_discovery_manifest and test_discovery_hybrid from gateway package with scenario-style tests using shared assertion helpers. Each scenario validates entity structure, capabilities, and mode-specific behavior. Refs #139
Migrated from gateway package with scenario-style structure. Using shared test_utils for launch helpers and base test case. Refs #139
- Add 'calibration_service' and 'long_calibration_action' aliases to DEMO_NODE_REGISTRY (fixes KeyError in test_entity_routing and test_operations_api) - Fix camelCase 'faultCode' -> snake_case 'fault_code' in test_scenario_fault_lifecycle to match actual API response - Replace strict assertExitCodes with SIGTERM-tolerant exit code check across all 24 test files (exit code -15 is expected during launch_testing shutdown) - Add polling loop in test_scenario_discovery_hybrid test_15 to wait for runtime linking (apps become online asynchronously after nodes start)
- long_calibration_action: replace detached thread with joinable thread and atomic shutdown flag; add try-catch for goal handle interactions; use set_terminate to handle rclcpp_action race during SIGINT shutdown - All timer-based demo nodes: cancel timers in destructor to prevent callbacks firing during node destruction (fixes SIGSEGV on Humble) - Revert gateway-only exit code guard: all processes must exit cleanly - Fix test_discovery_heuristic exit code check (was silently passing)
…n tests package - Add README.md with package structure, test templates, and GatewayTestCase API - Add design/index.rst with PlantUML architecture diagram and test catalog - Add symlink and toctree entry in docs/design/ - Add package description in docs/introduction.rst - Fix demo_nodes.launch.py package reference in getting_started.rst - Update devcontainer.rst test filtering for new package structure
- Add REQUIRED_APPS to test_hateoas discovery wait so temp_sensor is guaranteed to be discovered before tests run (fixes 404 on all platforms) - Increase fault_manager test timeout from 1s to 3s for Humble's slower DDS service discovery - Accept either "not available" or "timed out" error in service availability tests (wait_for_service behavior varies across distros) - Exclude vendored/ from coverage in both codecov.yml and CI lcov filter
On Humble (CycloneDDS), reusing the same node name across sequential GTest cases causes stale DDS participant discovery state. This corrupts service responses in the second test when the first test did not create a service. Using unique node names per test eliminates the collision. Fixes test_fault_manager on Humble CI (GetSnapshotsSuccessWithValidJson).
bburda
force-pushed
the
feat/improve-integration-tests
branch
from
86cd8f4 to
f0d0c3f
Compare
… race Add poll_endpoint() and poll_endpoint_until() to the base test case, replacing 8 duplicated polling helpers across 7 test files. The predicate-based poll_endpoint_until() supports both boolean checks and value extraction from response JSON. Fixes test_app_no_topics flake on Jazzy where the calibration service-only node transiently disappeared from the ROS 2 graph between the entity readiness check and the data request.
| f'{self.LIDAR_ENDPOINT}/faults', | ||
| lambda d: next( | ||
| ( | ||
| item.get('faultCode') |
There was a problem hiding this comment.
Bug: The poll condition uses faultCode (camelCase) but the listing API returns fault_code (snake_case) - confirmed in fault_manager.cpp:61. This means the condition never matches, poll_endpoint_until times out, and all 6 tests in this file silently skip via skip_on_timeout=True.
# Current (never matches):
item.get('faultCode')
# Should be:
item.get('fault_code')| <depend>ros2_medkit_msgs</depend> | ||
|
|
||
| <!-- Test dependencies (Python integration tests) --> | ||
| <test_depend>launch_testing_ament_cmake</test_depend> |
There was a problem hiding this comment.
Missing a few test dependencies that could bite on minimal build environments:
<test_depend>launch_testing</test_depend>
<test_depend>launch_ros</test_depend>
<test_depend>ament_index_python</test_depend>launch_testing_ament_cmake is the CMake glue, but the Python packages (launch_testing, launch_ros) and ament_index_python (used in coverage.py) are separate.
| time.sleep(DISCOVERY_INTERVAL) | ||
|
|
||
| # Deadline reached -- warn but continue; individual tests may fail. | ||
| print('Warning: Discovery timeout, some tests may fail') |
There was a problem hiding this comment.
_wait_for_discovery() only prints a warning on timeout, while _wait_for_gateway_health() raises SkipTest. This asymmetry means a slow discovery causes a wall of confusing cascading failures instead of a clean skip. Would it make sense to align these?
# Current:
print('Warning: Discovery timeout, some tests may fail')
# Suggested:
raise unittest.SkipTest(
f'Discovery incomplete after {DISCOVERY_TIMEOUT}s — '
f'found {len(discovered_apps)} apps, need {cls.MIN_EXPECTED_APPS}'
)| def test_16_app_has_runtime_topics(self): | ||
| """Online app has topics from runtime discovery.""" | ||
| # Wait a bit for runtime linking | ||
| time.sleep(3) |
There was a problem hiding this comment.
These tests (16-17, 22-23) wrap assertions in if response.status_code == 200: guards, which means they pass silently when the endpoint isn't ready. Compare with test_15 which correctly uses poll_endpoint_until(). Could we do the same here?
# Current (silently passes without asserting anything):
response = requests.get(f'{self.BASE_URL}/apps/engine-temp-sensor/data', timeout=5)
if response.status_code == 200:
data = response.json()
if 'items' in data and data['items']:
# ...assert...
# Suggested — poll until data arrives, then assert unconditionally:
data = self.poll_endpoint_until(
'/apps/engine-temp-sensor/data',
lambda d: d.get('items'),
timeout=15.0,
)
topic_names = [t.get('name', '') for t in data]
self.assertTrue(any('temperature' in name for name in topic_names))Same pattern in test_scenario_discovery_manifest.test.py:364 (test_25_discovery_stats).
| Online status depends on runtime linking timing. | ||
| """ | ||
| # Wait a bit for runtime linking | ||
| time.sleep(5) |
There was a problem hiding this comment.
Bare time.sleep(5) instead of polling. Flaky on slow CI, wasteful on fast machines. The hybrid test_15 already uses poll_endpoint_until() correctly - could this follow the same pattern?
Same issue in test_scenario_discovery_hybrid.test.py:298 (time.sleep(3)).
| # Wait for data availability on the discovered app | ||
| # Skip ROS 2 system topics that don't have continuous data flow | ||
| system_topics = {'/parameter_events', '/rosout'} | ||
| deadline = time.time() + 15.0 |
There was a problem hiding this comment.
Minor: time.time() here while the base class consistently uses time.monotonic() (immune to NTP clock adjustments). Worth aligning for consistency.
| @@ -223,16 +181,15 @@ def test_01_create_subscription_returns_201_with_correct_schema(self): | |||
| self._delete_subscription(sub['id']) | |||
There was a problem hiding this comment.
If an assertion fails before _delete_subscription(), the subscription leaks and can affect subsequent tests. addCleanup() would guarantee cleanup regardless:
sub = self._create_subscription()
self.addCleanup(self._delete_subscription, sub['id'])(I noticed addCleanup / tearDown aren't used anywhere in the package currently.)
|
|
||
| """ | ||
|
|
||
| PORT = DEFAULT_PORT |
There was a problem hiding this comment.
Nit: PORT class attribute is never read by any test (confirmed with grep). Subclasses override BASE_URL directly. Could just remove PORT to avoid confusion.
| self.assertIn('capabilities', data) | ||
|
|
||
| self.assertEqual(data['name'], 'ROS 2 Medkit Gateway') | ||
| self.assertEqual(data['version'], '0.1.0') |
There was a problem hiding this comment.
assertEqual(data['version'], '0.1.0') will break on every version bump. A format check (e.g. semver regex) would be more resilient.
| self.assertEqual(verify_data['x-medkit']['parameter']['value'], 80.0) | ||
|
|
||
| # Reset the value back to default using SOVD "data" field | ||
| requests.put( |
There was a problem hiding this comment.
If the assertion fails before this reset PUT, min_temp stays at 80.0. addCleanup() would guarantee the reset regardless of test outcome.
|
Stale references The launch file moved but a few references still point to the old package:
All say |
| def setUpClass(cls): | ||
| """Wait for gateway to be ready and get tokens.""" | ||
| cls._wait_for_gateway_health() | ||
| time.sleep(1) |
There was a problem hiding this comment.
If the auth endpoint isn't ready, response.status_code != 200 and the token is never stored. Then _auth_header() returns Bearer "" and auth tests fail with misleading errors. Maybe add a check after the loop?
missing = [r for r in ('admin', 'operator', 'viewer', 'configurator') if r not in cls.tokens]
if missing:
raise unittest.SkipTest(f'Could not acquire tokens for: {", ".join(missing)}')2 participants
Pull Request
Summary
Refactor the gateway integration test suite into a dedicated
ros2_medkit_integration_testspackage with a shared Python test library (ros2_medkit_test_utils), two-tier test organization (features + scenarios), CMake labels, and structural fixes for flaky tests.What changed:
ros2_medkit_integration_tests- contains all integration tests, demo nodes, launch files, config YAMLs, and a shared Python test libraryros2_medkit_test_utils(4 modules) - eliminates ~1,200 lines of duplicated code:get_coverage_env()(9 copies), health polling (9 variants), demo node launch boilerplate, HTTP helperstest_integration.test.py(4,990 lines, 141 tests) split into 16 feature test files (~90 tests) and 10 scenario test files (~50 tests) - each file launches its own gateway with only the demo nodes it needsintegration;feature,integration;scenario) enablecolcon test --ctest-args -L featurefilteringKey library modules:
constants.py- shared API paths, ports, timeouts (replaces 50+ duplicated lines)coverage.py- singleget_coverage_env()(replaces 9 copies, ~270 lines)launch_helpers.py-create_test_launch()factory (replaces ~500 lines of inline launch boilerplate)gateway_test_case.py-GatewayTestCasebase class with health polling, HTTP helpers, discovery waiters, assertion helpersIssue
Type
Testing
Build verification:
Gateway unit tests (unchanged):
Integration test registration:
Run integration tests:
colcon test --packages-select ros2_medkit_integration_tests colcon test-result --verboseCoverage still works from separate package -
get_coverage_env()resolvesros2_medkit_gateway's build dir viaament_index_python, gateway binary writes.gcdatoGCOV_PREFIX, CIlcov --capture --directory buildscans the entire build directory.Issue #222 (flaky test_101): The race condition is resolved structurally -
test_scenario_action_lifecycle.test.pyreplaces the problematic test_100 + test_101 sequence. Each test creates its own execution (no concurrent action goals from previous tests).Issue #139 (multi-discovery-mode): Two new scenario files (
test_scenario_discovery_manifest.test.py,test_scenario_discovery_hybrid.test.py) test manifest-only and hybrid modes. Shared assertion helpers (assert_entity_exists,assert_entity_has_capabilities, etc.) inGatewayTestCasesupport mode-specific validation. Runtime mode is tested implicitly by all feature tests (default mode).Checklist