search

moveit / moveit_ros

THIS REPO HAS MOVED TO https://github.com/ros-planning/moveit
69 stars 118 forks source link

moveit_ros integration tests #729

Closed rhaschke closed 7 years ago

rhaschke commented 7 years ago

integration tests for #442, #592 imported ROS Industrial's moveit_config for Fanuc robot rostest files launch move_group and run tests against it

davetcoleman commented 7 years ago

@gavanderhoorn can you review this since I see your name on some of the files?

gavanderhoorn commented 7 years ago

I'd be happy to, but I'm currently OoO, so I can't guarantee that I'll be able to do this soon.

Quick comment: there are still references to the fanuc_driver and other packages in the launch files and run_depends sections. Might want to take those out, or make things functional again.

rhaschke commented 7 years ago

Good point. I cleaned the repo up to remove (hopefully all) unnecessary stuff. Please squash before merging!

davetcoleman commented 7 years ago

Do you have any thoughts on our various testing robots in moveit? Seems like we should consolidate or at least understand what is being used:

PR2: https://github.com/ros-planning/moveit_resources/tree/master/test/urdf

KukakR210 and Motoman SIA20d: https://github.com/ros-planning/moveit_kinematics_tests

Even better - I think the urdfs you are using should also be moved to moveit_resources

rhaschke commented 7 years ago

PR2: https://github.com/ros-planning/moveit_resources/tree/master/test/urdf

I didn't know about moveit_resources! Instead of importing a new robot for testing, I suggest to use the existing PR2 model. @davetcoleman: Is there a moveit_config around for it too?

v4hn commented 7 years ago

On Fri, Jul 29, 2016 at 01:31:43AM -0700, Robert Haschke wrote:

I didn't know about moveit_resources! Instead of importing a new robot for testing, I suggest to use the existing PR2 model.

Having a small and easy robot around in the resources and to use it for unit tests sounds like a good idea to me.

I don't see a reason to depend on the whole PR2 for tests.

@davetcoleman: Is there a moveit_config around for it too?

You mean https://github.com/ros-planning/moveit_pr2/tree/indigo-devel/pr2_moveit_config ?

rhaschke commented 7 years ago

You mean https://github.com/ros-planning/moveit_pr2/tree/indigo-devel/pr2_moveit_config ?

Argh! That's yet another location. I think, there should be a full robot (URDF) and moveit config for testing in one central location. I see the following options:

  1. Use moveit_pr2 for integration testing. However, I agree with @v4hn, that it would be better to have a simpler robot around for testing.
  2. Add a moveit config for the PR2 in moveit_resources. This seems to be redundant with moveit_pr2 then.
  3. Move the test_robot config of this PR to moveit_resources. In that case, it would make sense to replace the PR2 stuff with the test_robot. Not sure though, were this is already used...

Finally I suggest to rename moveit_resources to moveit_test_resources.

v4hn commented 7 years ago

On Fri, Jul 29, 2016 at 02:04:53AM -0700, Robert Haschke wrote:

  1. Move the test_robot config of this PR to moveit_resources.

+1 This sounds like a good idea.

In that case, it would make sense to replace the PR2 stuff with the test_robot. Not sure though, were this is already used...

This has been around for a long time, so we shouldn't touch the existing files in indigo-devel. I would propose to add a test_robot directory in moveit_resources/test for now and move the pr2 related files to a pr2 subdirectory, or remove them completely. Either way, the second step might need some thorough greping through moveit to check whether anything used the current paths.

Finally I suggest to rename moveit_resources to moveit_test_resources.

-1 for indigo. That might make sense for kinetic though. The current name is a bit confusing, I agree.

davetcoleman commented 7 years ago

I think, there should be a full robot (URDF) and moveit config for testing in one central location.

moveit_robots is where we've historically kept all moveit config packages, i like the idea of keeping them there. Do you think the ones we share with users will need to be different than the ones we test with? I don't like the idea of having duplicates

I agree with @v4hn, that it would be better to have a simpler robot around for testing.

Yes I see no problem having a single arm robot alongside PR2 - though I am concerned about having too many robots. See these ik tests @jrgnicho

Move the test_robot config of this PR to moveit_resources. In that case, it would make sense to replace the PR2 stuff with the test_robot. Not sure though, were this is already used...

The pr2 is being used in moveit's limited tests already - we should def not remove it but moving it to a better labeled subfolder sounds great

Finally I suggest to rename moveit_resources to moveit_test_resources.

This is a released package so we can't do that for indigo and I don't think its worth the effort in the future. Just add a better README perhaps

rhaschke commented 7 years ago

As a cleanup of moveit_resources will involve adjustments in several other repos, I suggest to postpone this PR until the repos are merged. Closing here in favour of https://github.com/ros-planning/moveit/issues/17.