Skip to content

Commit

Permalink
Cleanup test_graph.cpp. (#1193)
Browse files Browse the repository at this point in the history 
1. Shorten the sleeps to 100 milliseconds, so we can
quit faster if things show up in the graph.
2. Switch from CamelCase to snake_case for test functions.
3. Revamp the rcl_action test_graph.cpp wait_for_alive_nodes
to look more like the rcl version of the same thing.

Signed-off-by: Chris Lalancette <clalancette@gmail.com>
  • Loading branch information
@clalancette
clalancette authored Oct 1, 2024
1 parent 8ddfd52 commit d680ff3
Show file tree
Hide file tree
Showing 2 changed files with 32 additions and 43 deletions.
26 changes: 13 additions & 13 deletions rcl/test/rcl/test_graph.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1057,17 +1057,16 @@ class NodeGraphMultiNodeFixture : public TestGraphFixture
// Some middlewares like rmw_zenoh remove node entries from the ROS graph
// once the context for the node is shutdown.
size_t attempts = 0u;
size_t max_attempts = 10u;
constexpr size_t max_attempts = 100u;
std::unordered_set<std::string> discovered_node_names = {};
bool found_expected_nodes = false;
do {
std::this_thread::sleep_for(std::chrono::seconds(1));
rcutils_string_array_t node_names = rcutils_get_zero_initialized_string_array();
rcutils_string_array_t node_namespaces = rcutils_get_zero_initialized_string_array();
ASSERT_EQ(
RCL_RET_OK,
rcl_get_node_names(this->remote_node_ptr, allocator, &node_names, &node_namespaces));
attempts++;
++attempts;
for (size_t name_idx = 0; name_idx < node_names.size; ++name_idx) {
discovered_node_names.insert(node_names.data[name_idx]);
}
Expand All @@ -1077,6 +1076,7 @@ class NodeGraphMultiNodeFixture : public TestGraphFixture
ASSERT_EQ(RCUTILS_RET_OK, rcutils_string_array_fini(&node_names));
ASSERT_EQ(RCUTILS_RET_OK, rcutils_string_array_fini(&node_namespaces));
ASSERT_LE(attempts, max_attempts) << "Unable to attain all required nodes";
std::this_thread::sleep_for(std::chrono::milliseconds(100));
} while (!found_expected_nodes);
}

Expand All @@ -1086,7 +1086,7 @@ class NodeGraphMultiNodeFixture : public TestGraphFixture
* \param node_state expected state of node
* \param remote_node_state expected state of remote node
*/
void VerifySubsystemCount(
void verify_subsystem_count(
const expected_node_state && node_state,
const expected_node_state && remote_node_state) const
{
Expand Down Expand Up @@ -1184,19 +1184,19 @@ TEST_F(NodeGraphMultiNodeFixture, test_node_info_subscriptions)
EXPECT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str;
rcl_reset_error();

VerifySubsystemCount(expected_node_state{1, 1, 0, 0}, expected_node_state{1, 1, 0, 0});
verify_subsystem_count(expected_node_state{1, 1, 0, 0}, expected_node_state{1, 1, 0, 0});

// Destroy the node's subscriber
ret = rcl_subscription_fini(&sub, this->node_ptr);
EXPECT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str;
rcl_reset_error();
VerifySubsystemCount(expected_node_state{1, 0, 0, 0}, expected_node_state{1, 1, 0, 0});
verify_subsystem_count(expected_node_state{1, 0, 0, 0}, expected_node_state{1, 1, 0, 0});

// Destroy the remote node's subdscriber
ret = rcl_subscription_fini(&sub2, this->remote_node_ptr);
EXPECT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str;
rcl_reset_error();
VerifySubsystemCount(expected_node_state{1, 0, 0, 0}, expected_node_state{1, 0, 0, 0});
verify_subsystem_count(expected_node_state{1, 0, 0, 0}, expected_node_state{1, 0, 0, 0});
}

TEST_F(NodeGraphMultiNodeFixture, test_node_info_publishers)
Expand All @@ -1209,14 +1209,14 @@ TEST_F(NodeGraphMultiNodeFixture, test_node_info_publishers)
ret = rcl_publisher_init(&pub, this->node_ptr, ts, this->topic_name.c_str(), &pub_ops);
EXPECT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str;
rcl_reset_error();
VerifySubsystemCount(expected_node_state{2, 0, 0, 0}, expected_node_state{1, 0, 0, 0});
verify_subsystem_count(expected_node_state{2, 0, 0, 0}, expected_node_state{1, 0, 0, 0});

RCUTILS_LOG_DEBUG_NAMED(ROS_PACKAGE_NAME, "Destroyed publisher");
// Destroy the publisher.
ret = rcl_publisher_fini(&pub, this->node_ptr);
EXPECT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str;
rcl_reset_error();
VerifySubsystemCount(expected_node_state{1, 0, 0, 0}, expected_node_state{1, 0, 0, 0});
verify_subsystem_count(expected_node_state{1, 0, 0, 0}, expected_node_state{1, 0, 0, 0});
}

TEST_F(NodeGraphMultiNodeFixture, test_node_info_services)
Expand All @@ -1228,12 +1228,12 @@ TEST_F(NodeGraphMultiNodeFixture, test_node_info_services)
auto ts1 = ROSIDL_GET_SRV_TYPE_SUPPORT(test_msgs, srv, BasicTypes);
ret = rcl_service_init(&service, this->node_ptr, ts1, service_name, &service_options);
ASSERT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str;
VerifySubsystemCount(expected_node_state{1, 0, 1, 0}, expected_node_state{1, 0, 0, 0});
verify_subsystem_count(expected_node_state{1, 0, 1, 0}, expected_node_state{1, 0, 0, 0});

// Destroy service.
ret = rcl_service_fini(&service, this->node_ptr);
EXPECT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str;
VerifySubsystemCount(expected_node_state{1, 0, 0, 0}, expected_node_state{1, 0, 0, 0});
verify_subsystem_count(expected_node_state{1, 0, 0, 0}, expected_node_state{1, 0, 0, 0});
}

TEST_F(NodeGraphMultiNodeFixture, test_node_info_clients)
Expand All @@ -1245,12 +1245,12 @@ TEST_F(NodeGraphMultiNodeFixture, test_node_info_clients)
auto ts = ROSIDL_GET_SRV_TYPE_SUPPORT(test_msgs, srv, BasicTypes);
ret = rcl_client_init(&client, this->node_ptr, ts, service_name, &client_options);
ASSERT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str;
VerifySubsystemCount(expected_node_state{1, 0, 0, 1}, expected_node_state{1, 0, 0, 0});
verify_subsystem_count(expected_node_state{1, 0, 0, 1}, expected_node_state{1, 0, 0, 0});

// Destroy client
ret = rcl_client_fini(&client, this->node_ptr);
EXPECT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str;
VerifySubsystemCount(expected_node_state{1, 0, 0, 0}, expected_node_state{1, 0, 0, 0});
verify_subsystem_count(expected_node_state{1, 0, 0, 0}, expected_node_state{1, 0, 0, 0});
}

/*
Expand Down
49 changes: 19 additions & 30 deletions rcl_action/test/rcl_action/test_graph.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -309,7 +309,7 @@ class TestActionGraphMultiNodeFixture : public TestActionGraphFixture
this->remote_node_name,
"",
std::placeholders::_2);
WaitForAllNodesAlive();
wait_for_all_nodes_alive();
}

void TearDown() override
Expand All @@ -326,51 +326,40 @@ class TestActionGraphMultiNodeFixture : public TestActionGraphFixture
EXPECT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str;
}

void WaitForAllNodesAlive()
void wait_for_all_nodes_alive()
{
rcl_ret_t ret;
rcutils_string_array_t node_names = rcutils_get_zero_initialized_string_array();
rcutils_string_array_t node_namespaces = rcutils_get_zero_initialized_string_array();
OSRF_TESTING_TOOLS_CPP_SCOPE_EXIT(
{
ret = rcutils_string_array_fini(&node_names);
ASSERT_EQ(RCUTILS_RET_OK, ret);
ret = rcutils_string_array_fini(&node_namespaces);
ASSERT_EQ(RCUTILS_RET_OK, ret);
});
// wait for a minimum of 2 nodes to be discovered: remote_node_name, test_graph_node_name.
// old_node may or may not be present in the ROS graph depending on the
// rmw_implementation since rcl_shutdown() was invoked on the
// old_context_ptr used to initialize this node within TestActionGraphFixture::Setup().
// Some middlewares like rmw_zenoh remove node entries from the ROS graph
// once the context for the node is shutdown.
size_t attempts = 0u;
size_t max_attempts = 4u;
constexpr size_t max_attempts = 100u;
std::unordered_set<std::string> discovered_node_names = {};
bool found_expected_nodes = false;
while (!found_expected_nodes) {
std::this_thread::sleep_for(std::chrono::seconds(1));
ret = rcl_get_node_names(&this->remote_node, allocator, &node_names, &node_namespaces);

do {
rcutils_string_array_t node_names = rcutils_get_zero_initialized_string_array();
rcutils_string_array_t node_namespaces = rcutils_get_zero_initialized_string_array();
ASSERT_EQ(
RCL_RET_OK,
rcl_get_node_names(&this->remote_node, allocator, &node_names, &node_namespaces));
++attempts;
for (size_t name_idx = 0; name_idx < node_names.size; ++name_idx) {
discovered_node_names.insert(node_names.data[name_idx]);
}
found_expected_nodes =
discovered_node_names.count(remote_node_name) > 0 &&
discovered_node_names.count(test_graph_node_name) > 0;
ASSERT_EQ(RCUTILS_RET_OK, rcutils_string_array_fini(&node_names));
ASSERT_EQ(RCUTILS_RET_OK, rcutils_string_array_fini(&node_namespaces));
ASSERT_LE(attempts, max_attempts) << "Unable to attain all required nodes";
if (!found_expected_nodes) {
ret = rcutils_string_array_fini(&node_names);
ASSERT_EQ(RCUTILS_RET_OK, ret);
ret = rcutils_string_array_fini(&node_namespaces);
ASSERT_EQ(RCUTILS_RET_OK, ret);
node_names = rcutils_get_zero_initialized_string_array();
node_namespaces = rcutils_get_zero_initialized_string_array();
}
}
std::this_thread::sleep_for(std::chrono::milliseconds(100));
} while (!found_expected_nodes);
}

void WaitForActionCount(
void wait_for_action_count(
GetActionsFunc func,
size_t expected_count,
std::chrono::milliseconds duration)
Expand Down Expand Up @@ -418,7 +407,7 @@ TEST_F(TestActionGraphMultiNodeFixture, test_action_get_names_and_types)
rcl_get_error_string().str;
});

WaitForActionCount(action_func, 1u, std::chrono::seconds(1));
wait_for_action_count(action_func, 1u, std::chrono::seconds(1));

// Check that there is exactly one action name
rcl_names_and_types_t nat = rcl_get_zero_initialized_names_and_types();
Expand Down Expand Up @@ -457,7 +446,7 @@ TEST_F(TestActionGraphMultiNodeFixture, test_action_get_names_and_types)
rcl_get_error_string().str;
});

WaitForActionCount(action_func, 2u, std::chrono::seconds(1));
wait_for_action_count(action_func, 2u, std::chrono::seconds(1));

ret = action_func(&this->node, &nat);
EXPECT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str;
Expand Down Expand Up @@ -528,7 +517,7 @@ TEST_F(TestActionGraphMultiNodeFixture, test_action_get_server_names_and_types_b
rcl_get_error_string().str;
});

WaitForActionCount(servers_by_node_func, 1u, std::chrono::seconds(1));
wait_for_action_count(servers_by_node_func, 1u, std::chrono::seconds(1));
ret = servers_by_node_func(&this->node, &nat);
EXPECT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str;
ASSERT_EQ(nat.names.size, 1u);
Expand Down Expand Up @@ -596,7 +585,7 @@ TEST_F(TestActionGraphMultiNodeFixture, test_action_get_client_names_and_types_b
rcl_get_error_string().str;
});

WaitForActionCount(clients_by_node_func, 1u, std::chrono::seconds(1));
wait_for_action_count(clients_by_node_func, 1u, std::chrono::seconds(1));
ret = clients_by_node_func(&this->node, &nat);
EXPECT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str;
ASSERT_EQ(nat.names.size, 1u);
Expand Down

0 comments on commit d680ff3

Please sign in to comment.