Make topic CLI tests wait on command output

[sylvesterkaczmarek]

Description

Address the timing failure tracked in #1093 by waiting on the actual ros2 topic bw, delay, or hz process instead of using the publishing test node as a readiness signal.

The previous test called self.node.count_publishers() on the same node that had just created the publisher, so that check could succeed immediately without saying anything about what the fresh DirectNode inside the CLI process had discovered. My first revision tried to use the daemon as an independent observer; review correctly pointed out that this still did not prove the CLI node's state and also introduced an unwanted daemon lifecycle dependency.

The revised change removes NodeStrategy entirely. For the basic bw/delay/hz cases, the test now launches the real CLI command, keeps spinning the publisher executor, and waits up to 15 seconds for the command itself to produce the expected success output or incompatible-QoS warning. The wait remains bounded and fails if the command under test never reaches the expected state.

I do not have a local Zenoh reproduction result for this revised head, so I am not claiming that the build-farm failure is confirmed fixed. This change replaces the invalid readiness proxy with a condition observed from the command under test; Zenoh/build-farm validation is still needed.

Addresses #1093.

Is this user-facing behavior change?

No. This only changes integration-test synchronization.

Additional Information

The change is now limited to the basic bw/delay/hz path where #1093 reported the failure. The existing multi-topic and --all tests are left unchanged.

Did you use Generative AI?

Yes. AI was used to assist with tests.

[mergify]

Tick the box to add this pull request to the merge queue (same as @mergifyio queue).

• Queue this pull request

[fujitatomoya]

Fix the race tracked in #1093 by waiting until the CLI daemon has discovered the test publishers before launching ros2 topic bw, delay, or hz.

this does not really make sense to me. can you explain why this can address the issue?

[sylvesterkaczmarek]

Fix the race tracked in #1093 by waiting until the CLI daemon has discovered the test publishers before launching ros2 topic bw, delay, or hz.

this does not really make sense to me. can you explain why this can address the issue?

Hi @fujitatomoya, you're right, the wording in the description is misleading imo. bw/delay/hz use a fresh DirectNode; the daemon is only being used here as an independent discovery observer.

The existing check calls self.node.count_publishers() on the same node that just created the publisher, so it can succeed before that endpoint has been discovered by any other participant. That's the race I was trying to remove: in #1093 the newly started CLI process reports that the topic does not appear to be published yet.

Waiting until the already-running isolated daemon can see the publisher gives us evidence that discovery has propagated beyond the publishing node before we launch the fresh CLI process.

If you'd prefer, I can also change the readiness probe to use a separate direct node instead of the daemon so it more closely mirrors the command under test. Thanks!

[fujitatomoya]

Waiting until the already-running isolated daemon can see the publisher gives us evidence that discovery has propagated beyond the publishing node before we launch the fresh CLI process.

bw / delay / hz run on DirectNode and never use the daemon intentionally. i think this guarantees nothing for this test. if you believe this reliably fixes the Zenoh failure, please share reproduction evidence and improvement result?

besides that (this can be even worse...), NodeStrategy(None) is a problem. None is not a valid args namespace, and if no daemon is running on the coordinated test domain, NodeStrategy will spawn one inside the test and leave it running after the test exits (2h inactivity timeout, bound to the domain port). we cleaned up stray isolated daemons before (#1098), this reintroduces the same problem into a test that previously did not depend on the daemon at all.

If you'd prefer, I can also change the readiness probe to use a separate direct node instead of the daemon

that removes the daemon lifecycle problem but still does not close the race, another observer node is still not the CLI's node.

btw, are you pasting AI output here to answer review questions? your disclosure covers tests only. per https://discourse.openrobotics.org/t/guidelines-for-ai-assisted-contributions-in-the-ros-project/55903 you are accountable for understanding and explaining your contribution yourself. please answer the technical questions above in your own words, with reproduction data. (this will apply to all of your PRs now.)

thanks,

[sylvesterkaczmarek]

Waiting until the already-running isolated daemon can see the publisher gives us evidence that discovery has propagated beyond the publishing node before we launch the fresh CLI process.

bw / delay / hz run on DirectNode and never use the daemon intentionally. i think this guarantees nothing for this test. if you believe this reliably fixes the Zenoh failure, please share reproduction evidence and improvement result?

besides that (this can be even worse...), NodeStrategy(None) is a problem. None is not a valid args namespace, and if no daemon is running on the coordinated test domain, NodeStrategy will spawn one inside the test and leave it running after the test exits (2h inactivity timeout, bound to the domain port). we cleaned up stray isolated daemons before (#1098), this reintroduces the same problem into a test that previously did not depend on the daemon at all.

If you'd prefer, I can also change the readiness probe to use a separate direct node instead of the daemon

that removes the daemon lifecycle problem but still does not close the race, another observer node is still not the CLI's node.

btw, are you pasting AI output here to answer review questions? your disclosure covers tests only. per https://discourse.openrobotics.org/t/guidelines-for-ai-assisted-contributions-in-the-ros-project/55903 you are accountable for understanding and explaining your contribution yourself. please answer the technical questions above in your own words, with reproduction data. (this will apply to all of your PRs now.)

thanks,

Hi @fujitatomoya, you're right about the daemon approach. NodeStrategy was the wrong readiness signal here, and it also introduced an unnecessary daemon lifecycle dependency. So I've removed it.

The revised test now starts the actual ros2 topic bw / delay / hz command and keeps spinning the publisher while waiting for that command itself to produce the expected output. So the test no longer infers readiness from another observer node.

I don't currently have a local Zenoh environment where I can reproduce the original build-farm failure, so I can't honestly claim reproduction/improvement numbers yet. This revision fixes the synchronization problem in the test design, but Zenoh CI/build-farm results are still needed to confirm that it resolves #1093.

Also, yes, I used AI assistance for the tests. I did and reviewed the technical changes myself, and I'm responsible for them. Thanks!