Add basic unit tests for refactored functions in rcl_yaml_param_parser (coverage part 1/3) by brawner · Pull Request #771 · ros2/rcl

brawner · 2020-08-27T21:49:11Z

This is part 1 of added unit tests for increasing coverage of rcl_yaml_param_parser to 95%. This adds additional unit tests for the refactored source code. Depends on #754 and #765

Signed-off-by: Stephen Brawner brawner@gmail.com

brawner · 2020-08-27T22:25:11Z

See #766 and #772 for parts 2 and 3 respectively.

Blast545

Some questions

rcl_yaml_param_parser/test/test_namespace.cpp

rcl_yaml_param_parser/test/test_node_params.cpp

rcl_yaml_param_parser/test/test_parse.cpp

rcl_yaml_param_parser/test/test_yaml_variant.cpp

brawner · 2020-08-28T22:56:41Z

Updated these files after running all the tests through valgrind. There were some memory errors with unitialized memory and a couple of leaks.

brawner · 2020-08-28T23:34:41Z

Also depends on #775

Blast545

LGTM, pending green CI

hidmic · 2020-08-31T16:12:24Z

rcl_yaml_param_parser/test/test_parse.cpp

+    RCUTILS_RET_OK,
+    parse_value(event, is_seq, node_idx, parameter_idx, &seq_data_type, params_st)) <<
+    rcutils_get_error_string().str;
+  ASSERT_NE(nullptr, params_st->params[0].parameter_values[0].bool_value);


@brawner nit^N: should it be

Suggested change

ASSERT_NE(nullptr, params_st->params[0].parameter_values[0].bool_value);

ASSERT_NE(nullptr, params_st->params[node_idx].parameter_values[parameter_idx].bool_value);

? Same elsewhere.

hidmic · 2020-08-31T16:17:14Z

rcl_yaml_param_parser/test/test_parse.cpp

+  ASSERT_EQ(RCUTILS_RET_OK, node_params_init(&params_st->params[0], allocator));
+  params_st->num_nodes = 1u;
+
+  // event.data.scaler.value is NULL, but event.data.scalar.length > 0


@brawner nit^N:

Suggested change

// event.data.scaler.value is NULL, but event.data.scalar.length > 0

// event.data.scalar.value is NULL, but event.data.scalar.length > 0

hidmic · 2020-08-31T16:18:07Z

rcl_yaml_param_parser/test/test_parse.cpp

+    parse_key(event, nullptr, &is_new_map, &node_idx, &parameter_idx, &ns_tracker, params_st)) <<
+    rcutils_get_error_string().str;
+  EXPECT_TRUE(rcutils_error_is_set());
+  rcutils_reset_error();


@brawner duplicate?

hidmic · 2020-08-31T16:21:01Z

rcl_yaml_param_parser/test/test_yaml_variant.cpp

+    ASSERT_NE(nullptr, dest_variant.field); \
+    EXPECT_EQ(*src_variant.field, *dest_variant.field); \
+    rcl_yaml_variant_fini(&dest_variant, allocator); \
+    variant.field = nullptr; \


@brawner should it be

Suggested change

variant.field = nullptr; \

src_variant.field = nullptr; \

?

Cleaned this macro up a bit. Initialization of src/dest_variants now happen inside this macro. Should help prevent stuff like this.

hidmic · 2020-08-31T16:21:39Z

rcl_yaml_param_parser/test/test_yaml_variant.cpp

+    ASSERT_NE(nullptr, src_variant.field->values); \
+    src_variant.field->size = array_size; \
+    for (size_t i = 0; i < array_size; ++i) { \
+      variant.field->values[i] = tmp_array[i]; \


@brawner should it be

Suggested change

variant.field->values[i] = tmp_array[i]; \

src_variant.field->values[i] = tmp_array[i]; \

?

hidmic · 2020-08-31T16:22:48Z

rcl_yaml_param_parser/test/test_yaml_variant.cpp

+    rcl_yaml_variant_fini(&src_variant, allocator); \
+    rcl_yaml_variant_fini(&dest_variant, allocator); \
+    src_variant.field = nullptr; \
+    dest_variant.field = nullptr; \


@brawner should this be in scoped exits in case asserts above fail?

I adjusted how everything was cleaned up and added a bunch more scope_exits (not just for these).

hidmic · 2020-08-31T16:25:20Z

rcl_yaml_param_parser/test/test_yaml_variant.cpp

+  } while (0)
+
+#define TEST_VARIANT_ARRAY_COPY( \
+    dest_variant, src_variant, field, array_type, value_type, tmp_array, array_size, allocator) \


@brawner nit: I think you can derive array_size from tmp_array considering tmp_array type is always a primitive array and its type has not decayed yet.

hidmic · 2020-08-31T16:28:11Z

rcl_yaml_param_parser/test/test_yaml_variant.cpp

+  } while (0)
+
+TEST(TestYamlVariant, copy_fini) {
+  rcl_variant_t variant =


@brawner nit^N: since this is C++,

Suggested change

rcl_variant_t variant =

rcl_variant_t variant{};

would also work.

Helps clean things up a bit, done.

hidmic · 2020-08-31T16:32:29Z

rcl_yaml_param_parser/test/test_yaml_variant.cpp

+    EXPECT_STREQ(variant.string_array_value->data[i], copy.string_array_value->data[i]);
+  }
+  rcl_yaml_variant_fini(&variant, allocator);
+  rcl_yaml_variant_fini(&copy, allocator);


@brawner should these be in scoped exits to avoid leaks if any of the asserts above fail?

Signed-off-by: Stephen Brawner <brawner@gmail.com>

brawner · 2020-09-01T23:18:31Z

Rebased on to master after merging #754. Testing --packages-select rcl_yaml_param_parser

Linux
Linux-aarch64
macOS
Windows

hidmic

LGTM, ready to go!

hidmic · 2020-09-02T13:55:03Z

rcl_yaml_param_parser/test/test_yaml_variant.cpp

+    src_variant.field = nullptr; \
+  } while (0)
+
+#define TEST_VARIANT_ARRAY_COPY(field, array_type, value_type, tmp_array) \


@brawner nit^N, safe to ignore: you could use decltype expressions to not have to pass types explicitly.

brawner · 2020-09-02T20:30:04Z

Linux
Linux-aarch64
macOS
Windows

Signed-off-by: Stephen Brawner <brawner@gmail.com>

brawner · 2020-09-02T21:03:18Z

Looks like some more type wrangling was necessary to avoid memory errors.

Linux
Linux-aarch64
macOS
Windows

…r (coverage part 1/3) (#771) * Add basic unit tests for refactored functions Signed-off-by: Stephen Brawner <brawner@gmail.com> * PR Fixup Signed-off-by: Stephen Brawner <brawner@gmail.com> * Fix memory errors Signed-off-by: Stephen Brawner <brawner@gmail.com> * headers moved Signed-off-by: Stephen Brawner <brawner@gmail.com> * Addressing feedback Signed-off-by: Stephen Brawner <brawner@gmail.com> * Switch to decltype Signed-off-by: Stephen Brawner <brawner@gmail.com>

brawner force-pushed the brawner/rcl_yaml-basic-unit-tests branch from cf14a57 to 56ba274 Compare August 27, 2020 21:58

brawner mentioned this pull request Aug 27, 2020

Add fault-injection unit tests (coverage part 2/3) #766

Merged

brawner changed the title ~~Add basic unit tests for refactored functions~~ Add basic unit tests for refactored functions (coverage part 1/3) Aug 27, 2020

brawner changed the title ~~Add basic unit tests for refactored functions (coverage part 1/3)~~ Add basic unit tests for refactored functions in rcl_yaml_param_parser (coverage part 1/3) Aug 27, 2020

brawner requested review from Blast545 and hidmic August 27, 2020 22:33

brawner self-assigned this Aug 27, 2020

Blast545 reviewed Aug 28, 2020

View reviewed changes

brawner force-pushed the brawner/rcl_yaml-basic-unit-tests branch from 5940d72 to 8b9dd40 Compare August 28, 2020 23:33

brawner changed the base branch from brawner/rcl_yaml-set-values-null to brawner/rcl_yaml-add_val_to_string-leak August 28, 2020 23:34

brawner force-pushed the brawner/rcl_yaml-add_val_to_string-leak branch from ac7cf6f to a9bab89 Compare August 29, 2020 00:46

brawner force-pushed the brawner/rcl_yaml-basic-unit-tests branch from 8b9dd40 to 422dd65 Compare August 29, 2020 00:50

Blast545 approved these changes Aug 31, 2020

View reviewed changes

hidmic reviewed Aug 31, 2020

View reviewed changes

brawner force-pushed the brawner/rcl_yaml-add_val_to_string-leak branch from a9bab89 to 7cd635f Compare August 31, 2020 17:50

brawner force-pushed the brawner/rcl_yaml-basic-unit-tests branch from 422dd65 to 337f9ee Compare August 31, 2020 22:00

brawner changed the base branch from brawner/rcl_yaml-add_val_to_string-leak to brawner/rcl_yaml-refactor-srcs August 31, 2020 22:01

brawner force-pushed the brawner/rcl_yaml-refactor-srcs branch 3 times, most recently from b062da9 to 8d72dc4 Compare September 1, 2020 22:11

brawner added 5 commits September 1, 2020 16:11

Add basic unit tests for refactored functions

46bde7d

Signed-off-by: Stephen Brawner <brawner@gmail.com>

PR Fixup

a59b343

Signed-off-by: Stephen Brawner <brawner@gmail.com>

Fix memory errors

f8e3a34

Signed-off-by: Stephen Brawner <brawner@gmail.com>

headers moved

3e3370c

Signed-off-by: Stephen Brawner <brawner@gmail.com>

Addressing feedback

37cac47

Signed-off-by: Stephen Brawner <brawner@gmail.com>

brawner mentioned this pull request Sep 1, 2020

Fix yaml parser error when meets .nan #741

Closed

brawner force-pushed the brawner/rcl_yaml-basic-unit-tests branch from 68fb9cc to 37cac47 Compare September 1, 2020 23:16

brawner changed the base branch from brawner/rcl_yaml-refactor-srcs to master September 1, 2020 23:16

hidmic approved these changes Sep 2, 2020

View reviewed changes

Switch to decltype

8cdc65a

Signed-off-by: Stephen Brawner <brawner@gmail.com>

brawner force-pushed the brawner/rcl_yaml-basic-unit-tests branch from 67df619 to 8cdc65a Compare September 2, 2020 21:02

brawner merged commit 191d8e5 into master Sep 2, 2020

delete-merged-branch bot deleted the brawner/rcl_yaml-basic-unit-tests branch September 2, 2020 21:25

	ASSERT_NE(nullptr, params_st->params[0].parameter_values[0].bool_value);
	ASSERT_NE(nullptr, params_st->params[node_idx].parameter_values[parameter_idx].bool_value);

	// event.data.scaler.value is NULL, but event.data.scalar.length > 0
	// event.data.scalar.value is NULL, but event.data.scalar.length > 0

	variant.field->values[i] = tmp_array[i]; \
	src_variant.field->values[i] = tmp_array[i]; \

Conversation

brawner commented Aug 27, 2020

Uh oh!

brawner commented Aug 27, 2020

Uh oh!

Blast545 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

brawner commented Aug 28, 2020

Uh oh!

brawner commented Aug 28, 2020

Uh oh!

Blast545 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brawner commented Sep 1, 2020

Uh oh!

hidmic left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brawner commented Sep 2, 2020

Uh oh!

brawner commented Sep 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

brawner commented Sep 2, 2020 •

edited

Loading