Add tests checking when a graph guard condition is triggered #736
Conversation
Signed-off-by: Ada-King <[email protected]>
| @@ -1295,6 +1295,32 @@ TEST_F(CLASSNAME(TestGraphFixture, RMW_IMPLEMENTATION), test_graph_guard_conditi | |||
| ASSERT_GE(graph_changes_count, 4ul); | |||
| } | |||
|
|
|||
| /* Test the graph when guard condition is triggered. | |||
There was a problem hiding this comment.
| /* Test the graph when guard condition is triggered. | |
| /* | |
| * Test the graph when guard condition is triggered. |
| [ = ]() { | ||
| std::this_thread::sleep_for(std::chrono::milliseconds(200)); | ||
| rcl_ret_t ret = | ||
| rcl_trigger_guard_condition(const_cast<rcl_guard_condition_t *>(graph_guard_condition)); |
There was a problem hiding this comment.
| rcl_trigger_guard_condition(const_cast<rcl_guard_condition_t *>(graph_guard_condition)); | |
| rcl_trigger_guard_condition(const_cast<rcl_guard_condition_t *>(graph_guard_condition)); |
| */ | ||
| TEST_F(CLASSNAME(TestGraphFixture, RMW_IMPLEMENTATION), test_graph_guard_condition_triggered) { | ||
| rcl_ret_t ret; | ||
| int64_t wait_time_out = -1; |
There was a problem hiding this comment.
| int64_t wait_time_out = -1; | |
| const int64_t wait_time_out = -1; |
Or consider getting rid of it completely, since it never changes.
|
|
||
| ret = rcl_wait(this->wait_set_ptr, wait_time_out); | ||
| EXPECT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str; | ||
| trigger_thread.join(); |
There was a problem hiding this comment.
What is the higher level purpose of this test? From my perspective it's essentially testing that triggering a guard condition will wake up a wait set, but is the goal to do something specific to the graph guard condition?
Reading #574, I don't think this test really addresses it, because it doesn't check that the graph guard condition is triggered by certain events, nor does it assert anything about behavior that utilizes the graph guard condition (e.g. the GraphListener in rclcpp).
@ivanpauno perhaps you could explain what kind of test you were expecting from #574?
There was a problem hiding this comment.
@ivanpauno perhaps you could explain what kind of test you were expecting from #574?
Sorry, my description of the issue in #574 is a bit poor.
AFAIR, my motivation for opening that issue was that some rmw implementations weren't triggering the graph guard condition in all graph changes.
My idea was to add more tests that check that after each function that can have a "graph side effect", the graph guard conditions is triggered within a timeout.
e.g.:
-> create a node
-> check that graph guard condition is triggered within a timeout
-> create a publisher
-> check that graph guard condition is triggered within a timeout
... etc for everything that can produce a graph change
I think that the only existing test is this one.
That test is a bit fragile, but my basic idea is to make that more extensive to cover all graph changes.
There are some cases that are hard to test, i.e. making sure that a graph guard condition is triggered when a publisher matching an existing subscription is detected (in that case the graph guard condition is triggered twice, once because the publisher was detected and the other because the subscription found a matching publisher).
What is the higher level purpose of this test?
I also don't see value in the test added here.
There was a problem hiding this comment.
I have edited #574 to make its purpose more clear.
|
@Ada-King Friendly ping here, are you able to respond to the comments from William and Ivan? |
Related issue : #574
Signed-off-by: Ada-King [email protected]