Regression test for set_parameters with bad callback reference #253
Conversation
| node->parameters_client_ = | ||
| std::make_shared<rclcpp::parameter_client::AsyncParametersClient>(node); | ||
| if (!node->parameters_client_->wait_for_service(20s)) { | ||
| ASSERT_TRUE(false) << "service not available after waiting"; |
There was a problem hiding this comment.
I saw that you copied it from above but do you know why we don't just use FAIL() here?
FAIL() << "service not available after waiting";
There was a problem hiding this comment.
perhaps it doesn't allow a message (it's only used as FAIL() elsewhere FWICS)
There was a problem hiding this comment.
Yeah we can look into what the differences are at a future date.
Note to future-self @mikaelarguedas: consider replacing those in the future by FAIL() or ADD_FAILURE() based on context.
references: places where we use it, gtest example
|
this PR still needs review CI OS X including a1ea944: |
| executor.add_node(node); | ||
| node->queue_set_parameter_request(executor); | ||
| // When the parameters client receives its response it will cancel the executor. | ||
| executor.spin(); |
There was a problem hiding this comment.
The only issue here is that there is no timeout for the spin. If the callback is not called the test will hang.
You could instead:
- create a promise and a future from the promise
- pass the promise to
queue_set_parameter_request - capture the promise in the lambda callback
- set the promise in the lambda callback
spin_until_future_completeon the future and with a timeout- assert the future is set after
spin_until_future_completereturns (i.e. assert it didn't timeout)
There was a problem hiding this comment.
Thanks for the idea, I wasn't aware of this way to make the test fail in a way other than timeout: it's a better approach since this test file has a really long time out because of all the wait_for_service calls. I won't have time to update this tonight (and the test shouldn't be failing as it stands) so I'll still merge as is for now
Connects to ros2/rclcpp#414
Regression test for ros2/rclcpp#414
Without ros2/rclcpp#414, the test will crash (I'm getting
C++ exception with description "std::bad_alloc" thrown in the test body.but in https://github.com/ros2/demos/pull/203/files#diff-a5a151889abb3214250ba9db05358e4bR92 it would segfault).CI including ros2/rclcpp#414