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

Change which node name cross-vendor tests are enabled. #428

Merged
merged 3 commits into from
May 1, 2020
Merged

Change which node name cross-vendor tests are enabled. #428

merged 3 commits into from
May 1, 2020

Conversation

sloretz
Copy link
Contributor

@sloretz sloretz commented Apr 30, 2020

Currently test_nodes_name is enabled for fastrtps/fastrtps and cyclonedds/connext. Cyclonedds uses one participant per context (ros2/rmw_cyclonedds#145) while Connext uses 1 participant per node (ros2/rmw_connext#381), so cyclone/connext is not expected to work and is currently failing in the nightlies.

This PR changes to what should work. FastRTPS uses 1 participant per context (https://github.com/ros2/rmw_fastrtps#312), so it should work with itself and Cyclonedds. Connext should only work with itself.

However, it turns out all cross-vendor tests are failing to communicate node names to each other

The following tests FAILED:
	 47 - test_node_name__rmw_cyclonedds_cpp__rmw_fastrtps_cpp (Failed)
	 48 - test_node_name__rmw_cyclonedds_cpp__rmw_fastrtps_dynamic_cpp (Failed)
	 71 - test_node_name__rmw_fastrtps_cpp__rmw_cyclonedds_cpp (Failed)
	 96 - test_node_name__rmw_fastrtps_dynamic_cpp__rmw_cyclonedds_cpp (Failed)

@ivanpauno ideas on why fastrtps/cyclonedds are failing to communicate node names?

@sloretz
Change which rmws do cross node name tests
eeccdd0 
Disable connext cross-vendor since it still uses 1 participant per node
Enable Fast-RTPS and Cyclone cross vendor

Signed-off-by: Shane Loretz<[email protected]>
Signed-off-by: Shane Loretz <[email protected]>
@sloretz sloretz added the bug Something isn't working label Apr 30, 2020
@sloretz sloretz requested a review from ivanpauno April 30, 2020 02:14
dirk-thomas
dirk-thomas reviewed Apr 30, 2020
View reviewed changes
Copy link
Member

@dirk-thomas dirk-thomas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not related to the PR itself but to the lines it touches:

  • is it intentional to skip all test_node_name tests on Windows?
  • the test should be created unconditionally and have the SKIP_TEST flag passed in the cases which are intentionally skipped

test_rclcpp/CMakeLists.txt Outdated
(rmw_implementation1_is_fastrtps AND rmw_implementation2_is_cyclonedds) OR
(rmw_implementation1_is_cyclonedds AND rmw_implementation2_is_fastrtps) OR
(rmw_implementation1_is_cyclonedds AND rmw_implementation2_is_cyclonedds) OR
(rmw_implementation1_is_connext AND rmw_implementation2_is_connext) OR
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new condition only conditionally enables known combinations. Therefore this test is not used for any other RMW impl. The logic should be inverted to mark the combinations know to not work as skipped - but enable the test for all other cases (including unknown combinations).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the test should be created unconditionally and have the SKIP_TEST flag passed in the cases which are intentionally skipped

Uses SKIP_TEST in e084224

The logic should be inverted to mark the combinations know to not work as skipped - but enable the test for all other cases (including unknown combinations).

I was going to do this, but on second thought I think whitelisting fits better. Blacklisting would make sense if it were assumed all unknown rmw implementations can communicate with any other unknown implementation. However; I think the real assumption is that only DDS based rmw implementations should be able to communicate with each other. In that case a whitelist can enable DDS-based cross-vendor tests while avoiding enabling cross-vendor tests for implementations we don't have info about. For example, the whitelist avoids enabling cross vendor tests for iceoryx and connext without adding knowledge of iceoryx's existance to test_rclcpp.

Copy link
Member

@dirk-thomas dirk-thomas Apr 30, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't know all DDS-based RMW implementations and it would be nice if those can run these these tests without requiring them to be modified. If they fail for them they can judge why that is the case.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't know all DDS-based RMW implementations and it would be nice if those can run these these tests without requiring them to be modified.

Do we have any CLI or environment variable options for forcing skipped tests to run? That would be nice for those unknown non-DDS implementations that want to run system tests without the cross-vendor tests.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know if ctest / pytest offer such an option. (Not applicable here but pytest has an option to run tests marked with xfail.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I opened an issue to track this for now #429

Copy link
Member

@dirk-thomas dirk-thomas Apr 30, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am pretty sure the time to write that ticket exceeded the necessary time to invert the logic in this PR...

@ivanpauno
Copy link
Member

@ivanpauno ideas on why fastrtps/cyclonedds are failing to communicate node names?

I'll take a look.

@ivanpauno ivanpauno mentioned this pull request Apr 30, 2020
Merged
@ivanpauno
Copy link
Member

ivanpauno commented Apr 30, 2020 •
edited
Loading

See ros2/rmw_cyclonedds#177.

@sloretz
Mark tests as skipped instead of not generating
e084224 
Generate all cross-vendor tests
Whitelist which cross-vendor tests are expected to work

Signed-off-by: Shane Loretz<[email protected]>
Signed-off-by: Shane Loretz <[email protected]>
ivanpauno
ivanpauno approved these changes Apr 30, 2020
View reviewed changes
Copy link
Member

@ivanpauno ivanpauno left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

I think CI should be run together with ros2/rmw_cyclonedds#177.

@sloretz
Copy link
Contributor Author

sloretz commented Apr 30, 2020

I think CI should be run together with ros2/rmw_cyclonedds#177.

+1

CI (testing test_rclcpp and rmw_cyclonedds_cpp)

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

@sloretz sloretz mentioned this pull request Apr 30, 2020
Open
@sloretz
Whitespace
b6de342 
Signed-off-by: Shane Loretz <[email protected]>
@sloretz
Copy link
Contributor Author

sloretz commented Apr 30, 2020

CI again with ros2/rmw_cyclonedds#177 updated and linter error fixed

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

@sloretz
Copy link
Contributor Author

sloretz commented May 1, 2020 •
edited
Loading

ros2/rmw_cyclonedds#177 merged and CI ok; merging ahead of nightlies

@sloretz sloretz reopened this May 1, 2020
@sloretz sloretz merged commit b33a19a into master May 1, 2020
@delete-merged-branch delete-merged-branch bot deleted the test_node_name_cross_vendor_one_participant branch May 1, 2020 01:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dirk-thomas dirk-thomas dirk-thomas left review comments

@ivanpauno ivanpauno ivanpauno approved these changes

Assignees
No one assigned
Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@sloretz @ivanpauno @dirk-thomas