Bump graph API test coverage. #132

hidmic · 2020-09-18T20:41:16Z

This pull request adds tests for each graph query API when used with bad arguments.

Test all query APIs with bad arguments. Signed-off-by: Michel Hidalgo <[email protected]>

hidmic · 2020-09-18T20:43:26Z

I considered de-duplication, but despite commonalities, APIs have enough small differences to end up with an obfuscated function-parameterized test.

brawner

I agree about reducing duplication. It's probably not warranted for these unit tests

brawner · 2020-09-18T21:49:56Z

test_rmw_implementation/test/test_graph_api.cpp

+
+  rmw_context_t context{rmw_get_zero_initialized_context()};
+  rmw_node_t * node{nullptr};
+  const char * const node_name = "my_test_node";


I'm not sure it makes sense to have these string literals as member variables. Maybe make them constants defined at the top of the file? They can also be constexpr

I do like keeping variables in fixture scope. I can make them static constexpr, but then I need explicit scope resolution. That's why I went with member variables for otherwise constant string literals.

brawner · 2020-09-18T21:54:22Z

test_rmw_implementation/test/test_graph_api.cpp

+
+TEST_F(
+  CLASSNAME(TestGraphAPI, RMW_IMPLEMENTATION),
+  get_node_names_with_enclaves_with_bad_arguments) {


This test is a bit long and can be difficult to follow what each chunk is doing. Could you either add comments to just make it more explicit, or create several different tests?

Sounds reasonable. Went with comments in 4435c51.

brawner · 2020-09-18T21:55:44Z

test_rmw_implementation/test/test_graph_api.cpp

+  rmw_reset_error();
+  EXPECT_EQ(RMW_RET_OK, rmw_names_and_types_check_zero(&topic_names_and_types));
+
+  ASSERT_EQ(RMW_RET_OK, rmw_names_and_types_init(&topic_names_and_types, 1u, &allocator)) <<


Could you add a comment describing why this is a bad argument?

Added in 4435c51.

brawner · 2020-09-18T21:56:29Z

test_rmw_implementation/test/test_graph_api.cpp

+  rmw_reset_error();
+  EXPECT_EQ(RMW_RET_OK, rmw_names_and_types_check_zero(&topic_names_and_types));
+
+  ASSERT_EQ(RMW_RET_OK, rmw_names_and_types_init(&topic_names_and_types, 1u, &allocator)) <<


Same for this one

Added in 4435c51.

brawner · 2020-09-18T21:58:00Z

test_rmw_implementation/test/test_graph_api.cpp

+  rcutils_allocator_t invalid_allocator = rcutils_get_zero_initialized_allocator();
+  EXPECT_EQ(
+    RMW_RET_INVALID_ARGUMENT, rmw_get_subscriber_names_and_types_by_node(
+      node, &invalid_allocator, other_node_name, other_node_namespace, no_demangle, nullptr));


Does the last argument need to be a nullptr for this?

Nope! Fixed in 4435c51.

brawner · 2020-09-18T21:59:13Z

test_rmw_implementation/test/test_graph_api.cpp

+      node, &allocator, other_node_name, other_node_namespace, no_demangle, nullptr));
+  rmw_reset_error();
+
+  ASSERT_EQ(RMW_RET_OK, rmw_names_and_types_init(&topic_names_and_types, 1u, &allocator)) <<


Add comment describing bad argument here and other similar instances

See 4435c51.

brawner · 2020-09-18T21:59:41Z

test_rmw_implementation/test/test_graph_api.cpp

+  rcutils_allocator_t invalid_allocator = rcutils_get_zero_initialized_allocator();
+  EXPECT_EQ(
+    RMW_RET_INVALID_ARGUMENT, rmw_get_publisher_names_and_types_by_node(
+      node, &invalid_allocator, other_node_name, other_node_namespace, no_demangle, nullptr));


Same q about last argument

Same as above, see 4435c51..

brawner · 2020-09-18T22:03:22Z

test_rmw_implementation/test/test_graph_api.cpp

@@ -0,0 +1,808 @@
+// Copyright 2020 Open Source Robotics Foundation, Inc.


Are you planning on adding versions of these tests that check the good cases?

Ideally yes, but likely not with this PR. Rationale here is to cover basic error handling cases as described in ros2/rmw#272 only, which are the ones definitely not covered by tests in upper layers.

Perfectly reasonable, just wanted to check.

hidmic · 2020-09-18T23:14:34Z

Depends on ros2/rmw_fastrtps#436, ros2/rmw_connext#459, and ros2/rmw_cyclonedds#243.

Signed-off-by: Michel Hidalgo <[email protected]>

hidmic · 2020-09-21T18:38:29Z

CI up to test_rmw_implementation:

Linux
Linux-aarch64
macOS
Windows

brawner · 2020-09-21T18:40:33Z

test_rmw_implementation/test/test_graph_api.cpp

@@ -0,0 +1,808 @@
+// Copyright 2020 Open Source Robotics Foundation, Inc.


Perfectly reasonable, just wanted to check.

Lobotuerk

LGTM after brawner comments

hidmic · 2020-09-22T13:53:52Z

CI up to test_rmw_implementation, rcl, and rclcpp:

Linux
Linux-aarch64
macOS
Windows

hidmic · 2020-09-24T13:15:10Z

@ros-pull-request-builder retest this please

hidmic · 2020-09-24T13:33:40Z

All CI's green! Merging.

Test all ROS graph query APIs with bad arguments. Signed-off-by: Michel Hidalgo <[email protected]>

Bump graph API test coverage.

5c260dd

Test all query APIs with bad arguments. Signed-off-by: Michel Hidalgo <[email protected]>

hidmic requested review from brawner and Lobotuerk September 18, 2020 20:41

brawner reviewed Sep 18, 2020

View reviewed changes

hidmic added 2 commits September 18, 2020 20:17

Bump test timeout.

d194763

Signed-off-by: Michel Hidalgo <[email protected]>

Address peer review comments.

4435c51

Signed-off-by: Michel Hidalgo <[email protected]>

hidmic requested a review from brawner September 21, 2020 18:27

Minor fixes.

e97c9ed

Signed-off-by: Michel Hidalgo <[email protected]>

brawner approved these changes Sep 21, 2020

View reviewed changes

Lobotuerk approved these changes Sep 22, 2020

View reviewed changes

hidmic merged commit 64f1fd9 into master Sep 24, 2020

delete-merged-branch bot deleted the hidmic/rmw-graph-api-tests branch September 24, 2020 13:34

ahcorde pushed a commit that referenced this pull request Oct 9, 2020

Bump graph API test coverage. (#132)

a61a8ed

Test all ROS graph query APIs with bad arguments. Signed-off-by: Michel Hidalgo <[email protected]>

ahcorde pushed a commit that referenced this pull request Oct 21, 2020

Bump graph API test coverage. (#132)

b69cc44

Test all ROS graph query APIs with bad arguments. Signed-off-by: Michel Hidalgo <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bump graph API test coverage. #132

Bump graph API test coverage. #132

hidmic commented Sep 18, 2020

hidmic commented Sep 18, 2020

brawner left a comment

brawner Sep 18, 2020

hidmic Sep 21, 2020

brawner Sep 18, 2020

hidmic Sep 21, 2020

brawner Sep 18, 2020

hidmic Sep 21, 2020

brawner Sep 18, 2020

hidmic Sep 21, 2020

brawner Sep 18, 2020

hidmic Sep 21, 2020

brawner Sep 18, 2020

hidmic Sep 21, 2020

brawner Sep 18, 2020

hidmic Sep 21, 2020

brawner Sep 18, 2020

hidmic Sep 21, 2020

brawner Sep 21, 2020

hidmic commented Sep 18, 2020

hidmic commented Sep 21, 2020

brawner Sep 21, 2020

Lobotuerk left a comment

hidmic commented Sep 22, 2020 •

edited

Loading

hidmic commented Sep 24, 2020

hidmic commented Sep 24, 2020

		@@ -0,0 +1,808 @@
		// Copyright 2020 Open Source Robotics Foundation, Inc.

Bump graph API test coverage. #132

Bump graph API test coverage. #132

Conversation

hidmic commented Sep 18, 2020

hidmic commented Sep 18, 2020

brawner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hidmic commented Sep 18, 2020

hidmic commented Sep 21, 2020

Choose a reason for hiding this comment

Lobotuerk left a comment

Choose a reason for hiding this comment

hidmic commented Sep 22, 2020 • edited Loading

hidmic commented Sep 24, 2020

hidmic commented Sep 24, 2020

hidmic commented Sep 22, 2020 •

edited

Loading