-
Notifications
You must be signed in to change notification settings - Fork 251
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
Test all RMW implementations for rosbag2_transport #349
Test all RMW implementations for rosbag2_transport #349
Conversation
4d52dde
to
d7bafb0
Compare
Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
d7bafb0
to
bc07f7e
Compare
Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
bc07f7e
to
2649ea0
Compare
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.
LGTM but maybe some comments to document the new functions a bit so that it might be a bit easier for new contributors to understand what's going on (or where to add new 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.
+1 on Anas comment, see e.g. how https://github.com/ros2/rmw/blob/master/rmw_implementation_cmake/cmake/call_for_each_rmw_implementation.cmake is documented, we should do the same for each function/macro we introduce. Consider also moving those functions into a separate cmake file so that the CMakeLists.txt is free of clutter.
Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
Ok - I moved it to a separate file and added documentation |
Depends on ament/ament_cmake#236 - there is a bug in
ament_add_gmock
that shows up when I start usingENV
Iterate over all RMW implementations for the rosbag2_transport tests, because it's been the case recently that some tests will pass on fastrtps but fail on cyclonedds.