Add service/client construction/destruction API test coverage.#138
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.
| 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
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
d766e41 to
399e059
Compare
|
CI up to
|
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
|
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 ! |
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Precisely what the title says. Depends on ros2/rmw_fastrtps#445, ros2/rmw_cyclonedds#247, and ros2/rmw_connext#464.