-
Notifications
You must be signed in to change notification settings - Fork 317
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 unit tests for server / client #13
Conversation
33e2992
to
ada6cb0
Compare
ada6cb0
to
94a64c7
Compare
For now the CMake only creates tests for the rmw variants which have been implemented: http://54.183.26.131:8080/view/ros2/job/ros2_batch_ci_linux/38/testReport/(root)/ To avoid diverging this should be merged as-is. The Connext XTypes test can be activated in ros2/ros2#27. |
|
||
auto wait_for_response_until = std::chrono::steady_clock::now() + | ||
std::chrono::seconds(1); | ||
std::future_status status; |
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.
You can cherry pick https://github.com/ros2/rclcpp/pull/20/files#diff-690567624ee368716009118718985564R81 and https://github.com/ros2/rclcpp/pull/20/files#diff-294cbb523fb3d56a1df2a0a7ff0beb9bR31 to replace this snippet with spin_until_future_complete
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.
Depending on which one gets merged first. If this get merged first I would say we should update the referenced PRs. If this one gets merged after the referenced PRs I can update this to use the new API.
+1 |
2 similar comments
+1 |
+1 |
add unit tests for server / client
Connects to ros2/ros2#27.
http://54.183.26.131:8080/view/ros2/job/ros2_batch_ci_linux/29/
The test passes for:
It fails for:
since support for services has not been implemented for that rmw implementation yet.