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

Increase rclcpp_action test coverage #1153

Merged
merged 4 commits into from
Jun 8, 2020
Merged

Increase rclcpp_action test coverage #1153

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5a722d4
Do not leak memory on action client/server destruction.
hidmic Jun 2, 2020
dcff3d8
Improve rclcpp_action test coverage.
hidmic Jun 1, 2020
c19ba06
Correct typos in comments.
hidmic Jun 3, 2020
7ba3914
Address peer review comments.
hidmic Jun 3, 2020
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
25 changes: 12 additions & 13 deletions rclcpp_action/include/rclcpp_action/create_client.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,20 +59,19 @@ create_client(
return;
}
auto shared_node = weak_node.lock();
if (!shared_node) {
brawner marked this conversation as resolved.
Show resolved Hide resolved
return;
}
// API expects a shared pointer, give it one with a deleter that does nothing.
std::shared_ptr<Client<ActionT>> fake_shared_ptr(ptr, [](Client<ActionT> *) {});
if (shared_node) {
// API expects a shared pointer, give it one with a deleter that does nothing.
std::shared_ptr<Client<ActionT>> fake_shared_ptr(ptr, [](Client<ActionT> *) {});

if (group_is_null) {
// Was added to default group
shared_node->remove_waitable(fake_shared_ptr, nullptr);
} else {
// Was added to a specfic group
auto shared_group = weak_group.lock();
if (shared_group) {
shared_node->remove_waitable(fake_shared_ptr, shared_group);
if (group_is_null) {
// Was added to default group
shared_node->remove_waitable(fake_shared_ptr, nullptr);
} else {
// Was added to a specific group
auto shared_group = weak_group.lock();
if (shared_group) {
shared_node->remove_waitable(fake_shared_ptr, shared_group);
}
}
}
delete ptr;
Expand Down
25 changes: 12 additions & 13 deletions rclcpp_action/include/rclcpp_action/create_server.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,20 +78,19 @@ create_server(
return;
}
auto shared_node = weak_node.lock();
if (!shared_node) {
brawner marked this conversation as resolved.
Show resolved Hide resolved
return;
}
// API expects a shared pointer, give it one with a deleter that does nothing.
std::shared_ptr<Server<ActionT>> fake_shared_ptr(ptr, [](Server<ActionT> *) {});
if (shared_node) {
// API expects a shared pointer, give it one with a deleter that does nothing.
std::shared_ptr<Server<ActionT>> fake_shared_ptr(ptr, [](Server<ActionT> *) {});

if (group_is_null) {
// Was added to default group
shared_node->remove_waitable(fake_shared_ptr, nullptr);
} else {
// Was added to a specfic group
auto shared_group = weak_group.lock();
if (shared_group) {
shared_node->remove_waitable(fake_shared_ptr, shared_group);
if (group_is_null) {
// Was added to default group
shared_node->remove_waitable(fake_shared_ptr, nullptr);
} else {
// Was added to a specific group
auto shared_group = weak_group.lock();
if (shared_group) {
shared_node->remove_waitable(fake_shared_ptr, shared_group);
}
}
}
delete ptr;
Expand Down
163 changes: 135 additions & 28 deletions rclcpp_action/test/test_client.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@

#include <test_msgs/action/fibonacci.hpp>

#include <future>
#include <map>
#include <memory>
#include <string>
Expand Down Expand Up @@ -73,7 +74,7 @@ class TestClient : public ::testing::Test
rclcpp::init(0, nullptr);
}

void SetUp()
void SetUpServer()
{
rcl_allocator_t allocator = rcl_get_default_allocator();

Expand Down Expand Up @@ -211,22 +212,34 @@ class TestClient : public ::testing::Test
ASSERT_TRUE(status_publisher != nullptr);
allocator.deallocate(status_topic_name, allocator.state);
server_executor.add_node(server_node);
}

void SetUp() override
{
client_node = std::make_shared<rclcpp::Node>(client_node_name, namespace_name);
client_executor.add_node(client_node);

ASSERT_EQ(RCL_RET_OK, rcl_enable_ros_time_override(clock.get_clock_handle()));
ASSERT_EQ(RCL_RET_OK, rcl_set_ros_time_override(clock.get_clock_handle(), RCL_S_TO_NS(1)));
}

void TearDown()
static void TearDownTestCase()
{
rclcpp::shutdown();
}

void TearDownServer()
{
status_publisher.reset();
feedback_publisher.reset();
cancel_service.reset();
result_service.reset();
goal_service.reset();
server_node.reset();
}

void TearDown() override
{
client_node.reset();
}

Expand Down Expand Up @@ -265,11 +278,37 @@ class TestClient : public ::testing::Test
typename rclcpp::Publisher<ActionStatusMessage>::SharedPtr status_publisher;
};

class TestClientAgainstServer : public TestClient
{
protected:
void SetUp() override
{
SetUpServer();
TestClient::SetUp();
}

void TearDown() override
{
TestClient::TearDown();
TearDownServer();
}
};


TEST_F(TestClient, construction_and_destruction)
{
ASSERT_NO_THROW(rclcpp_action::create_client<ActionType>(client_node, action_name).reset());
}

TEST_F(TestClient, construction_and_destruction_after_node)
{
ASSERT_NO_THROW(
{
auto action_client = rclcpp_action::create_client<ActionType>(client_node, action_name);
client_node.reset();
});
}

TEST_F(TestClient, construction_and_destruction_callback_group)
{
auto group = client_node->create_callback_group(
Expand All @@ -285,7 +324,24 @@ TEST_F(TestClient, construction_and_destruction_callback_group)
).reset());
}

TEST_F(TestClient, async_send_goal_no_callbacks)
TEST_F(TestClient, wait_for_action_server)
{
auto action_client = rclcpp_action::create_client<ActionType>(client_node, action_name);
EXPECT_FALSE(action_client->wait_for_action_server(0ms));
EXPECT_FALSE(action_client->wait_for_action_server(100ms));
Blast545 marked this conversation as resolved.
Show resolved Hide resolved
auto future = std::async(
std::launch::async, [&action_client]() {
return action_client->wait_for_action_server(WAIT_FOR_SERVER_TIMEOUT);
});
SetUpServer();
EXPECT_TRUE(future.get());
TearDownServer();

client_node.reset(); // Drop node before action client
EXPECT_THROW(action_client->wait_for_action_server(0ms), rclcpp::exceptions::InvalidNodeError);
}

TEST_F(TestClientAgainstServer, async_send_goal_no_callbacks)
{
auto action_client = rclcpp_action::create_client<ActionType>(client_node, action_name);
ASSERT_TRUE(action_client->wait_for_action_server(WAIT_FOR_SERVER_TIMEOUT));
Expand All @@ -306,7 +362,24 @@ TEST_F(TestClient, async_send_goal_no_callbacks)
EXPECT_FALSE(goal_handle->is_result_aware());
}

TEST_F(TestClient, async_send_goal_no_callbacks_wait_for_result)
TEST_F(TestClientAgainstServer, bad_goal_handles)
{
auto action_client0 = rclcpp_action::create_client<ActionType>(client_node, action_name);
ASSERT_TRUE(action_client0->wait_for_action_server(WAIT_FOR_SERVER_TIMEOUT));

ActionGoal goal;
goal.order = 0;
auto future_goal_handle = action_client0->async_send_goal(goal);
dual_spin_until_future_complete(future_goal_handle);
auto goal_handle = future_goal_handle.get();

auto action_client1 = rclcpp_action::create_client<ActionType>(client_node, action_name);
using rclcpp_action::exceptions::UnknownGoalHandleError;
EXPECT_THROW(action_client1->async_get_result(goal_handle), UnknownGoalHandleError);
EXPECT_THROW(action_client1->async_cancel_goal(goal_handle), UnknownGoalHandleError);
}

TEST_F(TestClientAgainstServer, async_send_goal_no_callbacks_wait_for_result)
{
auto action_client = rclcpp_action::create_client<ActionType>(client_node, action_name);
ASSERT_TRUE(action_client->wait_for_action_server(WAIT_FOR_SERVER_TIMEOUT));
Expand All @@ -329,13 +402,33 @@ TEST_F(TestClient, async_send_goal_no_callbacks_wait_for_result)
EXPECT_EQ(5, wrapped_result.result->sequence[5]);
}

TEST_F(TestClient, async_send_goal_with_goal_response_callback_wait_for_result)
TEST_F(TestClientAgainstServer, async_send_goal_no_callbacks_then_invalidate)
{
auto action_client = rclcpp_action::create_client<ActionType>(client_node, action_name);
ASSERT_TRUE(action_client->wait_for_action_server(WAIT_FOR_SERVER_TIMEOUT));

ActionGoal goal;
goal.order = 4;
goal.order = 5;
auto future_goal_handle = action_client->async_send_goal(goal);
dual_spin_until_future_complete(future_goal_handle);
auto goal_handle = future_goal_handle.get();
ASSERT_NE(nullptr, goal_handle);
EXPECT_EQ(rclcpp_action::GoalStatus::STATUS_ACCEPTED, goal_handle->get_status());
auto future_result = action_client->async_get_result(goal_handle);
EXPECT_TRUE(goal_handle->is_result_aware());

action_client.reset(); // Ensure goal handle is invalidated once client goes out of scope

EXPECT_EQ(rclcpp_action::GoalStatus::STATUS_UNKNOWN, goal_handle->get_status());
using rclcpp_action::exceptions::UnawareGoalHandleError;
EXPECT_THROW(future_result.get(), UnawareGoalHandleError);
}

TEST_F(TestClientAgainstServer, async_send_goal_with_goal_response_callback_wait_for_result)
{
auto action_client = rclcpp_action::create_client<ActionType>(client_node, action_name);
ASSERT_TRUE(action_client->wait_for_action_server(WAIT_FOR_SERVER_TIMEOUT));

bool goal_response_received = false;
auto send_goal_ops = rclcpp_action::Client<ActionType>::SendGoalOptions();
send_goal_ops.goal_response_callback =
Expand All @@ -347,23 +440,37 @@ TEST_F(TestClient, async_send_goal_with_goal_response_callback_wait_for_result)
goal_response_received = true;
}
};
auto future_goal_handle = action_client->async_send_goal(goal, send_goal_ops);
dual_spin_until_future_complete(future_goal_handle);
auto goal_handle = future_goal_handle.get();
EXPECT_TRUE(goal_response_received);
EXPECT_EQ(rclcpp_action::GoalStatus::STATUS_ACCEPTED, goal_handle->get_status());
EXPECT_FALSE(goal_handle->is_feedback_aware());
EXPECT_FALSE(goal_handle->is_result_aware());
auto future_result = action_client->async_get_result(goal_handle);
EXPECT_TRUE(goal_handle->is_result_aware());
dual_spin_until_future_complete(future_result);
auto wrapped_result = future_result.get();

ASSERT_EQ(5u, wrapped_result.result->sequence.size());
EXPECT_EQ(3, wrapped_result.result->sequence.back());
{
ActionGoal bad_goal;
bad_goal.order = -1;
auto future_goal_handle = action_client->async_send_goal(bad_goal, send_goal_ops);
dual_spin_until_future_complete(future_goal_handle);
auto goal_handle = future_goal_handle.get();
EXPECT_FALSE(goal_response_received);
EXPECT_EQ(nullptr, goal_handle);
}

{
ActionGoal goal;
goal.order = 4;
auto future_goal_handle = action_client->async_send_goal(goal, send_goal_ops);
dual_spin_until_future_complete(future_goal_handle);
auto goal_handle = future_goal_handle.get();
EXPECT_TRUE(goal_response_received);
EXPECT_EQ(rclcpp_action::GoalStatus::STATUS_ACCEPTED, goal_handle->get_status());
EXPECT_FALSE(goal_handle->is_feedback_aware());
EXPECT_FALSE(goal_handle->is_result_aware());
auto future_result = action_client->async_get_result(goal_handle);
EXPECT_TRUE(goal_handle->is_result_aware());
dual_spin_until_future_complete(future_result);
auto wrapped_result = future_result.get();
ASSERT_EQ(5u, wrapped_result.result->sequence.size());
EXPECT_EQ(3, wrapped_result.result->sequence.back());
}
}

TEST_F(TestClient, async_send_goal_with_feedback_callback_wait_for_result)
TEST_F(TestClientAgainstServer, async_send_goal_with_feedback_callback_wait_for_result)
{
auto action_client = rclcpp_action::create_client<ActionType>(client_node, action_name);
ASSERT_TRUE(action_client->wait_for_action_server(WAIT_FOR_SERVER_TIMEOUT));
Expand Down Expand Up @@ -397,7 +504,7 @@ TEST_F(TestClient, async_send_goal_with_feedback_callback_wait_for_result)
EXPECT_EQ(5, feedback_count);
}

TEST_F(TestClient, async_send_goal_with_result_callback_wait_for_result)
TEST_F(TestClientAgainstServer, async_send_goal_with_result_callback_wait_for_result)
{
auto action_client = rclcpp_action::create_client<ActionType>(client_node, action_name);
ASSERT_TRUE(action_client->wait_for_action_server(WAIT_FOR_SERVER_TIMEOUT));
Expand Down Expand Up @@ -432,7 +539,7 @@ TEST_F(TestClient, async_send_goal_with_result_callback_wait_for_result)
EXPECT_EQ(3, wrapped_result.result->sequence.back());
}

TEST_F(TestClient, async_get_result_with_callback)
TEST_F(TestClientAgainstServer, async_get_result_with_callback)
{
auto action_client = rclcpp_action::create_client<ActionType>(client_node, action_name);
ASSERT_TRUE(action_client->wait_for_action_server(WAIT_FOR_SERVER_TIMEOUT));
Expand Down Expand Up @@ -467,7 +574,7 @@ TEST_F(TestClient, async_get_result_with_callback)
EXPECT_EQ(3, wrapped_result.result->sequence.back());
}

TEST_F(TestClient, async_cancel_one_goal)
TEST_F(TestClientAgainstServer, async_cancel_one_goal)
{
auto action_client = rclcpp_action::create_client<ActionType>(client_node, action_name);
ASSERT_TRUE(action_client->wait_for_action_server(WAIT_FOR_SERVER_TIMEOUT));
Expand All @@ -485,7 +592,7 @@ TEST_F(TestClient, async_cancel_one_goal)
EXPECT_EQ(ActionCancelGoalResponse::ERROR_NONE, cancel_response->return_code);
}

TEST_F(TestClient, async_cancel_one_goal_with_callback)
TEST_F(TestClientAgainstServer, async_cancel_one_goal_with_callback)
{
auto action_client = rclcpp_action::create_client<ActionType>(client_node, action_name);
ASSERT_TRUE(action_client->wait_for_action_server(WAIT_FOR_SERVER_TIMEOUT));
Expand Down Expand Up @@ -519,7 +626,7 @@ TEST_F(TestClient, async_cancel_one_goal_with_callback)
EXPECT_TRUE(cancel_response_received);
}

TEST_F(TestClient, async_cancel_all_goals)
TEST_F(TestClientAgainstServer, async_cancel_all_goals)
{
auto action_client = rclcpp_action::create_client<ActionType>(client_node, action_name);
ASSERT_TRUE(action_client->wait_for_action_server(WAIT_FOR_SERVER_TIMEOUT));
Expand Down Expand Up @@ -555,7 +662,7 @@ TEST_F(TestClient, async_cancel_all_goals)
EXPECT_EQ(rclcpp_action::GoalStatus::STATUS_CANCELED, goal_handle1->get_status());
}

TEST_F(TestClient, async_cancel_all_goals_with_callback)
TEST_F(TestClientAgainstServer, async_cancel_all_goals_with_callback)
{
auto action_client = rclcpp_action::create_client<ActionType>(client_node, action_name);
ASSERT_TRUE(action_client->wait_for_action_server(WAIT_FOR_SERVER_TIMEOUT));
Expand Down Expand Up @@ -605,7 +712,7 @@ TEST_F(TestClient, async_cancel_all_goals_with_callback)
EXPECT_EQ(rclcpp_action::GoalStatus::STATUS_CANCELED, goal_handle1->get_status());
}

TEST_F(TestClient, async_cancel_some_goals)
TEST_F(TestClientAgainstServer, async_cancel_some_goals)
{
auto action_client = rclcpp_action::create_client<ActionType>(client_node, action_name);
ASSERT_TRUE(action_client->wait_for_action_server(WAIT_FOR_SERVER_TIMEOUT));
Expand Down Expand Up @@ -636,7 +743,7 @@ TEST_F(TestClient, async_cancel_some_goals)
EXPECT_EQ(rclcpp_action::GoalStatus::STATUS_CANCELED, goal_handle0->get_status());
}

TEST_F(TestClient, async_cancel_some_goals_with_callback)
TEST_F(TestClientAgainstServer, async_cancel_some_goals_with_callback)
{
auto action_client = rclcpp_action::create_client<ActionType>(client_node, action_name);
ASSERT_TRUE(action_client->wait_for_action_server(WAIT_FOR_SERVER_TIMEOUT));
Expand Down
Loading