fix: QoS incompatibilites are not expected with rmw_zenoh_cpp #1936
Conversation
Signed-off-by: yuanyuyuan <[email protected]>
|
@YuanYuYuan, Could you please provide more context to explain why this fix is needed and why the test fails with |
Hi @MichaelOrlov! This is a fix similar to what we did in rclcpp. ros2/rclcpp#2639. The reason is rmw_zenoh_cpp is not a DDS-backboned RMW. The assumption of QoS incompatibility of reliability doesn't mean the subscriber receives no message. On the other hand, the user can still use rmw_qos_profile_check_compatible to check the compatibility. |
| @@ -1244,8 +1245,14 @@ TEST_F(RosBag2PlayQosOverrideTestFixture, topic_qos_profiles_overridden_incompat | |||
| topic_name_, num_msgs_to_wait_for_, qos_request); | |||
| play_options_.topic_qos_profile_overrides = topic_qos_profile_overrides; | |||
|
|
|||
| // QoS incompatibilities are not expected with rmw_zenoh_cpp. | |||
| bool expect_timeout = true; | |||
| if (std::strcmp(rmw_get_implementation_identifier(), "rmw_zenoh_cpp") == 0) { | |||
There was a problem hiding this comment.
what about using here qos_check_compatible ? In other test we do somthing like:
if (qos_check_compatible(qos_profile_publisher,
qos_profile_subscription).compatibility != rclcpp::QoSCompatibility::Ok)
{
There was a problem hiding this comment.
this sounds good idea. this API can be called on anywhere before play_and_wait.
instead of putting the hard code with specific implementation in the test to be skipped, maybe asking rmw implementation if that is compatible or incompatible qos configuration, and based on that we change the expectation resut. this can be more flexible and maintainability since it does not depend on rmw specific code.
There was a problem hiding this comment.
Yes, this is a better modification. Updated in 21d59f5
There was a problem hiding this comment.
Sorry, I have a bit opposed opinion. If rmw_zenoh do not expect any QoS incompatibilities it doesn't make sense to run this test at all.
In my opinion, it would be much cleaner if we would skip this test for the rmw_zenoh with an explanation like
// If the RMW implementation is rmw_zenoh_cpp, we do not expect any QoS incompatibilities.
rather than trying to work around expectations in this test.
There was a problem hiding this comment.
IMO, incompatible or not, it needs to call the API to the rmw implementation since that is not what rosbag2 or clients interface can decide. in the case of other rmw implementation support or deprecating rmw implementation in the future, we do not need to come back here to maintain. i think, that is better for the maintenance and integrity of the test. but i am okay with @MichaelOrlov opinion, if anybody agrees with that 👍
There was a problem hiding this comment.
I agree with @fujitatomoya. Communicating through the API is better for long-term maintenance. With this change, the test now verifies RMWs that require QoS compatibility. It still makes sense to me.
Signed-off-by: yuanyuyuan <[email protected]>
YuanYuYuan
force-pushed
the
fix/rmw_zenoh_cpp-qos
branch
from
21d59f5 to
d14e6da
Compare
This PR aims to address ros2/rmw_zenoh#510