Fix Actor.cc copy operators and restructure tests #301
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
…verage Signed-off-by: Luca Della Vedova <luca@openrobotics.org>
Signed-off-by: Luca Della Vedova <luca@openrobotics.org>
Signed-off-by: Luca Della Vedova <luca@openrobotics.org>
Signed-off-by: Luca Della Vedova <luca@openrobotics.org>
Signed-off-by: Ian Chen <ichen@osrfoundation.org>
iche033
approved these changes
Jun 23, 2020
iche033
reviewed
Jun 23, 2020
src/Actor_TEST.cc
Outdated
| *_actor1.TrajectoryByIndex(traj_idx), | ||
| *_actor2.TrajectoryByIndex(traj_idx)); | ||
| } | ||
| return animations_equal && trajectories_equal && |
There was a problem hiding this comment.
nitpick: camelCase for animations_equal and trajectories_equal
There was a problem hiding this comment.
Done, a quick search for _equal showed that waypoints_equal had the same issue so fixed that as well.
Signed-off-by: Luca Della Vedova <luca@openrobotics.org>
scpeters
approved these changes
Jun 26, 2020
|
I don't know why the GitHub action didn't run for this PR |
I'm not sure how to fix that, can someone retrigger the action? |
dummy commit to trigger CI Signed-off-by: Steve Peters <scpeters@openrobotics.org>
|
@osrf-jenkins run tests please |
|
@osrf-jenkins run tests |
|
I think #310 should enable the actions CI for this branch |
Codecov Report
@@ Coverage Diff @@
## sdf8 #301 +/- ##
=======================================
Coverage ? 87.54%
=======================================
Files ? 54
Lines ? 7293
Branches ? 0
=======================================
Hits ? 6385
Misses ? 908
Partials ? 0 Continue to review full report at Codecov.
|
scpeters
added a commit
to scpeters/sdformat
that referenced
this pull request
Aug 21, 2020
* Simplify and fix copy constructors, restructure tests and increase coverage * Add joint and link tests * Add changelog Signed-off-by: Luca Della Vedova <luca@openrobotics.org> Co-authored-by: Ian Chen <ichen@osrfoundation.org> Co-authored-by: Steve Peters <scpeters@openrobotics.org>
scpeters
added a commit
that referenced
this pull request
Aug 21, 2020
* Simplify and fix copy constructors, restructure tests and increase coverage * Add joint and link tests * Add changelog Signed-off-by: Luca Della Vedova <luca@openrobotics.org> Co-authored-by: Ian Chen <ichen@osrfoundation.org> Co-authored-by: Steve Peters <scpeters@openrobotics.org>
scpeters
added a commit
to scpeters/sdformat
that referenced
this pull request
Aug 21, 2020
* Simplify and fix copy constructors, restructure tests and increase coverage * Add joint and link tests * Add changelog Signed-off-by: Luca Della Vedova <luca@openrobotics.org> Co-authored-by: Ian Chen <ichen@osrfoundation.org> Co-authored-by: Steve Peters <scpeters@openrobotics.org>
scpeters
added a commit
that referenced
this pull request
Aug 25, 2020
* Simplify and fix copy constructors, restructure tests and increase coverage * Add joint and link tests * Add changelog Signed-off-by: Luca Della Vedova <luca@openrobotics.org> Co-authored-by: Ian Chen <ichen@osrfoundation.org> Co-authored-by: Steve Peters <scpeters@openrobotics.org>
scpeters
added a commit
to scpeters/sdformat
that referenced
this pull request
Aug 25, 2020
* Simplify and fix copy constructors, restructure tests and increase coverage * Add joint and link tests * Add changelog Signed-off-by: Luca Della Vedova <luca@openrobotics.org> Co-authored-by: Ian Chen <ichen@osrfoundation.org> Co-authored-by: Steve Peters <scpeters@openrobotics.org>
scpeters
added a commit
that referenced
this pull request
Aug 25, 2020
* Simplify and fix copy constructors, restructure tests and increase coverage * Add joint and link tests * Add changelog Signed-off-by: Luca Della Vedova <luca@openrobotics.org> Co-authored-by: Ian Chen <ichen@osrfoundation.org> Co-authored-by: Steve Peters <scpeters@openrobotics.org>
traversaro
pushed a commit
to traversaro/sdformat
that referenced
this pull request
Sep 5, 2020
* Simplify and fix copy constructors, restructure tests and increase coverage * Add joint and link tests * Add changelog Signed-off-by: Luca Della Vedova <luca@openrobotics.org> Co-authored-by: Ian Chen <ichen@osrfoundation.org> Co-authored-by: Steve Peters <scpeters@openrobotics.org>
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 PR is a followup of #299, it changes all the copy operators to use the default copy operator of the private data object, fixing a few properties that were not copied in the manual copy (such as the actor properties trajectories, joints, links, sdf element).
I also took the chance to implement unit tests for every class (before only actors were being tested) and restructure the tests a bit to reduce code duplication.
The tests are now much more comprehensive. What can be set through setter functions is tested in the unit tests. The parts that are set on SDF parsing are tested in the integration test (the example sdf has also been updated to add joints and links, previously untested).
Open to feedback about what to do with the CopyFrom functions, right now they are broken since they don't copy all the properties, I wanted to deprecate them but is that OK to do in a minor release?