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

Improve rcl timer test coverage. #680

Merged
merged 7 commits into from
Jun 19, 2020
Merged

Improve rcl timer test coverage. #680

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
6bc2557
Improve rcl timer test coverage.
hidmic Jun 12, 2020
dc69a75
Fix flaky waits on Windows.
hidmic Jun 12, 2020
b19b6f0
Address peer review comments.
hidmic Jun 15, 2020
fd44afb
Please linter.
hidmic Jun 16, 2020
4041bae
Cover a few more lines using a faulty clock.
hidmic Jun 16, 2020
d2bd754
Skip rcl timer tests for rmw_connext_cpp.
hidmic Jun 16, 2020
6aa785e
Drop unnecessary rcl_reset_error() calls.
hidmic Jun 19, 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
24 changes: 17 additions & 7 deletions rcl/test/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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}
Expand Down Expand Up @@ -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}
Expand Down
204 changes: 202 additions & 2 deletions rcl/test/rcl/test_timer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,102 @@ class TestPreInitTimer : public TestTimerFixture
}
};

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;

Expand Down Expand Up @@ -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());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is an important change, can you add a comment so it's clearer why this value was chosen?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dropped a comment in b19b6f0.

ASSERT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str;

ret = rcl_timer_init(
Expand Down Expand Up @@ -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;

Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you're going to start adding in more rcl_reset_error() statements where you don't expect errors. It might be good to add, EXPECT_FALSE(rcl_error_is_set()) before it so you know you're not swallowing an error.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, do you mean we ensure we don't have false RCL_RET_OKs?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still not sure, so I'm rolling those back. See 6aa785e.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry I missed this comment. I meant to ensure that rcl_error_is_set() returns false before resetting it. That way if the error is set, we find out about it before resetting it for the next test segment.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, but we would've found out about it when the success expectation is broken. In any case, I rolled it back. We can discuss whether we should be doing those checks irrespective of return codes on a follow-up issue/PR.

EXPECT_EQ(times_called, 4);
}

Expand Down Expand Up @@ -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) {
Expand All @@ -669,14 +865,18 @@ 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) {
rcl_time_point_value_t time_sice_next_call_start = 0u;
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);
}