Addressing feedback

Signed-off-by: Stephen Brawner <brawner@gmail.com>
ros2 · Aug 27, 2020 · e4f7f0c · e4f7f0c
1 parent 907cd21
commit e4f7f0c
Show file tree

Hide file tree

Showing 4 changed files with 29 additions and 5 deletions.
diff --git a/rcl_action/test/rcl_action/test_action_client.cpp b/rcl_action/test/rcl_action/test_action_client.cpp
@@ -362,6 +362,8 @@ TEST_F(TestActionClientBaseFixture, test_action_client_init_fini_maybe_fail)
     std::string node_name = std::string("test_action_client_node_") + std::to_string(count);
     rcl_ret_t ret = rcl_node_init(&node, node_name.c_str(), "", &this->context, &node_options);
     if (RCL_RET_OK != ret) {
+      EXPECT_TRUE(rcl_error_is_set());
+      rcl_reset_error();
       continue;
     }
     const rosidl_action_type_support_t * action_typesupport = ROSIDL_GET_ACTION_TYPE_SUPPORT(
@@ -379,6 +381,10 @@ TEST_F(TestActionClientBaseFixture, test_action_client_init_fini_maybe_fail)
 
     if (RCL_RET_OK == ret) {
       ret = rcl_action_client_fini(&action_client, &node);
+      if (RCL_RET_OK != ret) {
+        // Not always guaranteed be set, but reset anyway
+        rcl_reset_error();
+      }
     } else {
       EXPECT_TRUE(rcl_error_is_set());
       rcl_reset_error();

diff --git a/rcl_action/test/rcl_action/test_action_communication.cpp b/rcl_action/test/rcl_action/test_action_communication.cpp
@@ -1128,17 +1128,23 @@ TEST_F(CLASSNAME(TestActionCommunication, RMW_IMPLEMENTATION), test_valid_feedba
     // Publish feedback with valid arguments
     rcl_ret_t ret = rcl_action_publish_feedback(&this->action_server, &outgoing_feedback);
     if (RCL_RET_OK != ret) {
+      // This isn't set in all cases, but reset anyway.
+      rcl_reset_error();
       continue;
     }
 
     ret = rcl_action_wait_set_add_action_client(
       &this->wait_set, &this->action_client, NULL, NULL);
     if (RCL_RET_OK != ret) {
+      EXPECT_TRUE(rcl_error_is_set());
+      rcl_reset_error();
       continue;
     }
 
     ret = rcl_wait(&this->wait_set, RCL_S_TO_NS(10));
     if (RCL_RET_OK != ret) {
+      EXPECT_TRUE(rcl_error_is_set());
+      rcl_reset_error();
       continue;
     }
 
@@ -1151,12 +1157,16 @@ TEST_F(CLASSNAME(TestActionCommunication, RMW_IMPLEMENTATION), test_valid_feedba
       &this->is_cancel_response_ready,
       &this->is_result_response_ready);
     if (RCL_RET_OK != ret) {
+      EXPECT_TRUE(rcl_error_is_set());
+      rcl_reset_error();
       continue;
     }
 
     // Take feedback with valid arguments
     ret = rcl_action_take_feedback(&this->action_client, &incoming_feedback);
     if (RCL_RET_OK != ret) {
+      EXPECT_TRUE(rcl_error_is_set());
+      rcl_reset_error();
       continue;
     }
 

diff --git a/rcl_action/test/rcl_action/test_action_server.cpp b/rcl_action/test/rcl_action/test_action_server.cpp
@@ -969,9 +969,8 @@ TEST_F(TestActionServerCancelPolicy, test_action_process_cancel_request_by_time_
 TEST_F(TestActionServer, action_server_init_fini_maybe_fail)
 {
   rcl_allocator_t allocator = rcl_get_default_allocator();
-  rcl_ret_t ret;
   rcl_init_options_t init_options = rcl_get_zero_initialized_init_options();
-  ret = rcl_init_options_init(&init_options, allocator);
+  rcl_ret_t ret = rcl_init_options_init(&init_options, allocator);
   ASSERT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str;
   rcl_context_t context = rcl_get_zero_initialized_context();
   ret = rcl_init(0, nullptr, &init_options, &context);
@@ -1012,6 +1011,7 @@ TEST_F(TestActionServerCancelPolicy, test_action_process_cancel_request_maybe_fa
   {
     rcl_ret_t ret = rcl_action_process_cancel_request(
       &this->action_server, &cancel_request, &cancel_response);
+    // Regardless of return, fini should be able to succeed
     (void)ret;
     EXPECT_EQ(RCL_RET_OK, rcl_action_cancel_response_fini(&cancel_response));
   });
@@ -1027,7 +1027,8 @@ TEST_F(TestActionServerCancelPolicy, test_action_expire_goals_maybe_fail)
   {
     rcl_ret_t ret = rcl_action_expire_goals(
       &this->action_server, expired_goals, capacity, &num_expired);
-    (void)ret;
-    rcl_reset_error();
+    if (RCL_RET_OK != ret) {
+      rcl_reset_error();
+    }
   });
 }
diff --git a/rcl_action/test/rcl_action/test_graph.cpp b/rcl_action/test/rcl_action/test_graph.cpp
@@ -603,9 +603,10 @@ TEST_F(TestActionGraphMultiNodeFixture, get_names_and_types_maybe_fail)
     rcl_ret_t ret = rcl_action_get_names_and_types(&this->node, &this->allocator, &nat);
     if (RCL_RET_OK == ret) {
       ret = rcl_names_and_types_fini(&nat);
-      if (ret != RCL_RET_OK) {
+      if (RCL_RET_OK != ret) {
         EXPECT_TRUE(rcutils_error_is_set());
         rcutils_reset_error();
+        EXPECT_EQ(RCL_RET_OK, rcl_names_and_types_fini(&nat));
       }
     }
   });
@@ -629,6 +630,10 @@ TEST_F(TestActionGraphMultiNodeFixture, action_client_init_maybe_fail)
 
     if (RCL_RET_OK == ret) {
       ret = rcl_action_client_fini(&action_client, &this->remote_node);
+      if (RCL_RET_OK != ret) {
+        // This isn't always set, but just in case reset anyway
+        rcutils_reset_error();
+      }
     }
   });
 }
@@ -645,6 +650,7 @@ TEST_F(TestActionGraphMultiNodeFixture, rcl_get_client_names_and_types_by_node_m
       if (ret != RCL_RET_OK) {
         EXPECT_TRUE(rcutils_error_is_set());
         rcutils_reset_error();
+        EXPECT_EQ(RCL_RET_OK, rcl_names_and_types_fini(&nat));
       }
     }
   });
@@ -662,6 +668,7 @@ TEST_F(TestActionGraphMultiNodeFixture, rcl_get_server_names_and_types_by_node_m
       if (ret != RCL_RET_OK) {
         EXPECT_TRUE(rcutils_error_is_set());
         rcutils_reset_error();
+        EXPECT_EQ(RCL_RET_OK, rcl_names_and_types_fini(&nat));
       }
     }
   });