Complete init/shutdown API test coverage. #107
Conversation
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
|
These tests need ros2/rmw#242 and connected PRs to pass. |
| rmw_context_t context = rmw_get_zero_initialized_context(); | ||
| rmw_init_options_t options = rmw_get_zero_initialized_init_options(); | ||
| rmw_ret_t ret = rmw_init_options_init(&options, rcutils_get_default_allocator()); | ||
| EXPECT_NE(RMW_RET_OK, rmw_shutdown(&context)); |
There was a problem hiding this comment.
Is there a specific error you are expecting? RMW_RET_ERROR?
There was a problem hiding this comment.
Based on rmw_shutdown() API contract, an RMW implementation could equally return RMW_RET_INVALID_ARGUMENT or RMW_RET_INCORRECT_IMPLEMENTATION. That's why I don't check for a specific return code.
There was a problem hiding this comment.
Since you already test the incorrect_implementation error below, wouldn't it make more sense to test the invalid_argument error here? Set context.implementation_identifier to a known valid identifier, then check that it returns invalid argument?
There was a problem hiding this comment.
Hmm, problem is that we get into bad initialization territory. Here I deal with the four (4) known context states: zero-initialized, initialized and not shutdown, initialized and shutdown, finalized. Anything outside that is UB. So if we want to be specific about the assertion we have to further clarify API contracts i.e. what it means for a context to be invalid for all rmw implementations.
There was a problem hiding this comment.
If you don't want to monkey around with the identifier field to force the invalid argument, then I recommend to at least narrow down the expect statement to only two errors you expect. Something like:
EXPECT_TRUE((ret == RMW_RET_INVALID_ARGUMENT ||
ret == RMW_RET_INCORRECT_IMPLEMENTATION))
There was a problem hiding this comment.
Sure, we can do that. See 9bff7f2
|
CI up to
|
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
|
Hold on, as tests may change with ros2/rmw#243. |
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
|
Ready for re-review ! |
|
Does this need to be retested on CI? |
More than that. RMW implementations have to be released into |
|
CI up to
|
|
Same as in #108, I'll hold off merging this until I've re-released RMWs so that rolling PR jobs pass. |
|
@ros-pull-request-builder retest this please |
|
Alright, all's green! Going in. |
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Precisely what the title says. Follow-up after #106.