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

Adding tests to arguments.c #752

Merged
merged 10 commits into from
Aug 20, 2020
Merged

Adding tests to arguments.c #752

Changes from 6 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
e1b3b79
Add nullptr tests get_param_files
Blast545 Aug 13, 2020
95c07a6
Add bad alloc tests
Blast545 Aug 14, 2020
27ce30c
Add missing tests
Blast545 Aug 14, 2020
d672046
Add bad alloc tests rcl_arguments_copy
Blast545 Aug 14, 2020
80850b6
Remove repeated test
Blast545 Aug 18, 2020
7ba3c2f
Remove spaces
Blast545 Aug 18, 2020
af3adb5
Restore erased test
Blast545 Aug 19, 2020
2d43cd8
Relocate test
Blast545 Aug 19, 2020
2ac0015
Refactor bomb allocator test
Blast545 Aug 19, 2020
cf35778
Add missing rcl_reset_error() checks
Blast545 Aug 20, 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
129 changes: 128 additions & 1 deletion rcl/test/rcl/test_arguments.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -309,6 +309,7 @@ TEST_F(CLASSNAME(TestArgumentsFixture, RMW_IMPLEMENTATION), test_bad_alloc_unpar
const int argc = sizeof(argv) / sizeof(const char *);
rcl_arguments_t parsed_args = rcl_get_zero_initialized_arguments();
rcl_allocator_t bad_alloc = get_failing_allocator();
rcl_allocator_t allocator = rcl_get_default_allocator();
rcl_ret_t ret = rcl_parse_arguments(argc, argv, rcl_get_default_allocator(), &parsed_args);
ASSERT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str;
EXPECT_EQ(2, rcl_arguments_get_count_unparsed(&parsed_args));
Expand All @@ -317,8 +318,16 @@ TEST_F(CLASSNAME(TestArgumentsFixture, RMW_IMPLEMENTATION), test_bad_alloc_unpar
EXPECT_EQ(
RCL_RET_BAD_ALLOC, rcl_arguments_get_unparsed(&parsed_args, bad_alloc, &actual_unparsed));
rcl_reset_error();

EXPECT_EQ(
RCL_RET_BAD_ALLOC, rcl_arguments_get_unparsed_ros(&parsed_args, bad_alloc, &actual_unparsed));
Blast545 marked this conversation as resolved.
Show resolved Hide resolved
RCL_RET_INVALID_ARGUMENT,
rcl_arguments_get_unparsed_ros(nullptr, allocator, &actual_unparsed));
rcl_reset_error();

rcl_arguments_t empty_parsed_args = rcl_get_zero_initialized_arguments();
EXPECT_EQ(
RCL_RET_INVALID_ARGUMENT,
rcl_arguments_get_unparsed(&empty_parsed_args, allocator, &actual_unparsed));
Blast545 marked this conversation as resolved.
Show resolved Hide resolved
rcl_reset_error();

EXPECT_EQ(RCL_RET_OK, rcl_arguments_fini(&parsed_args));
Expand Down Expand Up @@ -698,6 +707,35 @@ TEST_F(CLASSNAME(TestArgumentsFixture, RMW_IMPLEMENTATION), test_bad_remove_ros_
EXPECT_EQ(0, nonros_argc);
}

TEST_F(CLASSNAME(TestArgumentsFixture, RMW_IMPLEMENTATION), test_bad_alloc_remove_ros_args) {
const char * const argv[] = {
"process_name", "-d", "--ros-args", "-r", "__ns:=/foo/bar", "-r", "__ns:=/fiz/buz", "--",
"--foo=bar", "--baz", "--ros-args", "--ros-args", "-p", "bar:=baz", "--", "--", "arg",
};
const int argc = sizeof(argv) / sizeof(const char *);

rcl_allocator_t alloc = rcl_get_default_allocator();
rcl_arguments_t parsed_args = rcl_get_zero_initialized_arguments();
rcl_ret_t ret = rcl_parse_arguments(argc, argv, alloc, &parsed_args);
ASSERT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str;
OSRF_TESTING_TOOLS_CPP_SCOPE_EXIT(
{
EXPECT_EQ(RCL_RET_OK, rcl_arguments_fini(&parsed_args));
});

int nonros_argc = 0;
const char ** nonros_argv = NULL;
rcl_allocator_t bomb_alloc = get_time_bombed_allocator();
Copy link
Contributor

Choose a reason for hiding this comment

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

Where is get_time_bombed_allocator coming from? It's not spelled correctly, but secondly I don't see an appropriate include.

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 added that allocator with #743. As it also tested arguments.c it's already included in `test_arguments.cpp'.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, I didn't look at allocator_testing_utils.h close enough. Thanks for the info! But it should be get_time_bomb_allocator, "time_bombed" doesn't make a ton of sense. Feel free to fix in a follow-up PR.

set_time_bombed_allocator_count(bomb_alloc, 1);
ret = rcl_remove_ros_arguments(
argv,
&parsed_args,
bomb_alloc,
&nonros_argc,
&nonros_argv);
EXPECT_EQ(RCL_RET_BAD_ALLOC, ret);
Copy link
Contributor

Choose a reason for hiding this comment

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

@Blast545 nit: missing error state check/reset?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All the allocator tests added return that error without actually setting the error state. However, other bad allocators through the arguments file do set them. I was wondering that this should be standardized based on the open conversations of error check/handling.

Should I add a rcl_reset_error() to be "safe" even when the error is not expected in the meantime?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, it should be standardized. Let's defer it to follow-up PRs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Bad alloc check, pending standarization

}

TEST_F(CLASSNAME(TestArgumentsFixture, RMW_IMPLEMENTATION), test_remove_ros_args) {
const char * const argv[] = {
"process_name", "-d", "--ros-args", "-r", "__ns:=/foo/bar", "-r", "__ns:=/fiz/buz", "--",
Expand Down Expand Up @@ -854,6 +892,17 @@ TEST_F(CLASSNAME(TestArgumentsFixture, RMW_IMPLEMENTATION), test_param_argument_
}
alloc.deallocate(parameter_files, alloc.state);

// Test bad alloc
rcl_allocator_t bad_alloc = get_failing_allocator();
rcl_params_t * params_test = NULL;
rcl_allocator_t saved_alloc = parsed_args.impl->allocator;
parsed_args.impl->parameter_overrides->allocator = bad_alloc;
ret = rcl_arguments_get_param_overrides(&parsed_args, &params_test);
EXPECT_EQ(RCL_RET_BAD_ALLOC, ret) << rcl_get_error_string().str;
Copy link
Contributor

Choose a reason for hiding this comment

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

@Blast545 nit: missing error state check/reset?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Bad alloc check, pending standarization

EXPECT_EQ(NULL, params_test);
parsed_args.impl->parameter_overrides->allocator = saved_alloc;

// Expected usage
rcl_params_t * params = NULL;
ret = rcl_arguments_get_param_overrides(&parsed_args, &params);
EXPECT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str;
Expand Down Expand Up @@ -943,6 +992,35 @@ TEST_F(CLASSNAME(TestArgumentsFixture, RMW_IMPLEMENTATION), test_param_argument_
EXPECT_FALSE(param_value->bool_array_value->values[2]);
}

TEST_F(CLASSNAME(TestArgumentsFixture, RMW_IMPLEMENTATION), test_param_arguments_copy) {
const std::string parameters_filepath1 = (test_path / "test_parameters.1.yaml").string();
const std::string parameters_filepath2 = (test_path / "test_parameters.2.yaml").string();
const char * const argv[] = {
"process_name", "--ros-args", "--params-file", parameters_filepath1.c_str(),
"-r", "__ns:=/namespace", "random:=arg", "--params-file", parameters_filepath2.c_str()
};
const int argc = sizeof(argv) / sizeof(const char *);
rcl_ret_t ret;

rcl_allocator_t alloc = rcl_get_default_allocator();
rcl_arguments_t parsed_args = rcl_get_zero_initialized_arguments();

ret = rcl_parse_arguments(argc, argv, alloc, &parsed_args);
ASSERT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str;
OSRF_TESTING_TOOLS_CPP_SCOPE_EXIT(
{
EXPECT_EQ(RCL_RET_OK, rcl_arguments_fini(&parsed_args));
});

int parameter_filecount = rcl_arguments_get_param_files_count(&parsed_args);
EXPECT_EQ(2, parameter_filecount);

rcl_arguments_t copied_args = rcl_get_zero_initialized_arguments();
ret = rcl_arguments_copy(&parsed_args, &copied_args);
EXPECT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str;
EXPECT_EQ(2, rcl_arguments_get_param_files_count(&copied_args));
}

TEST_F(CLASSNAME(TestArgumentsFixture, RMW_IMPLEMENTATION), test_no_param_overrides) {
const char * const argv[] = {"process_name"};
const int argc = sizeof(argv) / sizeof(const char *);
Expand All @@ -966,6 +1044,10 @@ TEST_F(CLASSNAME(TestArgumentsFixture, RMW_IMPLEMENTATION), test_no_param_overri
EXPECT_EQ(RCL_RET_INVALID_ARGUMENT, ret) << rcl_get_error_string().str;
rcl_reset_error();

rcl_arguments_t empty_parsed_arg = rcl_get_zero_initialized_arguments();
ret = rcl_arguments_get_param_overrides(&empty_parsed_arg, &params);
EXPECT_EQ(RCL_RET_INVALID_ARGUMENT, ret) << rcl_get_error_string().str;

Copy link
Contributor

Choose a reason for hiding this comment

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

@Blast545 nit: missing error state/reset?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added only a reset to match the rest of the old tests in the group, see cf35778

rcl_params_t preallocated_params;
params = &preallocated_params;
ret = rcl_arguments_get_param_overrides(&parsed_args, &params);
Expand Down Expand Up @@ -1099,3 +1181,48 @@ TEST_F(CLASSNAME(TestArgumentsFixture, RMW_IMPLEMENTATION), test_bad_allocs_copy
}
parsed_args.impl->allocator = saved_alloc;
}

TEST_F(CLASSNAME(TestArgumentsFixture, RMW_IMPLEMENTATION), test_null_get_param_files) {
const std::string parameters_filepath1 = (test_path / "test_parameters.1.yaml").string();
const char * const argv[] = {
"process_name", "--ros-args", "--params-file", parameters_filepath1.c_str()
};
const int argc = sizeof(argv) / sizeof(const char *);
Blast545 marked this conversation as resolved.
Show resolved Hide resolved

rcl_arguments_t parsed_args = rcl_get_zero_initialized_arguments();
char ** parameter_files = NULL;
rcl_allocator_t allocator = rcl_get_default_allocator();
rcl_ret_t ret = rcl_parse_arguments(argc, argv, allocator, &parsed_args);
ASSERT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str;
OSRF_TESTING_TOOLS_CPP_SCOPE_EXIT(
{
EXPECT_EQ(RCL_RET_OK, rcl_arguments_fini(&parsed_args));
});

ret = rcl_arguments_get_param_files(nullptr, allocator, &parameter_files);
EXPECT_EQ(RCL_RET_INVALID_ARGUMENT, ret) << rcl_get_error_string().str;
Copy link
Contributor

Choose a reason for hiding this comment

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

@Blast545 nit: here and below, missing error state check/reset?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a check + reset cf35778


ret = rcl_arguments_get_param_files(&parsed_args, allocator, nullptr);
EXPECT_EQ(RCL_RET_INVALID_ARGUMENT, ret) << rcl_get_error_string().str;

rcl_arguments_t empty_parsed_args = rcl_get_zero_initialized_arguments();
ret = rcl_arguments_get_param_files(&empty_parsed_args, allocator, &parameter_files);
EXPECT_EQ(RCL_RET_INVALID_ARGUMENT, ret) << rcl_get_error_string().str;
}

TEST_F(CLASSNAME(TestArgumentsFixture, RMW_IMPLEMENTATION), test_bad_alloc_parse_arg) {
const std::string parameters_filepath1 = (test_path / "test_parameters.1.yaml").string();
const char * const argv[] = {
"process_name", "--ros-args", "--params-file", parameters_filepath1.c_str()
};
const int argc = sizeof(argv) / sizeof(const char *);

rcl_arguments_t parsed_args = rcl_get_zero_initialized_arguments();
rcl_allocator_t bomb_alloc = get_time_bombed_allocator();

for (int i = 0; i < 9; i++) {
set_time_bombed_allocator_count(bomb_alloc, i);
rcl_ret_t ret = rcl_parse_arguments(argc, argv, bomb_alloc, &parsed_args);
EXPECT_EQ(RCL_RET_BAD_ALLOC, ret) << rcl_get_error_string().str;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

@Blast545 meta: hmm, I wonder if this isn't better suited for fault injection. That magic 9 is a bit fragile.

Copy link
Contributor

Choose a reason for hiding this comment

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

I am working on a followup PR to rcutils that will include a fault_injection_allocator that accomplishes similar functionality to the time_bomb_allocator. If you think that would be useful to this PR, I can prioritize that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@hidmic I agree it's somewhat fragile. In fact I was considering some sort of macro for testing functions that received allocators to iterate until the test passed. Although probably the fault injection approach will be better in the long run, and that's why I left the fixed number for this PR.

@brawner I'm not fully familiarized yet with the fault_injection tools you are working at the moment. Although I imagined the injection tools to be included in the default_allocator used through the ROS 2 code to be able of testing bad allocator fails like this one.

We can always merge this PR and updated it with injection tools once these are ready, WDYT @brawner @hidmic ?

Copy link
Contributor

@brawner brawner Aug 19, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Although I imagined the injection tools to be included in the default_allocator

That is probably a better approach. While most C code requires passing in an allocator, I think some c++ code just uses the default allocator blindly.

To make the test a little less fragile, you can do something like:

  for (int i = 0; i < 100; i++) {
    set_time_bombed_allocator_count(bomb_alloc, i);
    rcl_ret_t ret = rcl_parse_arguments(argc, argv, bomb_alloc, &parsed_args);
    if (RCL_RET_OK == ret) {
      EXPECT_EQ(RCL_RET_OK, rcl_arguments_fini(&parsed_args));
    } else {
      EXPECT_EQ(RCL_RET_BAD_ALLOC, ret);
      EXPECT_TRUE(rcl_error_is_set());  // I don't know if this is true
      rcl_error_reset();
    }
  }

Copy link
Contributor

Choose a reason for hiding this comment

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

Err, edited to change the 9 in for loop to something large, like 100.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

With the missing break, right?

  for (int i = 0; i < 100; i++) {
    set_time_bombed_allocator_count(bomb_alloc, i);
    rcl_ret_t ret = rcl_parse_arguments(argc, argv, bomb_alloc, &parsed_args);
    if (RCL_RET_OK == ret) {
      EXPECT_EQ(RCL_RET_OK, rcl_arguments_fini(&parsed_args));
      break;
    } else {
      EXPECT_EQ(RCL_RET_BAD_ALLOC, ret);
      EXPECT_TRUE(rcl_error_is_set());
      rcl_error_reset();
    }
  }

Copy link
Contributor

Choose a reason for hiding this comment

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

Oooh, good idea. I like it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done 2ac0015

}