search

ros-industrial-consortium / descartes

ROS-Industrial Special Project: Cartesian Path Planner
Apache License 2.0
126 stars 92 forks source link

Install Space Issues and Testing #198

Closed Jmeyer1292 closed 7 years ago

Jmeyer1292 commented 7 years ago

So the descartes_trajectory library defines a few "helper" classes meant solely for testing. These are exported via the catkin_package command, but were not installed (presumably because these test artifacts don't need to be in the final product). These include a cartesian_robot header and class that is used in descartes_trajectory, descartes_planners, and descartes_moveit.

For the moment, I'm just turning off testing in the install space. Long term though, I assume that I will need to either:

@gavanderhoorn @130s Do you have any advice? What's the correct way to share headers and libraries solely meant for testing between packages?

130s commented 7 years ago

Interesting!

turning off testing in the install space.

With industrial_ci more proper way to do so might be to set NOT_TEST_INSTALL=true (doc).

  • Make a descartes_tests package so the testing specific items can be isolated (and not installed).
  • Add install rules for the headers and libraries needed specifically for testing and find a way to link against them. I could simply add a test subfolder to each package's include directory.

Isolating tests reminds me of a discussion in ROS Testing, Continuous Integration (CI), and Deployment (video after 58'20) where using "mocking" might be one ideal way for that (I've always been willing to give a try on gmock (C++) or unittest.mock (Python) and contribute tutorial for it).

IMO creating *_tests package has an advantage since other packages can re-use the tests in it, and is flexible where other packages don't need to care if the dependency chain is appropriate or not. It may be more costly to maintain since it's a package, not just test resources in an existing package.

Another way, which I often do, is to export as much tests as possible. With this dependency can become an issue as I just mentioned, but works fine as long as you don't need to care about dependency. I'm not if there is any other significant downside with this approach.

HTH!

p.s. This is such an interesting topic to me. I'd be much interested if discussed on discourse.ros.org or somewhere.

Jmeyer1292 commented 7 years ago

So I changed my command from the no-install command I had before to NOT_TEST_INSTALL=true and I get failing builds because the test header was not installed.

See here.

130s commented 7 years ago

I found an issue in industrial_ci. Will get back once it's resolved. Sorry for inconvenience. If this is urgent then you could use "no install" as this PR originally was using.

Jmeyer1292 commented 7 years ago

@130s It's not urgent at all. I just want to do things "right", and I appreciate your help in figuring out what that looks like.

mathias-luedtke commented 7 years ago

(@Jmeyer1292 wrote:)

So the descartes_trajectory library defines a few "helper" classes meant solely for testing. These are exported via the catkin_package command, but were not installed (presumably because these test artifacts don't need to be in the final product)

This is the same use case as https://github.com/catkin/catkin_tools/issues/427, and it is not supported by catkin-tools.

What's the correct way to share headers and libraries solely meant for testing between packages?

I would go for a descartes_tests package as well.

Make a descartes_tests package so the testing specific items can be isolated (and not installed).

IMHO it is: make a descartes_tests AND install your artifacts. descartes_tests

(@130s wrote:)

With industrial_ci more proper way to do so might be to set NOT_TEST_INSTALL=true

No, NOT_TEST_INSTALL just turns off these rather obscure tests of *.test files in install space. fb4e321 if the correct way to turn off the install space at the moment. (A CATKIN_CONFIG option would be preferable)

Jmeyer1292 commented 7 years ago

Thank you for your input @130s and @ipa-mdl.

Jmeyer1292 commented 7 years ago

I'm going to merge this --no-install change as a temporary fix. My next PR against this will then by a refactoring of the test code, probably into its own package wherever it make sense (where resources are shared).

I will resist the temptation to blow all of the tests away.

130s commented 7 years ago

Now you can set CATKIN_CONFIG='--no-install' (a new feature added in https://github.com/ros-industrial/industrial_ci/pull/177).