-
Notifications
You must be signed in to change notification settings - Fork 429
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
Add basic nominal test of a client #931
Conversation
Signed-off-by: Monika Idzik <[email protected]>
rcl_interfaces::msg::ParameterValue resp; | ||
resp.string_value = request_str; | ||
response->values.push_back(resp); | ||
return; |
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.
nitpick: you can remove this return statement.
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.
Thank you for adding the test. Note though, that we have a similar integration test in test_rclcpp and in test_communication (both executing tests for different RMWs). I think this is why we don't see pub/sub or client/server communication tests in rclcpp
; it's a bit redundant.
Since this test is already covered elsewhere and adding it creates more code to maintain, I'm inclined to reject this PR, unless someone thinks otherwise (@ros2/team)
I don't feel strongly about it, in the past we had the As for duplication, I agree that we should try to avoid it, but I'm ambivalent about having a basic service client test here. Another dimension to consider is a service client/server test in a single process/node versus in separate processes/nodes, where the latter seems to be more common in |
Closing as there's already a test in https://github.com/ros2/system_tests/blob/99ec3f9d03366f521b9f188159ab1acbd3f87017/test_rclcpp/test/test_services_client.cpp. |
Signed-off-by: Kentaro Tanaka <[email protected]>
* Add `start_offset` field to `PlayOptions`, exposed to CLI as `ros2 bag play --start-offset SECONDS`, which allows for starting playback after the beginning of a bag. Signed-off-by: Abrar Rahman Protyasha <[email protected]>
No description provided.