Enable ament_lint tests

[tylerjw]

Signed-off-by: Tyler Weaver [email protected]

Description

This enables the default set of linters in ament_lint_common. It selectively disables all the tests that do not currently pass. This should make it easy for people to fix the issues found by these linters by removing the one line and running the test to see what they need to fix. This PR enables over 200 lint tests that already pass.

These could create excellent first-timer-only issues or moveit wold day issues.

[codecov]

Codecov Report

Merging #340 (7e02ffa) into main (bbe859e) will decrease coverage by 0.16%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main     #340      +/-   ##
==========================================
- Coverage   48.05%   47.90%   -0.15%     
==========================================
  Files         156      157       +1     
  Lines       14941    14874      -67     
==========================================
- Hits         7179     7124      -55     
+ Misses       7762     7750      -12     

Impacted Files	Coverage Δ
...kinematic_constraints/src/kinematic_constraint.cpp	72.55% <0.00%> (-1.26%)	⬇️
moveit_core/utils/src/robot_model_test_utils.cpp	73.04% <0.00%> (-0.99%)	⬇️
moveit_ros/moveit_servo/src/servo_calcs.cpp	77.39% <0.00%> (-0.83%)	⬇️
...it_servo/include/moveit_servo/servo_parameters.cpp	60.83% <0.00%> (-0.39%)	⬇️
...bot_state/include/moveit/robot_state/robot_state.h	88.47% <0.00%> (-0.26%)	⬇️
moveit_core/robot_model/src/joint_model_group.cpp	53.40% <0.00%> (-0.14%)	⬇️
...del/include/moveit/robot_model/joint_model_group.h	100.00% <0.00%> (ø)
...lude/moveit/collision_detection/collision_common.h	75.76% <0.00%> (ø)
...os/moveit_servo/include/moveit_servo/servo_calcs.h	100.00% <0.00%> (ø)
moveit_core/robot_state/src/robot_state.cpp	34.07% <0.00%> (+0.46%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 44e9487...13ccf4a. Read the comment docs.

[henningkayser]

Can't we simply set and enable all tests in moveit_common only, possibly providing an extra macro? That way we don't have linters enabled all over the place and we can enforce what checks need to pass in all MoveIt packages. Also, what do you think about using a different build flag than BUILD_TESTING so that we can separate the CI stages?

[tylerjw]

Using the colcon mixin system when linters are enabled this way there is an explicit way to run only linters or only non-linters.

Here is the documentation for how to enable colcon mixin: https://github.com/colcon/colcon-mixin-repository

When running tests with colon you can then only linters:

colcon test --mixin linters-only

Skip running linters:

colcon test --mixin linters-skip

[tylerjw]

Can't we simply set and enable all tests in moveit_common only

I tried this and I couldn't figure out how to get them to propagate. I really wish there was a way to specify linters per-repo instead of per-ros-package. I asked about this here: colcon/colcon-core#421 to see if I could instead find some other explicit per-repo way to specify them.

The other issues is that even if we get the cmake stuff handled with a macro there is the issue of the package.xml. The ament_lint system specifically enables linters based on which ones the package <test_depends> on. This still has to be done per-package sadly.

The other thing is that even if we figured out how to enable them for all packages from moveit_common, until all packages pass these linters we need a way to exclude them per-package.

[tylerjw]

One thing you'll notice is that I am not enabling clang-tidy or clang-format this way. Eventually, I'd like to use the ament system for those linters as well. However, there are existing issues with the ament scripts for how we would like to use those linters. I have gotten some PRs merged to resolve some of those issues but they are not 100% there. Specifically, these are the issues that remain:

clang-tidy

1. Execution time is really long to run this linter so in our CI system we have a workaround where we run it only on packages that have changed in the given PR. I have a colcon/ament solution to this here: https://github.com/tylerjw/moveit_ci_tools While the --packages-select arg PR got merged there is another issue.
2. clang-tidy-10 is not supported by ament_clang_tidy. The issue is the regex for building the xunit files only works with clang-tidy-6. I am locally just modifying ament_clang_tidy to use clang-tidy-10 for my local testing but before we can use this method we would that regex fixed. I just haven't had time to work on that yet.
3. The config file. We currently have a different .clang-tidy config from what is used by ROS2 itself. I think eventually it'd be really nice to standardize moveit2 to use the same clang-tidy config as ROS2.

clang-format

1. There was an issue with using clang-format-10 and I got it fixed in a PR. One thing to note is that until ament_lint is released if we wanted to use this we'd need to depend on the source of ament_lint.
2. The config file, ROS2 (and therefore ament_clang_format) uses a different config file (partially because they are using clang-6 vs 10). There is a solution to use our config file, it would just require me to write some CMake code for moveit_common and I haven't gotten around to doing that yet.

Long term I'd like the CI system to lint packages changed by a PR and run tests on those packages, and the dependencies of those packages. This could greatly speed up testing in CI. The testing part could impact how code coverage reporting works and I haven't thought of a fix for that yet.

Lastly, it would be really nice if there was an easy way for people to locally run tests and linters on packages they have changed. The colcon_list_packages_changed script I created and linked to is my current thoughts on that. Eventually, I'd like to contribute this to be a first-class feature of colcon.

[henningkayser]

This is awesome! Somehow, I ran into an issue where moveit_ros_planning_interface fails to find MONGODB when compiling. Not sure how this could possibly be related to this PR, though.

[JafarAbdi]

I fixed a conflict, will merge this once CI pass

[tylerjw]

There is something wrong with the dependencies of the latest released ament_lint_auto:

--- stderr: moveit_ros_planning_interface                                   
CMake Error at /home/ubuntu/code/ws_moveit2/install/warehouse_ros_mongo/share/warehouse_ros_mongo/cmake/ament_cmake_export_dependencies-extras.cmake:21 (find_package):
  By not providing "FindMONGODB.cmake" in CMAKE_MODULE_PATH this project has
  asked CMake to find a package configuration file provided by "MONGODB", but
  CMake did not find one.

  Could not find a package configuration file provided by "MONGODB" with any
  of the following names:

    MONGODBConfig.cmake
    mongodb-config.cmake

  Add the installation prefix of "MONGODB" to CMAKE_PREFIX_PATH or set
  "MONGODB_DIR" to a directory containing one of the above files.  If
  "MONGODB" provides a separate development package or SDK, be sure it has
  been installed.
Call Stack (most recent call first):
  /home/ubuntu/code/ws_moveit2/install/warehouse_ros_mongo/share/warehouse_ros_mongo/cmake/warehouse_ros_mongoConfig.cmake:41 (include)
  /opt/ros/foxy/share/ament_lint_auto/cmake/ament_lint_auto_find_test_dependencies.cmake:36 (find_package)
  CMakeLists.txt:125 (ament_lint_auto_find_test_dependencies)