Skip to content

Commit

Permalink
Refactor
Browse files Browse the repository at this point in the history 
* Move graph API common implementation to local function
* Refactor tests

Signed-off-by: Jacob Perron <jacob@openrobotics.org>
  • Loading branch information
@jacobperron
jacobperron committed Apr 2, 2019
1 parent b8cc83d commit 87186b1
Show file tree
Hide file tree
Showing 2 changed files with 85 additions and 73 deletions.
46 changes: 15 additions & 31 deletions rcl_action/src/rcl_action/graph.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,20 +30,22 @@ extern "C"
static
rcl_ret_t
_filter_action_names(
rcl_names_and_types_t * input_names_and_types,
rcl_names_and_types_t * topic_names_and_types,
rcl_allocator_t * allocator,
const char * action_name_suffix,
const char * action_type_suffix,
rcl_names_and_types_t * action_names_and_types)
{
assert(input_names_and_types);
assert(topic_names_and_types);
assert(allocator);
assert(action_name_suffix);
assert(action_names_and_types);

// Assumption: actions provide a topic name with the suffix "/_action/feedback"
// and it has type with the suffix "_FeedbackMessage"
const char * action_name_suffix = "/_action/feedback";
const char * action_type_suffix = "_FeedbackMessage";

rcl_ret_t ret;
const size_t num_names = input_names_and_types->names.size;
char ** names = input_names_and_types->names.data;
const size_t num_names = topic_names_and_types->names.size;
char ** names = topic_names_and_types->names.data;

// Count number of actions to determine how much memory to allocate
size_t num_actions = 0u;
Expand Down Expand Up @@ -84,7 +86,7 @@ _filter_action_names(
// Allocate storage for type list
rcutils_ret_t rcutils_ret = rcutils_string_array_init(
&action_names_and_types->types[j],
input_names_and_types->types[i].size,
topic_names_and_types->types[i].size,
allocator);
if (RCUTILS_RET_OK != rcutils_ret) {
RCL_SET_ERROR_MSG(rcutils_get_error_string().str);
Expand All @@ -93,8 +95,8 @@ _filter_action_names(
}

// Populate types list
for (size_t k = 0u; k < input_names_and_types->types[i].size; ++k) {
char * type_name = input_names_and_types->types[i].data[k];
for (size_t k = 0u; k < topic_names_and_types->types[i].size; ++k) {
char * type_name = topic_names_and_types->types[i].data[k];
size_t action_type_len = strlen(type_name);
// Trim type name suffix, if provided
if (action_type_suffix) {
Expand Down Expand Up @@ -143,15 +145,9 @@ rcl_action_get_client_names_and_types_by_node(
return ret;
}

// Assumption: actions provide a topic name with the suffix "/_action/feedback"
// and it has type with the suffix "_FeedbackMessage"
const char * action_name_suffix = "/_action/feedback";
const char * action_type_suffix = "_FeedbackMessage";
ret = _filter_action_names(
&topic_names_and_types,
allocator,
action_name_suffix,
action_type_suffix,
action_names_and_types);
return ret;
}
Expand All @@ -174,15 +170,9 @@ rcl_action_get_server_names_and_types_by_node(
return ret;
}

// Assumption: actions provide a topic name with the suffix "/_action/feedback"
// and it has type with the suffix "_FeedbackMessage"
const char * action_name_suffix = "/_action/feedback";
const char * action_type_suffix = "_FeedbackMessage";
ret = _filter_action_names(
&topic_names_and_types,
allocator,
action_name_suffix,
action_type_suffix,
action_names_and_types);
return ret;
}
Expand All @@ -194,21 +184,15 @@ rcl_action_get_names_and_types(
rcl_names_and_types_t * action_names_and_types)
{
RCL_CHECK_ARGUMENT_FOR_NULL(action_names_and_types, RCL_RET_INVALID_ARGUMENT);
rcl_names_and_types_t service_names_and_types = rcl_get_zero_initialized_names_and_types();
rcl_ret_t ret = rcl_get_service_names_and_types(node, allocator, &service_names_and_types);
rcl_names_and_types_t topic_names_and_types = rcl_get_zero_initialized_names_and_types();
rcl_ret_t ret = rcl_get_topic_names_and_types(node, allocator, false, &topic_names_and_types);
if (RCL_RET_OK != ret) {
return ret;
}

// Assumption: actions provide a service name with the suffix "/_action/send_goal"
// and it has a type name with the suffix "_SendGoal"
const char * action_name_suffix = "/_action/send_goal";
const char * action_type_suffix = "_SendGoal";
ret = _filter_action_names(
&service_names_and_types,
&topic_names_and_types,
allocator,
action_name_suffix,
action_type_suffix,
action_names_and_types);
return ret;
}
Expand Down
112 changes: 70 additions & 42 deletions rcl_action/test/rcl_action/test_graph.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,6 @@ class CLASSNAME (TestActionGraphFixture, RMW_IMPLEMENTATION) : public ::testing:
rcl_context_t context;
rcl_node_t old_node;
rcl_node_t node;
rcl_wait_set_t wait_set;
const char * test_graph_node_name = "test_action_graph_node";
const char * test_graph_old_node_name = "test_action_graph_old_node_name";

Expand Down Expand Up @@ -73,11 +72,6 @@ class CLASSNAME (TestActionGraphFixture, RMW_IMPLEMENTATION) : public ::testing:
this->node = rcl_get_zero_initialized_node();
ret = rcl_node_init(&this->node, test_graph_node_name, "", &this->context, &node_options);
ASSERT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str;

this->wait_set = rcl_get_zero_initialized_wait_set();
ret = rcl_wait_set_init(
&this->wait_set, 0, 1, 0, 0, 0, &this->context, this->allocator);
ASSERT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str;
}

void TearDown()
Expand All @@ -86,9 +80,6 @@ class CLASSNAME (TestActionGraphFixture, RMW_IMPLEMENTATION) : public ::testing:
ret = rcl_node_fini(&this->old_node);
EXPECT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str;

ret = rcl_wait_set_fini(&this->wait_set);
EXPECT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str;

ret = rcl_node_fini(&this->node);
EXPECT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str;

Expand Down Expand Up @@ -233,6 +224,12 @@ TEST_F(
EXPECT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str;
}

/**
* Type define a get actions function.
*/
typedef std::function<rcl_ret_t(const rcl_node_t *,
rcl_names_and_types_t *)> GetActionsFunc;

/**
* Extend the TestActionGraphFixture with a multi-node fixture for node discovery and node-graph
* perspective.
Expand All @@ -244,6 +241,7 @@ class TestActionGraphMultiNodeFixture : public CLASSNAME(TestActionGraphFixture,
const char * action_name = "/test_action_info_functions__";
rcl_node_t remote_node;
rcl_context_t remote_context;
GetActionsFunc action_func, clients_by_node_func, servers_by_node_func;

void SetUp() override
{
Expand All @@ -267,6 +265,23 @@ class TestActionGraphMultiNodeFixture : public CLASSNAME(TestActionGraphFixture,
ret = rcl_node_init(
&this->remote_node, this->remote_node_name, "", &this->remote_context, &node_options);
ASSERT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str;

action_func = std::bind(rcl_action_get_names_and_types,
std::placeholders::_1,
&this->allocator,
std::placeholders::_2);
clients_by_node_func = std::bind(rcl_action_get_client_names_and_types_by_node,
std::placeholders::_1,
&this->allocator,
this->remote_node_name,
"",
std::placeholders::_2);
servers_by_node_func = std::bind(rcl_action_get_server_names_and_types_by_node,
std::placeholders::_1,
&this->allocator,
this->remote_node_name,
"",
std::placeholders::_2);
WaitForAllNodesAlive();
}

Expand Down Expand Up @@ -305,6 +320,30 @@ class TestActionGraphMultiNodeFixture : public CLASSNAME(TestActionGraphFixture,
ASSERT_LE(attempts, max_attempts) << "Unable to attain all required nodes";
}
}

void WaitForActionCount(
GetActionsFunc func,
size_t expected_count,
std::chrono::milliseconds duration)
{
auto start_time = std::chrono::system_clock::now();
auto curr_time = start_time;

rcl_ret_t ret;
while ((curr_time - start_time) < duration) {
rcl_names_and_types_t nat = rcl_get_zero_initialized_names_and_types();
ret = func(&this->node, &nat);
ASSERT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str;
size_t action_count = nat.names.size;
ret = rcl_names_and_types_fini(&nat);
EXPECT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str;
if (action_count == expected_count) {
return;
}
std::this_thread::sleep_for(std::chrono::milliseconds(200));
curr_time = std::chrono::system_clock::now();
}
}
};

// Note, this test could be affected by other communication on the same ROS domain
Expand All @@ -327,13 +366,16 @@ TEST_F(TestActionGraphMultiNodeFixture, test_action_get_names_and_types) {
EXPECT_EQ(RCL_RET_OK, rcl_action_client_fini(&action_client, &this->remote_node)) <<
rcl_get_error_string().str;
});

WaitForActionCount(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();
ret = rcl_action_get_names_and_types(&this->node, &this->allocator, &nat);
ret = action_func(&this->node, &nat);
EXPECT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str;
EXPECT_EQ(nat.names.size, 1u);
ASSERT_EQ(nat.names.size, 1u);
EXPECT_STREQ(nat.names.data[0], client_action_name);
EXPECT_EQ(nat.types[0].size, 1u);
ASSERT_EQ(nat.types[0].size, 1u);
EXPECT_STREQ(nat.types[0].data[0], "test_msgs/Fibonacci");

ret = rcl_names_and_types_fini(&nat);
Expand Down Expand Up @@ -362,14 +404,16 @@ TEST_F(TestActionGraphMultiNodeFixture, test_action_get_names_and_types) {
rcl_get_error_string().str;
});

ret = rcl_action_get_names_and_types(&this->node, &this->allocator, &nat);
WaitForActionCount(action_func, 2u, std::chrono::seconds(1));

ret = action_func(&this->node, &nat);
EXPECT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str;
EXPECT_EQ(nat.names.size, 2u);
ASSERT_EQ(nat.names.size, 2u);
EXPECT_STREQ(nat.names.data[0], client_action_name);
EXPECT_STREQ(nat.names.data[1], server_action_name);
EXPECT_EQ(nat.types[0].size, 1u);
ASSERT_EQ(nat.types[0].size, 1u);
EXPECT_STREQ(nat.types[0].data[0], "test_msgs/Fibonacci");
EXPECT_EQ(nat.types[1].size, 1u);
ASSERT_EQ(nat.types[1].size, 1u);
EXPECT_STREQ(nat.types[1].data[0], "test_msgs/Fibonacci");

ret = rcl_names_and_types_fini(&nat);
Expand Down Expand Up @@ -400,7 +444,7 @@ TEST_F(TestActionGraphMultiNodeFixture, test_action_get_server_names_and_types_b
ret = rcl_action_get_server_names_and_types_by_node(
&this->node, &this->allocator, this->remote_node_name, "", &nat);
EXPECT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str;
EXPECT_EQ(nat.names.size, 0u);
ASSERT_EQ(nat.names.size, 0u);

ret = rcl_names_and_types_fini(&nat);
EXPECT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str;
Expand All @@ -427,20 +471,12 @@ TEST_F(TestActionGraphMultiNodeFixture, test_action_get_server_names_and_types_b
rcl_get_error_string().str;
});

// Wait for server to be ready
bool is_available = false;
do {
ret = rcl_action_server_is_available(&this->remote_node, &action_client, &is_available);
EXPECT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str;
std::this_thread::sleep_for(std::chrono::milliseconds(100));
} while (!is_available);

ret = rcl_action_get_server_names_and_types_by_node(
&this->node, &this->allocator, this->remote_node_name, "", &nat);
WaitForActionCount(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;
EXPECT_EQ(nat.names.size, 1u);
ASSERT_EQ(nat.names.size, 1u);
EXPECT_STREQ(nat.names.data[0], this->action_name);
EXPECT_EQ(nat.types[0].size, 1u);
ASSERT_EQ(nat.types[0].size, 1u);
EXPECT_STREQ(nat.types[0].data[0], "test_msgs/Fibonacci");

ret = rcl_names_and_types_fini(&nat);
Expand Down Expand Up @@ -479,7 +515,7 @@ TEST_F(TestActionGraphMultiNodeFixture, test_action_get_client_names_and_types_b
ret = rcl_action_get_client_names_and_types_by_node(
&this->node, &this->allocator, this->remote_node_name, "", &nat);
EXPECT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str;
EXPECT_EQ(nat.names.size, 0u);
ASSERT_EQ(nat.names.size, 0u);

ret = rcl_names_and_types_fini(&nat);
EXPECT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str;
Expand All @@ -499,20 +535,12 @@ TEST_F(TestActionGraphMultiNodeFixture, test_action_get_client_names_and_types_b
rcl_get_error_string().str;
});

// Wait for server to be ready
bool is_available = false;
do {
ret = rcl_action_server_is_available(&this->remote_node, &action_client, &is_available);
EXPECT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str;
std::this_thread::sleep_for(std::chrono::milliseconds(100));
} while (!is_available);

ret = rcl_action_get_client_names_and_types_by_node(
&this->node, &this->allocator, this->remote_node_name, "", &nat);
WaitForActionCount(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;
EXPECT_EQ(nat.names.size, 1u);
ASSERT_EQ(nat.names.size, 1u);
EXPECT_STREQ(nat.names.data[0], this->action_name);
EXPECT_EQ(nat.types[0].size, 1u);
ASSERT_EQ(nat.types[0].size, 1u);
EXPECT_STREQ(nat.types[0].data[0], "test_msgs/Fibonacci");

ret = rcl_names_and_types_fini(&nat);
Expand Down

0 comments on commit 87186b1

Please sign in to comment.