-
Notifications
You must be signed in to change notification settings - Fork 172
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[ros2param] Convert test_verb_dump into launch test #485
Conversation
I can reproduce the Windows failure locally. It looks like that launch_testing is timing out waiting for the |
Rebuild on Windows with timeout increased (8bbe967): |
Disregard: there is a bug in the test, see #486 |
56e34da
to
6643e55
Compare
Rebased to get changes from #486 |
😞 Now I'm noticing test failures locally with fastrtps because of a warning introduced in ros2/rclpy#536. I think it's going to cause a lot of other CLI tests to fail too (e.g. |
Fixes #480 The actual tests are the same, except with the use of launch_testing we ensure the CLI daemon is restarted between tests. This follows a similar pattern as the other ros2cli tests. I've left a TODO to test with all RMW implementations. I did not enable this now since I do not expect the tests to pass with all RMW implementations. See #482 Signed-off-by: Jacob Perron <[email protected]>
This helps test pass on Windows where CLI invocations sometimes take more than 5 seconds. Signed-off-by: Jacob Perron <[email protected]>
Signed-off-by: Jacob Perron <[email protected]>
Signed-off-by: Jacob Perron <[email protected]>
Other CLI tests are skipped on Windows since #489. To be reverted when ros2/build_farmer#248 is resolved. Signed-off-by: Jacob Perron <[email protected]>
998e845
to
912b1ba
Compare
Signed-off-by: Jacob Perron <[email protected]>
Jenkins doesn't warn about it, but there was a deprecation warning that I've fixed in 8fef210 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM but for a couple minor comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Largely agree with @hidmic and I am immensely grateful for the time spent on these improvements to the tests. 🙇
Signed-off-by: Jacob Perron <[email protected]>
Fixes #480
The actual tests are the same, except with the use of launch_testing we ensure the CLI daemon
is restarted between tests. This follows a similar pattern as the other ros2cli tests.
I've left a TODO to test with all RMW implementations. I did not enable this now since I do
not expect the tests to pass with all RMW implementations.
See #482