Add service/client construction/destruction API test coverage. #138
Conversation
|
CI up to
|
| ASSERT_NE(nullptr, srv) << rmw_get_error_string().str; | ||
|
|
||
| // Destroying service with invalid arguments fails. | ||
| rmw_ret_t ret = rmw_destroy_service(nullptr, srv); |
There was a problem hiding this comment.
If this succeeds, couldn't the failure bleed into the other checks below? Should you be creating a valid service every time?
There was a problem hiding this comment.
Failures would bleed out, yes, and likely cause a crash, as the service has been freed.
We could split each bad destruction attempt into its own test, and assert that the destruction call did not succeed before proceeding to destruct it (which is part of the test). But either way we have to trust rmw_destroy_service() outcome. There's no other way to test for validity.
Would you have rather have N tests? I don't feel strongly about it.
There was a problem hiding this comment.
I do like N TEST_Fs a bit better, especially if you created a small test fixture. I'm realizing I probably did something like what you've done here plenty of times, but more recently, I've been leaning towards more TEST_F rather than fewer so issues in failing tests can be isolated quickly.
There was a problem hiding this comment.
Fair enough. See 8680b3e.
| ASSERT_NE(nullptr, client) << rmw_get_error_string().str; | ||
|
|
||
| // Destroying client with invalid arguments fails. | ||
| rmw_ret_t ret = rmw_destroy_client(nullptr, client); |
There was a problem hiding this comment.
If this unintentionally modifies client, it could impact the other checks below. It might be better to recreate client each time you try to destroy it, or check it's valid each time
hidmic
changed the title
Signed-off-by: Michel Hidalgo <[email protected]>
Signed-off-by: Michel Hidalgo <[email protected]>
hidmic
force-pushed
the
hidmic/rmw-service-construction-destruction-tests
branch
from
d766e41 to
399e059
Compare
|
CI up to
|
Signed-off-by: Michel Hidalgo <[email protected]>
|
CI up to
|
|
@ros-pull-request-builder retest this please. |
|
@ros-pull-request-builder retest this please. |
1 similar comment
|
@ros-pull-request-builder retest this please. |
|
Finally all's green ! |
delete-merged-branch
bot
deleted the
hidmic/rmw-service-construction-destruction-tests
branch
Signed-off-by: Michel Hidalgo <[email protected]>
Signed-off-by: Michel Hidalgo <[email protected]>
Precisely what the title says. Depends on ros2/rmw_fastrtps#445, ros2/rmw_cyclonedds#247, and ros2/rmw_connext#464.