test(interop): stop passing when the C++ node fails - #306
Merged
Conversation
2 tasks
Two of the three hiroz-to-C++ interop tests asserted nothing about the C++ process. The talker test piped the listener's stdout and never read it; the action test slept for ten seconds and printed 'Test passed'. Both pass whether the C++ node consumes hiroz output, crashes, or never starts. That mattered for #303: the double-free there looked confined to the service path, but it was confined to the only test that checks. These two could have been failing identically all along. The listener never exits on its own, so OutputCapture now appends line by line and exposes a snapshot, rather than only yielding text at EOF.
The listener assertion looked at stdout only and saw an empty buffer, so it reported that the C++ node received nothing. ROS 2 logging goes to stderr by default: the I heard lines were never on stdout, and the test was piping stdout while sending stderr to /dev/null. The three previously-green distros caught this, which is what they were there for.
YuanYuYuan
force-pushed
the
test/interop-assert-child-outcome
branch
from
August 14, 2026 11:47
8377c5d to
dad9049
Compare
OutputCapture::finish joins the reader threads, and they block on the open pipes until the child exits. Both timeout branches called it while the child was provably still running, so the panic never rendered: the test stalled until nextest's 120s kill and printed nothing. That is the exact failure the capture exists to prevent. The add_two_ints branch has this defect on main already, from #304. Fixed in both call sites. Two smaller changes from the same review: - The listener assertion sampled once after a fixed 1s sleep. The talker publishes a finite ~900ms burst, so a single sample races CI-load stalls. It now polls on a 15s deadline, as the reverse-direction test already does for the same reason. - The action client's 30s deadline was decorative. Its server lives 10s and the client starts ~2s in, so a client that has not succeeded by then never will. Bounded at 15s and named.
There was a problem hiding this comment.
Pull request overview
Strengthens ROS 2 C++ interoperability tests so they verify spawned processes actually succeed.
Changes:
- Captures and polls listener output for received messages.
- Validates the Fibonacci client’s exit and diagnostics.
- Prevents output capture from blocking before child termination.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
crates/hiroz-tests/tests/demo_nodes.rs |
Adds C++ process assertions and safer timeout cleanup. |
crates/hiroz-tests/tests/common/mod.rs |
Supports snapshots of concurrently captured output. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…the guard A zero exit status did not prove the client completed the action. action_tutorials_cpp calls rclcpp::shutdown() when its 10s wait_for_action_server expires, so the client exits 0 after logging 'Action server not available after waiting'. The assertion therefore passed in the one case it exists to catch: the hiroz server never discovered. Verified against ros2/demos: 'Result received: ' is written only in the SUCCEEDED arm of the result callback. The test now requires it. Separately, try_wait reaps the child but left it in ProcessGuard. Drop signals the process group by negative PID, and for the action test that runs only after the 10s server thread joins -- by which point the PGID may have been recycled. Both tests now take the child out of the guard as soon as an exit status is observed.
YuanYuYuan
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Two of the three hiroz → C++ interop tests asserted nothing about the C++ process. They passed whether it worked, crashed, or never started.
Before / after
test_hiroz_talker_to_rcl_listenerI heardon a 15s deadlinetest_hiroz_fibonacci_action_server_to_rcl_clienttest_hiroz_add_two_ints_server_to_rcl_clientfinish()fix belowwait_for_readyisthread::sleep. Neither of the first two read the child's output or its exit status, so neither could fail because of the C++ side.Two details behind the "after" column. ROS 2 logs to stderr by default, so the listener assertion reads both streams. The action client's budget is 15s because its server lives 10s. A client that has not succeeded by then never will.
A defect this also fixes on
mainOutputCapture::finish()joins the reader threads, and they block on the open pipes until the child exits. Both timeout branches called it while the child was still running, so the panic never rendered: the test stalled until nextest's 120s kill and printed nothing. That is the failure the capture exists to prevent.add_two_intscarries that defect onmainfrom #304. Both call sites now kill the child before rendering.Evidence
27 checks green, 0 failed. Both new assertions pass on humble, jazzy, kilted and lyrical.
Note
No failing baseline on hiroz code, and none claimed — this adds assertions and changes no hiroz behaviour. The check that they are not over-strict is that all four distros pass with them in place.
The
finish()fix is not exercised by a green run: that branch needs a client that never exits. It was verified by reading, not by execution.Breaking changes
None. Test-only.