-
Notifications
You must be signed in to change notification settings - Fork 164
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
Added tests for ros2action and ros2msg #287
Conversation
Signed-off-by: ivanpauno <ivanpauno@ekumenlabs.com>
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 left a couple comments, but overall it looks like it's going in the right direction.
Though I recall that @cevans87 was recently working on something over these lines. Not sure exactly, but you may join efforts, or at least not duplicate.
test_ros2cli/package.xml
Outdated
<name>test_ros2cli</name> | ||
<version>0.7.4</version> | ||
<description> | ||
Test of the ROS2 command line tools Framework. |
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.
@ivanpauno consider:
Test of the ROS2 command line tools Framework. | |
Tests for ROS2 command line tools. |
@@ -0,0 +1,107 @@ | |||
# Copyright 2019 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.
@ivanpauno both here and in the next config_*.py
file, I wonder if we can aggregate the information that's currently distributed in multiple collections into a single one so that's easier to read and extend. For instance, a list of dictionaries that keep verb, flags, output, etc. What do you think?
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.
The Config
class could also use the __slots__
attribute and/or a __init__
method for setting the members that the test code expects. These could then be documented to make it easier for future readers adding tests.
I think you're right. This looks similar to what I prototyped last week.
The only major difference I see is that I tested cli functionality by using python subprocess. If this automates the manual release testing and it's easy to test with any rmw implementation, I'm on board. It looks like we can translate the tests I've written to live in this repo and look like these tests. |
Signed-off-by: ivanpauno <ivanpauno@ekumenlabs.com>
Yes, probably using
Sounds good. Also, the cli will probably look different soon: #288. So I will wait a little bit, until that PR gets merged. |
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 @hidmic that it would be easier to understand and extend the tests if the config structure was changed.
It would be nice if we defined something like a 'TestCase' object that contained the command, verb with options, other launch actions, and the expected output. This could then be passed as a single list as the test configuration.
E.g.
class TestCase:
command = 'msg'
verb = 'show std_msgs/msg/String'
launch_actions = []
expected_output = ['string data']
Edit: maybe command
and verb
could be combined?
@@ -0,0 +1,107 @@ | |||
# Copyright 2019 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.
The Config
class could also use the __slots__
attribute and/or a __init__
method for setting the members that the test code expects. These could then be documented to make it easier for future readers adding tests.
I don't expect these to go away any time soon. |
Signed-off-by: ivanpauno <ivanpauno@ekumenlabs.com>
Sounds logical, thanks for clarifying. Addressed your comments in: 3bef9d3. |
Signed-off-by: ivanpauno <ivanpauno@ekumenlabs.com>
Signed-off-by: ivanpauno <ivanpauno@ekumenlabs.com>
Signed-off-by: ivanpauno <ivanpauno@ekumenlabs.com>
Signed-off-by: ivanpauno <ivanpauno@ekumenlabs.com>
Signed-off-by: ivanpauno <ivanpauno@ekumenlabs.com>
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
Signed-off-by: ivanpauno <ivanpauno@ekumenlabs.com>
The last CI have some failures related to connext, and it seems that some verbs of the action command aren't working well with it. I will try to dig a little bit more into the problem. |
Signed-off-by: ivanpauno <ivanpauno@ekumenlabs.com>
@hidmic @jacobperron I think this is ready for merging, but it needs a new review after last commit. |
@@ -40,11 +41,16 @@ def generate_test_description(config, ready_fn): | |||
output='screen', | |||
sigterm_timeout=LaunchConfiguration('sigterm_timeout', default=60) | |||
) | |||
if '@RMW_IMPLEMENTATION@' == 'rmw_connext_cpp': |
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.
Since the test code is otherwise RMW agnostic, I would rather see this logic moved to the tests
macro in CMakeLists.txt. Also, with a comment explaining why this is required.
Signed-off-by: ivanpauno <ivanpauno@ekumenlabs.com>
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.
I'm curious, why is the delay needed for Connext? It's probably worth adding a comment.
Signed-off-by: ivanpauno <ivanpauno@ekumenlabs.com>
Sorry, I forgot about adding the comment. |
Signed-off-by: Audrow Nash <audrow@hey.com>
The idea of this is start increasing testing coverage in all the ros2cli related command line tools, to avoid manual testing in the next release.
Continuation of ros2/system_tests#372.