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.

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

Add test_rmw_implementation package. #106

Merged
merged 6 commits into from
Jun 22, 2020
Merged

Add test_rmw_implementation package. #106

merged 6 commits into from
Jun 22, 2020

Conversation

hidmic
Copy link
Contributor

@hidmic hidmic commented Jun 19, 2020

Precisely what the title says. To serve as a basis for upcoming generic RMW API tests. Includes dummy init/shutdown test.

Includes dummy init/shutdown test.

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
@hidmic
Copy link
Contributor Author

hidmic commented Jun 19, 2020

Interestingly enough, this test as-is fails for rmw_cyclonedds_cpp. It cannot handle the RMW_DEFAULT_DOMAIN_ID and fails with:

[rmw_cyclonedds_cpp]: rmw_create_node: domain id out of range

It appears we didn't notice before because rcl_get_default_domain_id() implementation will always force the domain id to 0 if no ROS_DOMAIN_ID env var is present or it if contains anything other than a number (because of strtoul use).

Shall we change RMW_DEFAULT_DOMAIN_ID to reflect that? Or should it be honored?

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Copy link
Member

@wjwwood wjwwood left a comment

Choose a reason for hiding this comment

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

lgtm, with @brawner's point addressed.

Shall we change RMW_DEFAULT_DOMAIN_ID to reflect that? Or should it be honored?

I think having a way to let the middleware pick the default is useful, so maybe we should change it so it is honored.

test_rmw_implementation/test/test_init_shutdown.cpp Outdated Show resolved Hide resolved
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
@Karsten1987
Copy link
Contributor

just my 2cts, but why do we need a dedicated package for this? Can't this be part of system tests?

@hidmic
Copy link
Contributor Author

hidmic commented Jun 19, 2020

just my 2cts, but why do we need a dedicated package for this? Can't this be part of system tests?

This package is not meant to include system tests, but (ideally) unit tests for every portion of the documented rmw API.

While much of rmw's functionality, as well as rcl's, rclpy's and rclcpp's, is indirectly covered by integration and system tests up the stack, many corner cases cannot be easily (or even at all) checked. The fact that by writing the simplest test case I could come up with already uncovered a bug is at least promising.

@hidmic
Copy link
Contributor Author

hidmic commented Jun 19, 2020

I think having a way to let the middleware pick the default is useful, so maybe we should change it so it is honored.

About this. Seems like neither rmw_fastrtps_cpp nor rmw_connext_cpp are doing anything with it. They just pass along whatever ID it came their way. rmw_cyclonedds_cpp is more opinionated and has specific code to reject such default. Assuming we still want that freedom for rmw implementation authors, I'll send PRs to those implementation forcing the domain ID to 0 if the default is given.

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
@hidmic
Copy link
Contributor Author

hidmic commented Jun 19, 2020

CI up to rcl and test_rmw_implementation against all RMW implementations:

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

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
@hidmic
Copy link
Contributor Author

hidmic commented Jun 19, 2020

I was missing one last bit! See 7fde494.

@hidmic hidmic merged commit 34b3b0c into master Jun 22, 2020
@delete-merged-branch delete-merged-branch bot deleted the hidmic/test-package branch June 22, 2020 15:08
@dirk-thomas
Copy link
Member

@hidmic please follow up with releases as necessary to make this job pass again: http://build.ros2.org/job/Rdev__rmw_implementation__ubuntu_focal_amd64/2/testReport/

@sloretz
Copy link
Contributor

sloretz commented Jun 24, 2020

Looks like this PR caused two CI jobs to fail last night. I think the fix is just to add a <test_depend> for ament_cmake_gtest. Opening a PR to try that now.

ahcorde pushed a commit that referenced this pull request Oct 9, 2020
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
ahcorde pushed a commit that referenced this pull request Oct 21, 2020
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants