From f587c2c5e7026d7360859ea600164aed371763c8 Mon Sep 17 00:00:00 2001 From: Henry Moore Date: Fri, 4 Oct 2024 12:29:09 -0600 Subject: [PATCH] Add clang tidy to CI (#5) * Add clang tidy to CI --- .clang-tidy | 43 +++++++++---------- .github/workflows/ci.yaml | 25 +++++++++++ .../{pre-commit.yml => pre-commit.yaml} | 0 .github/workflows/ros2.yml | 15 ------- .gitignore | 3 ++ Jenkinsfile | 20 --------- .../test/launch_tests/test_parameters.cpp | 41 ++++++++++-------- run_clang_tidy.bash | 4 ++ 8 files changed, 77 insertions(+), 74 deletions(-) rename .github/workflows/{pre-commit.yml => pre-commit.yaml} (100%) delete mode 100644 .github/workflows/ros2.yml delete mode 100644 Jenkinsfile create mode 100755 run_clang_tidy.bash diff --git a/.clang-tidy b/.clang-tidy index 693d15f05..3d6d474cf 100644 --- a/.clang-tidy +++ b/.clang-tidy @@ -1,26 +1,25 @@ ---- -# This was copied over from the moveit_studio repository. -# -# TODO(henningkayser): Re-enable performance-unnecessary-value-param once #214 is resolved -Checks: '-*, - performance-*, - -performance-unnecessary-value-param, - llvm-namespace-comment, - modernize-redundant-void-arg, - modernize-use-nullptr, - modernize-use-default, - modernize-use-override, - modernize-loop-convert, - readability-braces-around-statements, - readability-named-parameter, - readability-redundant-smartptr-get, - readability-redundant-string-cstr, - readability-simplify-boolean-expr, - readability-container-size-empty, - readability-identifier-naming, - ' +Checks: -*, + bugprone-*, + clang-analyzer-*, + concurrency-*, + cppcoreguidelines-*, + google-*, + misc-*, + modernize-*, + performance-*, + portability-*, + readability-*, + -bugprone-easily-swappable-parameters, + -bugprone-exception-escape, + -cppcoreguidelines-avoid-magic-numbers, + -google-readability-casting, + -modernize-use-trailing-return-type, + -readability-identifier-length, + -readability-function-cognitive-complexity, + -readability-magic-numbers, + -readability-uppercase-literal-suffix, + -cppcoreguidelines-pro-type-vararg, HeaderFilterRegex: '' -AnalyzeTemporaryDtors: false CheckOptions: - key: llvm-namespace-comment.ShortNamespaceLines value: '10' diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index 03bdea8c8..e13deb86f 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -70,3 +70,28 @@ jobs: if: always() run: colcon test-result --verbose working-directory: /colcon_ws + + clang_tidy: + needs: + # Ensure the test job runs after the build job finishes instead of attempting to run in parallel + - build-ws + name: clang-tidy + runs-on: ubuntu-24.04 + container: + # Run on the Docker image we tagged and pushed to a private repo in the job above + image: ghcr.io/picknikrobotics/fuse:${{ github.run_id }} + steps: + - name: Changed Files + id: changed-cpp-files + uses: tj-actions/changed-files@v45.0.3 + with: + # Avoid using single or double quotes for multiline patterns + files: | + **.cpp + **.hpp + - run: run-clang-tidy -j $(nproc --all) -p build/ -export-fixes clang-tidy-fixes.yaml -config-file src/fuse/.clang-tidy ${{ steps.changed-cpp-files.outputs.all_changed_files }} + working-directory: /colcon_ws + - uses: asarium/clang-tidy-action@v1.0 + with: + fixesFile: /colcon_ws/clang-tidy-fixes.yaml + noFailOnIssue: false diff --git a/.github/workflows/pre-commit.yml b/.github/workflows/pre-commit.yaml similarity index 100% rename from .github/workflows/pre-commit.yml rename to .github/workflows/pre-commit.yaml diff --git a/.github/workflows/ros2.yml b/.github/workflows/ros2.yml deleted file mode 100644 index 02f231083..000000000 --- a/.github/workflows/ros2.yml +++ /dev/null @@ -1,15 +0,0 @@ -name: ros2 - -on: [push, pull_request] - -jobs: - industrial_ci: - strategy: - matrix: - env: - - {ROS_DISTRO: rolling, ROS_REPO: main} - runs-on: ubuntu-latest - steps: - - uses: actions/checkout@v1 - - uses: 'ros-industrial/industrial_ci@master' - env: ${{matrix.env}} diff --git a/.gitignore b/.gitignore index 60cf591f7..d613a68a4 100644 --- a/.gitignore +++ b/.gitignore @@ -33,3 +33,6 @@ # Python compiled bytecode files *.pyc + +# clang tidy fixes +clang-tidy-fixes.yaml diff --git a/Jenkinsfile b/Jenkinsfile deleted file mode 100644 index 06c18cd4a..000000000 --- a/Jenkinsfile +++ /dev/null @@ -1,20 +0,0 @@ -#!/usr/bin/env groovy -@Library('tailor-meta@0.1.24')_ -tailorTestPipeline( - // Name of job that generated this test definition. - rosdistro_job: '/ci/rosdistro/master', - // Distribution name - rosdistro_name: 'ros2', - // Release track to test branch against. - release_track: 'hotdog', - // Release label to pull test images from. - release_label: 'hotdog', - // OS distributions to test. - distributions: ['jammy'], - // Version of tailor_meta to build against - tailor_meta: '0.1.24', - // Master or release branch associated with this track - source_branch: 'rolling', - // Docker registry where test image is stored - docker_registry: 'https://084758475884.dkr.ecr.us-east-1.amazonaws.com/locus-tailor' -) diff --git a/fuse_core/test/launch_tests/test_parameters.cpp b/fuse_core/test/launch_tests/test_parameters.cpp index 17197b019..aff635707 100644 --- a/fuse_core/test/launch_tests/test_parameters.cpp +++ b/fuse_core/test/launch_tests/test_parameters.cpp @@ -33,12 +33,18 @@ */ #include -#include +#include +#include +#include +#include +#include +#include +#include #include #include #include -#include +#include "fuse_core/eigen.hpp" class TestParameters : public ::testing::Test { @@ -59,6 +65,7 @@ class TestParameters : public ::testing::Test executor_.reset(); } +private: std::thread spinner_; //!< Internal thread for spinning the executor rclcpp::executors::SingleThreadedExecutor::SharedPtr executor_; }; @@ -102,15 +109,15 @@ TEST_F(TestParameters, getPositiveParam) TEST_F(TestParameters, GetCovarianceDiagonalParam) { // Build expected covariance matrix: - constexpr int Size = 3; + constexpr int size = 3; constexpr double variance = 1.0e-3; - constexpr double default_variance = 0.0; + constexpr double defaultVariance = 0.0; fuse_core::Matrix3d expected_covariance = fuse_core::Matrix3d::Identity(); expected_covariance *= variance; fuse_core::Matrix3d default_covariance = fuse_core::Matrix3d::Identity(); - default_covariance *= default_variance; + default_covariance *= defaultVariance; auto node = rclcpp::Node::make_shared("test_parameters_node"); @@ -123,10 +130,10 @@ TEST_F(TestParameters, GetCovarianceDiagonalParam) try { - const auto covariance = fuse_core::getCovarianceDiagonalParam(*node, parameter_name, default_variance); + const auto covariance = fuse_core::getCovarianceDiagonalParam(*node, parameter_name, defaultVariance); - EXPECT_EQ(Size, covariance.rows()); - EXPECT_EQ(Size, covariance.cols()); + EXPECT_EQ(size, covariance.rows()); + EXPECT_EQ(size, covariance.cols()); EXPECT_EQ(expected_covariance.rows() * expected_covariance.cols(), expected_covariance.cwiseEqual(covariance).count()) @@ -148,10 +155,10 @@ TEST_F(TestParameters, GetCovarianceDiagonalParam) try { - const auto covariance = fuse_core::getCovarianceDiagonalParam(*node, parameter_name, default_variance); + const auto covariance = fuse_core::getCovarianceDiagonalParam(*node, parameter_name, defaultVariance); - EXPECT_EQ(Size, covariance.rows()); - EXPECT_EQ(Size, covariance.cols()); + EXPECT_EQ(size, covariance.rows()); + EXPECT_EQ(size, covariance.cols()); EXPECT_EQ(default_covariance.rows() * default_covariance.cols(), default_covariance.cwiseEqual(covariance).count()) << "Expected\n" @@ -170,7 +177,7 @@ TEST_F(TestParameters, GetCovarianceDiagonalParam) ASSERT_FALSE(node->has_parameter(parameter_name)); - EXPECT_THROW(fuse_core::getCovarianceDiagonalParam(*node, parameter_name, default_variance), + EXPECT_THROW(fuse_core::getCovarianceDiagonalParam(*node, parameter_name, defaultVariance), std::invalid_argument); } @@ -180,7 +187,7 @@ TEST_F(TestParameters, GetCovarianceDiagonalParam) ASSERT_FALSE(node->has_parameter(parameter_name)); - EXPECT_THROW(fuse_core::getCovarianceDiagonalParam(*node, parameter_name, default_variance), + EXPECT_THROW(fuse_core::getCovarianceDiagonalParam(*node, parameter_name, defaultVariance), std::invalid_argument); } @@ -190,7 +197,7 @@ TEST_F(TestParameters, GetCovarianceDiagonalParam) ASSERT_FALSE(node->has_parameter(parameter_name)); - EXPECT_THROW(fuse_core::getCovarianceDiagonalParam(*node, parameter_name, default_variance), + EXPECT_THROW(fuse_core::getCovarianceDiagonalParam(*node, parameter_name, defaultVariance), std::invalid_argument); } @@ -200,7 +207,7 @@ TEST_F(TestParameters, GetCovarianceDiagonalParam) const std::string parameter_name{ "covariance_diagonal_with_strings" }; ASSERT_FALSE(node->has_parameter(parameter_name)); - EXPECT_THROW(fuse_core::getCovarianceDiagonalParam(*node, parameter_name, default_variance), + EXPECT_THROW(fuse_core::getCovarianceDiagonalParam(*node, parameter_name, defaultVariance), rclcpp::exceptions::InvalidParameterTypeException); // NOTE(CH3): A covariance diagonal with invalid element type used to not throw, and used to @@ -217,7 +224,7 @@ TEST_F(TestParameters, GetCovarianceDiagonalParam) const std::string parameter_name{ "covariance_diagonal_with_string" }; ASSERT_FALSE(node->has_parameter(parameter_name)); - EXPECT_THROW(fuse_core::getCovarianceDiagonalParam(*node, parameter_name, default_variance), + EXPECT_THROW(fuse_core::getCovarianceDiagonalParam(*node, parameter_name, defaultVariance), rclcpp::exceptions::InvalidParameterTypeException); // NOTE(CH3): A covariance diagonal with invalid element type used to not throw, and used to @@ -234,7 +241,7 @@ int main(int argc, char** argv) { rclcpp::init(argc, argv); testing::InitGoogleTest(&argc, argv); - int ret = RUN_ALL_TESTS(); + int const ret = RUN_ALL_TESTS(); rclcpp::shutdown(); return ret; } diff --git a/run_clang_tidy.bash b/run_clang_tidy.bash new file mode 100755 index 000000000..9fb7598e8 --- /dev/null +++ b/run_clang_tidy.bash @@ -0,0 +1,4 @@ +#!/bin/bash + +# -j $(nproc --all) runs with all cores, but the prepended nice runs with a low priority so it won't make your computer unusable while clang tidy is going. The "$@" at the end passes all the filenames from pre-commit so it should only look for clang tidy fixes in the files you directly changed in the commit that is being checked. +nice run-clang-tidy -p ../../build_dbg -j $(nproc --all) -quiet -fix "$@"