-
Notifications
You must be signed in to change notification settings - Fork 47
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.
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
Bump graph API test coverage. #132
Conversation
Test all query APIs with bad arguments. Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
I considered de-duplication, but despite commonalities, APIs have enough small differences to end up with an obfuscated function-parameterized test. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree about reducing duplication. It's probably not warranted for these unit tests
|
||
rmw_context_t context{rmw_get_zero_initialized_context()}; | ||
rmw_node_t * node{nullptr}; | ||
const char * const node_name = "my_test_node"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
|
||
TEST_F( | ||
CLASSNAME(TestGraphAPI, RMW_IMPLEMENTATION), | ||
get_node_names_with_enclaves_with_bad_arguments) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds reasonable. Went with comments in 4435c51.
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)) << |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add a comment describing why this is a bad argument?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added in 4435c51.
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)) << |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same for this one
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added in 4435c51.
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)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the last argument need to be a nullptr for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope! Fixed in 4435c51.
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)) << |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add comment describing bad argument here and other similar instances
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See 4435c51.
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)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same q about last argument
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above, see 4435c51..
@@ -0,0 +1,808 @@ | |||
// Copyright 2020 Open Source Robotics Foundation, Inc. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you planning on adding versions of these tests that check the good cases?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perfectly reasonable, just wanted to check.
Depends on ros2/rmw_fastrtps#436, ros2/rmw_connext#459, and ros2/rmw_cyclonedds#243. |
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
@@ -0,0 +1,808 @@ | |||
// Copyright 2020 Open Source Robotics Foundation, Inc. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perfectly reasonable, just wanted to check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM after brawner comments
@ros-pull-request-builder retest this please |
All CI's green! Merging. |
Test all ROS graph query APIs with bad arguments. Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Test all ROS graph query APIs with bad arguments. Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
This pull request adds tests for each graph query API when used with bad arguments.