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

Cleanup test_count_matched test to handle non-DDS RMWs #1164

Merged
merged 5 commits into from
Jul 1, 2024

Conversation

clalancette
Copy link
Contributor

The main goal of this PR is to update the test_count_matched test so that it handles non-DDS RMWs. Along the way, it also does some cleanup in the test. The patches are:

  1. Slightly refactor things so that check_state is a protected class method, rather than a free function. That way it can use more of the internal details of the class, which it depends on anyway.
  2. Renames "ops" to "opts", as that better reflects that these are options.
  3. Uses scope_exit to do some cleanup in the second test. We purposefully do not do this in the first test, as it specifically tests what happens as various things get destroyed.
  4. We add compatibility with non-DDS RMWs. Other RMWs may have different matching semantics between different QoS settings. In particular, rmw_zenoh will happily match BEST_EFFORT publishers to RELIABLE subscriptions, while the DDS RMWs will not. Luckily, we have the function called rmw_qos_profile_check_compatible that we can call to tell whether the underlying RMW would match them, so we can use that here.

This allows us to pass fewer parameters into each
each invocation, and allows us to hide some more of
the implementation inside the class.

Signed-off-by: Chris Lalancette <[email protected]>
It just better reflects what these structures are.

Signed-off-by: Chris Lalancette <[email protected]>
This just ensures that they are always cleaned up, even
if we exit early.  Note that we specifically do *not*
use it for test_count_matched_functions, since the cleanup
is intentionally interleaved with other tests.

Signed-off-by: Chris Lalancette <[email protected]>
Some RMWs may have different compatibility than DDS, so
check with the RMW layer to see what we should expect for
the number of publishers and subscriptions.

Signed-off-by: Chris Lalancette <[email protected]>
@clalancette
Copy link
Contributor Author

CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Linux-rhel Build Status
  • Windows Build Status

Copy link
Contributor

@ahcorde ahcorde left a comment

Choose a reason for hiding this comment

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

Two minor comments, LGTM with green CI

rcl/test/rcl/test_count_matched.cpp Show resolved Hide resolved
rcl/test/rcl/test_count_matched.cpp Outdated Show resolved Hide resolved
Signed-off-by: Chris Lalancette <[email protected]>
@clalancette
Copy link
Contributor Author

clalancette commented Jun 28, 2024

New CI after changes from review:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Linux-rhel Build Status
  • Windows Build Status

Copy link
Collaborator

@fujitatomoya fujitatomoya 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 green CI

@clalancette clalancette merged commit 3e7ce76 into rolling Jul 1, 2024
3 checks passed
@clalancette clalancette deleted the clalancette/cleanup-rcl-qos-compat-test branch July 1, 2024 23:59
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.

4 participants