-
Notifications
You must be signed in to change notification settings - Fork 948
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
adaptions to testing code to match ros-planning/moveit_resources#7 #83
adaptions to testing code to match ros-planning/moveit_resources#7 #83
Conversation
(renamed folder moveit_resources/test to moveit_resources/pr2_description)
@@ -0,0 +1,19 @@ | |||
cmake_minimum_required(VERSION 2.8.12) | |||
project(moveit_ros_integration_tests) |
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 think we should move away from the moveit_ros subfolders eventually. Why not just call this moveit_integration_tests
and leave in root folder?
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 are actually ROS-related integration tests. That's why I decided to put them into moveit_ros originally.
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 isn't a big deal, but don't you think its redundant - what other type of integration test could we have? We could have non-ROS unit tests, but ROS is what MoveIt! uses for all high level integration
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.
That's true, of course. However, the name underlines, that those integrations tests require a ROS environment ;-)
There is an open issue: Some moveit_core tests fail due to changes in moveit/moveit_resources#7. Will have a look. |
future reference: would be nice if there were separate PR2 for what this includes: 1) moving the old pr2 tests because of moveit_resources change, and 2) adding new tests |
in fact - i think you should separate this PR because part 1) must be cherry-picked for I/J too since moveit_resources only has a master branch. then you could work on all the new features as step 2 |
using variable MOVEIT_TEST_RESOURCES_DIR provided by config.h instead of calling ros::package::getPath()
@davetcoleman I have split the PR as requested, keeping part 1 here and opening a new one for part 2. |
Dave, if you happen to merge this PR, please change the commit comment to not mention integration tests, which is pulled in by the branch name if I remember correctly. |
std::string urdf_file = (res_path / "test/urdf/robot.xml").string(); | ||
std::string srdf_file = (res_path / "test/srdf/robot.xml").string(); | ||
kinect_dae_resource_ = "package://moveit_resources/test/urdf/meshes/sensors/kinect_v0/kinect.dae"; | ||
boost::filesystem::path res_path(MOVEIT_TEST_RESOURCES_DIR); |
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 slick!
Thanks for splitting this PR up, much easier to review that way. |
Once https://travis-ci.org/ros-planning/moveit/builds/155413640 passes to verify kinetic branch is stable, I'll cherry pick to I/J |
Passed, cherry-picked to I/J |
Depending on moveit/moveit_resources#7 this is the second part of #17
UPDATE: The integration tests will be filed as a separate PR. They should be disabled for now anyway, because they are blocked by:
-
pyassimp
failure inkinetic
: That's amoveit_commander
regression!- cleanup.test: ros/ros_comm#871
- validate_traj.test: #63
- robot_state_update.test: https://github.com/rhaschke/moveit/pull/1