catkin / catkin_tools

Command line tools for working with catkin
Apache License 2.0
163 stars 146 forks source link

Add `catkin test` verb #676

Closed timonegk closed 3 years ago

timonegk commented 3 years ago

This pull request adds the long-awaited catkin test verb.

Summary of the changes

The test verb aims to replace the current aliases for test and run_tests. Currently, calling catkin test does the following:

Things to do before the merge

Test it

Please test the new verb on your setup and report any problems or requests in this thread! You can install this version with the command pip3 install git+https://github.com/catkin/catkin_tools.git@catkin_test. If the installation was successful, catkin --version should give the version 0.8.0.

mikepurvis commented 3 years ago

Nice work— seems reasonable to me. I tried some time ago but attempted that more ambitious path of trying to reach inside packages and then run individual test targets under catkin's parallelism:

https://github.com/mikepurvis/catkin_tools_test/blob/master/catkin_tools_test/test.py

Unsurprisingly, this was a total failure; your more restrained approach is the right way to go.

ftsell commented 3 years ago

I cannot attest to the complete correctness or code styles but I have tested this PR locally on some packages with different options and it works very well for me.

Axel13fr commented 3 years ago

Cheers @timonegk, I've used it on a couple of packages and that looked good to me. Tried with -i, -p options on catkin ROS packages with gtest and rostests.

asherikov commented 3 years ago

I think --make-args is necessary for this verb still, for example in order to pass ctest parameters which are accepted by cmake test target via ARGS: make test ARGS="--output-log a.log"

timonegk commented 3 years ago

Thanks for all of the positive feedback! I just added the --make-args option, maybe you could test if it works correctly @asherikov. I would like to open a new issue for the last point that is still unchecked (making the output directory configurable) to be done later and get this merged soon, as it is already a huge improvement over the existing state.

asherikov commented 3 years ago

Thanks,

LucasHaug commented 3 years ago

Tested here and works very well!! Congrats

timonegk commented 3 years ago

@asherikov, I fixed your issue. I am going to merge this now.