search

ros-controls / ros_controllers

Generic robotic controllers to accompany ros_control
http://wiki.ros.org/ros_control
BSD 3-Clause "New" or "Revised" License
562 stars 524 forks source link

Separate tests into dedicated packages #453

Open matthew-reynolds opened 4 years ago

matthew-reynolds commented 4 years ago

I'm proposing splitting out our tests into dedicated packages. These packages will live in the repo, but will not be part of the metapackage, and will not be released. This is similar to ros_comm.

The rationale for this change is to pull out the heavy test dependencies from the metapackage. In particular, the metapackage currently transitively depends on gazebo_ros, since it is used by tests in ackerman_steering_controller, diff_drive_controller, and four_wheel_steering_controller.

Would appreciate feedback before I open a PR. Also curious on your thoughts on naming of the test packages (In ros_control, our test packages are suffixed with _tests but were originally designed with a different intention. In ros_comm, they are prefixed test_).

bmagyar commented 4 years ago

I am happy with either approach, would probably be better to follow ros_comms example. What's your take on this @ipa-mdl?

mathias-luedtke commented 4 years ago

IMHO tests belong with the tested code.

The existing test packages have been created to not expose the test plugins.

mathias-luedtke commented 4 years ago

This is similar to ros_comm.

The ros_comm test packages create a lot of test message and service definitions, I guess that its why they were moved out (8 years ago).

but will not be part of the metapackage, and will not be released

I am okay with removing them from the metapackage (who uses them anyway?). But we could still release them, in case others want to use the test plugins in their tests.

In particular, the metapackage currently transitively depends on gazebo_ros

The released metapackage does not depend on gazebo_ros.

matthew-reynolds commented 4 years ago

IMHO tests belong with the tested code

I would agree, but I think the cost of pulling in all these heavy dependencies outweighs the preference of putting the tests alongside the tested code.

But we could still release them

Good point.

The released metapackage does not depend on gazebo_ros

This is not true - On Melodic, rosinstall_generator --deps ros_controllers lists gazebo_dev, gazebo_msgs, and gazebo_ros as recursive dependencies, since they are required by ackermann_steering_controller. These can also be seen using rospack or rqt_dep.