Skip to content

Conversation

Smona
Copy link

@Smona Smona commented Jun 27, 2023

Most ROS packages now allow disabling test runs by setting CATKIN_ENABLE_TESTING to false.

When I try to build laser_scan_matcher with CATKIN_ENABLE_TESTING set to false, I get this error:

CMake Error at /nix/store/y2dbjs0hpkdmra1jcnvih069nq27j9ka-ros-noetic-catkin-0.8.10-r1/share/catkin/cmake/test/tests.cmake:18 (message):
  () is not available when tests are not enabled.  The CMake code should only
  use it inside a conditional block which checks that testing is enabled:

  if(CATKIN_ENABLE_TESTING)

    (...)

  endif()

Call Stack (most recent call first):
  CMakeLists.txt:71 (add_rostest)

This PR just uses the recommended method to disable tests when the user requests it.

Copy link
Collaborator

@130s 130s left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Making the test as an option makes sense to me. For developers' usecases they may not always want to run tests.
    • I found rostest.add_rostest is a very strict so that can be even more strong motivation to disable the tests.
  • I just merged CI change #88 (it was opened a while ago but had been unmerged) into ros1 branch, the same branch this PR is aiming to. Would @Smona you mind enabling the tests in the CI config?

@130s
Copy link
Collaborator

130s commented Jun 27, 2023

Also do you mind @Smona cherry-picking the same changes into ros2 branch? Nevermind if you do.

@Smona
Copy link
Author

Smona commented Jul 5, 2023

Great, glad you are interested in merging this!

I've create a ros2 branch rebase of this in #91. I'm not sure what test configuration you're asking for, since my understanding is that CATKIN_ENABLE_TESTING is set to true by default.

@Smona Smona force-pushed the fix/allow-disabling-tests branch from 89300b8 to 018d67a Compare September 9, 2023 01:15
@Smona
Copy link
Author

Smona commented Oct 16, 2023

@130s i've merged in the CI check, but I think you need to approve the run.

Copy link
Collaborator

@130s 130s left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for catching this up! Approving.
I'm not sure why CI has not run for this PR though, will see if it runs post-merge.

@130s 130s merged commit fc76e85 into CCNYRoboticsLab:ros1 Oct 17, 2023
@Smona Smona deleted the fix/allow-disabling-tests branch October 17, 2023 20:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants