-
Notifications
You must be signed in to change notification settings - Fork 943
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
add tests for move_group interface #1995
add tests for move_group interface #1995
Conversation
a851223
to
878dfb9
Compare
878dfb9
to
755002f
Compare
Codecov Report
@@ Coverage Diff @@
## master #1995 +/- ##
==========================================
+ Coverage 51.82% 54.02% +2.20%
==========================================
Files 319 319
Lines 24959 24959
==========================================
+ Hits 12934 13485 +551
+ Misses 12025 11474 -551
Continue to review full report at Codecov.
|
8ff7563
to
3fff271
Compare
I've tried rebasing this on earlier versions of master to get it past travis (failing unrelated tests) but gave up when I couldn't find a version where it doesn't fail on clang. I think this must be as a result of some external dependency changing. I created an issue for it here: #1996 The clang CI failure is unrelated (this new test passes). |
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.
Great work. Please try to avoid code duplication, but introduce functions instead.
This helps to increase code readability.
moveit_ros/planning_interface/test/move_group_interface_cpp_test.test
Outdated
Show resolved
Hide resolved
EXPECT_EQ(move_group_->getGoalPositionTolerance(), 0.0001); | ||
EXPECT_EQ(move_group_->getGoalOrientationTolerance(), 0.001); | ||
|
||
const std::vector<std::string> NAMED_TARGETS = { "ready", "extended" }; |
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.
These tests rely on specific details of panda_moveit_config.
Changing these details, e.g. adding more named targets, renaming joints, etc. will make this test fail for no reason.
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.
Changing parts of the panda_moveit_config could cause the tutorials to stop working. I'm not sure we need this level of testing on this. Would you like me to just drop this test?
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.
I suggest to adapt the test such that only required prerequisites are validated.
Thus, if you need let's say the named target ready
in your test, just test for its existence, but don't constrain the list of other named targets.
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.
Ok, I'll update this test to still call those interfaces but loosen what it tests for to just what is needed for the tutorial to work.
moveit_ros/planning_interface/test/move_group_interface_cpp_test.cpp
Outdated
Show resolved
Hide resolved
moveit_ros/planning_interface/test/move_group_interface_cpp_test.cpp
Outdated
Show resolved
Hide resolved
This is currently failing one of the tests on travis, and is therefore WIP, I'll post here when I get it fully working again. |
Weird, that the bullet collision test are failing now. You just added a test, which should not affect other tests. |
282fbf3
to
a0be191
Compare
…st.cpp Co-Authored-By: Michael Görner <me@v4hn.de>
ef2ca3a
to
528fb02
Compare
This succeeded on travis. |
@v4hn Thank you for the review. CI is passing with my latest changes. I am ready for another pass on this. |
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.
CI succeeded, though it did not report back to github again. sigh
I'm happy with the current patch as is.
As @rhaschke also commented here before I will leave the final pass&merge to him or any other maintainer :)
I just saw @mlautman's approval a few days ago, so I'll merge right away.
Thanks again @tylerjw !
<arg name="allow_trajectory_execution" value="true"/> | ||
<arg name="fake_execution" value="true"/> | ||
<arg name="info" value="true"/> | ||
</include> |
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.
This is a lot of redundancy in the launch files and the only reason why you did not use the demo.launch
is because it does not include a flag to disable rviz.
I'm fine with merging this patch as-is, but maybe we want to have such a flag in the demo.launch in the future so that tests can just include that one?
@v4hn thank you for the help getting this in. We are slowly making a dent in the code coverage. |
Sorry, just noticed this now: Our test cases shouldn't use |
One more addendum: MoveIt's fake controllers support multiple simulation modes and it makes sense to have tests use different modes to avoid unnecessary runtimes. For this test, I propose to set the types to 'last point', as you do not test the actual execution of the trajectory. This should be achievable via
or
But the syntax to address individual list elements ( I just digged through the rosparam APIs and cannot find a way to set an individual ros parameter inside a list at all and this seems to go all the way down to Alternatively, we could allow a global default for the fake controllers to be set in |
* add tests for move_group tutorials * easier to reach joint positions for less flaky test * allow more time for planning * remove move to start pose * reduce epsilon to 1e-5
* add tests for move_group tutorials * easier to reach joint positions for less flaky test * allow more time for planning * remove move to start pose * reduce epsilon to 1e-5
* add tests for move_group tutorials * easier to reach joint positions for less flaky test * allow more time for planning * remove move to start pose * reduce epsilon to 1e-5
* add tests for move_group tutorials * easier to reach joint positions for less flaky test * allow more time for planning * remove move to start pose * reduce epsilon to 1e-5
Description
I realized there was no test of the c++ interface of move_group. This PR fixes that to defend the interface used by our newest users against unintended breakages.
Checklist