Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[ros2param] Convert test_verb_dump into launch test #485

Merged
merged 8 commits into from
Apr 18, 2020
Merged

[ros2param] Convert test_verb_dump into launch test #485

merged 8 commits into from
Apr 18, 2020

Conversation

jacobperron
Copy link
Member

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

@jacobperron jacobperron changed the title Convert test_verb_dump into launch test [ros2param] Convert test_verb_dump into launch test Apr 13, 2020
@jacobperron
Copy link
Member Author

jacobperron commented Apr 13, 2020 •
edited
Loading

Testing with additional ros2cli packages since the failures this PR is trying to fix involves left-over daemons from other tests:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

Edit: I'm looking into the Windows test failures.

@jacobperron
Copy link
Member Author

I can reproduce the Windows failure locally. It looks like that launch_testing is timing out waiting for the ros2 param commands to finish executing. Indeed on my machine ros2 param dump can take several seconds to complete.

@jacobperron
Copy link
Member Author

Rebuild on Windows with timeout increased (8bbe967): Build Status

@jacobperron
Copy link
Member Author

jacobperron commented Apr 14, 2020 •
edited
Loading

Anecdotally, I'll note that converting to launch tests adds significant overhead. In particular on my machine, the time it takes to run the tests has doubled, from 1min to 2min (with one RMW). I think this extra time is mainly coming from waiting for the daemon to restart (which I think is unavoidable for the moment).

Disregard: there is a bug in the test, see #486

@jacobperron jacobperron force-pushed the jacob/restart_daemon_ros2param branch from 56e34da to 6643e55 Compare April 15, 2020 18:26
@jacobperron
Copy link
Member Author

Rebased to get changes from #486

@jacobperron
Copy link
Member Author

jacobperron commented Apr 15, 2020 •
edited
Loading

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

Edit: doh, I forgot to add the logic to filter extra output known to come from the RMW

@jacobperron
Copy link
Member Author

😞 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. ros2 topic echo also produces the new warning).

@jacobperron jacobperron self-assigned this Apr 16, 2020
@jacobperron
Convert test_verb_dump into launch test
f39793f 
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 <jacob@openrobotics.org>
@jacobperron
Increase test timeout
e46dfe1 
This helps test pass on Windows where CLI invocations sometimes take
more than 5 seconds.

Signed-off-by: Jacob Perron <jacob@openrobotics.org>
@jacobperron
Parameterize tests on rmw implementation
d4941be 
Signed-off-by: Jacob Perron <jacob@openrobotics.org>
@jacobperron
Filter output coming from RMWs
933f8da 
Signed-off-by: Jacob Perron <jacob@openrobotics.org>
@jacobperron
Skip tests on Windows
912b1ba 
Other CLI tests are skipped on Windows since #489.

To be reverted when ros2/build_farmer#248 is resolved.

Signed-off-by: Jacob Perron <jacob@openrobotics.org>
@jacobperron jacobperron force-pushed the jacob/restart_daemon_ros2param branch from 998e845 to 912b1ba Compare April 16, 2020 21:42
@jacobperron
Copy link
Member Author

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@jacobperron
Fix deprecation warning
8fef210 
Signed-off-by: Jacob Perron <jacob@openrobotics.org>
@jacobperron
Copy link
Member Author

Jenkins doesn't warn about it, but there was a deprecation warning that I've fixed in 8fef210
Rerunning Linux for sanity: Build Status

hidmic
hidmic approved these changes Apr 17, 2020
View reviewed changes
Copy link
Contributor

@hidmic hidmic left a 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

ros2param/test/test_verb_dump.py Outdated Show resolved Hide resolved
ros2param/test/test_verb_dump.py Outdated Show resolved Hide resolved
nuclearsandwich
nuclearsandwich approved these changes Apr 17, 2020
View reviewed changes
Copy link
Member

@nuclearsandwich nuclearsandwich left a 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. 🙇

ros2param/test/test_verb_dump.py Show resolved Hide resolved
ros2param/test/test_verb_dump.py Outdated Show resolved Hide resolved
@jacobperron
Copy link
Member Author

jacobperron commented Apr 17, 2020 •
edited by nuclearsandwich
Loading

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@jacobperron jacobperron merged commit 75e0ea1 into master Apr 18, 2020
@delete-merged-branch delete-merged-branch bot deleted the jacob/restart_daemon_ros2param branch April 18, 2020 00:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nuclearsandwich nuclearsandwich nuclearsandwich approved these changes

@hidmic hidmic hidmic approved these changes

Assignees

@jacobperron jacobperron

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

ros2param test failures (cross-platform)
3 participants
@jacobperron @nuclearsandwich @hidmic