Skip to content

Conversation

@bmchalenv
Copy link
Contributor

@bmchalenv bmchalenv commented Jan 28, 2026

Smoke tests for the README in CI always return true. Failures are currently not being caught because of this. Updated the smoke tests to run the TUI applications and their demo modes. Updated the hz.launch.py test to measure topics we publish in the background so it doesn't error.

@bmchalenv bmchalenv marked this pull request as draft January 28, 2026 04:02
@greptile-apps
Copy link

greptile-apps bot commented Jan 28, 2026

Greptile Overview

Greptile Summary

This PR improves CI smoke tests by removing the || true fallbacks that were hiding test failures. The updated tests now properly validate README commands by:

  • Running both normal and --demo modes for ncurses_dashboard and r2s_gw_dashboard
  • Re-adding the hz.launch.py test with background topic publishers (/topic1 and /topic2) to provide the required test data
  • Installing ros-base package and building r2s_gw with colcon instead of just pip installing it
  • Exporting TERM=xterm for proper terminal emulation

Key improvements:

  • Tests now fail on errors (set -e without || true)
  • Better coverage with demo mode testing
  • hz.launch.py test restored with proper topic setup

Issue found:

  • Line 136: The hz.launch.py test will fail with exit code 130 when timeout sends SIGINT, but the script has set -e enabled. The test expects either a clean exit (0) or timeout signal (130), but won't handle the 130 case correctly.

Confidence Score: 3/5

  • This PR has one critical exit code handling issue that will cause the hz.launch.py test to fail in CI
  • The changes improve test coverage and properly detect failures by removing || true, but the hz.launch.py test on line 136 will fail because timeout's SIGINT (exit 130) is not handled with set -e active. This is a logical error that will break CI.
  • .github/workflows/ros-tests.yml line 136 needs exit code handling for the timeout command

Important Files Changed

Filename Overview
.github/workflows/ros-tests.yml Improved smoke tests by removing `

Sequence Diagram

sequenceDiagram
    participant CI as GitHub CI
    participant Setup as Setup Phase
    participant Test as Smoke Test
    participant ROS as ROS2 Runtime
    participant Topics as Topic Publishers
    
    CI->>Setup: Install ros-base package
    CI->>Setup: Build greenwave_monitor
    CI->>Setup: Install r2s_gw from requirements.txt
    CI->>Setup: Build r2s_gw with colcon
    
    CI->>Test: Run smoke tests (set -e enabled)
    
    Test->>ROS: timeout 10s ncurses_dashboard + 'q' input
    ROS-->>Test: Exit 0 (success)
    
    Test->>ROS: timeout 10s ncurses_dashboard --demo + 'q' input
    ROS-->>Test: Exit 0 (success)
    
    Test->>ROS: timeout 10s r2s_gw_dashboard + 'q' input
    ROS-->>Test: Exit 0 (success)
    
    Test->>ROS: timeout 10s r2s_gw_dashboard --demo + 'q' input
    ROS-->>Test: Exit 0 (success)
    
    Test->>Topics: Start /topic1 publisher (background)
    Test->>Topics: Start /topic2 publisher (background)
    Test->>Test: Set trap to kill publishers on EXIT
    Test->>Test: sleep 1s (allow publishers to start)
    
    Test->>ROS: timeout --signal=INT --preserve-status 5s hz.launch.py
    ROS-->>Test: Monitor topics until SIGINT
    Note over ROS,Test: Expects exit 130 (SIGINT) or 0 (clean exit)
    
    Test->>Topics: trap executes: kill publishers
    Test-->>CI: Return exit code (0 = pass, non-zero = fail)
Loading

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

3 files reviewed, no comments

Edit Code Review Agent Settings | Greptile

@bmchalenv bmchalenv force-pushed the fix-readme-smoke-tests branch from 0d0da99 to 7ce3300 Compare January 28, 2026 04:16
Signed-off-by: Blake McHale <bmchale@nvidia.com>
Signed-off-by: Blake McHale <bmchale@nvidia.com>
Signed-off-by: Blake McHale <bmchale@nvidia.com>
Signed-off-by: Blake McHale <bmchale@nvidia.com>
Signed-off-by: Blake McHale <bmchale@nvidia.com>
@bmchalenv bmchalenv force-pushed the fix-readme-smoke-tests branch from 887651c to 06b847c Compare January 28, 2026 04:35
@bmchalenv bmchalenv marked this pull request as ready for review January 28, 2026 04:36
Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

1 file reviewed, 1 comment

Edit Code Review Agent Settings | Greptile

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

1 file reviewed, no comments

Edit Code Review Agent Settings | Greptile

Signed-off-by: Blake McHale <bmchale@nvidia.com>
@bmchalenv bmchalenv force-pushed the fix-readme-smoke-tests branch from 5b6e798 to 4c55aa2 Compare January 28, 2026 05:29
Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

1 file reviewed, 1 comment

Edit Code Review Agent Settings | Greptile

Signed-off-by: Blake McHale <bmchale@nvidia.com>
Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

1 file reviewed, 2 comments

Edit Code Review Agent Settings | Greptile

@sgillen
Copy link
Collaborator

sgillen commented Jan 28, 2026

Thankyou but can you please update the PR description, you actually did fix the hz.launch.py test as well.

@bmchalenv
Copy link
Contributor Author

Updated the description. Realized I could fix it so decided to not remove.

@sgillen sgillen merged commit 64e5ad5 into NVIDIA-ISAAC-ROS:main Jan 28, 2026
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants