From dbf272fdf37373e84209a8be41c65a512f9f90cc Mon Sep 17 00:00:00 2001 From: Stephen Brawner Date: Thu, 27 Aug 2020 14:27:12 -0700 Subject: [PATCH 1/4] Fault injection tests for rcl_yaml Signed-off-by: Stephen Brawner --- rcl_yaml_param_parser/CMakeLists.txt | 2 + rcl_yaml_param_parser/test/test_namespace.cpp | 49 +++++++++++ rcl_yaml_param_parser/test/test_parser.cpp | 86 ++++++++++--------- 3 files changed, 98 insertions(+), 39 deletions(-) diff --git a/rcl_yaml_param_parser/CMakeLists.txt b/rcl_yaml_param_parser/CMakeLists.txt index 692f9a520..131cec6ea 100644 --- a/rcl_yaml_param_parser/CMakeLists.txt +++ b/rcl_yaml_param_parser/CMakeLists.txt @@ -67,6 +67,7 @@ if(BUILD_TESTING) "rcutils" "osrf_testing_tools_cpp" ) + target_compile_definitions(test_namespace PUBLIC RCUTILS_ENABLE_FAULT_INJECTION) target_link_libraries(test_namespace ${PROJECT_NAME}) endif() @@ -114,6 +115,7 @@ if(BUILD_TESTING) "osrf_testing_tools_cpp" ) target_link_libraries(test_parser ${PROJECT_NAME}) + target_compile_definitions(test_parser PUBLIC RCUTILS_ENABLE_FAULT_INJECTION) endif() ament_add_gtest(test_yaml_variant diff --git a/rcl_yaml_param_parser/test/test_namespace.cpp b/rcl_yaml_param_parser/test/test_namespace.cpp index 8c685a78f..84e0db59c 100644 --- a/rcl_yaml_param_parser/test/test_namespace.cpp +++ b/rcl_yaml_param_parser/test/test_namespace.cpp @@ -104,3 +104,52 @@ TEST(TestNamespace, replace_ns) { EXPECT_STREQ(expected_param_ns, ns_tracker.parameter_ns); EXPECT_EQ(3u, ns_tracker.num_parameter_ns); } + +TEST(TestNamespace, replace_ns_maybe_fail) { + rcutils_allocator_t allocator = rcutils_get_default_allocator(); + namespace_tracker_t ns_tracker; + ns_tracker.node_ns = rcutils_strdup("node1/node2", allocator); + ASSERT_STREQ("node1/node2", ns_tracker.node_ns); + ns_tracker.parameter_ns = rcutils_strdup("param1.param2", allocator); + ASSERT_STREQ("param1.param2", ns_tracker.parameter_ns); + ns_tracker.num_node_ns = 2; + ns_tracker.num_parameter_ns = 2; + + char * expected_ns = rcutils_strdup("new_ns1/new_ns2/new_ns3", allocator); + ASSERT_STREQ("new_ns1/new_ns2/new_ns3", expected_ns); + OSRF_TESTING_TOOLS_CPP_SCOPE_EXIT( + { + allocator.deallocate(expected_ns, allocator.state); + }); + + char * expected_param_ns = + rcutils_strdup("new_param1.new_param2.new_param3", allocator); + ASSERT_STREQ("new_param1.new_param2.new_param3", expected_param_ns); + OSRF_TESTING_TOOLS_CPP_SCOPE_EXIT( + { + allocator.deallocate(expected_param_ns, allocator.state); + }); + + RCUTILS_FAULT_INJECTION_TEST( + { + rcutils_ret_t ret = + replace_ns(&ns_tracker, expected_ns, 3, NS_TYPE_NODE, allocator); + if (RCUTILS_RET_OK != ret) { + EXPECT_EQ(nullptr, ns_tracker.node_ns); + rcutils_reset_error(); + } else { + EXPECT_EQ(RCUTILS_RET_OK, ret) << rcutils_get_error_string().str; + EXPECT_STREQ(expected_ns, ns_tracker.node_ns); + EXPECT_EQ(3u, ns_tracker.num_node_ns); + } + + ret = replace_ns(&ns_tracker, expected_param_ns, 3, NS_TYPE_PARAM, allocator); + if (RCUTILS_RET_OK != ret) { + EXPECT_EQ(nullptr, ns_tracker.parameter_ns); + rcutils_reset_error(); + } else { + EXPECT_STREQ(expected_param_ns, ns_tracker.parameter_ns); + EXPECT_EQ(3u, ns_tracker.num_parameter_ns); + } + }); +} diff --git a/rcl_yaml_param_parser/test/test_parser.cpp b/rcl_yaml_param_parser/test/test_parser.cpp index 34dc84485..694956ca9 100644 --- a/rcl_yaml_param_parser/test/test_parser.cpp +++ b/rcl_yaml_param_parser/test/test_parser.cpp @@ -12,11 +12,16 @@ // See the License for the specific language governing permissions and // limitations under the License. +#include +#include + #include "gtest/gtest.h" + #include "osrf_testing_tools_cpp/scope_exit.hpp" #include "rcl_yaml_param_parser/parser.h" #include "rcutils/error_handling.h" #include "rcutils/filesystem.h" +#include "rcutils/testing/fault_injection.h" #include "./time_bomb_allocator_testing_utils.h" TEST(RclYamlParamParser, node_init_fini) { @@ -308,46 +313,49 @@ TEST(RclYamlParamParser, test_parse_file_with_bad_allocator) { { allocator.deallocate(test_path, allocator.state); }); - char * path = rcutils_join_path(test_path, "correct_config.yaml", allocator); - ASSERT_TRUE(NULL != path) << rcutils_get_error_string().str; - OSRF_TESTING_TOOLS_CPP_SCOPE_EXIT( - { - allocator.deallocate(path, allocator.state); - }); - ASSERT_TRUE(rcutils_exists(path)) << "No test YAML file found at " << path; - - rcl_params_t * params_hdl = rcl_yaml_node_struct_init(allocator); - EXPECT_TRUE(rcl_parse_yaml_file(path, params_hdl)); - rcl_yaml_node_struct_fini(params_hdl); - params_hdl = NULL; - - // Check sporadic failing malloc calls - for (int i = 0; i < 100; ++i) { - params_hdl = rcl_yaml_node_struct_init(get_time_bomb_allocator()); - ASSERT_TRUE(NULL != params_hdl) << rcutils_get_error_string().str; - - set_time_bomb_allocator_malloc_count(params_hdl->allocator, i); - bool res = rcl_parse_yaml_file(path, params_hdl); - // Not verifying res is true or false here, because eventually it will come back with an ok - // result. We're just trying to make sure that bad allocations are properly handled - (void)res; - rcl_yaml_node_struct_fini(params_hdl); - params_hdl = NULL; - } - - // Check sporadic failing calloc calls - for (int i = 0; i < 100; ++i) { - params_hdl = rcl_yaml_node_struct_init(get_time_bomb_allocator()); - ASSERT_TRUE(NULL != params_hdl) << rcutils_get_error_string().str; - - set_time_bomb_allocator_calloc_count(params_hdl->allocator, i); - bool res = rcl_parse_yaml_file(path, params_hdl); - // Not verifying res is true or false here, because eventually it will come back with an ok - // result. We're just trying to make sure that bad allocations are properly handled - (void)res; - rcl_yaml_node_struct_fini(params_hdl); - params_hdl = NULL; + const std::vector filenames = { + "correct_config.yaml", + "empty_string.yaml", + "indented_name_space.yaml", + "max_num_params.yaml", + "multi_ns_correct.yaml", + "no_alias_support.yaml", + "no_value1.yaml", + "overlay.yaml", + "params_with_no_node.yaml", + "root_ns.yaml", + "seq_map1.yaml", + "seq_map2.yaml", + "string_array_with_quoted_number.yaml" + }; + + for (auto & filename : filenames) { + SCOPED_TRACE(filename); + char * path = rcutils_join_path(test_path, filename.c_str(), allocator); + ASSERT_TRUE(NULL != path) << rcutils_get_error_string().str; + OSRF_TESTING_TOOLS_CPP_SCOPE_EXIT( + { + allocator.deallocate(path, allocator.state); + }); + ASSERT_TRUE(rcutils_exists(path)) << "No test YAML file found at " << path; + + // Check sporadic failing malloc calls + RCUTILS_FAULT_INJECTION_TEST( + { + rcutils_allocator_t allocator = rcutils_get_default_allocator(); + rcl_params_t * params_hdl = rcl_yaml_node_struct_init(allocator); + if (NULL == params_hdl) { + continue; + } + + bool res = rcl_parse_yaml_file(path, params_hdl); + // Not verifying res is true or false here, because eventually it will come back with an ok + // result. We're just trying to make sure that bad allocations are properly handled + (void)res; + rcl_yaml_node_struct_fini(params_hdl); + params_hdl = NULL; + }); } } From 137820b9c513e4c6ad7b9335b7b645aaa6e27cb4 Mon Sep 17 00:00:00 2001 From: Stephen Brawner Date: Fri, 28 Aug 2020 15:21:06 -0700 Subject: [PATCH 2/4] PR Feedback Signed-off-by: Stephen Brawner --- rcl_yaml_param_parser/test/test_namespace.cpp | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/rcl_yaml_param_parser/test/test_namespace.cpp b/rcl_yaml_param_parser/test/test_namespace.cpp index 84e0db59c..7a3c65fa2 100644 --- a/rcl_yaml_param_parser/test/test_namespace.cpp +++ b/rcl_yaml_param_parser/test/test_namespace.cpp @@ -114,6 +114,11 @@ TEST(TestNamespace, replace_ns_maybe_fail) { ASSERT_STREQ("param1.param2", ns_tracker.parameter_ns); ns_tracker.num_node_ns = 2; ns_tracker.num_parameter_ns = 2; + OSRF_TESTING_TOOLS_CPP_SCOPE_EXIT( + { + allocator.deallocate(ns_tracker.node_ns, allocator.state); + allocator.deallocate(ns_tracker.parameter_ns, allocator.state); + }); char * expected_ns = rcutils_strdup("new_ns1/new_ns2/new_ns3", allocator); ASSERT_STREQ("new_ns1/new_ns2/new_ns3", expected_ns); From c4352a141e10afcb36c1712b4085092794ffca36 Mon Sep 17 00:00:00 2001 From: Stephen Brawner Date: Mon, 31 Aug 2020 16:07:25 -0700 Subject: [PATCH 3/4] Pause fault injection Signed-off-by: Stephen Brawner --- rcl_yaml_param_parser/test/test_parser.cpp | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/rcl_yaml_param_parser/test/test_parser.cpp b/rcl_yaml_param_parser/test/test_parser.cpp index 694956ca9..a124a52a4 100644 --- a/rcl_yaml_param_parser/test/test_parser.cpp +++ b/rcl_yaml_param_parser/test/test_parser.cpp @@ -353,7 +353,14 @@ TEST(RclYamlParamParser, test_parse_file_with_bad_allocator) { // Not verifying res is true or false here, because eventually it will come back with an ok // result. We're just trying to make sure that bad allocations are properly handled (void)res; + + // If `rcutils_string_array_fini` fails, there will be a small memory leak here. + // Pausing fault injection so this test runs clean + int64_t count = rcutils_fault_injection_get_count(); + rcutils_fault_injection_set_count(RCUTILS_FAULT_INJECTION_NEVER_FAIL); rcl_yaml_node_struct_fini(params_hdl); + rcutils_fault_injection_set_count(count); + params_hdl = NULL; }); } From 8ca6f42f290336ca5ba3d9ee2a4eedbbcc6cc38b Mon Sep 17 00:00:00 2001 From: Stephen Brawner Date: Tue, 1 Sep 2020 10:58:37 -0700 Subject: [PATCH 4/4] variant_copy Signed-off-by: Stephen Brawner --- rcl_yaml_param_parser/test/test_parser.cpp | 7 +--- .../test/test_yaml_variant.cpp | 32 +++++++++++++++++++ 2 files changed, 33 insertions(+), 6 deletions(-) diff --git a/rcl_yaml_param_parser/test/test_parser.cpp b/rcl_yaml_param_parser/test/test_parser.cpp index a124a52a4..abf88bd7e 100644 --- a/rcl_yaml_param_parser/test/test_parser.cpp +++ b/rcl_yaml_param_parser/test/test_parser.cpp @@ -340,7 +340,6 @@ TEST(RclYamlParamParser, test_parse_file_with_bad_allocator) { }); ASSERT_TRUE(rcutils_exists(path)) << "No test YAML file found at " << path; - // Check sporadic failing malloc calls RCUTILS_FAULT_INJECTION_TEST( { rcutils_allocator_t allocator = rcutils_get_default_allocator(); @@ -355,12 +354,8 @@ TEST(RclYamlParamParser, test_parse_file_with_bad_allocator) { (void)res; // If `rcutils_string_array_fini` fails, there will be a small memory leak here. - // Pausing fault injection so this test runs clean - int64_t count = rcutils_fault_injection_get_count(); - rcutils_fault_injection_set_count(RCUTILS_FAULT_INJECTION_NEVER_FAIL); + // However, it's necessary for coverage rcl_yaml_node_struct_fini(params_hdl); - rcutils_fault_injection_set_count(count); - params_hdl = NULL; }); } diff --git a/rcl_yaml_param_parser/test/test_yaml_variant.cpp b/rcl_yaml_param_parser/test/test_yaml_variant.cpp index 67ea7b6b8..ecfdaeec2 100644 --- a/rcl_yaml_param_parser/test/test_yaml_variant.cpp +++ b/rcl_yaml_param_parser/test/test_yaml_variant.cpp @@ -182,3 +182,35 @@ TEST(TestYamlVariant, copy_string_array_values) { src_variant.string_array_value->data[i], dest_variant.string_array_value->data[i]); } } + +TEST(TestYamlVariant, copy_string_array_maybe_fail) { + rcl_variant_t src_variant{}; + rcutils_allocator_t allocator = rcutils_get_default_allocator(); + constexpr size_t size = 3u; + src_variant.string_array_value = + static_cast( + allocator.allocate(sizeof(rcutils_string_array_t), allocator.state)); + ASSERT_NE(nullptr, src_variant.string_array_value); + OSRF_TESTING_TOOLS_CPP_SCOPE_EXIT( + { + rcl_yaml_variant_fini(&src_variant, allocator); + }); + *src_variant.string_array_value = rcutils_get_zero_initialized_string_array(); + ASSERT_EQ( + RCUTILS_RET_OK, rcutils_string_array_init(src_variant.string_array_value, size, &allocator)); + src_variant.string_array_value->size = size; + src_variant.string_array_value->data[0] = rcutils_strdup("string1", allocator); + src_variant.string_array_value->data[1] = rcutils_strdup("string2", allocator); + src_variant.string_array_value->data[2] = rcutils_strdup("string3", allocator); + for (size_t i = 0; i < size; ++i) { + ASSERT_NE(nullptr, src_variant.string_array_value->data[i]); + } + + RCUTILS_FAULT_INJECTION_TEST( + { + rcl_variant_t dest_variant{}; + rcutils_ret_t ret = rcl_yaml_variant_copy(&dest_variant, &src_variant, allocator); + (void)ret; + rcl_yaml_variant_fini(&dest_variant, allocator); + }); +}