Increase coverage rclcpp_action to 95% #1290

brawner · 2020-09-04T23:57:54Z

This adds unit tests for rclcpp_action and increases the coverage to just bit over 95%. As part of this work, an issue was found with the client implementation, which is potentially addressed in #1292. This branch is currently targeting that PR branch for easy review.

Testing --packages-select rclcpp_action

Linux
Linux-aarch64
macOS
Windows

Signed-off-by: Stephen Brawner brawner@gmail.com

brawner · 2020-09-09T00:29:03Z

Linux coverage:

brawner · 2020-09-09T00:31:41Z

rclcpp_action/test/mocking_utils/patch.hpp

+ * \tparam ReturnT Return value type.
+ * \tparam ArgTx Argument types.
+ */
+template<size_t ID, typename ReturnT,


This file is unchanged, except this overload was added to accommodate 7 arguments.

brawner · 2020-09-09T00:32:20Z

rclcpp_action/test/test_client.cpp

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


These checks were flaky on my machine

Really? The server's not up yet, how could it return true? Perhaps an issue with the ROS graph?

It must have been a weird issue. Also it turns out these two checks were responsible for about 20 lines of coverage. I added them back in.

Blast545

LGTM, left some minor questions

Blast545 · 2020-09-09T16:53:06Z

rclcpp_action/test/test_client.cpp

+  {
+    auto mock = mocking_utils::patch_and_return(
+      "lib:rclcpp_action", rcl_action_client_fini, RCL_RET_ERROR);
+    // It just logs an error message and continues


Is it there any way to check this error logged msg? maybe checking with rcl_error_is_set() ?

Also seems a bit curious to me if rcl_action_client_fini is called when using rclcpp_action::create_client, does it create an auxiliar internal or something like that?

rcl_action_client_fini won't be called during create_client. Instead, the smart pointer is reset in this line so the action client's destructor will be called. The smart pointer to the rcl_action_server_t is created with a custom deleter, which just calls RCLCPP_ERROR (

rclcpp/rclcpp_action/src/client.cpp

Lines 54 to 57 in 01d6f52

RCLCPP_ERROR(

rclcpp::get_logger(rcl_node_get_logger_name(handle.get())).get_child("rclcpp_action"),

"Error in destruction of rcl action client handle: %s", rcl_get_error_string().str);

rcl_reset_error();

).

Blast545 · 2020-09-09T17:28:46Z

rclcpp_action/test/test_server_goal_handle.cpp

@@ -0,0 +1,159 @@
+// Copyright 2018 Open Source Robotics Foundation, Inc.


Suggested change

// Copyright 2018 Open Source Robotics Foundation, Inc.

// Copyright 2020 Open Source Robotics Foundation, Inc.

Blast545 · 2020-09-09T17:30:59Z

rclcpp_action/test/test_server_goal_handle.cpp

+  {
+    auto mock_get_status = mocking_utils::patch_and_return(
+      "lib:rclcpp_action", rcl_action_goal_handle_get_status, RCL_RET_ERROR);
+    EXPECT_FALSE(handle_->cancel());


Do this sets some error at some point?

I mean, the test case implies that it fails silently, which I don't think might be desirable

The coverage this test is trying to add is to the protected try_canceling method in ServerGoalHandleBase, which is a noexcept method, and just returns true or false whether cancel succeeded or failed. I exposed it publicly in FibonacciServerGoalHandle with cancel(), which does imply something slightly different with its name.

I changed cancel() to be named try_cancel() to give a better indication to its use.

hidmic

Overall looks good! Perhaps some comments next to each test case would help clarify what it is that they are testing.

hidmic · 2020-09-10T19:21:32Z

rclcpp_action/test/test_client.cpp

+{
+  {
+    auto mock = mocking_utils::patch_and_return(
+      "lib:rclcpp_action", rcl_action_client_fini, RCL_RET_ERROR);


@brawner consider using mocking_utils::inject_on_return() instead.

hidmic · 2020-09-10T19:23:24Z

rclcpp_action/test/test_client.cpp

+    std::launch::async, [&action_client]() {
+      return action_client->wait_for_action_server(WAIT_FOR_SERVER_TIMEOUT);
+    });
+  EXPECT_TRUE(future.get());


@brawner how isn't this is equivalent to calling wait_for_action_server() directly?

This was copied from wait_for_action_server. You might have to ask the author of #1153 about that one :P. I removed the async bit and also from wait_for_action_server.

Had to think hard to remember why I did what I did 😅

The difference is that in #1153 the action server is brought up while the action client is waiting for it, which ensures (well, attempts to ensure) wait_for_action_server() won't return immediately here. In this case, the action server is already up and thus the separate thread is not necessary. Does that make sense?

hidmic · 2020-09-10T19:24:42Z

rclcpp_action/test/test_client.cpp

+  auto send_goal_ops = rclcpp_action::Client<ActionType>::SendGoalOptions();
+  send_goal_ops.result_callback =
+    [](
+    const typename ActionGoalHandle::WrappedResult &) mutable {};


@brawner out of curiosity, why mutable?

Not needed, removed.

hidmic · 2020-09-10T19:25:19Z

rclcpp_action/test/test_client.cpp

+  auto send_goal_ops = rclcpp_action::Client<ActionType>::SendGoalOptions();
+  send_goal_ops.result_callback =
+    [](
+    const typename ActionGoalHandle::WrappedResult &) mutable {};


@brawner same question.

hidmic · 2020-09-10T19:27:09Z

rclcpp_action/test/test_client.cpp

+    auto goal_handle = future_goal_handle.get();
+    EXPECT_THROW(
+    {
+      auto future_result = action_client->async_get_result(goal_handle);


@brawner should this be outside the expectation?

I think it can be. Moved

hidmic · 2020-09-10T20:27:53Z

rclcpp_action/test/test_server.cpp

+    using GoalHandle = rclcpp_action::ServerGoalHandle<Fibonacci>;
+    if (nullptr != node_) {
+      node_.reset();
+    }


@brawner nit: if SetupActionServerAndSpin returned all relevant state instead of persisting it, preventive resets would not be necessary.

Taking a fresh look at this today, I chopped up TestBasicServer into two derived test fixtures so node, uuid, and action_server are now created during SetUp(). I also separated each patch call into its own test to prevent test crossover.

hidmic · 2020-09-10T20:30:18Z

rclcpp_action/test/test_server_goal_handle.cpp

+    std::function<void(
+      std::shared_ptr<test_msgs::action::Fibonacci::Impl::FeedbackMessage>)> publish_feedback)
+  : rclcpp_action::ServerGoalHandle<test_msgs::action::Fibonacci>(
+      rcl_handle, uuid, goal, on_terminal_state, on_executing, publish_feedback) {}


@brawner nit: you can "use" base class constructors too (see here).

While I did learn something today, it appears that's not strictly usable because ServerGoalHandle's constructor is protected.

Argh, almost :)

brawner · 2020-09-11T00:41:32Z

Linux
Linux-aarch64
macOS
Windows

brawner · 2020-09-14T16:59:07Z

Retesting after rebasing onto #1311

Linux
Linux-aarch64
macOS
Windows

daisukes · 2020-09-15T05:40:06Z

rclcpp_action/test/test_server.cpp

+    std::chrono::milliseconds timeout = std::chrono::milliseconds(-1)) override
+  {
+    send_goal_request(node_, uuid_, timeout);
+    goal_handle_->execute();


@brawner Hi, I found that this throws an exception while writing my test case based on your test.
If you call SendClientRequest without EXPECT_THROW, you get

C++ exception with description "goal_handle attempted invalid transition from state EXECUTING with event EXECUTE, at /opt/ros/src/ros2/rcl/rcl_action/src/rcl_action/goal_handle.c:95" thrown in the test body.

Without this line, some tests got failed.

[1.870s] 2: [ FAILED ] TestGoalRequestServer.publish_result_send_result_response_errors [1.870s] 2: [ FAILED ] TestGoalRequestServer.publish_status_rcl_errors [1.871s] 2: [ FAILED ] TestGoalRequestServer.publish_status_send_cancel_response_errors [1.871s] 2: [ FAILED ] TestGoalRequestServer.get_result_rcl_errors [1.871s] 2: [ FAILED ] TestGoalRequestServer.send_result_rcl_errors

You're correct. I wrote these tests just to cover different errors that can occur, but it won't work as it is anyway. I think it makes sense for me to add a correctly functioning unit test to prove the test fixture code is good.

Ok, I updated these test fixtures. I now run a nominal check that shouldn't throw, so you can refer either TestGoalRequestServer or TestCancelRequestServer if need be (but it sounds like you were able to write one).

hidmic

LGTM

Signed-off-by: Stephen Brawner <brawner@gmail.com>

brawner · 2020-09-21T19:02:49Z

Rebasing onto master after #1311 was merged.

Linux
Linux-aarch64
macOS
Windows

* Increase coverage rclcpp_action to 95% Signed-off-by: Stephen Brawner <brawner@gmail.com> * PR fixup Signed-off-by: Stephen Brawner <brawner@gmail.com> * Address PR Feedback Signed-off-by: Stephen Brawner <brawner@gmail.com> * Rebase onto #1311 Signed-off-by: Stephen Brawner <brawner@gmail.com> * rcutils test depend Signed-off-by: Stephen Brawner <brawner@gmail.com> * Cleaning up Signed-off-by: Stephen Brawner <brawner@gmail.com>

brawner mentioned this pull request Sep 8, 2020

Set promise value after try-catch #1292

Closed

brawner force-pushed the brawner/rclcpp_action-test-coverage branch from 9b832d0 to 3bb2a25 Compare September 9, 2020 00:24

brawner marked this pull request as ready for review September 9, 2020 00:25

brawner changed the base branch from master to brawner/rclcpp_action-set-value-after-exception September 9, 2020 00:25

brawner requested review from hidmic and Blast545 September 9, 2020 00:27

brawner changed the title ~~WIP: Increase coverage rclcpp_action to 95%~~ Increase coverage rclcpp_action to 95% Sep 9, 2020

brawner commented Sep 9, 2020

View reviewed changes

brawner force-pushed the brawner/rclcpp_action-test-coverage branch from 3bb2a25 to 1476715 Compare September 9, 2020 00:33

Blast545 approved these changes Sep 9, 2020

View reviewed changes

hidmic reviewed Sep 10, 2020

View reviewed changes

brawner force-pushed the brawner/rclcpp_action-test-coverage branch from d916bff to 306a1c7 Compare September 11, 2020 00:34

brawner force-pushed the brawner/rclcpp_action-test-coverage branch from 306a1c7 to b252517 Compare September 14, 2020 16:43

brawner changed the base branch from brawner/rclcpp_action-set-value-after-exception to jacob/actions_refactor September 14, 2020 16:44

This was referenced Sep 14, 2020

Pass goal handle to goal response callback instead of a future #1311

Merged

Fix action server deadlock (#1285) #1313

Merged

daisukes reviewed Sep 15, 2020

View reviewed changes

hidmic approved these changes Sep 15, 2020

View reviewed changes

jacobperron force-pushed the jacob/actions_refactor branch 2 times, most recently from 4a3e745 to 9cb1776 Compare September 15, 2020 18:37

brawner force-pushed the brawner/rclcpp_action-test-coverage branch from cfe0311 to b1271f6 Compare September 15, 2020 21:53

brawner added 4 commits September 21, 2020 11:58

Increase coverage rclcpp_action to 95%

62b82fd

Signed-off-by: Stephen Brawner <brawner@gmail.com>

PR fixup

ec4ccbf

Signed-off-by: Stephen Brawner <brawner@gmail.com>

Address PR Feedback

4f9b5a0

Signed-off-by: Stephen Brawner <brawner@gmail.com>

Rebase onto #1311

9fe358c

Signed-off-by: Stephen Brawner <brawner@gmail.com>

brawner added 2 commits September 21, 2020 11:58

rcutils test depend

82908ad

Signed-off-by: Stephen Brawner <brawner@gmail.com>

Cleaning up

deaa2c5

Signed-off-by: Stephen Brawner <brawner@gmail.com>

brawner force-pushed the brawner/rclcpp_action-test-coverage branch from b1271f6 to deaa2c5 Compare September 21, 2020 19:01

brawner changed the base branch from jacob/actions_refactor to master September 21, 2020 19:02

brawner merged commit a855a7d into master Sep 22, 2020

delete-merged-branch bot deleted the brawner/rclcpp_action-test-coverage branch September 22, 2020 17:59

	RCLCPP_ERROR(
	rclcpp::get_logger(rcl_node_get_logger_name(handle.get())).get_child("rclcpp_action"),
	"Error in destruction of rcl action client handle: %s", rcl_get_error_string().str);
	rcl_reset_error();

		@@ -0,0 +1,159 @@
		// Copyright 2018 Open Source Robotics Foundation, Inc.

	// Copyright 2018 Open Source Robotics Foundation, Inc.
	// Copyright 2020 Open Source Robotics Foundation, Inc.

Increase coverage rclcpp_action to 95% #1290

Increase coverage rclcpp_action to 95% #1290

Conversation

brawner commented Sep 4, 2020 • edited Loading

brawner commented Sep 9, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hidmic Sep 15, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Blast545 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hidmic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hidmic Sep 15, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brawner commented Sep 11, 2020

brawner commented Sep 14, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hidmic left a comment

Choose a reason for hiding this comment

brawner commented Sep 21, 2020

brawner commented Sep 4, 2020 •

edited

Loading

hidmic Sep 15, 2020 •

edited

Loading

hidmic Sep 15, 2020 •

edited

Loading