-
Notifications
You must be signed in to change notification settings - Fork 161
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
Remove ros2interface test dependencies on builtin interface #579
Conversation
Signed-off-by: Audrow <audrow.nash@gmail.com>
Signed-off-by: Audrow Nash <audrow.nash@gmail.com>
Signed-off-by: Audrow Nash <audrow.nash@gmail.com>
Signed-off-by: Audrow Nash <audrow.nash@gmail.com>
Signed-off-by: Audrow Nash <audrow.nash@gmail.com>
Signed-off-by: Audrow Nash <audrow.nash@gmail.com>
703e512
to
3056860
Compare
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 (one minor comment)
We should also run CI and test the new package ros2cli_test_interfaces
.
I believe the PR job failure is the same as #577. It seems safe to ignore, since this change is only touching |
Signed-off-by: Audrow Nash <audrow.nash@gmail.com>
I don't think rebasing is necessary when using a branch (instead of a repos file) for CI. |
Fixes #541 and is an alternative to the PRs associated with #547.
I think this is an important change because, as mentioned in ros2/system_tests#442 (comment), testing against these interface definitions allows us to cover behavior of
ros2 interface show
that is currently not covered, such as the bug fixed by #548. This PR also removes aros2interface
test dependency onbuiltin_interfaces
(#541).I've opted for this approach rather than the one used in #547 to avoid slowing down the test time and for simplicity. As @jacobperron mentions in ros2/test_interface_files#11 (comment) that adding these interfaces to may increase the time required for all of the tests to run. It also is much more complicated to add messages to
test_msgs
as it requires PRs in four repositories.