Conversation
Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
Yadunund
left a comment
There was a problem hiding this comment.
Apart from deadline and lifetime QoS events, rmw_zenoh supports all other events including matched pub/sub. I've left some suggestions on how we can have some of these tests run for rmw_zenoh.
|
|
||
| TEST_F(TestQosEvent, test_sub_matched_event_by_option_event_callback) | ||
| { | ||
| std::string rmw_implementation_str = std::string(rmw_get_implementation_identifier()); |
There was a problem hiding this comment.
matched events should definitely work else there is a bug in rmw_zenoh.
There was a problem hiding this comment.
failing here too
Expected equality of these values:
s.total_count
Which is: 7
matched_expected_result.total_count
Which is: 1
/home/ahcorde/ros2_rolling/src/ros2/rclcpp/rclcpp/test/rclcpp/test_qos_event.cpp:620: Failure
Expected equality of these values:
s.total_count_change
Which is: 2
matched_expected_result.total_count_change
Which is: 1
/home/ahcorde/ros2_rolling/src/ros2/rclcpp/rclcpp/test/rclcpp/test_qos_event.cpp:621: Failure
Expected equality of these values:
s.current_count
Which is: 3
matched_expected_result.current_count
Which is: 1
/home/ahcorde/ros2_rolling/src/ros2/rclcpp/rclcpp/test/rclcpp/test_qos_event.cpp:619: Failure
Expected equality of these values:
s.total_count
Which is: 8
matched_expected_result.total_count
Which is: 1
/home/ahcorde/ros2_rolling/src/ros2/rclcpp/rclcpp/test/rclcpp/test_qos_event.cpp:620: Failure
Expected equality of these values:
s.total_count_change
Which is: 1
matched_expected_result.total_count_change
Which is: 0
/home/ahcorde/ros2_rolling/src/ros2/rclcpp/rclcpp/test/rclcpp/test_qos_event.cpp:621: Failure
Expected equality of these values:
s.current_count
Which is: 2
matched_expected_result.current_count
Which is: 0
There was a problem hiding this comment.
So this relates to what I mentioned above where we should update the test similar to 4) in ros2/rcl#1164. Basically more QoS combinations are compatible in rmw_zenoh so we should set the matched_expected_result.current_count to the output of rmw_qos_profile_check_compatible checks.
There was a problem hiding this comment.
There might be some tests that we can fix with your comment, but the subscription counter is wrong, it's only increasing.
If you comment out some tests the numbers are different which somehow don't clean the counters between tests.
There was a problem hiding this comment.
This looks like a bug in rmw_zenoh. I would suggest we don't skip this test and instead have it fail so that we can address it.
We should only skip the tests for functionalities we do not support.
There was a problem hiding this comment.
After looking at the problem closer; the issue is that
- rmw_init() is called once in
SetUpTestCase()which creates the Zenoh session and initializes the ROS graph. - Each time a new test case runs, we run
SetUp()andTearDown()which creates and destroys the node and some publishes and subscriptions. However, since the session is still the same throughout the tests each time a new pub/sub match is found, the sametotal_countandtotal_count_changeupdates. From the sessions's perspective, the total count is indeed valid since it was never shutdown.
In 6c9baba, I moved the rmw_init() command inside SetUp() and the rmw_shutdown() inside TearDown(), the total_count numbers are accurate. With ros2/rmw_zenoh#287 all events tests are passing.
Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
…ean_rmw_zenoh_cpp_test
|
@ahcorde we should also skip |
…ean_rmw_zenoh_cpp_test
Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
…cpp_test' into ahcorde/rolling/clean_rmw_zenoh_cpp_test
Signed-off-by: Yadunund <yadunund@intrinsic.ai>
Signed-off-by: Yadunund <yadunund@intrinsic.ai>
Signed-off-by: Yadunund <yadunund@intrinsic.ai>
Yadunund
left a comment
There was a problem hiding this comment.
I pushed some commits to make sure we run a lot of the tests here and only skip the ones that we truly don't support in rmw_zenoh. Tests that were checking for construction/destruction of event callbacks are updated to use events that we support. The diff looks big but that's because I moved some code within an indented block that runs only if the middleware is not rmw_zenoh_cpp.
One big behavioral change is that I initialize and shutdown the middleware between each test case. Without this, the ROS graph which is tied to the context (and hence initialized and shutdown only once) is reused for each test.
|
Pulls: #2626
|
Related to this issue ros2/rmw_zenoh#280