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 test failures (cross-platform) #480

Closed
jacobperron opened this issue Apr 9, 2020 · 5 comments · Fixed by #481 or #485
Closed

ros2param test failures (cross-platform) #480

jacobperron opened this issue Apr 9, 2020 · 5 comments · Fixed by #481 or #485
Assignees
@jacobperron
Labels
bug Something isn't working

Comments

@jacobperron
Copy link
Member

jacobperron commented Apr 9, 2020 •
edited
Loading

Bug report

Required Info:

  • Operating System:
    • All
  • Installation type:
    • source
  • Version or commit hash:
  • DDS implementation:
    • N/A

Steps to reproduce issue

See nightly failures:

Expected behavior

Tests pass

Actual behavior

Tests fail

Additional information

Roughly, it looks like the failures started on April 5th (based on the nightly Linux repeated build).
@jacobperron jacobperron self-assigned this Apr 9, 2020
@jacobperron jacobperron added the bug Something isn't working label Apr 9, 2020
@jacobperron
Copy link
Member Author

It looks like the failures are because not enough time is given for discover to happen between the test node and the CLI node (ie. the daemon).

If I switch the RMW implementation to rmw_connext_cpp, then I am able to reproduce reliably (I think because it takes longer for Connext to initialize).

jacobperron added a commit that referenced this issue Apr 9, 2020
@jacobperron
[ros2param] Wait for discovery before running tests
601f174 
Fixes #480.

We catch expected exceptions from rclpy (or transitively as xmlrpc.client.Fault) while we wait for discovery in the test setup.

Signed-off-by: Jacob Perron <[email protected]>
jacobperron added a commit that referenced this issue Apr 9, 2020
@jacobperron
[ros2param] Wait for discovery before running tests
2d06009 
Fixes #480.

Catch expected exceptions from rclpy (or transitively as xmlrpc.client.Fault) while we wait for discovery in the test setup.

Signed-off-by: Jacob Perron <[email protected]>
@jacobperron jacobperron mentioned this issue Apr 9, 2020
Merged
jacobperron added a commit that referenced this issue Apr 10, 2020
@jacobperron
[ros2param] Wait for discovery before running tests (#481)
8e6e051 
Fixes #480.

Catch expected exceptions from rclpy (or transitively as xmlrpc.client.Fault) while we wait for discovery in the test setup.

Signed-off-by: Jacob Perron <[email protected]>
@jacobperron
Copy link
Member Author

#481 isn't the fix we were looking for. Now we can see a timeout happening during the newly added wait for discovery: https://ci.ros2.org/view/nightly/job/nightly_linux-aarch64_repeated/1158/testReport/ros2param.src.ros2.ros2cli.ros2param.test.test_verb_dump/TestVerbDump/test_verb_dump/

@jacobperron jacobperron reopened this Apr 10, 2020
@jacobperron
Copy link
Member Author

I can reproduce the timeouts locally by first starting the daemon with an RMW implementation that is different than the one used by the tests:

RMW_IMPLEMENTATION=rmw_connext_cpp ros2 daemon start

And then running the tests

colcon test --packages-select ros2param

@hidmic
Copy link
Contributor

hidmic commented Apr 10, 2020 •
edited
Loading

Aha! That's why most ros2cli launch tests systematically stop that daemon e.g. https://github.com/ros2/ros2cli/blob/master/ros2topic/test/test_cli.py#L116.

And #389 never landed. I can go back and update it to replace these if you'd like.

@jacobperron
Copy link
Member Author

@hidmic I noticed that #389 doesn't touch test_verb_dump.py, so I started converting that set of tests to launch tests, independent of #389.

jacobperron added a commit that referenced this issue Apr 13, 2020
@jacobperron
Convert test_verb_dump into launch test
5ba0bc0 
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]>
@jacobperron jacobperron mentioned this issue Apr 13, 2020
Merged
@rotu rotu mentioned this issue Apr 14, 2020
Closed
jacobperron added a commit that referenced this issue Apr 15, 2020
@jacobperron
Convert test_verb_dump into launch test
07487b7 
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]>
jacobperron added a commit that referenced this issue 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 <[email protected]>
@nuclearsandwich nuclearsandwich mentioned this issue Apr 17, 2020
Merged
jacobperron added a commit that referenced this issue Apr 18, 2020
@jacobperron
[ros2param] Convert test_verb_dump into launch test (#485)
75e0ea1 
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.

In addition to converting to launch tests, this change also runs the tests for all RMW implementations.

For now, we are skipping tests on Windows. 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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@jacobperron jacobperron

Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
2 participants
@jacobperron @hidmic