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

Skip all multi-vendor pub/sub tests with zenoh #560

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

Conversation

Yadunund
Copy link
Member

@Yadunund Yadunund commented Feb 25, 2025

Address #559.

Note: we already skip all multi-vendor tests between other RMWs for services & actions.

@Yadunund Yadunund marked this pull request as draft February 26, 2025 19:55
Signed-off-by: Yadunund <[email protected]>
@Yadunund Yadunund marked this pull request as ready for review February 26, 2025 21:39
@cottsay
Copy link
Member

cottsay commented Feb 26, 2025

I'm not 100% certain about the mechanics at play here, but I think it would be more correct to restore the previous behavior using the call to continue(). Skipping a test carries some implication that the test is valid but we're choosing not to run it, where I think in this case the test isn't valid - we'd never want to attempt this sort of communication. If I'm reading the CMake correctly, the call to continue() results in no test being created at all, which I think is what we want.

@Yadunund
Copy link
Member Author

I'm not 100% certain about the mechanics at play here, but I think it would be more correct to restore the previous behavior using the call to continue(). Skipping a test carries some implication that the test is valid but we're choosing not to run it, where I think in this case the test isn't valid - we'd never want to attempt this sort of communication. If I'm reading the CMake correctly, the call to continue() results in no test being created at all, which I think is what we want.

I had assumed the same but with continue(), I still saw cross-vendor tests being generated with rmw_zenoh in my local testing. Will look closer at what's going on.

(rmw_implementation1_is_zenoh AND NOT rmw_implementation2_is_zenoh) OR
(rmw_implementation2_is_zenoh AND NOT rmw_implementation1_is_zenoh)
)
set(SKIP_TEST "SKIP_TEST")
Copy link
Contributor

Choose a reason for hiding this comment

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

i think if we call continue() here, it will start the test with add_launch_test with timeout 0. it seems strange that there is no message type WStrings.msg either in "rosidl_interfaces" or "test_msgs"... maybe in the past, there were cases to skip those cases?

i would suggest that we can call set(SKIP_TEST "SKIP_TEST") line 206 right after validating which rmws are going to be tested? we do not have to check that in the foreach loop of message types, which are irrelevant from the criteria?

Copy link
Member Author

Choose a reason for hiding this comment

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

i think if we call continue() here, it will start the test with add_launch_test with timeout 0.

Thanks for providing that insight. Instead of timing out, wouldn't it be better to skip this test entirely with set(SKIP_TEST "SKIP_TEST") as I have it now?

it seems strange that there is no message type WStrings.msg either in "rosidl_interfaces" or "test_msgs"... maybe in the past, there were cases to skip those cases?

My understanding is that all rosidls generate the WString message by default and without a WString.msg file defined in test_msgs. See ros2/rosidl#352. This message is then included here in test_communication. I've checked my local workspace and indeed there is such a header file present. So I don't think we want to skip these tests entirely...

Copy link
Contributor

Choose a reason for hiding this comment

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

i see, that makes more sense. thanks for sharing the information.

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually with continue(), I'm seeing the test timeout being generated for 60s 🤔

yadunund@ubuntu-24-04:~/ros2_rolling_ci$ colcon test --event-handlers console_cohesion+ console_direct+ --packages-select test_communication --ctest-args -R test_publisher_subscriber__rclcpp__rmw_zenoh_cpp__rmw_fastrtps_dynamic_cpp
Starting >>> test_communication
UpdateCTestConfiguration  from :/home/yadunund/ros2_rolling_ci/build/test_communication/CTestConfiguration.ini
Parse Config file:/home/yadunund/ros2_rolling_ci/build/test_communication/CTestConfiguration.ini
   Build name: (empty)
 Add coverage exclude regular expressions.
Create new tag: 20250303-2050 - Experimental
UpdateCTestConfiguration  from :/home/yadunund/ros2_rolling_ci/build/test_communication/CTestConfiguration.ini
Parse Config file:/home/yadunund/ros2_rolling_ci/build/test_communication/CTestConfiguration.ini
Test project /home/yadunund/ros2_rolling_ci/build/test_communication
Constructing a list of tests
Done constructing a list of tests                
Updating test list for fixtures
Added 0 tests to meet fixture requirements
Checking test dependency graph...
Checking test dependency graph end
test 242
    Start 242: test_publisher_subscriber__rclcpp__rmw_zenoh_cpp__rmw_fastrtps_dynamic_cpp

242: Test command: /usr/bin/python3 "-u" "/home/yadunund/ros2_rolling_ci/install/ament_cmake_test/share/ament_cmake_test/cmake/run_test.py" "/home/yadunund/ros2_rolling_ci/build/test_communication/test_results/test_communication/test_publisher_subscriber__rclcpp__rmw_zenoh_cpp__rmw_fastrtps_dynamic_cpp.xunit.xml" "--package-name" "test_communication" "--output-file" "/home/yadunund/ros2_rolling_ci/build/test_communication/launch_test/test_publisher_subscriber__rclcpp__rmw_zenoh_cpp__rmw_fastrtps_dynamic_cpp.txt" "--env" "PYTHONDONTWRITEBYTECODE=1" "--append-env" "LD_LIBRARY_PATH=/home/yadunund/ros2_rolling_ci/build/test_communication" "--command" "/usr/bin/python3" "-m" "launch_testing.launch_test" "/home/yadunund/ros2_rolling_ci/build/test_communication/test_publisher_subscriber__rclcpp__rmw_zenoh_cpp__rmw_fastrtps_dynamic_cpp_Release.py" "--junit-xml=/home/yadunund/ros2_rolling_ci/build/test_communication/test_results/test_communication/test_publisher_subscriber__rclcpp__rmw_zenoh_cpp__rmw_fastrtps_dynamic_cpp.xunit.xml" "--package-name=test_communication"
242: Working Directory: /home/yadunund/ros2_rolling_ci/build/test_communication
242: Test timeout computed to be: 60
242: -- run_test.py: extra environment variables:
242:  - PYTHONDONTWRITEBYTECODE=1
242: -- run_test.py: extra environment variables to append:
242:  - LD_LIBRARY_PATH+=/home/yadunund/ros2_rolling_ci/build/test_communication
242: -- run_test.py: invoking following command in '/home/yadunund/ros2_rolling_ci/build/test_communication':
242:  - /usr/bin/python3 -m launch_testing.launch_test /home/yadunund/ros2_rolling_ci/build/test_communication/test_publisher_subscriber__rclcpp__rmw_zenoh_cpp__rmw_fastrtps_dynamic_cpp_Release.py --junit-xml=/home/yadunund/ros2_rolling_ci/build/test_communication/test_results/test_communication/test_publisher_subscriber__rclcpp__rmw_zenoh_cpp__rmw_fastrtps_dynamic_cpp.xunit.xml --package-name=test_communication
242: [INFO] [launch]: Default logging verbosity is set to INFO
242: [INFO] [daemon-stop-1]: process started with pid [102325]
242: [INFO] [daemon-stop-1]: process has finished cleanly [pid 102325]
242: test_subscriber_terminates_in_a_finite_amount_of_time[] (test_communication.TestPublisherSubscriber.test_subscriber_terminates_in_a_finite_amount_of_time[])
242: Test that the subscriber terminates after a finite amount of time. ... [INFO] [test_publisher-2]: process started with pid [102362]
242: [INFO] [test_subscriber-3]: process started with pid [102363]
242: [test_publisher-2] Unknown message argument ''
242: [ERROR] [test_publisher-2]: process has died [pid 102362, exit code 1, cmd '/home/yadunund/ros2_rolling_ci/build/test_communication/test_publisher_cpp /test_time_20_50_15'].
242: [test_subscriber-3] Unknown message argument ''
242: [ERROR] [test_subscriber-3]: process has died [pid 102363, exit code 1, cmd '/home/yadunund/ros2_rolling_ci/build/test_communication/test_subscriber_cpp /test_time_20_50_15'].
242: ok

Copy link
Member

Choose a reason for hiding this comment

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

Just gaining context here, but I guess this is unexpected behavior? If the test is skipped, why are we still getting a 60 second timeout?

Copy link
Contributor

@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

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