-
Notifications
You must be signed in to change notification settings - Fork 430
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 some tests in test_qos_event and run others with event types supported by rmw_zenoh #2626
Conversation
Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -561,6 +618,11 @@ TEST_F(TestQosEvent, test_pub_matched_event_by_option_event_callback) | |||
|
|||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
matched events should definitely work else there is a bug in rmw_zenoh.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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_count
andtotal_count_change
updates. 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>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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