Skip to content
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

Closed
wants to merge 1 commit into from

Conversation

monidzik
Copy link
Contributor

No description provided.

Signed-off-by: Monika Idzik <[email protected]>
rcl_interfaces::msg::ParameterValue resp;
resp.string_value = request_str;
response->values.push_back(resp);
return;
Copy link
Member

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.

Copy link
Member

@jacobperron jacobperron left a 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)

@wjwwood
Copy link
Member

wjwwood commented Dec 4, 2019

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 test_rclcpp package because it was not possible to make tests that ran against multiple rmw implementations from within the rclcpp package (it was a technical limitation that has since been removed). So in principal it would make sense for many, if not all of those test to get "repatriated" to rclcpp so that the tests could live closer to the code. In the mean time, however, there may be new reasons to keep things in test_rclcpp, like dependencies we don't want in rclcpp or just keeping the test time down in rclcpp itself...

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 test_rclcpp.

@ivanpauno
Copy link
Member

Closing as there's already a test in https://github.com/ros2/system_tests/blob/99ec3f9d03366f521b9f188159ab1acbd3f87017/test_rclcpp/test/test_services_client.cpp.
Tests in test_rclcpp could be moved to rclcpp, to avoid this confusions.
Opened tracking issue ros2/system_tests#396.

@ivanpauno ivanpauno closed this Jan 30, 2020
nnmm pushed a commit to ApexAI/rclcpp that referenced this pull request Jul 9, 2022
DensoADAS pushed a commit to DensoADAS/rclcpp that referenced this pull request Aug 5, 2022
* 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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants