Skip to content

Check joint parent link names in Model::Load#726

Merged
scpeters merged 3 commits intogazebosim:sdf10from
scpeters:issue_719
Oct 15, 2021
Merged

Check joint parent link names in Model::Load#726
scpeters merged 3 commits intogazebosim:sdf10from
scpeters:issue_719

Conversation

@scpeters
Copy link
Copy Markdown
Member

@scpeters scpeters commented Oct 5, 2021

🦟 Bug fix

Fixes #719 on sdf10 (a separate fix is needed for sdf11 and later)

Summary

As noted in #710, several example files in the test/sdf/ folder specify non-existent links in the //joint/parent element, which violates the spec. I added an ign sdf --check test/sdf/joint_axis_infinite_limits.sdf test case in b47f9bf, which properly notes the failure, though the sdf::Root::Load call in INTEGRATION_joint_axis_dom does not report an error. To detect the error in sdf::Root::Load, I added a check in Model::Load that //joint/parent contains a valid link name and fixed the test/sdf/ files in ce8c032.

A different fix will be needed in sdf11 to account for changes in the spec that allow any frame to be named in //joint/parent.

Checklist

  • Signed all commits for DCO
  • Added tests
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • codecheck passed (See contributing)
  • All tests passed (See test coverage)
  • While waiting for a review on your PR, please help review another open pull request to support the maintainers

Note to maintainers: Remember to use Squash-Merge

Signed-off-by: Steve Peters <scpeters@openrobotics.org>
Signed-off-by: Steve Peters <scpeters@openrobotics.org>
Signed-off-by: Steve Peters <scpeters@openrobotics.org>
@codecov-commenter
Copy link
Copy Markdown

Codecov Report

Merging #726 (fa37f00) into sdf10 (a55f9e3) will decrease coverage by 0.05%.
The diff coverage is 100.00%.

❗ Current head fa37f00 differs from pull request most recent head ce8c032. Consider uploading reports for the commit ce8c032 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##            sdf10     #726      +/-   ##
==========================================
- Coverage   87.81%   87.76%   -0.06%     
==========================================
  Files          65       65              
  Lines       10403    10409       +6     
==========================================
  Hits         9135     9135              
- Misses       1268     1274       +6     
Impacted Files Coverage Δ
src/Model.cc 86.60% <100.00%> (+0.26%) ⬆️
src/ign.cc 70.90% <0.00%> (-1.82%) ⬇️
src/parser.cc 79.22% <0.00%> (-0.56%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a55f9e3...ce8c032. Read the comment docs.

@scpeters
Copy link
Copy Markdown
Member Author

scpeters commented Oct 5, 2021

code coverage is dropping because there is redundant error checking

Comment thread src/ign_TEST.cc
// Check an SDF file with the infinite values for joint axis limits.
// This is a valid file.
{
std::string path = pathBase +"/joint_axis_infinite_limits.sdf";
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: same comment as the sdf11 PR, not sure if this is necessary since the error is now caught in Model and other tests should already have this file covered.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as I wrote in #727 (comment), I would keep the test

@scpeters scpeters merged commit aa9362b into gazebosim:sdf10 Oct 15, 2021
@scpeters scpeters deleted the issue_719 branch October 15, 2021 20:55
@osrf-triage
Copy link
Copy Markdown

This pull request has been mentioned on Gazebo Community. There might be relevant details there:

https://community.gazebosim.org/t/new-ignition-releases-2022-03-25-fortress-edifice-citadel/1343/1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🔮 dome Ignition Dome

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants