From 4306e2d9537a2bedcd43bcb7a8aa4406d47d912d Mon Sep 17 00:00:00 2001 From: Jorge Perez Date: Wed, 26 Aug 2020 19:01:37 -0300 Subject: [PATCH 01/10] Add coverage tests wait module Signed-off-by: Jorge Perez --- rcl/src/rcl/wait.c | 1 + rcl/test/CMakeLists.txt | 2 +- rcl/test/rcl/test_wait.cpp | 66 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 68 insertions(+), 1 deletion(-) diff --git a/rcl/src/rcl/wait.c b/rcl/src/rcl/wait.c index a23c77f7b..3e8d00b7c 100644 --- a/rcl/src/rcl/wait.c +++ b/rcl/src/rcl/wait.c @@ -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); + SET_RESIZE( subscription, SET_RESIZE_RMW_DEALLOC( diff --git a/rcl/test/CMakeLists.txt b/rcl/test/CMakeLists.txt index 404cdd1da..def4ac147 100644 --- a/rcl/test/CMakeLists.txt +++ b/rcl/test/CMakeLists.txt @@ -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" ) diff --git a/rcl/test/rcl/test_wait.cpp b/rcl/test/rcl/test_wait.cpp index 82244406f..c903b6f91 100644 --- a/rcl/test/rcl/test_wait.cpp +++ b/rcl/test/rcl/test_wait.cpp @@ -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 @@ -684,3 +685,68 @@ 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(); + + { + // 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); + 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(); + } +} From 4c836b6f9918ee63aba6a655ba0d13f578745fee Mon Sep 17 00:00:00 2001 From: Jorge Perez Date: Mon, 31 Aug 2020 13:16:41 -0300 Subject: [PATCH 02/10] Add fault injection test Signed-off-by: Jorge Perez --- rcl/test/rcl/test_wait.cpp | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/rcl/test/rcl/test_wait.cpp b/rcl/test/rcl/test_wait.cpp index c903b6f91..59bbedbcb 100644 --- a/rcl/test/rcl/test_wait.cpp +++ b/rcl/test/rcl/test_wait.cpp @@ -750,3 +750,32 @@ TEST_F(CLASSNAME(WaitSetTestFixture, RMW_IMPLEMENTATION), wait_set_failed_fini) 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( + { + 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)); + } 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)); + } else { + EXPECT_TRUE(RCL_RET_WAIT_SET_INVALID == ret || RCL_RET_BAD_ALLOC == ret); + rcl_reset_error(); + } + }); +} From 4c5c4e36d272682a2c1e95db41dd7acc8a7bbfa7 Mon Sep 17 00:00:00 2001 From: Jorge Perez Date: Mon, 31 Aug 2020 17:23:39 -0300 Subject: [PATCH 03/10] Add removed blank line Signed-off-by: Jorge Perez --- rcl/src/rcl/wait.c | 1 - 1 file changed, 1 deletion(-) diff --git a/rcl/src/rcl/wait.c b/rcl/src/rcl/wait.c index 3e8d00b7c..a23c77f7b 100644 --- a/rcl/src/rcl/wait.c +++ b/rcl/src/rcl/wait.c @@ -382,7 +382,6 @@ 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); - SET_RESIZE( subscription, SET_RESIZE_RMW_DEALLOC( From 9000f99e379e7e0515a173b548e7475b68786c3e Mon Sep 17 00:00:00 2001 From: Jorge Perez Date: Mon, 31 Aug 2020 17:39:02 -0300 Subject: [PATCH 04/10] Add zero_init allocator test to init Signed-off-by: Jorge Perez --- rcl/test/rcl/test_wait.cpp | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/rcl/test/rcl/test_wait.cpp b/rcl/test/rcl/test_wait.cpp index 59bbedbcb..6cdda7faa 100644 --- a/rcl/test/rcl/test_wait.cpp +++ b/rcl/test/rcl/test_wait.cpp @@ -702,6 +702,14 @@ TEST_F(CLASSNAME(WaitSetTestFixture, RMW_IMPLEMENTATION), wait_set_failed_init) EXPECT_TRUE(rcl_error_is_set()); rcl_reset_error(); + rcl_allocator_t zero_init_allocator = + static_cast(rcutils_get_zero_initialized_allocator()); + ret = + rcl_wait_set_init(&wait_set, 1, 1, 1, 1, 1, 0, context_ptr, zero_init_allocator); + EXPECT_EQ(RCL_RET_INVALID_ARGUMENT, ret); + EXPECT_TRUE(rcl_error_is_set()); + rcl_reset_error(); + { // Mock rmw implementation to fail init auto mock = mocking_utils::patch_and_return( From c111d2a285907b1063833c8055e5d8d90109b2f6 Mon Sep 17 00:00:00 2001 From: Jorge Perez Date: Mon, 31 Aug 2020 17:39:34 -0300 Subject: [PATCH 05/10] Remove bomb allocator test Signed-off-by: Jorge Perez --- rcl/test/rcl/test_wait.cpp | 21 --------------------- 1 file changed, 21 deletions(-) diff --git a/rcl/test/rcl/test_wait.cpp b/rcl/test/rcl/test_wait.cpp index 6cdda7faa..25169bf14 100644 --- a/rcl/test/rcl/test_wait.cpp +++ b/rcl/test/rcl/test_wait.cpp @@ -722,27 +722,6 @@ TEST_F(CLASSNAME(WaitSetTestFixture, RMW_IMPLEMENTATION), wait_set_failed_init) } } -// 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); - 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(); From 6c2938c59a9c8d31629f1cfbdd30eeac66e2e1fe Mon Sep 17 00:00:00 2001 From: Jorge Perez Date: Mon, 31 Aug 2020 17:43:19 -0300 Subject: [PATCH 06/10] Add better handling fault injection Signed-off-by: Jorge Perez --- rcl/test/rcl/test_wait.cpp | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/rcl/test/rcl/test_wait.cpp b/rcl/test/rcl/test_wait.cpp index 25169bf14..8911a5e9b 100644 --- a/rcl/test/rcl/test_wait.cpp +++ b/rcl/test/rcl/test_wait.cpp @@ -748,7 +748,10 @@ TEST_F(CLASSNAME(WaitSetTestFixture, RMW_IMPLEMENTATION), wait_set_test_maybe_in { 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)); + ret = rcl_wait_set_fini(&wait_set); + if(ret != RCL_RET_OK) { + rcl_reset_error(); + } } else { EXPECT_TRUE(RCL_RET_WAIT_SET_INVALID == ret || RCL_RET_BAD_ALLOC == ret); rcl_reset_error(); @@ -759,6 +762,10 @@ TEST_F(CLASSNAME(WaitSetTestFixture, RMW_IMPLEMENTATION), wait_set_test_maybe_in { ret = rcl_wait_set_init(&wait_set, 1, 1, 1, 1, 1, 1, context_ptr, alloc); if (RCL_RET_OK == ret) { + ret = rcl_wait_set_fini(&wait_set); + if(ret != RCL_RET_OK) { + rcl_reset_error(); + } EXPECT_EQ(RCL_RET_OK, rcl_wait_set_fini(&wait_set)); } else { EXPECT_TRUE(RCL_RET_WAIT_SET_INVALID == ret || RCL_RET_BAD_ALLOC == ret); From 2ac1b42734df4594fd759a2352a6de38bea0f3e7 Mon Sep 17 00:00:00 2001 From: Jorge Perez Date: Mon, 31 Aug 2020 17:48:51 -0300 Subject: [PATCH 07/10] Replace tabs Signed-off-by: Jorge Perez --- rcl/test/rcl/test_wait.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/rcl/test/rcl/test_wait.cpp b/rcl/test/rcl/test_wait.cpp index 8911a5e9b..c93a69c6d 100644 --- a/rcl/test/rcl/test_wait.cpp +++ b/rcl/test/rcl/test_wait.cpp @@ -750,7 +750,7 @@ TEST_F(CLASSNAME(WaitSetTestFixture, RMW_IMPLEMENTATION), wait_set_test_maybe_in if (RCL_RET_OK == ret) { ret = rcl_wait_set_fini(&wait_set); if(ret != RCL_RET_OK) { - rcl_reset_error(); + rcl_reset_error(); } } else { EXPECT_TRUE(RCL_RET_WAIT_SET_INVALID == ret || RCL_RET_BAD_ALLOC == ret); @@ -764,7 +764,7 @@ TEST_F(CLASSNAME(WaitSetTestFixture, RMW_IMPLEMENTATION), wait_set_test_maybe_in if (RCL_RET_OK == ret) { ret = rcl_wait_set_fini(&wait_set); if(ret != RCL_RET_OK) { - rcl_reset_error(); + rcl_reset_error(); } EXPECT_EQ(RCL_RET_OK, rcl_wait_set_fini(&wait_set)); } else { From bb3cdca19a8f08de8436ddda3dd3c74c8cf3259c Mon Sep 17 00:00:00 2001 From: Jorge Perez Date: Mon, 31 Aug 2020 17:49:40 -0300 Subject: [PATCH 08/10] Fix uncrustify Signed-off-by: Jorge Perez --- rcl/test/rcl/test_wait.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/rcl/test/rcl/test_wait.cpp b/rcl/test/rcl/test_wait.cpp index c93a69c6d..6cade53e2 100644 --- a/rcl/test/rcl/test_wait.cpp +++ b/rcl/test/rcl/test_wait.cpp @@ -749,7 +749,7 @@ TEST_F(CLASSNAME(WaitSetTestFixture, RMW_IMPLEMENTATION), wait_set_test_maybe_in ret = rcl_wait_set_init(&wait_set, 1, 0, 0, 1, 1, 0, context_ptr, alloc); if (RCL_RET_OK == ret) { ret = rcl_wait_set_fini(&wait_set); - if(ret != RCL_RET_OK) { + if (ret != RCL_RET_OK) { rcl_reset_error(); } } else { @@ -763,7 +763,7 @@ TEST_F(CLASSNAME(WaitSetTestFixture, RMW_IMPLEMENTATION), wait_set_test_maybe_in ret = rcl_wait_set_init(&wait_set, 1, 1, 1, 1, 1, 1, context_ptr, alloc); if (RCL_RET_OK == ret) { ret = rcl_wait_set_fini(&wait_set); - if(ret != RCL_RET_OK) { + if (ret != RCL_RET_OK) { rcl_reset_error(); } EXPECT_EQ(RCL_RET_OK, rcl_wait_set_fini(&wait_set)); From 40740933feb7e06f2518af51dd259f487c52085f Mon Sep 17 00:00:00 2001 From: Jorge Perez Date: Mon, 31 Aug 2020 17:59:23 -0300 Subject: [PATCH 09/10] Add comment to fault injection tests Signed-off-by: Jorge Perez --- rcl/test/rcl/test_wait.cpp | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/rcl/test/rcl/test_wait.cpp b/rcl/test/rcl/test_wait.cpp index 6cade53e2..0fcf6de13 100644 --- a/rcl/test/rcl/test_wait.cpp +++ b/rcl/test/rcl/test_wait.cpp @@ -746,6 +746,8 @@ TEST_F(CLASSNAME(WaitSetTestFixture, RMW_IMPLEMENTATION), wait_set_test_maybe_in RCUTILS_FAULT_INJECTION_TEST( { + // Test init in the case where guard_conditions_size + timers_size = 0 + // (used for guard condition size) ret = rcl_wait_set_init(&wait_set, 1, 0, 0, 1, 1, 0, context_ptr, alloc); if (RCL_RET_OK == ret) { ret = rcl_wait_set_fini(&wait_set); @@ -760,6 +762,8 @@ TEST_F(CLASSNAME(WaitSetTestFixture, RMW_IMPLEMENTATION), wait_set_test_maybe_in RCUTILS_FAULT_INJECTION_TEST( { + // Test init wait_set using at least one of each of the possible elements that receives + // (subs, guard conditions, timers, clients, services, events) ret = rcl_wait_set_init(&wait_set, 1, 1, 1, 1, 1, 1, context_ptr, alloc); if (RCL_RET_OK == ret) { ret = rcl_wait_set_fini(&wait_set); From ba06cb9cf2d7f7e92372cd50a474d0817d9cb655 Mon Sep 17 00:00:00 2001 From: Jorge Perez Date: Tue, 1 Sep 2020 10:08:20 -0300 Subject: [PATCH 10/10] Move test to another section Signed-off-by: Jorge Perez --- rcl/test/rcl/test_wait.cpp | 55 +++++++++++++++++++------------------- 1 file changed, 27 insertions(+), 28 deletions(-) diff --git a/rcl/test/rcl/test_wait.cpp b/rcl/test/rcl/test_wait.cpp index 0fcf6de13..895b9d593 100644 --- a/rcl/test/rcl/test_wait.cpp +++ b/rcl/test/rcl/test_wait.cpp @@ -647,6 +647,28 @@ TEST_F(CLASSNAME(WaitSetTestFixture, RMW_IMPLEMENTATION), wait_set_valid_argumen rcl_wait_set_init(&wait_set, 1, 1, 1, 1, 1, 0, ¬_init_context, rcl_get_default_allocator()); EXPECT_EQ(RCL_RET_NOT_INIT, ret) << rcl_get_error_string().str; rcl_reset_error(); + + // nullptr failures + 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(); + + rcl_allocator_t zero_init_allocator = + static_cast(rcutils_get_zero_initialized_allocator()); + ret = + rcl_wait_set_init(&wait_set, 1, 1, 1, 1, 1, 0, context_ptr, zero_init_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, context_ptr, rcl_get_default_allocator()); EXPECT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str; @@ -689,37 +711,14 @@ TEST_F(CLASSNAME(WaitSetTestFixture, RMW_IMPLEMENTATION), wait_set_get_allocator // 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 + // Mock rmw implementation to fail init + auto mock = mocking_utils::patch_and_return( + "lib:rcl", rmw_create_wait_set, nullptr); 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(); - - rcl_allocator_t zero_init_allocator = - static_cast(rcutils_get_zero_initialized_allocator()); - ret = - rcl_wait_set_init(&wait_set, 1, 1, 1, 1, 1, 0, context_ptr, zero_init_allocator); - EXPECT_EQ(RCL_RET_INVALID_ARGUMENT, 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(); - - { - // 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 wait set fini failure cases using mocks