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

kinematics tests #1272

Merged
merged 13 commits into from Jan 23, 2019

Conversation

Projects
None yet
4 participants
@rhaschke
Copy link
Collaborator

commented Dec 16, 2018

This is another attempt for #1147, addressing #4. Similarly to ros-planning/moveit_kinematics_tests#7, I tried to separate the tests from robot-specific code: The tests as designed by @jrgnicho are robot- and ik-plugin-agnostic and can be run (in an appropriate environment) for any robot and plugin.
Additionally to the initial feasibility tests, I added some real validation to FK tests: RobotState's FK and KinematicsPlugin's one should coincide. A failure there pointed me to #1271.
All original tests are random feasibility tests that turned out to be flaky, which is why I disabled them.

Instead of adding additional robots to moveit_resources, I'm running those tests on Fanuc + Panda.
I'm planning to add a ikfast test for these robots as well.

@rhaschke

This comment has been minimized.

Copy link
Collaborator Author

commented Dec 17, 2018

Note, that the unit test fails due to #1271.

@rhaschke rhaschke force-pushed the ubi-agni:kinematics-tests branch 2 times, most recently from 17b9ca0 to 23526f0 Dec 19, 2018

@v4hn v4hn force-pushed the ubi-agni:kinematics-tests branch from 23526f0 to ad83b7f Dec 21, 2018

@jrgnicho

This comment has been minimized.

Copy link
Contributor

commented Dec 21, 2018

@rhaschke thanks for reviving this PR, do you plan on making further changes or can it now be reviewed?

@rhaschke

This comment has been minimized.

Copy link
Collaborator Author

commented Dec 27, 2018

Do you plan on making further changes or can it now be reviewed?

I'm still working on it. Changed the title accordingly.

@rhaschke rhaschke changed the title kinematics tests WIP: kinematics tests Dec 27, 2018

@rhaschke rhaschke force-pushed the ubi-agni:kinematics-tests branch 10 times, most recently from f91b05a to 36d6a99 Dec 27, 2018

@rhaschke rhaschke changed the title WIP: kinematics tests kinematics tests Dec 31, 2018

@rhaschke

This comment has been minimized.

Copy link
Collaborator Author

commented Dec 31, 2018

@jrgnicho This is ready for reviewing now.

@rhaschke rhaschke requested a review from jrgnicho Dec 31, 2018

@rhaschke

This comment has been minimized.

Copy link
Collaborator Author

commented Jan 2, 2019

@jrgnicho: The test getNearestIKSolution checks that getPositionIK() returns the single closest solution to the seed, compared to the multiple-solution method getPositionIK(). However, as I pointed out in #1285, the semantics of these methods is not yet well defined.

@rhaschke rhaschke force-pushed the ubi-agni:kinematics-tests branch 3 times, most recently from 61abe24 to bfabb75 Jan 3, 2019

rhaschke added some commits Dec 14, 2018

speedup + cleanup
- read parameters only once and share them between all tests
- reduce code duplication, introducing expectNear()
- remove timing (done by gtest as well)
- simplify getNearestIKSolution()

@rhaschke rhaschke force-pushed the ubi-agni:kinematics-tests branch from 42a0fb4 to a967df4 Jan 14, 2019

@rhaschke

This comment has been minimized.

Copy link
Collaborator Author

commented Jan 14, 2019

@jrgnicho @davetcoleman I addressed your comments. Please have another look.

@rhaschke rhaschke force-pushed the ubi-agni:kinematics-tests branch from a967df4 to afcf6b7 Jan 14, 2019

@jrgnicho

This comment has been minimized.

Copy link
Contributor

commented Jan 15, 2019

Other than the linking error it looks good to me.

@jrgnicho

This comment has been minimized.

Copy link
Contributor

commented Jan 21, 2019

The linking issue seems to be caused in the linking step to libmoveit_test_utils

@rhaschke

This comment has been minimized.

Copy link
Collaborator Author

commented Jan 21, 2019

The linking issue has the following underlying reason:

  • libmoveit_test_utils uses gtest functions and thus requires GTEST_LIBRARIES.
  • Since 2525a54 I do explicitly link against GTEST_LIBRARIES. But the gtest lib is only locally available within the current package, i.e. moveit_core.
  • Any binary, here moveit_occupancy_map_server, usually links against catkin_LIBRARIES, which also includes libmoveit_test_utils if that's required for testing.
  • While g++ doesn't actually link libraries that are not needed by the executable, clang attempts to link each and every library listed, which is a waste of resources and causes the issue, because GTEST_LIBRARIES are not found from another package than moveit_core.

I searched for a while how to convince clang to link as lazy as gcc does. No solution so far.
I'm afraid we cannot move the testing functions into libmoveit_test_utils, but we need to revert those changes.

@rhaschke rhaschke force-pushed the ubi-agni:kinematics-tests branch from 2525a54 to 44ad02d Jan 22, 2019

@rhaschke

This comment has been minimized.

Copy link
Collaborator Author

commented Jan 22, 2019

I reverted the last changes (moving some testing functions to libmoveit_test_utils.

@rhaschke rhaschke force-pushed the ubi-agni:kinematics-tests branch from 44ad02d to b4d247e Jan 23, 2019

@rhaschke rhaschke force-pushed the ubi-agni:kinematics-tests branch from b4d247e to bed7cc2 Jan 23, 2019

@rhaschke rhaschke merged commit bed7cc2 into ros-planning:melodic-devel Jan 23, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

rhaschke added a commit that referenced this pull request Jan 23, 2019

@rhaschke rhaschke deleted the ubi-agni:kinematics-tests branch Jan 23, 2019

@davetcoleman
Copy link
Member

left a comment

You never officially got an approval for this PR. I understand we were taking longer than you'd prefer to review, but notably I was busy in China representing MoveIt!. Please ping us more next time rather than breaking our review policy


public:
testing::AssertionResult isNear(const char* expr1, const char* expr2, const char* abs_error_expr,
const geometry_msgs::Point& val1, const geometry_msgs::Point& val2, double abs_error)

This comment has been minimized.

Copy link
@davetcoleman

davetcoleman Jan 23, 2019

Member

Thanks for trying this.

Why can't we install gtest? Testing is important to software and shouldn't be held back due to the desire to reduce dependencies

@rhaschke

This comment has been minimized.

Copy link
Collaborator Author

commented Jan 24, 2019

I considered Jorge's comment in #1272 (comment) as an approval and I pinged you in #1272 (comment).
Sorry, but I wanted to progress with some pending feature branches that are built on top of this.

@rhaschke

This comment has been minimized.

Copy link
Collaborator Author

commented Jan 24, 2019

Why can't we install gtest?

Because it's only for testing. This shouldn't be installed. Actually, it's Google's philosophy that each and every software package builds gtest on its own from source. If it would be installed, you could even trigger conflicts with other source packages.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.