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. #2684

Open
wants to merge 5 commits into
base: rolling
Choose a base branch
from

Conversation

clalancette
Copy link
Contributor

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

@clalancette
Copy link
Contributor Author

clalancette commented Nov 22, 2024

Pulls: #2684
Gist: https://gist.githubusercontent.com/clalancette/f7ff700eafc747ed8ae903f04c4c9090/raw/11b53a012ea22e5bea0fa425ea847fae021f9d2d/ros2.repos
BUILD args: --packages-up-to rclcpp
TEST args: --packages-select rclcpp
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/14849

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

@clalancette clalancette force-pushed the clalancette/connext-test-failure-fixes branch from 54455b3 to 871cd7a Compare November 22, 2024 17:52
@fujitatomoya
Copy link
Collaborator

@clalancette i was trying to fix #2613 with #2651, but i was able to reproduce the issue after QoS depth 10. i will try your patch to make sure if that problem goes away. (i have never tried ServiceQoSProfile yet.)

@fujitatomoya
Copy link
Collaborator

@clalancette unfortunately still fails...

export RMW_IMPLEMENTATION=rmw_connextdds
colcon test --event-handlers console_direct+ --packages-select rclcpp --ctest-args -R test_service_introspection --retest-until-fail 100
...
74: Test command: /usr/bin/python3 "-u" "/root/ros2_ws/work/install/ament_cmake_test/share/ament_cmake_test/cmake/run_test.py" "/root/ros2_ws/work/build/rclcpp/test_results/rclcpp/test_service_introspection.gtest.xml" "--package-name" "rclcpp" "--output-file" "/root/ros2_ws/work/build/rclcpp/ament_cmake_gmock/test_service_introspection.txt" "--command" "/root/ros2_ws/work/build/rclcpp/test/rclcpp/test_service_introspection" "--gtest_output=xml:/root/ros2_ws/work/build/rclcpp/test_results/rclcpp/test_service_introspection.gtest.xml"
74: Working Directory: /root/ros2_ws/work/build/rclcpp/test/rclcpp
74: Test timeout computed to be: 60
74: -- run_test.py: invoking following command in '/root/ros2_ws/work/build/rclcpp/test/rclcpp':
74:  - /root/ros2_ws/work/build/rclcpp/test/rclcpp/test_service_introspection --gtest_output=xml:/root/ros2_ws/work/build/rclcpp/test_results/rclcpp/test_service_introspection.gtest.xml
74: Running main() from gmock_main.cc
74: [==========] Running 3 tests from 1 test suite.
74: [----------] Global test environment set-up.
74: [----------] 3 tests from TestServiceIntrospection
74: [ RUN      ] TestServiceIntrospection.service_introspection_nominal
74: RTI Connext DDS Non-commercial license is for academic, research, evaluation and personal use only. USE FOR COMMERCIAL PURPOSES IS PROHIBITED. See RTI_LICENSE.TXT for terms. Download free tools at rti.com/ncl. License issued to Non-Commercial User [email protected] For non-production use only.
74: Expires on 00-jan-00 See www.rti.com for more information.
74: [       OK ] TestServiceIntrospection.service_introspection_nominal (305 ms)
74: [ RUN      ] TestServiceIntrospection.service_introspection_enable_disable_events
74: RTI Connext DDS Non-commercial license is for academic, research, evaluation and personal use only. USE FOR COMMERCIAL PURPOSES IS PROHIBITED. See RTI_LICENSE.TXT for terms. Download free tools at rti.com/ncl. License issued to Non-Commercial User [email protected] For non-production use only.
74: Expires on 00-jan-00 See www.rti.com for more information.
74: /root/ros2_ws/work/src/ros2/rclcpp/rclcpp/test/rclcpp/test_service_introspection.cpp:232: Failure
74: Expected equality of these values:
74:   events.size()
74:     Which is: 3
74:   4U
74:     Which is: 4
74:
74: [  FAILED  ] TestServiceIntrospection.service_introspection_enable_disable_events (2305 ms)

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 <[email protected]>
Signed-off-by: Chris Lalancette <[email protected]>
Signed-off-by: Chris Lalancette <[email protected]>
@clalancette clalancette force-pushed the clalancette/connext-test-failure-fixes branch from 871cd7a to ceb7e76 Compare November 25, 2024 17:04
@clalancette
Copy link
Contributor Author

@clalancette i was trying to fix #2613 with #2651, but i was able to reproduce the issue after QoS depth 10. i will try your patch to make sure if that problem goes away. (i have never tried ServiceQoSProfile yet.)

Ah, sorry, I forgot about your PR.

@clalancette unfortunately still fails...

Good call on this. I was able to reproduce, and I found out the actual problem. What we needed to do was wait until our subscription in the test got matched with the introspection publishers coming from the service introspection. I did that in 309cd85, and it now seems to work for me. Can you give it another shot and see if that fixes it for you?

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

Pulls: #2684
Gist: https://gist.githubusercontent.com/clalancette/650739ed96ad68a0e4c2af0856849cc8/raw/11b53a012ea22e5bea0fa425ea847fae021f9d2d/ros2.repos
BUILD args: --packages-up-to rclcpp
TEST args: --packages-select rclcpp
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/14871

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants