Allow empty urdf tag #1017
Merged
Allow empty urdf tag #1017
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
requested review from
anfemosa,
bmagyar,
destogl,
homalozoa,
mcbed,
progtologist and
sachinkum0009
bmagyar
reviewed
May 11, 2023
| { | ||
| const std::string broken_urdf_string = | ||
| std::string(ros2_control_test_assets::urdf_head) + | ||
| ros2_control_test_assets::invalid_urdf_ros2_control_parameter_empty + | ||
| ros2_control_test_assets::urdf_tail; | ||
|
|
||
| ASSERT_THROW(parse_control_resources_from_urdf(broken_urdf_string), std::runtime_error); | ||
| ASSERT_NO_THROW(parse_control_resources_from_urdf(broken_urdf_string)); |
There was a problem hiding this comment.
While this may be technically correct. I don't think we should do it. We need a new test explicitly to make sure that empty tags as here are permitted but not a fully broken urdf as per the tests suggests
There was a problem hiding this comment.
Thanks for the feedback, absolutely valid point. I replaced the test with a more consistent one in 4c02c65.
This replaces the test that checks that empty parameter tags work and actually checks the parsed urdf.
Codecov Report
❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more. @@ Coverage Diff @@
## master #1017 +/- ##
==========================================
- Coverage 34.61% 32.65% -1.97%
==========================================
Files 52 91 +39
Lines 2981 9566 +6585
Branches 1855 6445 +4590
==========================================
+ Hits 1032 3124 +2092
- Misses 310 704 +394
- Partials 1639 5738 +4099
Flags with carried forward coverage won't be shown. Click here to find out more.
|
bmagyar
approved these changes
May 19, 2023
|
@Mergifyio backport humble |
✅ Backports have been created
|
mergify bot
pushed a commit
that referenced
this pull request
May 19, 2023
* Do not explode with empty tag * Update tests to allow empty URDF parameter to hardware interface * Test checks that empty parameter tags work and actually checks the parsed urdf. --------- Co-authored-by: Bence Magyar <[email protected]> Co-authored-by: Dr. Denis <[email protected]> (cherry picked from commit 7c07430) # Conflicts: # hardware_interface/src/component_parser.cpp
fmauch
added a commit
to fmauch/ros2_control
that referenced
this pull request
May 28, 2023
* Do not explode with empty tag * Update tests to allow empty URDF parameter to hardware interface * Test checks that empty parameter tags work and actually checks the parsed urdf. --------- Co-authored-by: Bence Magyar <[email protected]> Co-authored-by: Dr. Denis <[email protected]> (cherry picked from commit 7c07430) # Conflicts: # hardware_interface/src/component_parser.cpp
fmauch
added a commit
to fmauch/ros2_control
that referenced
this pull request
May 28, 2023
* Do not explode with empty tag * Update tests to allow empty URDF parameter to hardware interface * Test checks that empty parameter tags work and actually checks the parsed urdf. --------- Co-authored-by: Bence Magyar <[email protected]> Co-authored-by: Dr. Denis <[email protected]> (cherry picked from commit 7c07430) # Conflicts: # hardware_interface/src/component_parser.cpp
destogl
pushed a commit
that referenced
this pull request
May 30, 2023
* [URDF Parser] Allow empty urdf tag, e.g., parameter (#1017) * Do not explode with empty tag * Update tests to allow empty URDF parameter to hardware interface * Test checks that empty parameter tags work and actually checks the parsed urdf. * Patch test updates for humble API --------- Co-authored-by: Bence Magyar <[email protected]> Co-authored-by: Dr. Denis <[email protected]> (cherry picked from commit 7c07430)
flochre
pushed a commit
to flochre/ros2_control
that referenced
this pull request
Jul 5, 2023
) * [URDF Parser] Allow empty urdf tag, e.g., parameter (ros-controls#1017) * Do not explode with empty tag * Update tests to allow empty URDF parameter to hardware interface * Test checks that empty parameter tags work and actually checks the parsed urdf. * Patch test updates for humble API --------- Co-authored-by: Bence Magyar <[email protected]> Co-authored-by: Dr. Denis <[email protected]> (cherry picked from commit 7c07430)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is #1008 with updated tests.