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

Separate tests into dedicated packages #453

Open
matthew-reynolds opened this issue Feb 5, 2020 · 4 comments
Open

Separate tests into dedicated packages #453

matthew-reynolds opened this issue Feb 5, 2020 · 4 comments

Comments

@matthew-reynolds
Copy link
Member

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
Copy link
Member

bmagyar commented Feb 7, 2020

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
Copy link
Contributor

IMHO tests belong with the tested code.

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

@mathias-luedtke
Copy link
Contributor

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
Copy link
Member Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants