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

Clean and independent tests? #12

Open
JeroenDM opened this issue Jan 8, 2019 · 9 comments
Open

Clean and independent tests? #12

JeroenDM opened this issue Jan 8, 2019 · 9 comments
Labels
enhancement New feature or request

Comments

@JeroenDM
Copy link
Owner

JeroenDM commented Jan 8, 2019

Some tests depend on kuka_kr6r700_moveit_config, this is probably not so good as you have to generate this package yourself with the MoveIt! setup assistant.

Also, I build the tests as a normal ROS node and use a standard launch file to run them. This is not the right way to do it, but I really don't like the current test output of catkin tools.

@simonschmeisser
Copy link
Collaborator

My idea would be to reuse the generic kinematic tests from MoveIt! (going to be) introduced in moveit/moveit#1272 together with the fanuc package from https://github.com/ros-planning/moveit_resources . This will give us a proven independent test for free. Unfortunately it's just one 6-axis robot, panda has 7 and pr2 has 2x7 (?) axis.

For the kuka_kr6r700 it would be good to publish it somewhere, probably as a separate repo. You can then add a (or so) in package.xml. Or maybe it is sufficient to strip down the few thing we actually need from the moveit_config and just include that in the launch file in this repo. (I haven't looked how that is done in above PR but we can probably copy-pasta from there)

@JeroenDM JeroenDM mentioned this issue Jan 18, 2019
5 tasks
@JeroenDM
Copy link
Owner Author

JeroenDM commented Jan 21, 2019

Thank for the input @simonschmeisser!
I read through the code and pull request you mentioned, tried some stuff in a new pull request and came up with a plan.
The summary below is mainly aimed at myself because I'm still slightly confused.

Tests within this package

Write a single test source file that can be used for different robots using different rostest launch files.
Here the plugin is accessed by creating an instance of the plugin in a test fixture:

moveit_opw_kinematics_plugin::MoveItOPWKinematicsPlugin plugin_;

And a test for a specific robot is setup with a rostest file that looks something like this:

<launch>
  <rosparam command="load" file="$(find robot_moveit_config)/test/kinematics.yaml" />
  
  <include file="$(findrobot_moveit_config)/launch/planning_context.launch">
    <arg name="load_robot_description" value="true"/>
  </include>

  <test pkg="moveit_opw_kinematics_plugin" type="moveit_opw_kinematics_plugin_test_robot" test-name="test_kuka">
  </test>
</launch>

For the robot_moveit_config packages I can wait for this pull request to have more than one robot to test on. This is probably better than publishing arbitrary robot MoveIt! configs on my github account.

Note: using this tests, the consistency of the kinematics.yaml file with the urdf is never checked. So maybe I do need another test that also uses forward kinematics from the RobotState class in MoveIt!.

Free tests from future MoveIt!

The tests in MoveIt! (from the pull request you mentioned) access the plugin through:

kinematics::KinematicsBasePtr kinematics_solver_;

in a plugin-independent way (using the plugin name as a parameter).

Other options

I thought about doing something like this:

robot_model::JointModelGroup* joint_model_group_;
// and then in the test function:
const auto solver = joint_model_group_->getSolverInstance();

As is used inside descartes_moveit in the descartes package. I cannot get this working at the moment and I doubt whether it will add value if the two test options above are implemented.

7-DOF Robots

This could work for specific robots if we add functionality to specify a fixed value for a specific joint, either in opw_kinematics or in this plugin.
Maybe an extra parameter in the kinematics.yaml file to enable / disable the 7-dof case.

@simonschmeisser
Copy link
Collaborator

Hey @JeroenDM could you publish the kuka package to a moveit_opw_kinematics_plugin_resources (shorter name would also be fine :D ) separate repo? Then it would be possible to automate the tests
Thanks!

@JeroenDM
Copy link
Owner Author

Do you mean the moveit config for the kr6r700?

A list of the dependencies you need to add manually from source taken from a closed pull request:

Dependencies for the kuka test:

Dependencies for the fanuc test:

Maybe I should do a pull request to add a light version of the kuka dependencies to the moveit_resources package...that would be cleaner.

@simonschmeisser
Copy link
Collaborator

simonschmeisser commented May 31, 2019

oh, my bad, didn't look closely enough ... I'll add a travis PR later on then (and fix it on our internal jenkins)

Maybe I should do a pull request to add a light version of the kuka dependencies to the moveit_resources package...that would be cleaner.

Certainly, I'm not sure what the requirements are for that are however.

@JeroenDM JeroenDM added the enhancement New feature or request label May 31, 2019
@JeroenDM
Copy link
Owner Author

Ok, perfect, I just started with travis but I'll wait for your pull request then :)

@simonschmeisser
Copy link
Collaborator

I tested this approach on our jenkins but the problem is this will also install the kuka packages later on. Might be our fault but it would be way more convenient for me to have a "resources" package without any install part in the cmakelists

@JeroenDM
Copy link
Owner Author

Something like this: kuka_test_resources ?
(There is still some stuff in the support package that can be left out without influencing the tests, but this is just a proposal.)

My original objection was that doing this is just copy pasting someone elses code in my own repository.
This also means that upstream changes need to be added manually to the resources package if needed.

I planned to add this directly to a fork of the moveit_resources package, but since there are some install instructions in the cmakelists there, that may also give problems?

@simonschmeisser
Copy link
Collaborator

I fixed our internal system now, using the resource packages during testing and adding CATKIN_IGNORE for the installation/packaging part. Works very well, thanks a lot!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants