Skip to content
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

Test included model folders missing model.config #422

Merged
merged 3 commits into from
Nov 23, 2020

Conversation

scpeters
Copy link
Member

@scpeters scpeters commented Nov 21, 2020

Currently there is a confusing error message if a file is loaded that finds a folder matching the name of a model to
be included but the folder does not have a model.config file. This improves the first error message and stops further loading
to prevent additional confusing messages. To illustrate this, I've added a model folder box_missing_config to test/integration/model containing a model.sdf but no model.config and a test that tries to include a model matching that folder name. Without the change to parser.cc, the test prints the following:

[ RUN      ] IncludesTest.IncludeModelMissingConfig
Error [parser.cc:749] Could not find model.config or manifest.xml for the model
Error [parser.cc:749] Could not find model.config or manifest.xml for the model
Error [parser.cc:397] File [] doesn't exist.
Error:   Could not find the 'robot' element in the xml file
         at line 80 in /build/urdfdom-YMMa9X/urdfdom-1.0.0/urdf_parser/src/model.cpp
Error [parser_urdf.cc:3193] Unable to call parseURDF on robot model
Error [parser.cc:488] parse as old deprecated model file failed.
Error Code 1 Msg: Unable to read file[]
Error Code 8 Msg: Error reading element <sdf>
/home/scpeters/clone/sdformat/test/integration/includes.cc:337: Failure
Value of: sdf::readString(stream.str(), sdfParsed)
  Actual: false
Expected: true
[  FAILED  ] IncludesTest.IncludeModelMissingConfig (51 ms)

Note that box_missing_config is not listed anywhere, which makes it hard to figure out what is the cause of the problem. It also causes readString to fail. After modifying parser.cc, the parser doesn't fail and the test shows the following console output (the second message comes from printing out the errors returned by Root::Load):

[ RUN      ] IncludesTest.IncludeModelMissingConfig
Error [parser.cc:749] Could not find model.config or manifest.xml in [/home/scpeters/clone/sdformat/test/integration/model/box_missing_config]
Error Code 12 Msg: Unable to resolve uri[box_missing_config] to model path [/home/scpeters/clone/sdformat/test/integration/model/box_missing_config] since it does not contain a model.config file.
[       OK ] IncludesTest.IncludeModelMissingConfig (34 ms)

EDIT: it doesn't actually cause sdf::readString to fail, though it does return / print an error depending on which overload is called.

Currently there is a confusing error message if a file is
loaded that finds a folder matching the name of a model to
be included but the folder does not have a model.config file.
This improves the first error message and stops further loading
to prevent additional confusing messages.

Signed-off-by: Steve Peters <scpeters@openrobotics.org>
@scpeters scpeters requested a review from chapulina November 21, 2020 01:28
@github-actions github-actions bot added the 🏰 citadel Ignition Citadel label Nov 21, 2020
@codecov-io
Copy link

codecov-io commented Nov 21, 2020

Codecov Report

Merging #422 (0efbcdf) into sdf9 (0e77816) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##             sdf9     #422   +/-   ##
=======================================
  Coverage   86.25%   86.26%           
=======================================
  Files          59       59           
  Lines        9181     9187    +6     
=======================================
+ Hits         7919     7925    +6     
  Misses       1262     1262           
Impacted Files Coverage Δ
src/parser.cc 77.58% <100.00%> (+0.15%) ⬆️

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 0e77816...0efbcdf. Read the comment docs.

src/parser.cc Show resolved Hide resolved
test/integration/includes.cc Show resolved Hide resolved
Signed-off-by: Steve Peters <scpeters@openrobotics.org>
Signed-off-by: Steve Peters <scpeters@openrobotics.org>
src/parser.cc Show resolved Hide resolved
@scpeters scpeters merged commit 8cef85f into gazebosim:sdf9 Nov 23, 2020
@scpeters scpeters deleted the include_error_message_9 branch November 23, 2020 18:49
scpeters added a commit to scpeters/sdformat that referenced this pull request Dec 14, 2020
Currently there is a confusing error message if a file is
loaded that finds a folder matching the name of a model to
be included but the folder does not have a model.config file.
This improves the first error message and stops further loading
to prevent additional confusing messages.

Signed-off-by: Steve Peters <scpeters@openrobotics.org>
scpeters added a commit to scpeters/sdformat that referenced this pull request Dec 15, 2020
Currently there is a confusing error message if a file is
loaded that finds a folder matching the name of a model to
be included but the folder does not have a model.config file.
This improves the first error message and stops further loading
to prevent additional confusing messages.

Signed-off-by: Steve Peters <scpeters@openrobotics.org>
scpeters added a commit that referenced this pull request Dec 15, 2020
Currently there is a confusing error message if a file is
loaded that finds a folder matching the name of a model to
be included but the folder does not have a model.config file.
This improves the first error message and stops further loading
to prevent additional confusing messages.

Signed-off-by: Steve Peters <scpeters@openrobotics.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏰 citadel Ignition Citadel
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants