Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add tests checking when a graph guard condition is triggered #736

Closed
wants to merge 1 commit into from
Closed

Add tests checking when a graph guard condition is triggered #736

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 26 additions & 0 deletions rcl/test/rcl/test_graph.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/* Test the graph when guard condition is triggered.
/*
* Test the graph when guard condition is triggered.

*/
TEST_F(CLASSNAME(TestGraphFixture, RMW_IMPLEMENTATION), test_graph_guard_condition_triggered) {
rcl_ret_t ret;
int64_t wait_time_out = -1;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
int64_t wait_time_out = -1;
const int64_t wait_time_out = -1;

Or consider getting rid of it completely, since it never changes.

const rcl_guard_condition_t * graph_guard_condition =
rcl_node_get_graph_guard_condition(this->node_ptr);
ASSERT_NE(nullptr, graph_guard_condition) << rcl_get_error_string().str;
ret = rcl_wait_set_clear(this->wait_set_ptr);
ASSERT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str;
ret = rcl_wait_set_add_guard_condition(this->wait_set_ptr, graph_guard_condition, NULL);
ASSERT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str;

std::thread trigger_thread(
[ = ]() {
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));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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));

EXPECT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str;
});

ret = rcl_wait(this->wait_set_ptr, wait_time_out);
EXPECT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str;
trigger_thread.join();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have edited #574 to make its purpose more clear.

}

/* Test the rcl_service_server_is_available function.
*/
TEST_F(CLASSNAME(TestGraphFixture, RMW_IMPLEMENTATION), test_rcl_service_server_is_available) {
Expand Down