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

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

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
eeccdd0
Change which rmws do cross node name tests
sloretz Apr 30, 2020
e084224
Mark tests as skipped instead of not generating
sloretz Apr 30, 2020
b6de342
Whitespace
sloretz Apr 30, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 22 additions & 1 deletion test_rclcpp/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,10 +192,31 @@ if(BUILD_TESTING)
set(rmw_implementation2_is_fastrtps TRUE)
endif()

set(rmw_implementation1_is_connext FALSE)
set(rmw_implementation2_is_connext FALSE)
if(rmw_implementation1 MATCHES "(.*)connext(.*)")
set(rmw_implementation1_is_connext TRUE)
endif()
if(rmw_implementation2 MATCHES "(.*)connext(.*)")
set(rmw_implementation2_is_connext TRUE)
endif()

set(rmw_implementation1_is_cyclonedds FALSE)
set(rmw_implementation2_is_cyclonedds FALSE)
if(rmw_implementation1 MATCHES "(.*)cyclonedds(.*)")
set(rmw_implementation1_is_cyclonedds TRUE)
endif()
if(rmw_implementation2 MATCHES "(.*)cyclonedds(.*)")
set(rmw_implementation2_is_cyclonedds TRUE)
endif()

# Skipped tests between fastrtps and others, as the node names are communicated with a different mechanism.
# TODO(ivanpauno): Reenable this tests after the other implementations also use one Participant per Context.
if(
(NOT rmw_implementation1_is_fastrtps AND NOT rmw_implementation2_is_fastrtps) OR
(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...

(rmw_implementation1_is_fastrtps AND rmw_implementation2_is_fastrtps)
)
custom_launch_test_two_executables(test_node_name
Expand Down