Wait for server in test_rmw_implementation service tests #191
Conversation
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
| rmw_ret_t ret = rmw_service_server_is_available(node, client, &is_available); | ||
| EXPECT_EQ(RMW_RET_OK, ret) << rmw_get_error_string().str; | ||
| ASSERT_FALSE(is_available); |
There was a problem hiding this comment.
Just so I understand properly; we don't need to wait here because we never created a server at all, right?
There was a problem hiding this comment.
That's my recollection, yes.
| EXPECT_EQ(RMW_RET_OK, ret) << rmw_get_error_string().str; | ||
| rmw_reset_error(); | ||
| if (is_available) { |
There was a problem hiding this comment.
I'm not sure that this is the right logic here (I also don't think the previous logic is correct either).
What we are doing here is looping for a specified amount of time, waiting to see if the service server has become available. From what I understand from the documentation, rmw_service_server_is_available should always return RMW_RET_OK, unless the typesupport didn't match or an unexpected error occurred. But in those latter 2 cases, that is clearly a failure of this test, and so further waiting doesn't seem warranted. Thus, I would think this logic would be:
if (ret != RMW_RET_OK) {
break;
}
if (is_available) {
break;
}
And then we keep both expectations below. What do you think?
(this same comment applies to both loops below)
There was a problem hiding this comment.
I'd be fine with it. We could also just ASSERT_EQ(RMW_RET_OK, ret) << rmw_get_error_string().str; right after calling rmw_service_is_available. Which one would you prefer?
There was a problem hiding this comment.
Both alternatives sound fine to me
There was a problem hiding this comment.
I'd be fine with it. We could also just
ASSERT_EQ(RMW_RET_OK, ret) << rmw_get_error_string().str;right after callingrmw_service_is_available. Which one would you prefer?
The downside to doing an ASSERT is that we'd never hit the cleanup. While that doesn't matter too much in a test, it has the side-effect of making clang static analysis complain, which I'd rather avoid.
There was a problem hiding this comment.
Oh, but that's because the test is not doing clean-up properly ! See 2da26ad.
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
|
Regular CI up to
|
delete-merged-branch
bot
deleted the
hidmic/wait-for-service-server-in-tests
branch
Current tests assume service server and client in the same process discover each other immediately. That doesn't seem to be true for
rmw_connextddsand thus the flakes in amd64 Linux repeated CI, which can be reproduced locally (FYI @asorbini).This patch amends this.
Repeated CI up to
test_rmw_implementation: