Adds action interaction tests #352
Conversation
|
|
||
| #include "test_msgs/action/fibonacci.h" | ||
|
|
||
| #ifdef RMW_IMPLEMENTATION |
There was a problem hiding this comment.
I guess we'll need some cmake logic for testing multiple RMW implementations. Perhaps we can rebase after #351 is merged.
There was a problem hiding this comment.
you're right, just did as suggested. tests are now running against multiple RWM implementations. thanks!
| ASSERT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str; | ||
| context = rcl_get_zero_initialized_context(); | ||
| ret = rcl_init(0, nullptr, &init_options, &context); | ||
|
|
There was a problem hiding this comment.
nit: Remove the empty line since the assert below is associated with rcl_init above.
| EXPECT_EQ(ret, RCL_RET_OK) << rcl_get_error_string().str; | ||
| rcl_reset_error(); | ||
|
|
||
| EXPECT_TRUE(this->is_goal_request_ready) << rcl_get_error_string().str; |
There was a problem hiding this comment.
If there was an error string, it was reset at L218.
In general, I'm not sure if we need to call rcl_reset_error() in the tests unless we're expecting an error.
There was a problem hiding this comment.
just did as suggested, thanks!
| EXPECT_EQ(outgoing_goal_request.order, incoming_goal_request.order); | ||
| rcl_reset_error(); | ||
| EXPECT_TRUE(uuidcmp(outgoing_goal_request.uuid, incoming_goal_request.uuid)); | ||
| rcl_reset_error(); |
There was a problem hiding this comment.
This rcl_reset_error() is redundant with the previous since uuidcmp does not set an error. But again, we might be able to remove most, if not all, of the rcl_reset_error() calls.
| test_msgs__action__Fibonacci_Result_Request__fini(&incoming_result_request); | ||
| test_msgs__action__Fibonacci_Result_Request__fini(&outgoing_result_request); | ||
| test_msgs__action__Fibonacci_Result_Response__fini(&incoming_result_response); | ||
| test_msgs__action__Fibonacci_Result_Response__fini(&outgoing_result_response); |
There was a problem hiding this comment.
Missing finalize calls for feedback messages
There was a problem hiding this comment.
oops, fixed. thanks!
| outgoing_cancel_request.goal_info.stamp.nanosec, | ||
| incoming_cancel_request.goal_info.stamp.nanosec); | ||
|
|
||
| rcl_action_cancel_request_t cancel_request = rcl_action_get_zero_initialized_cancel_request(); |
There was a problem hiding this comment.
rcl_action_action_msgs__srv__CancelGoal_Request is typedef'd to rcl_action_cancel_request_t, so I think this logic for doing the "conversion" is not necessary. I may be mistaken, but I think we can use incoming_cancel_request directly with rcl_action_process_cancel_request. Note, the same is not true for rcl_action_cancel_response_t.
There was a problem hiding this comment.
You're right, it wasn't needed. Done!
| &this->action_server, &cancel_request, &cancel_response); | ||
| EXPECT_EQ(ret, RCL_RET_OK) << rcl_get_error_string().str; | ||
|
|
||
| // Initialize cancel response |
There was a problem hiding this comment.
Do we really need to do this, or can we use cancel_response.msg directly? Perhaps asserting some things about it first, e.g. EXPECT_EQ(cancel_response.msg.goals_canceling.size, 2u).
There was a problem hiding this comment.
Just removed that initialization part
| ASSERT_EQ(ret, RCL_RET_OK) << rcl_get_error_string().str; | ||
| rcl_reset_error(); | ||
|
|
||
| ret = rcl_wait(&this->wait_set, 1000000); |
| // the server accepting the goal (synchronous). Upon accepting the goal, the | ||
| // action server starts a user defined execution method for completing the goal. | ||
| // Following the goal request, the client makes an asynchronous request for the | ||
| // result. The user defined method publishes feedback to the action client as it |
There was a problem hiding this comment.
The sentences about the "user defined method" are not really relevant to rcl, but are referring to client library implementations built on top of rcl. We can probably omit that part of the comment.
There was a problem hiding this comment.
you're right. done, thanks!
|
|
||
| // Exercises the "Example 2" sequence diagram found in the actions_proposal document. | ||
| // This example is almost identical to the first, but this time the action client requests | ||
| // for the goal to be canceled mid-execution. Note that the user defined method is allowed |
There was a problem hiding this comment.
Same thing about the "user defined method", we can probably remove that sentence.
|
CI status: |
|
Thanks for the review @jacobperron , just finished addressing your comments. PTAL |
Adds two tests based on the action_proposal sequence diagrams found here and here.