Add test to ompl interface for StateValidityChecker

[JeroenDM]

Description

I added some minimal tests for the ompl_interface::StateValidityChecker. My intend is to better understand how all the pieces fit together, not necessarily thorough testing of this class.

The test uses a new generic test class to make it easy to run the same test on several robots. (See the documentation in the code.) Addmiditly it might come across as a bit overengineered, but I plan to use this for other tests in the future.

As mentioned in the comment, I could simplify it by using rostest and passing robot specific setup over the parameter server. But these tests would run slower and as this package is outside moveit_ros (so ideally ROS independent I guess).

Or is there a better way?

Checklist

• Required by CI: Code is auto formatted using clang-format
• Extend the tutorials / documentation reference
• Document API changes relevant to the user in the MIGRATION.md notes
• Create tests, which fail without this PR reference
• Include a screenshot if changing a GUI
• While waiting for someone to review your request, please help review another open pull request to support the maintainers

[mamoll]

Looks good to me.

[JeroenDM]

@mamoll The tests with path constraints just started working today. It must have been some issue with my local environment, although I have no idea what caused the issue. But if the tests also succeed on Travis I will take that as proof that it was my local environment.

[codecov]

Codecov Report

Merging #2247 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #2247   +/-   ##
=======================================
  Coverage   56.38%   56.38%           
=======================================
  Files         287      287           
  Lines       24451    24451           
=======================================
  Hits        13785    13785           
  Misses      10666    10666           

Impacted Files	Coverage Δ
.../ompl_interface/src/detail/constrained_sampler.cpp	42.86% <0.00%> (-17.14%)	⬇️
...meterization/work_space/pose_model_state_space.cpp	83.09% <0.00%> (-1.47%)	⬇️
moveit_core/robot_state/src/robot_state.cpp	46.03% <0.00%> (+0.11%)	⬆️
...raint_samplers/src/default_constraint_samplers.cpp	81.89% <0.00%> (+0.37%)	⬆️
...dl_kinematics_plugin/src/kdl_kinematics_plugin.cpp	76.55% <0.00%> (+0.45%)	⬆️
...on/pick_place/src/approach_and_translate_stage.cpp	72.96% <0.00%> (+0.82%)	⬆️
...pl_interface/src/detail/state_validity_checker.cpp	44.12% <0.00%> (+5.89%)	⬆️

---

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 144adb7...071a835. Read the comment docs.

[tylerjw]

Why not use ROS logging macros and setting them to output DEBUG with a namespace for this test?

[JeroenDM]

True! It's an old habit from when I didn't bother setting up my ROS debug levels correctly... I will fix this.

[JeroenDM]

Hmm, I will change the log statements, but I also use it to set the verbosity of the state validity checker

checker->setVerbose(VERBOSE);

which in turn is passed on to the collision request. Maybe I can make it an optional command-line argument for the test. The verbose output from the collision checker can be really useful when debugging. I can also set it to true by default.

[v4hn]

Talking about "minor" patches, it's really annoying that we can't specify arbitrary output streams for these methods. :-)

[JeroenDM]

I think this is ready to be merged after another review.
Then I can rebase #2248 (which reuses some of the code) and finish it.

[mamoll]

This looks ready to be merged to me. @tylerjw / @v4hn, can one of you review one more time?

[JeroenDM]

I just noticed I did not add the new test dependencies to the package.xml file, as this needs to be done for every test robot separately since recently.

<test_depend>moveit_resources_fanuc_description</test_depend>
<test_depend>moveit_resources_panda_description</test_depend>

[tylerjw]

Thank you for updating this. I made a couple comments about a minor nit but overall I think this test looks really good so I'm aproving it to make it easy to merge.

[JeroenDM]

@tylerjw @mamoll I applied the last suggestions. This should be good to go now if CI says it's ok.

[JeroenDM]

@mamoll ping

[JeroenDM]

thanks!