Improve rcl timer test coverage. (#680)

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

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_timer.cpp

@@ -114,6 +114,102 @@ public:
   }
 };
 
+TEST_F(TestTimerFixture, test_timer_init_with_invalid_arguments) {
+  rcl_clock_t clock;
+  rcl_allocator_t allocator = rcl_get_default_allocator();
+  rcl_ret_t ret = rcl_clock_init(RCL_STEADY_TIME, &clock, &allocator);
+  ASSERT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str;
+  rcl_timer_t timer = rcl_get_zero_initialized_timer();
+
+  ret = rcl_timer_init(
+    nullptr, &clock, this->context_ptr, RCL_MS_TO_NS(50), nullptr, allocator);
+  EXPECT_EQ(RCL_RET_INVALID_ARGUMENT, ret);
+  rcl_reset_error();
+
+  ret = rcl_timer_init(
+    &timer, nullptr, this->context_ptr, RCL_MS_TO_NS(50), nullptr, allocator);
+  EXPECT_EQ(RCL_RET_INVALID_ARGUMENT, ret);
+  rcl_reset_error();
+
+  ret = rcl_timer_init(
+    &timer, &clock, nullptr, RCL_MS_TO_NS(50), nullptr, allocator);
+  EXPECT_EQ(RCL_RET_INVALID_ARGUMENT, ret);
+  rcl_reset_error();
+
+  ret = rcl_timer_init(
+    &timer, &clock, this->context_ptr, -1, nullptr, allocator);
+  EXPECT_EQ(RCL_RET_INVALID_ARGUMENT, ret);
+  rcl_reset_error();
+
+  rcl_allocator_t invalid_allocator = rcutils_get_zero_initialized_allocator();
+  ret = rcl_timer_init(
+    &timer, &clock, this->context_ptr, RCL_MS_TO_NS(50), nullptr, invalid_allocator);
+  EXPECT_EQ(RCL_RET_INVALID_ARGUMENT, ret);
+  rcl_reset_error();
+}
+
+TEST_F(TestTimerFixture, test_timer_with_invalid_clock) {
+  rcl_clock_t clock;
+  rcl_allocator_t allocator = rcl_get_default_allocator();
+  rcl_ret_t ret = rcl_clock_init(RCL_CLOCK_UNINITIALIZED, &clock, &allocator);
+  ASSERT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str;
+
+  rcl_timer_t timer = rcl_get_zero_initialized_timer();
+  ret = rcl_timer_init(
+    &timer, &clock, this->context_ptr, 0, nullptr, allocator);
+  EXPECT_EQ(RCL_RET_ERROR, ret);
+  rcl_reset_error();
+
+  ret = rcl_clock_init(RCL_ROS_TIME, &clock, &allocator);
+  ASSERT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str;
+  OSRF_TESTING_TOOLS_CPP_SCOPE_EXIT(
+  {
+    rcl_ret_t ret = rcl_clock_fini(&clock);
+    EXPECT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str;
+  });
+
+  ret = rcl_timer_init(
+    &timer, &clock, this->context_ptr, 0, nullptr, allocator);
+  ASSERT_EQ(RCL_RET_OK, ret);
+  OSRF_TESTING_TOOLS_CPP_SCOPE_EXIT(
+  {
+    rcl_ret_t ret = rcl_timer_fini(&timer);
+    EXPECT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str;
+  });
+
+  rcl_clock_t * timer_clock;
+  ret = rcl_timer_clock(&timer, &timer_clock);
+  ASSERT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str;
+  timer_clock->get_now = nullptr;
+
+  // Trigger clock jump callbacks
+  ret = rcl_enable_ros_time_override(timer_clock);
+  EXPECT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str;
+
+  ret = rcl_timer_call(&timer);
+  EXPECT_EQ(RCL_RET_ERROR, ret);
+  rcl_reset_error();
+
+  int64_t time_until_next_call;
+  ret = rcl_timer_get_time_until_next_call(&timer, &time_until_next_call);
+  EXPECT_EQ(RCL_RET_ERROR, ret);
+  rcl_reset_error();
+
+  bool ready;
+  ret = rcl_timer_is_ready(&timer, &ready);
+  EXPECT_EQ(RCL_RET_ERROR, ret);
+  rcl_reset_error();
+
+  rcl_time_point_value_t time_since_last_call;
+  ret = rcl_timer_get_time_since_last_call(&timer, &time_since_last_call);
+  EXPECT_EQ(RCL_RET_ERROR, ret);
+  rcl_reset_error();
+
+  ret = rcl_timer_reset(&timer);
+  EXPECT_EQ(RCL_RET_ERROR, ret);
+  rcl_reset_error();
+}
+
 TEST_F(TestTimerFixture, test_two_timers) {
   rcl_ret_t ret;
 
@@ -182,8 +278,9 @@ TEST_F(TestTimerFixture, test_two_timers_ready_before_timeout) {
   rcl_timer_t timer = rcl_get_zero_initialized_timer();
   rcl_timer_t timer2 = rcl_get_zero_initialized_timer();
 
+  // Keep the first timer period low enough so that rcl_wait() doesn't timeout too early.
   ret = rcl_timer_init(
-    &timer, &clock, this->context_ptr, RCL_MS_TO_NS(50), nullptr, rcl_get_default_allocator());
+    &timer, &clock, this->context_ptr, RCL_MS_TO_NS(10), nullptr, rcl_get_default_allocator());
   ASSERT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str;
 
   ret = rcl_timer_init(
@@ -274,6 +371,95 @@ TEST_F(TestTimerFixture, test_timer_not_ready) {
   EXPECT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str;
 }
 
+TEST_F(TestTimerFixture, test_timer_overrun) {
+  rcl_clock_t clock;
+  rcl_allocator_t allocator = rcl_get_default_allocator();
+  rcl_ret_t ret = rcl_clock_init(RCL_STEADY_TIME, &clock, &allocator);
+  ASSERT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str;
+  OSRF_TESTING_TOOLS_CPP_SCOPE_EXIT(
+  {
+    rcl_ret_t ret = rcl_clock_fini(&clock);
+    EXPECT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str;
+  });
+
+  rcl_timer_t timer = rcl_get_zero_initialized_timer();
+  ret = rcl_timer_init(
+    &timer, &clock, this->context_ptr, RCL_MS_TO_NS(200), nullptr, rcl_get_default_allocator());
+  ASSERT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str;
+  OSRF_TESTING_TOOLS_CPP_SCOPE_EXIT(
+  {
+    rcl_ret_t ret = rcl_timer_fini(&timer);
+    EXPECT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str;
+  });
+
+  rcl_wait_set_t wait_set = rcl_get_zero_initialized_wait_set();
+  ret = rcl_wait_set_init(&wait_set, 0, 0, 1, 0, 0, 0, context_ptr, rcl_get_default_allocator());
+  ASSERT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str;
+  OSRF_TESTING_TOOLS_CPP_SCOPE_EXIT(
+  {
+    rcl_ret_t ret = rcl_wait_set_fini(&wait_set);
+    EXPECT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str;
+  });
+
+  // Force multiple timer timeouts.
+  ret = rcl_wait(&wait_set, RCL_MS_TO_NS(500));
+  EXPECT_EQ(RCL_RET_TIMEOUT, ret) << rcl_get_error_string().str;
+  rcl_reset_error();
+
+  bool is_ready = false;
+  ret = rcl_timer_is_ready(&timer, &is_ready);
+  EXPECT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str;
+  EXPECT_TRUE(is_ready);
+
+  EXPECT_EQ(RCL_RET_OK, rcl_timer_call(&timer)) << rcl_get_error_string().str;
+
+  ret = rcl_wait_set_add_timer(&wait_set, &timer, NULL);
+  EXPECT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str;
+
+  // Ensure period is re-aligned.
+  ret = rcl_wait(&wait_set, RCL_MS_TO_NS(10));
+  EXPECT_EQ(RCL_RET_TIMEOUT, ret) << rcl_get_error_string().str;
+  rcl_reset_error();
+
+  ret = rcl_timer_is_ready(&timer, &is_ready);
+  EXPECT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str;
+  EXPECT_FALSE(is_ready);
+}
+
+TEST_F(TestTimerFixture, test_timer_with_zero_period) {
+  rcl_clock_t clock;
+  rcl_allocator_t allocator = rcl_get_default_allocator();
+  rcl_ret_t ret = rcl_clock_init(RCL_STEADY_TIME, &clock, &allocator);
+  ASSERT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str;
+  OSRF_TESTING_TOOLS_CPP_SCOPE_EXIT(
+  {
+    rcl_ret_t ret = rcl_clock_fini(&clock);
+    EXPECT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str;
+  });
+
+  rcl_timer_t timer = rcl_get_zero_initialized_timer();
+  ret = rcl_timer_init(
+    &timer, &clock, this->context_ptr, 0, nullptr, rcl_get_default_allocator());
+  ASSERT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str;
+  OSRF_TESTING_TOOLS_CPP_SCOPE_EXIT(
+  {
+    rcl_ret_t ret = rcl_timer_fini(&timer);
+    EXPECT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str;
+  });
+
+  bool is_ready = false;
+  ret = rcl_timer_is_ready(&timer, &is_ready);
+  EXPECT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str;
+  EXPECT_TRUE(is_ready) << rcl_get_error_string().str;
+
+  int64_t time_until_next_call = 0;
+  ret = rcl_timer_get_time_until_next_call(&timer, &time_until_next_call);
+  EXPECT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str;
+  EXPECT_LE(time_until_next_call, 0);
+
+  EXPECT_EQ(RCL_RET_OK, rcl_timer_call(&timer)) << rcl_get_error_string().str;
+}
+
 TEST_F(TestTimerFixture, test_canceled_timer) {
   rcl_ret_t ret;
 
@@ -567,6 +753,7 @@ TEST_F(TestPreInitTimer, test_timer_get_allocator) {
   EXPECT_TRUE(rcutils_allocator_is_valid(allocator_returned));
 
   EXPECT_EQ(NULL, rcl_timer_get_allocator(nullptr));
+  rcl_reset_error();
 }
 
 TEST_F(TestPreInitTimer, test_timer_clock) {
@@ -599,8 +786,15 @@ TEST_F(TestPreInitTimer, test_timer_call) {
   EXPECT_EQ(RCL_RET_OK, rcl_timer_get_time_until_next_call(&timer, &next_call_end));
   EXPECT_GT(next_call_start, next_call_end);
 
+  EXPECT_EQ(RCL_RET_OK, rcl_enable_ros_time_override(&this->clock)) << rcl_get_error_string().str;
+  EXPECT_EQ(RCL_RET_OK, rcl_set_ros_time_override(&this->clock, -1)) << rcl_get_error_string().str;
+  EXPECT_EQ(RCL_RET_ERROR, rcl_timer_call(&timer));
+  rcl_reset_error();
+  EXPECT_EQ(times_called, 4);
+
   EXPECT_EQ(RCL_RET_OK, rcl_timer_cancel(&timer)) << rcl_get_error_string().str;
   EXPECT_EQ(RCL_RET_TIMER_CANCELED, rcl_timer_call(&timer));
+  rcl_reset_error();
   EXPECT_EQ(times_called, 4);
 }
 
@@ -654,13 +848,15 @@ TEST_F(TestPreInitTimer, test_invalid_init_fini) {
     RCL_RET_ALREADY_INIT, rcl_timer_init(
       &timer, &clock, this->context_ptr, 500, nullptr,
       rcl_get_default_allocator())) << rcl_get_error_string().str;
+  rcl_reset_error();
 
   ASSERT_EQ(
     RCL_RET_BAD_ALLOC, rcl_timer_init(
       &timer_fail, &clock, this->context_ptr, RCL_S_TO_NS(1), timer_callback_test,
       bad_allocator)) << rcl_get_error_string().str;
+  rcl_reset_error();
 
-  EXPECT_EQ(RCL_RET_OK, rcl_timer_fini(nullptr));
+  EXPECT_EQ(RCL_RET_OK, rcl_timer_fini(nullptr)) << rcl_get_error_string().str;
 }
 
 TEST_F(TestPreInitTimer, test_timer_get_period) {
@@ -669,7 +865,9 @@ TEST_F(TestPreInitTimer, test_timer_get_period) {
   EXPECT_EQ(RCL_S_TO_NS(1), period);
 
   EXPECT_EQ(RCL_RET_INVALID_ARGUMENT, rcl_timer_get_period(nullptr, &period));
+  rcl_reset_error();
   EXPECT_EQ(RCL_RET_INVALID_ARGUMENT, rcl_timer_get_period(&timer, nullptr));
+  rcl_reset_error();
 }
 
 TEST_F(TestPreInitTimer, test_time_since_last_call) {
@@ -677,6 +875,8 @@ TEST_F(TestPreInitTimer, test_time_since_last_call) {
   rcl_time_point_value_t time_sice_next_call_end = 0u;
 
   ASSERT_EQ(RCL_RET_OK, rcl_timer_get_time_since_last_call(&timer, &time_sice_next_call_start));
+  // Cope with coarse system time resolution.
+  std::this_thread::sleep_for(std::chrono::milliseconds(1));
   ASSERT_EQ(RCL_RET_OK, rcl_timer_get_time_since_last_call(&timer, &time_sice_next_call_end));
   EXPECT_GT(time_sice_next_call_end, time_sice_next_call_start);
 }