Skip to content

Improve wait sets test coverage. #683

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

Merged
merged 3 commits into from
Jun 18, 2020
Merged

Improve wait sets test coverage. #683

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d5a74da
Improve wait sets test coverage.
hidmic Jun 12, 2020
e7ce68a
Address peer review comments.
hidmic Jun 15, 2020
248e009
Address peer review comments.
hidmic Jun 17, 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
69 changes: 69 additions & 0 deletions rcl/test/rcl/test_wait.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,14 +29,20 @@

#include "rcutils/logging_macros.h"

#include "./allocator_testing_utils.h"

#ifdef RMW_IMPLEMENTATION
# define CLASSNAME_(NAME, SUFFIX) NAME ## __ ## SUFFIX
# define CLASSNAME(NAME, SUFFIX) CLASSNAME_(NAME, SUFFIX)
#else
# define CLASSNAME(NAME, SUFFIX) NAME
#endif

#ifndef _WIN32
#define TOLERANCE RCL_MS_TO_NS(6)
#else
#define TOLERANCE RCL_MS_TO_NS(15)
#endif

class CLASSNAME (WaitSetTestFixture, RMW_IMPLEMENTATION) : public ::testing::Test
{
Expand Down Expand Up @@ -86,6 +92,25 @@ TEST_F(CLASSNAME(WaitSetTestFixture, RMW_IMPLEMENTATION), wait_set_is_valid) {
EXPECT_FALSE(rcl_wait_set_is_valid(&wait_set));
}

TEST_F(CLASSNAME(WaitSetTestFixture, RMW_IMPLEMENTATION), test_failed_resize) {
// Initialize a wait set with a subscription and then resize it to zero.
rcl_allocator_t allocator = get_failing_allocator();
rcl_wait_set_t wait_set = rcl_get_zero_initialized_wait_set();
set_failing_allocator_is_failing(allocator, false);
rcl_ret_t ret =
rcl_wait_set_init(&wait_set, 1, 1, 1, 1, 1, 0, context_ptr, allocator);
EXPECT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str;

set_failing_allocator_is_failing(allocator, true);
ret = rcl_wait_set_resize(&wait_set, 0, 1, 0, 0, 0, 0);
EXPECT_EQ(RCL_RET_BAD_ALLOC, ret);
rcl_reset_error();

set_failing_allocator_is_failing(allocator, false);
ret = rcl_wait_set_fini(&wait_set);
ASSERT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str;
}

TEST_F(CLASSNAME(WaitSetTestFixture, RMW_IMPLEMENTATION), test_resize_to_zero) {
// Initialize a wait set with a subscription and then resize it to zero.
rcl_wait_set_t wait_set = rcl_get_zero_initialized_wait_set();
Expand Down Expand Up @@ -257,6 +282,50 @@ TEST_F(CLASSNAME(WaitSetTestFixture, RMW_IMPLEMENTATION), zero_timeout_triggered
EXPECT_LE(diff, TOLERANCE);
}

// Test rcl_wait with a negative timeout value (blocking ferver) and an overrun timer
Copy link
Contributor

@brawner brawner Jun 12, 2020

Choose a reason for hiding this comment

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

Suggested change
// Test rcl_wait with a negative timeout value (blocking ferver) and an overrun timer
// Test rcl_wait with a negative timeout value (blocking forever) and an overrun timer

Copy link
Contributor Author

@hidmic hidmic Jun 15, 2020

Choose a reason for hiding this comment

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

See e7ce68a.

TEST_F(CLASSNAME(WaitSetTestFixture, RMW_IMPLEMENTATION), zero_timeout_overrun_timer) {
rcl_wait_set_t wait_set = rcl_get_zero_initialized_wait_set();
rcl_ret_t ret =
rcl_wait_set_init(&wait_set, 0, 0, 1, 0, 0, 0, context_ptr, rcl_get_default_allocator());
EXPECT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str;
OSRF_TESTING_TOOLS_CPP_SCOPE_EXIT(
{
ret = rcl_wait_set_fini(&wait_set);
EXPECT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str;
});
rcl_clock_t clock;
rcl_allocator_t allocator = rcl_get_default_allocator();
ret = rcl_clock_init(RCL_STEADY_TIME, &clock, &allocator);
OSRF_TESTING_TOOLS_CPP_SCOPE_EXIT(
{
ret = rcl_clock_fini(&clock);
EXPECT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str;
});
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, rcl_get_default_allocator());
EXPECT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str;
OSRF_TESTING_TOOLS_CPP_SCOPE_EXIT(
{
ret = rcl_timer_fini(&timer);
EXPECT_EQ(RCL_RET_OK, ret) << 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;

// Time spent during wait should be negligible.
Copy link
Contributor

@brawner brawner Jun 12, 2020

Choose a reason for hiding this comment

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

If this fails, and rcl_wait doesn't return will the test just hang?

Also, I can tell you've already figured this out, but timer based tests are notoriously unreliable on our Windows build farm, and it would be good to have as few of them as possible. It's not clear to me that TOLERANCE is a requirement of whether this test passes or succeeds. With the efforts of performance testing underway, I think measurements of how long rcl_wait takes to return should be put there.

What's the behavior you're trying to assert on this test?

Copy link
Contributor Author

@hidmic hidmic Jun 15, 2020

Choose a reason for hiding this comment

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

If this fails, and rcl_wait doesn't return will the test just hang?

If wait sets' implementation misbehaves you mean? It may hang, though at that point it's not clear one can trust the given timeout will be honored either. At any rate, the CTest timeout will eventually kick in.

Also, I can tell you've already figured this out, but timer based tests are notoriously unreliable on our Windows build farm, and it would be good to have as few of them as possible.

Indeed. Not entirely convinced ignoring them is a good idea though.

It's not clear to me that TOLERANCE is a requirement of whether this test passes or succeeds.

The test needs to assert that rmw_wait() didn't block, or, actually, that it was requested to not block. Without mocks, we can only resort to this indirect technique.

What's the behavior you're trying to assert on this test?

That this snippet has the intended effect.

Copy link
Contributor

@brawner brawner Jun 15, 2020

Choose a reason for hiding this comment

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

What sort of event does wait_set wait on here? Can you just ASSERT that the event didn't finish before the function returned? That is, make the event something long but finite, say 1-10 seconds so that wait will return in either case. But it will assert success if the event didn't finish, and failure if it did?

Copy link
Contributor Author

@hidmic hidmic Jun 15, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Hmm, not sure I follow. This test is covering the situation in which a timer (and it is that specific because timers are handled with in a special way) is way past its deadline by the time you wait on it for an X amount of time. In which case the function should realize, adjust its internal timeout and return almost immediately. That's what the test is checking. This other test does the same thing for a timer that is on-time.

The scenario you describe is being (indirectly) covered by this test.

If this fails, and rcl_wait doesn't return will the test just hang?

We can make that timeout finite. We cannot change the assertion itself though.

Copy link
Contributor

@brawner brawner Jun 16, 2020

Choose a reason for hiding this comment

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

Ok, I think I understand better. I still think having a tolerance that is so tight it has to be different on different platforms sort of shows that the final EXPECT_LE is not cleanly deciding which behavior was followed in the code under test. Can you make timeout longer like 1-10 seconds so that it is very obvious whether it returned immediately or not?

Copy link
Contributor

@brawner brawner Jun 16, 2020

Choose a reason for hiding this comment

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

And make TOLERANCE correspondingly larger

Copy link
Contributor Author

@hidmic hidmic Jun 17, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Hmm, I'm not sure I follow you here. Which timeout do you mean? The timer period is 0, the wait timeout is -1 (block forever). The reason why I believe I may have had to bump up the tolerance for Windows is it's rather large scheduler time slice and the flakyness it introduces.

int64_t timeout = -1;
std::chrono::steady_clock::time_point before_sc = std::chrono::steady_clock::now();
ret = rcl_wait(&wait_set, timeout);
std::chrono::steady_clock::time_point after_sc = std::chrono::steady_clock::now();
// We don't expect a timeout here (since the guard condition had already been triggered)
ASSERT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str;
int64_t diff = std::chrono::duration_cast<std::chrono::nanoseconds>(after_sc - before_sc).count();
EXPECT_LE(diff, TOLERANCE);
}

// Check that a canceled timer doesn't wake up rcl_wait
TEST_F(CLASSNAME(WaitSetTestFixture, RMW_IMPLEMENTATION), canceled_timer) {
rcl_wait_set_t wait_set = rcl_get_zero_initialized_wait_set();
Expand Down