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

Jump to bottom

Add coverage tests wait module #769

Merged
merged 10 commits into from
Sep 1, 2020
Merged

Add coverage tests wait module #769

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
4306e2d
Add coverage tests wait module
Blast545 Aug 26, 2020
4c836b6
Add fault injection test
Blast545 Aug 31, 2020
4c5c4e3
Add removed blank line
Blast545 Aug 31, 2020
9000f99
Add zero_init allocator test to init
Blast545 Aug 31, 2020
c111d2a
Remove bomb allocator test
Blast545 Aug 31, 2020
6c2938c
Add better handling fault injection
Blast545 Aug 31, 2020
2ac1b42
Replace tabs
Blast545 Aug 31, 2020
bb3cdca
Fix uncrustify
Blast545 Aug 31, 2020
4074093
Add comment to fault injection tests
Blast545 Aug 31, 2020
ba06cb9
Move test to another section
Blast545 Sep 1, 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
1 change: 1 addition & 0 deletions rcl/src/rcl/wait.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -382,6 +382,7 @@ rcl_wait_set_resize(
{
RCL_CHECK_ARGUMENT_FOR_NULL(wait_set, RCL_RET_INVALID_ARGUMENT);
RCL_CHECK_ARGUMENT_FOR_NULL(wait_set->impl, RCL_RET_WAIT_SET_INVALID);

Blast545 marked this conversation as resolved.
Show resolved Hide resolved
SET_RESIZE(
subscription,
SET_RESIZE_RMW_DEALLOC(
Expand Down
2 changes: 1 addition & 1 deletion rcl/test/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -265,7 +265,7 @@ function(test_target_function)
SRCS rcl/test_wait.cpp
ENV ${rmw_implementation_env_var}
APPEND_LIBRARY_DIRS ${extra_lib_dirs}
LIBRARIES ${PROJECT_NAME}
LIBRARIES ${PROJECT_NAME} mimick
AMENT_DEPENDENCIES ${rmw_implementation} "osrf_testing_tools_cpp"
)

Expand Down
95 changes: 95 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 @@ -30,6 +30,7 @@
#include "rcutils/logging_macros.h"

#include "./allocator_testing_utils.h"
#include "../mocking_utils/patch.hpp"

#ifdef RMW_IMPLEMENTATION
# define CLASSNAME_(NAME, SUFFIX) NAME ## __ ## SUFFIX
Expand Down Expand Up @@ -684,3 +685,97 @@ TEST_F(CLASSNAME(WaitSetTestFixture, RMW_IMPLEMENTATION), wait_set_get_allocator
ret = rcl_wait_set_fini(&wait_set);
EXPECT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str;
}

// Test wait set init failure cases using mocks
TEST_F(CLASSNAME(WaitSetTestFixture, RMW_IMPLEMENTATION), wait_set_failed_init) {
rcl_wait_set_t wait_set = rcl_get_zero_initialized_wait_set();
// nullptr failures
rcl_ret_t ret =
rcl_wait_set_init(nullptr, 1, 1, 1, 1, 1, 0, context_ptr, rcl_get_default_allocator());
EXPECT_EQ(RCL_RET_INVALID_ARGUMENT, ret);
EXPECT_TRUE(rcl_error_is_set());
rcl_reset_error();

ret =
rcl_wait_set_init(&wait_set, 1, 1, 1, 1, 1, 0, nullptr, rcl_get_default_allocator());
EXPECT_EQ(RCL_RET_INVALID_ARGUMENT, ret);
EXPECT_TRUE(rcl_error_is_set());
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.

@Blast545 shoud this two go to the test case immediately above? Also, should we test using an invalid (zero initialized) allocator?

Copy link
Contributor Author

@Blast545 Blast545 Aug 31, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

You mean making them part of wait_set_get_allocator test?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oops, no, the one above that one, wait_set_valid_arguments. I misread the diff.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree, moved with ba06cb9


{
// Mock rmw implementation to fail init
auto mock = mocking_utils::patch_and_return(
"lib:rcl", rmw_create_wait_set, nullptr);
ret =
rcl_wait_set_init(&wait_set, 1, 1, 1, 1, 1, 0, context_ptr, rcl_get_default_allocator());
EXPECT_EQ(RCL_RET_WAIT_SET_INVALID, ret);
EXPECT_TRUE(rcl_error_is_set());
rcl_reset_error();
}
}

// Test failure init when using a bad allocators
TEST_F(CLASSNAME(WaitSetTestFixture, RMW_IMPLEMENTATION), wait_set_failed_init_bomb_alloc) {
rcl_wait_set_t wait_set = rcl_get_zero_initialized_wait_set();
rcl_ret_t ret = RCL_RET_OK;

// Pass bomb allocator to make resize function fail
rcl_allocator_t bomb_alloc = get_time_bombed_allocator();
for (int i = 0; i < 10; i++) {
set_time_bombed_allocator_count(bomb_alloc, i);
Blast545 marked this conversation as resolved.
Show resolved Hide resolved
ret =
rcl_wait_set_init(&wait_set, 1, 0, 0, 1, 1, 0, context_ptr, bomb_alloc);
if (RCL_RET_OK == ret) {
EXPECT_EQ(RCL_RET_OK, rcl_wait_set_fini(&wait_set));
break;
} else {
EXPECT_EQ(RCL_RET_BAD_ALLOC, ret);
rcl_reset_error();
}
}
}

// Test wait set fini failure cases using mocks
TEST_F(CLASSNAME(WaitSetTestFixture, RMW_IMPLEMENTATION), wait_set_failed_fini) {
rcl_wait_set_t wait_set = rcl_get_zero_initialized_wait_set();
rcl_ret_t ret =
rcl_wait_set_init(&wait_set, 1, 1, 1, 1, 1, 0, context_ptr, rcl_get_default_allocator());
ASSERT_EQ(RCL_RET_OK, ret);
{
// Mock rmw implementation to fail fini
auto mock = mocking_utils::inject_on_return(
"lib:rcl", rmw_destroy_wait_set, RMW_RET_ERROR);
EXPECT_EQ(RCL_RET_WAIT_SET_INVALID, rcl_wait_set_fini(&wait_set));
EXPECT_TRUE(rcl_error_is_set());
rcl_reset_error();
}
}

// Test proper error handling with a fault injection test
TEST_F(CLASSNAME(WaitSetTestFixture, RMW_IMPLEMENTATION), wait_set_test_maybe_init_fail) {
rcl_wait_set_t wait_set = rcl_get_zero_initialized_wait_set();
rcl_ret_t ret = RCL_RET_OK;
rcl_allocator_t alloc = rcl_get_default_allocator();

RCUTILS_FAULT_INJECTION_TEST(
Copy link
Contributor

Choose a reason for hiding this comment

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

I think these two fault injection blocks could use some comments describing what each one tests. Besides the initial capacities, they look the same to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I used the two main coverage paths, I added an explanation with 4074093, let me know if you think it's clear

{
ret = rcl_wait_set_init(&wait_set, 1, 0, 0, 1, 1, 0, context_ptr, alloc);
if (RCL_RET_OK == ret) {
EXPECT_EQ(RCL_RET_OK, rcl_wait_set_fini(&wait_set));
Blast545 marked this conversation as resolved.
Show resolved Hide resolved
} else {
EXPECT_TRUE(RCL_RET_WAIT_SET_INVALID == ret || RCL_RET_BAD_ALLOC == ret);
rcl_reset_error();
}
});

RCUTILS_FAULT_INJECTION_TEST(
{
ret = rcl_wait_set_init(&wait_set, 1, 1, 1, 1, 1, 1, context_ptr, alloc);
if (RCL_RET_OK == ret) {
EXPECT_EQ(RCL_RET_OK, rcl_wait_set_fini(&wait_set));
Blast545 marked this conversation as resolved.
Show resolved Hide resolved
} else {
EXPECT_TRUE(RCL_RET_WAIT_SET_INVALID == ret || RCL_RET_BAD_ALLOC == ret);
rcl_reset_error();
}
});
}