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

Omnibus fixes for running tests with Connext. (backport #2684) #2690

Merged
merged 2 commits into from
Dec 3, 2024

Conversation

mergify[bot]
Copy link
Contributor

@mergify mergify bot commented Nov 30, 2024

When running the tests with RTI Connext as the default RMW, some of the tests are failing. There are three different failures fixed here:

  1. Setting the liveliness duration to a value smaller than a microsecond causes Connext to throw an error. Set it to a millisecond.

  2. Using the SystemDefaultsQoS sets the QoS to KEEP_LAST 1. Connext is somewhat slow in this regard, so it can be the case that we are overwriting a previous service introspection event with the next one. Switch to the ServicesDefaultQoS in the test, which ensures we will not lose events.

  3. Connext is slow to match publishers and subscriptions. Thus, when creating a subscription "on-the-fly", we should wait for the publisher to match it before expecting the subscription to actually receive data from it.

With these fixes in place, the test_client_common, test_generic_service, test_service_introspection, and test_executors tests all pass for me with rmw_connextdds.

This should fix #2613, #2611, and #2646. @Crola1702 FYI


This is an automatic backport of pull request #2684 done by Mergify.

* Omnibus fixes for running tests with Connext.

When running the tests with RTI Connext as the default
RMW, some of the tests are failing.  There are three
different failures fixed here:

1.  Setting the liveliness duration to a value smaller than
a microsecond causes Connext to throw an error.  Set it to
a millisecond.

2.  Using the SystemDefaultsQoS sets the QoS to KEEP_LAST 1.
Connext is somewhat slow in this regard, so it can be the case
that we are overwriting a previous service introspection event
with the next one.  Switch to the ServicesDefaultQoS in the test,
which ensures we will not lose events.

3.  Connext is slow to match publishers and subscriptions.  Thus,
when creating a subscription "on-the-fly", we should wait for the
publisher to match it before expecting the subscription to actually
receive data from it.

With these fixes in place, the test_client_common, test_generic_service,
test_service_introspection, and test_executors tests all pass for
me with rmw_connextdds.

Signed-off-by: Chris Lalancette <clalancette@gmail.com>

* Fixes for executors.

Signed-off-by: Chris Lalancette <clalancette@gmail.com>

* One more fix for services.

Signed-off-by: Chris Lalancette <clalancette@gmail.com>

* More fixes for service_introspection.

Signed-off-by: Chris Lalancette <clalancette@gmail.com>

* More fixes for introspection.

Signed-off-by: Chris Lalancette <clalancette@gmail.com>

---------

Signed-off-by: Chris Lalancette <clalancette@gmail.com>
(cherry picked from commit 9984197)

# Conflicts:
#	rclcpp/test/rclcpp/executors/test_executors.cpp
#	rclcpp/test/rclcpp/test_generic_service.cpp
@mergify mergify bot added the conflicts label Nov 30, 2024
Copy link
Contributor Author

mergify bot commented Nov 30, 2024

Cherry-pick of 9984197 has failed:

On branch mergify/bp/jazzy/pr-2684
Your branch is up to date with 'origin/jazzy'.

You are currently cherry-picking commit 9984197c.
  (fix conflicts and run "git cherry-pick --continue")
  (use "git cherry-pick --skip" to skip this patch)
  (use "git cherry-pick --abort" to cancel the cherry-pick operation)

Changes to be committed:
	modified:   rclcpp/test/rclcpp/test_client_common.cpp
	modified:   rclcpp/test/rclcpp/test_service.cpp
	modified:   rclcpp/test/rclcpp/test_service_introspection.cpp

Unmerged paths:
  (use "git add/rm <file>..." as appropriate to mark resolution)
	both modified:   rclcpp/test/rclcpp/executors/test_executors.cpp
	deleted by us:   rclcpp/test/rclcpp/test_generic_service.cpp

To fix up this pull request, you can check it out locally. See documentation: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/checking-out-pull-requests-locally

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.

@clalancette base code is a bit different, but would be better to backport this to jazzy for long term? what do you think? i can patch it to resolve conflict.

@clalancette
Copy link
Contributor

@clalancette base code is a bit different, but would be better to backport this to jazzy for long term? what do you think? i can patch it to resolve conflict.

Yeah, I think it would be good to fix this in Jazzy. It should help with https://build.ros2.org/view/Jci/job/Jci__nightly-connext_ubuntu_noble_amd64/

Signed-off-by: Tomoya Fujita <Tomoya.Fujita@sony.com>
@fujitatomoya
Copy link
Collaborator

Pulls: #2690
Gist: https://gist.githubusercontent.com/fujitatomoya/a2a843ab74aed02ea260040b2f231d68/raw/93a86a9cc1ca1c2225d08afc899c4694cb5ddcc7/ros2.repos
BUILD args: --packages-up-to rclcpp
TEST args: --packages-select rclcpp
ROS Distro: jazzy
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/14905

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

@fujitatomoya
Copy link
Collaborator

@clalancette requesting review just in case!

Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

Looks good to me with green CI.

@fujitatomoya fujitatomoya merged commit a7b8ada into jazzy Dec 3, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants