add tests for move_group interface #1995

tylerjw · 2020-03-28T22:40:25Z

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

Required by CI: Code is auto formatted using clang-format
Moves the CodeCov report in the right direction ⬆️ ⬆️ ⬆️

codecov-io · 2020-03-28T23:23:16Z

Codecov Report

Merging #1995 into master will increase coverage by 2.20%.
The diff coverage is n/a.

@@            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

Impacted Files	Coverage Δ
moveit_core/robot_state/src/robot_state.cpp	`45.61% <0.00%> (+0.40%)`	⬆️
moveit_core/robot_state/src/conversions.cpp	`43.49% <0.00%> (+0.44%)`	⬆️
...ution_manager/src/trajectory_execution_manager.cpp	`46.86% <0.00%> (+0.51%)`	⬆️
...mpl_interface/src/model_based_planning_context.cpp	`58.55% <0.00%> (+0.53%)`	⬆️
...raint_samplers/src/default_constraint_samplers.cpp	`83.63% <0.00%> (+1.09%)`	⬆️
...dl_kinematics_plugin/src/kdl_kinematics_plugin.cpp	`78.92% <0.00%> (+1.34%)`	⬆️
...anning_scene_monitor/src/current_state_monitor.cpp	`52.82% <0.00%> (+1.53%)`	⬆️
moveit_core/planning_scene/src/planning_scene.cpp	`57.68% <0.00%> (+2.44%)`	⬆️
...nning_scene_monitor/src/planning_scene_monitor.cpp	`70.95% <0.00%> (+2.84%)`	⬆️
...veit_core/robot_model/src/floating_joint_model.cpp	`48.35% <0.00%> (+3.29%)`	⬆️
... and 15 more

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 d989220...528fb02. Read the comment docs.

tylerjw · 2020-03-29T01:17:57Z

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).

rhaschke

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

rhaschke · 2020-03-29T14:46:18Z

moveit_ros/planning_interface/test/move_group_interface_cpp_test.cpp

+  EXPECT_EQ(move_group_->getGoalPositionTolerance(), 0.0001);
+  EXPECT_EQ(move_group_->getGoalOrientationTolerance(), 0.001);
+
+  const std::vector<std::string> NAMED_TARGETS = { "ready", "extended" };


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.

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?

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.

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

tylerjw · 2020-03-31T19:44:53Z

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.

rhaschke · 2020-04-01T08:09:57Z

Weird, that the bullet collision test are failing now. You just added a test, which should not affect other tests.

…st.cpp Co-Authored-By: Michael Görner <me@v4hn.de>

tylerjw · 2020-04-07T21:09:17Z

Rebased on master, this is ready for review again (assuming it passes CI) @v4hn and @rhaschke. Thank you for the reviews so far.

tylerjw · 2020-04-07T22:59:41Z

This succeeded on travis.

tylerjw · 2020-04-08T15:51:49Z

@v4hn Thank you for the review. CI is passing with my latest changes. I am ready for another pass on this.

v4hn

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 !

v4hn · 2020-04-08T15:54:44Z

moveit_ros/planning_interface/test/move_group_interface_cpp_test.test

+    <arg name="allow_trajectory_execution" value="true"/>
+    <arg name="fake_execution" value="true"/>
+    <arg name="info" value="true"/>
+  </include>


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?

tylerjw · 2020-04-08T18:01:10Z

@v4hn thank you for the help getting this in. We are slowly making a dent in the code coverage.

rhaschke · 2020-04-08T19:30:59Z

Sorry, just noticed this now: Our test cases shouldn't use panda_moveit_config (see #1904).
@tylerjw, could you please replace this package with moveit_resources? The patch moveit/panda_moveit_config#63 should be applied there as well.

v4hn · 2020-04-09T10:02:47Z

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

<param name="/move_group/controller_list[0]/type" value="last point"/>
<param name="/move_group/controller_list[1]/type" value="last point"/>

or

<rosparam param='move_group/controller_list'>[ { type: 'last point'}, { type: 'last point' }]</rosparam>

But the syntax to address individual list elements ([0]) does not work and the latter command overrides the rest of the yaml tree, removing the main controller configuration...

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 xmlrpc, although one could probably write a cumbersome construct to parse this in rosparam and add a patch there grml.

Alternatively, we could allow a global default for the fake controllers to be set in /move_group/controller_list/type, but someone would have to write a patch for that too ~~(assuming it does not work yet)~~. 👀

* 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

tylerjw requested a review from rhaschke as a code owner March 28, 2020 22:40

tylerjw force-pushed the p/tylerjw/move_group_test branch 3 times, most recently from a851223 to 878dfb9 Compare March 28, 2020 23:15

tylerjw requested review from bmagyar, BryceStevenWilley, davetcoleman, gavanderhoorn, IanTheEngineer, jrgnicho, mamoll, mikeferguson, mlautman and v4hn as code owners March 28, 2020 23:15

tylerjw force-pushed the p/tylerjw/move_group_test branch from 878dfb9 to 755002f Compare March 28, 2020 23:16

tylerjw force-pushed the p/tylerjw/move_group_test branch 5 times, most recently from 8ff7563 to 3fff271 Compare March 29, 2020 00:51

rhaschke reviewed Mar 29, 2020

View reviewed changes

mlautman approved these changes Mar 31, 2020

View reviewed changes

tylerjw changed the title ~~add tests for move_group tutorials~~ WIP: add tests for move_group tutorials Mar 31, 2020

tylerjw force-pushed the p/tylerjw/move_group_test branch 2 times, most recently from 282fbf3 to a0be191 Compare April 3, 2020 19:59

tylerjw changed the title ~~WIP: add tests for move_group tutorials~~ add tests for move_group interface Apr 6, 2020

tylerjw and others added 9 commits April 7, 2020 15:08

respond to review

a10bdc3

easier to reach joint positions for less flaky test

075ae05

increase planning time to 30s

7a493fe

clang tidy fixes

4b7c26a

allow more time for planning

a4c5734

remove move to start pose

8bad3fd

Update moveit_ros/planning_interface/test/move_group_interface_cpp_te…

120eb7e

…st.cpp Co-Authored-By: Michael Görner <me@v4hn.de>

respond to feedback

e9f1767

add comment about link names

528fb02

tylerjw force-pushed the p/tylerjw/move_group_test branch from ef2ca3a to 528fb02 Compare April 7, 2020 21:08

put epsilon back at 1cm

4c1cf13

respond to feedback

46c188d

v4hn approved these changes Apr 8, 2020

View reviewed changes

v4hn merged commit 9681f50 into moveit:master Apr 8, 2020

tylerjw mentioned this pull request Apr 8, 2020

Add variable to demo.launch to control starting rviz for simpler test launch files. moveit/panda_moveit_config#62

Closed

tylerjw deleted the p/tylerjw/move_group_test branch April 9, 2020 16:09

tylerjw mentioned this pull request Apr 9, 2020

Code coverage (testing our core functionality) #2016

Open

4 tasks

tylerjw mentioned this pull request May 8, 2020

July 2020 Melodic Release #2066

Closed

20 tasks

tylerjw mentioned this pull request May 11, 2020

[melodic-devel] bug fix backport #2080

Merged

v4hn mentioned this pull request May 20, 2020

bugfix backport #2098

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add tests for move_group interface #1995

add tests for move_group interface #1995

tylerjw commented Mar 28, 2020 •

edited

Loading

codecov-io commented Mar 28, 2020 •

edited

Loading

tylerjw commented Mar 29, 2020

rhaschke left a comment

rhaschke Mar 29, 2020

tylerjw Mar 29, 2020

rhaschke Apr 1, 2020

tylerjw Apr 6, 2020

tylerjw commented Mar 31, 2020

rhaschke commented Apr 1, 2020

tylerjw commented Apr 7, 2020

tylerjw commented Apr 7, 2020 •

edited

Loading

tylerjw commented Apr 8, 2020

v4hn left a comment •

edited

Loading

v4hn Apr 8, 2020

tylerjw commented Apr 8, 2020

rhaschke commented Apr 8, 2020

v4hn commented Apr 9, 2020 •

edited

Loading

add tests for move_group interface #1995

add tests for move_group interface #1995

Conversation

tylerjw commented Mar 28, 2020 • edited Loading

Description

Checklist

codecov-io commented Mar 28, 2020 • edited Loading

Codecov Report

tylerjw commented Mar 29, 2020

rhaschke left a comment

Choose a reason for hiding this comment

rhaschke Mar 29, 2020

Choose a reason for hiding this comment

tylerjw Mar 29, 2020

Choose a reason for hiding this comment

rhaschke Apr 1, 2020

Choose a reason for hiding this comment

tylerjw Apr 6, 2020

Choose a reason for hiding this comment

tylerjw commented Mar 31, 2020

rhaschke commented Apr 1, 2020

tylerjw commented Apr 7, 2020

tylerjw commented Apr 7, 2020 • edited Loading

tylerjw commented Apr 8, 2020

v4hn left a comment • edited Loading

Choose a reason for hiding this comment

v4hn Apr 8, 2020

Choose a reason for hiding this comment

tylerjw commented Apr 8, 2020

rhaschke commented Apr 8, 2020

v4hn commented Apr 9, 2020 • edited Loading

tylerjw commented Mar 28, 2020 •

edited

Loading

codecov-io commented Mar 28, 2020 •

edited

Loading

tylerjw commented Apr 7, 2020 •

edited

Loading

v4hn left a comment •

edited

Loading

v4hn commented Apr 9, 2020 •

edited

Loading