Skip to content

ROS: verify topic data in integration tests#549

Merged
sgrizan-nv merged 2 commits into
mainfrom
sgrizan/branch5
May 21, 2026
Merged

ROS: verify topic data in integration tests#549
sgrizan-nv merged 2 commits into
mainfrom
sgrizan/branch5

Conversation

@sgrizan-nv
Copy link
Copy Markdown
Collaborator

@sgrizan-nv sgrizan-nv commented May 20, 2026

Summary by CodeRabbit

  • New Features

    • MCAP replay mode lets the teleop app run from recorded ROS 2 data via a new replay parameter.
  • Documentation

    • Added "MCAP Replay" documentation with Docker examples and usage notes.
  • Tests

    • Added integration tests: deterministic MCAP fixture generator and a topic verifier to validate teleop behavior in CI.

Review Change Stack

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 20, 2026

Important

Review skipped

Auto incremental reviews are disabled on this repository.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: fd908836-e180-40a8-9617-2817d77c83b6

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

This PR introduces MCAP-based replay capability for integration testing the teleop_ros2 system. A new C++ utility (teleop_ros2_mcap_generator) produces deterministic MCAP fixture files containing synthetic controller, hand, pedal, and full-body pose/joint data. The node gains an optional mcap_replay_path parameter to replay from these fixtures instead of live input, replacing the previous mock/plugin operator system. A Python ROS 2 verifier subscribes to published topics, validates message contents with type-specific assertions, and tracks validation timeouts, reporting success/failure via exit code. The CI workflow now generates MCAP files, launches the node with replay paths for each mode, and runs the topic verifier in a container to gate the workflow on validation success.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 5.56% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main change: adding topic data verification in integration tests, which is the core purpose reflected across workflow updates, new verifier scripts, and MCAP replay infrastructure.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch sgrizan/branch5

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@examples/teleop_ros2/cpp/integration_tests/mcap_generator.cpp`:
- Around line 126-159: The PedalChannels constructor in write_fixture currently
uses core::PedalRecordingTraits::replay_channels which omits the "pedals"
channel; change the PedalChannels instantiation to use
core::PedalRecordingTraits::recording_channels so the pedal fixture matches
ControllerChannels/HandChannels/FullBodyChannels and includes both "pedals" and
"pedals_tracked" (update the argument passed to PedalChannels that currently
references replay_channels to recording_channels).

In `@examples/teleop_ros2/python/integration_tests/teleop_ros2_topic_verifier.py`:
- Around line 43-167: The tests use bare assert statements that can be disabled
under Python optimization; update each validator to perform explicit runtime
checks and raise appropriate exceptions (e.g., ValueError or RuntimeError)
instead of using assert. Replace all asserts in _assert_pose_array,
_assert_joint_state, _assert_twist, _assert_root_pose,
_assert_controller_payload, and _assert_full_body_payload with if-not-condition:
raise ValueError("...") (preserving the existing error messages) and ensure
sequence/length checks (_is_finite_sequence checks and any(...) checks) follow
the same pattern so validation is always enforced.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: cacd78db-18a9-41e5-837a-47bc2d56bce3

📥 Commits

Reviewing files that changed from the base of the PR and between 4f9c2a6 and f964c8c.

📒 Files selected for processing (7)
  • .github/workflows/build-ubuntu.yml
  • examples/teleop_ros2/CMakeLists.txt
  • examples/teleop_ros2/README.md
  • examples/teleop_ros2/cpp/integration_tests/CMakeLists.txt
  • examples/teleop_ros2/cpp/integration_tests/mcap_generator.cpp
  • examples/teleop_ros2/python/integration_tests/teleop_ros2_topic_verifier.py
  • examples/teleop_ros2/python/teleop_ros2_node.py

Comment thread examples/teleop_ros2/cpp/integration_tests/mcap_generator.cpp
@sgrizan-nv sgrizan-nv force-pushed the sgrizan/branch5 branch 4 times, most recently from 5780c14 to 544a029 Compare May 20, 2026 10:50
Comment thread examples/teleop_ros2/cpp/integration_tests/mcap_generator.cpp Outdated
@sgrizan-nv sgrizan-nv enabled auto-merge (squash) May 21, 2026 19:20
@sgrizan-nv sgrizan-nv merged commit 1ff81a2 into main May 21, 2026
43 checks passed
@sgrizan-nv sgrizan-nv deleted the sgrizan/branch5 branch May 21, 2026 19:39
@github-project-automation github-project-automation Bot moved this from TODO to DONE in Isaac Teleop Planning May 21, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: DONE

Development

Successfully merging this pull request may close these issues.

2 participants