[Backport Foxy] Increased RCL coverage

rcl/src/rcl/context.c

@@ -95,11 +95,6 @@ rcl_context_get_rmw_context(rcl_context_t * context)
 void
 __cleanup_context(rcl_context_t * context)
 {
-  // if null, nothing can be done
-  if (NULL == context) {
-    return;
-  }
-
   // reset the instance id to 0 to indicate "invalid" (should already be 0, but this is defensive)
   rcutils_atomic_store((atomic_uint_least64_t *)(&context->instance_id_storage), 0);
 

rcl/src/rcl/time.c

@@ -171,11 +171,8 @@ rcl_ros_clock_fini(
     return RCL_RET_ERROR;
   }
   rcl_clock_generic_fini(clock);
-  if (!clock->data) {
-    RCL_SET_ERROR_MSG("clock data invalid");
-    return RCL_RET_ERROR;
-  }
-  clock->allocator.deallocate((rcl_ros_clock_storage_t *)clock->data, clock->allocator.state);
+  clock->allocator.deallocate(clock->data, clock->allocator.state);
+  clock->data = NULL;
   return RCL_RET_OK;
 }
 
@@ -293,10 +290,8 @@ rcl_enable_ros_time_override(rcl_clock_t * clock)
     return RCL_RET_ERROR;
   }
   rcl_ros_clock_storage_t * storage = (rcl_ros_clock_storage_t *)clock->data;
-  if (!storage) {
-    RCL_SET_ERROR_MSG("Clock storage is not initialized, cannot enable override.");
-    return RCL_RET_ERROR;
-  }
+  RCL_CHECK_FOR_NULL_WITH_MSG(
+    storage, "Clock storage is not initialized, cannot enable override.", return RCL_RET_ERROR);
   if (!storage->active) {
     rcl_time_jump_t time_jump;
     time_jump.delta.nanoseconds = 0;
@@ -316,12 +311,9 @@ rcl_disable_ros_time_override(rcl_clock_t * clock)
     RCL_SET_ERROR_MSG("Clock is not of type RCL_ROS_TIME, cannot disable override.");
     return RCL_RET_ERROR;
   }
-  rcl_ros_clock_storage_t * storage = \
-    (rcl_ros_clock_storage_t *)clock->data;
-  if (!storage) {
-    RCL_SET_ERROR_MSG("Clock storage is not initialized, cannot disable override.");
-    return RCL_RET_ERROR;
-  }
+  rcl_ros_clock_storage_t * storage = (rcl_ros_clock_storage_t *)clock->data;
+  RCL_CHECK_FOR_NULL_WITH_MSG(
+    storage, "Clock storage is not initialized, cannot enable override.", return RCL_RET_ERROR);
   if (storage->active) {
     rcl_time_jump_t time_jump;
     time_jump.delta.nanoseconds = 0;
@@ -344,12 +336,9 @@ rcl_is_enabled_ros_time_override(
     RCL_SET_ERROR_MSG("Clock is not of type RCL_ROS_TIME, cannot query override state.");
     return RCL_RET_ERROR;
   }
-  rcl_ros_clock_storage_t * storage = \
-    (rcl_ros_clock_storage_t *)clock->data;
-  if (!storage) {
-    RCL_SET_ERROR_MSG("Clock storage is not initialized, cannot query override state.");
-    return RCL_RET_ERROR;
-  }
+  rcl_ros_clock_storage_t * storage = (rcl_ros_clock_storage_t *)clock->data;
+  RCL_CHECK_FOR_NULL_WITH_MSG(
+    storage, "Clock storage is not initialized, cannot enable override.", return RCL_RET_ERROR);
   *is_enabled = storage->active;
   return RCL_RET_OK;
 }
@@ -364,8 +353,10 @@ rcl_set_ros_time_override(
     RCL_SET_ERROR_MSG("Clock is not of type RCL_ROS_TIME, cannot set time override.");
     return RCL_RET_ERROR;
   }
-  rcl_time_jump_t time_jump;
   rcl_ros_clock_storage_t * storage = (rcl_ros_clock_storage_t *)clock->data;
+  RCL_CHECK_FOR_NULL_WITH_MSG(
+    storage, "Clock storage is not initialized, cannot enable override.", return RCL_RET_ERROR);
+  rcl_time_jump_t time_jump;
   if (storage->active) {
     time_jump.clock_change = RCL_ROS_TIME_NO_CHANGE;
     rcl_time_point_value_t current_time;
@@ -453,19 +444,18 @@ rcl_clock_remove_jump_callback(
   }
 
   // Shrink size of the callback array
-  if (clock->num_jump_callbacks == 1) {
+  if (--(clock->num_jump_callbacks) == 0) {
     clock->allocator.deallocate(clock->jump_callbacks, clock->allocator.state);
     clock->jump_callbacks = NULL;
   } else {
     rcl_jump_callback_info_t * callbacks = clock->allocator.reallocate(
-      clock->jump_callbacks, sizeof(rcl_jump_callback_info_t) * (clock->num_jump_callbacks - 1),
+      clock->jump_callbacks, sizeof(rcl_jump_callback_info_t) * clock->num_jump_callbacks,
       clock->allocator.state);
     if (NULL == callbacks) {
       RCL_SET_ERROR_MSG("Failed to shrink jump callbacks");
       return RCL_RET_BAD_ALLOC;
     }
     clock->jump_callbacks = callbacks;
   }
-  --(clock->num_jump_callbacks);
   return RCL_RET_OK;
 }

rcl/test/CMakeLists.txt

@@ -50,6 +50,23 @@ function(test_target_function)
     AMENT_DEPENDENCIES ${rmw_implementation}
   )
 
+  # TODO(hidmic): re-enable timer tests against RTI Connext once
+  #               https://github.com/ros2/rcl/issues/687 is resolved
+  set(AMENT_GTEST_ARGS "")
+  if(rmw_implementation STREQUAL "rmw_connext_cpp")
+    message(STATUS "Skipping test_timer${target_suffix} test.")
+    set(AMENT_GTEST_ARGS "SKIP_TEST")
+  endif()
+
+  rcl_add_custom_gtest(test_timer${target_suffix}
+    SRCS rcl/test_timer.cpp
+    ENV ${rmw_implementation_env_var}
+    APPEND_LIBRARY_DIRS ${extra_lib_dirs}
+    LIBRARIES ${PROJECT_NAME} osrf_testing_tools_cpp::memory_tools
+    AMENT_DEPENDENCIES ${rmw_implementation}
+    ${AMENT_GTEST_ARGS}
+  )
+
   rcl_add_custom_gtest(test_context${target_suffix}
     SRCS rcl/test_context.cpp
     ENV ${rmw_implementation_env_var} ${memory_tools_ld_preload_env_var}
@@ -350,13 +367,6 @@ rcl_add_custom_gtest(test_expand_topic_name
   LIBRARIES ${PROJECT_NAME}
 )
 
-rcl_add_custom_gtest(test_timer${target_suffix}
-  SRCS rcl/test_timer.cpp
-  APPEND_LIBRARY_DIRS ${extra_lib_dirs}
-  LIBRARIES ${PROJECT_NAME}
-  AMENT_DEPENDENCIES "osrf_testing_tools_cpp"
-)
-
 rcl_add_custom_gtest(test_security
   SRCS rcl/test_security.cpp
   APPEND_LIBRARY_DIRS ${extra_lib_dirs}

rcl/test/rcl/test_context.cpp

@@ -124,3 +124,8 @@ TEST_F(CLASSNAME(TestContextFixture, RMW_IMPLEMENTATION), nominal) {
   ret = rcl_init_options_fini(&init_options);
   EXPECT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str;
 }
+
+TEST_F(CLASSNAME(TestContextFixture, RMW_IMPLEMENTATION), bad_fini) {
+  EXPECT_EQ(RCL_RET_INVALID_ARGUMENT, rcl_context_fini(nullptr));
+  rcl_reset_error();
+}

rcl/test/rcl/test_expand_topic_name.cpp

@@ -23,6 +23,8 @@
 
 #include "rcl/error_handling.h"
 
+#include "./allocator_testing_utils.h"
+
 using namespace std::string_literals;
 
 TEST(test_expand_topic_name, normal) {
@@ -121,6 +123,16 @@ TEST(test_expand_topic_name, invalid_arguments) {
     rcl_reset_error();
   }
 
+  // pass failing allocator
+  {
+    rcl_allocator_t bad_allocator = get_failing_allocator();
+    EXPECT_EQ(
+      RCL_RET_BAD_ALLOC,
+      rcl_expand_topic_name("/absolute", node, ns, &subs, bad_allocator, &expanded_topic));
+    EXPECT_STREQ(NULL, expanded_topic);
+    rcl_reset_error();
+  }
+
   ret = rcutils_string_map_fini(&subs);
   ASSERT_EQ(RCL_RET_OK, ret);
 }