Skip to content

Replace ROS test timeout with a readiness poll#380

Merged
sgrizan-nv merged 1 commit intomainfrom
sgrizan/branch3
Apr 9, 2026
Merged

Replace ROS test timeout with a readiness poll#380
sgrizan-nv merged 1 commit intomainfrom
sgrizan/branch3

Conversation

@sgrizan-nv
Copy link
Copy Markdown
Collaborator

@sgrizan-nv sgrizan-nv commented Apr 9, 2026

Summary by CodeRabbit

  • Chores
    • Improved reliability and error reporting in the automated testing infrastructure to provide clearer diagnostics during the build and test process.

@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Apr 9, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: f275471c-c44a-4695-84ba-31b37f3d2d4f

📥 Commits

Reviewing files that changed from the base of the PR and between 18c8e36 and fda65c7.

📒 Files selected for processing (1)
  • .github/workflows/build-ubuntu.yml

📝 Walkthrough

Walkthrough

Modified the GitHub Actions workflow for test-teleop-ros2 container execution. Replaced synchronous docker run --rm with detached docker run -d using a deterministic container name. Added explicit readiness polling that checks Docker logs for a startup marker (TeleopSession started successfully) with a 30-second timeout and 1-second polling intervals. Implemented differentiated error handling based on container state (running vs exited) at timeout, then collects logs, prints output, and enforces readiness marker observation before proceeding.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'Replace ROS test timeout with a readiness poll' directly and accurately summarizes the main change: replacing a timeout-based approach with explicit readiness polling for container startup.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ 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/branch3

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

@sgrizan-nv sgrizan-nv enabled auto-merge (squash) April 9, 2026 23:34
@sgrizan-nv sgrizan-nv merged commit cd903fd into main Apr 9, 2026
36 checks passed
@sgrizan-nv sgrizan-nv deleted the sgrizan/branch3 branch April 9, 2026 23:44
@github-project-automation github-project-automation bot moved this from TODO to DONE in Isaac Teleop Planning Apr 9, 2026
else
echo "No test log captured at $LOG_FILE"
running=$(docker inspect -f '{{.State.Running}}' "$RUN_NAME" 2>/dev/null || echo false)
if [ "$running" = "true" ]; then
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

just being paranoid, any chance that two build-ubuntu.yml workflow running together and one will kill the other? can that happen? can we check how long a container is running and only kill if it's long lasting?

Copy link
Copy Markdown
Collaborator Author

@sgrizan-nv sgrizan-nv Apr 10, 2026

Choose a reason for hiding this comment

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

There is a chance, if the workflows are executed on the same runner. To handle that, we'll need something like #382

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.

3 participants