search

colcon / colcon-cmake

Extension for colcon to support CMake packages
http://colcon.readthedocs.io
Apache License 2.0
16 stars 26 forks source link

use rc 0 when CTest has failing tests #3

Closed dirk-thomas closed 6 years ago

dirk-thomas commented 6 years ago

While I couldn't find any documentation about it CTest returns 8 when tests are failing. Since tests failures should not be considered an "error" in terms of invoking the test verb that case is handled differently. This unifies the return value semantic across other types.

Before the failing bridge test made the build fail: https://ci.ros2.org/view/colcon/job/colcon_ci_packaging_linux-aarch64/5/ After the failing bridge test make the build only unstable: https://ci.ros2.org/view/colcon/job/colcon_ci_packaging_linux-aarch64/7/

mikaelarguedas commented 6 years ago

For future reference: there is no documentation about ctest return codes anywhere. This is ticketed at https://gitlab.kitware.com/cmake/cmake/issues/16151

mikaelarguedas commented 6 years ago

I remember (couldnt find the discussion so far) user requests in the past to have the option of making builds fail on test failure. Can this override of the return code be made optional so that this can be toggled when invoking colcon test?

dirk-thomas commented 6 years ago

I remember (couldnt find the discussion so far) user requests in the past to have the option of making builds fail on test failure. Can this override of the return code be made optional so that this can be toggled when invoking colcon test?

I guess you are remember this discussion: https://github.com/ros-infrastructure/ros_buildfarm/issues/367#issuecomment-279564316

At the time we settled on using the test result verb to get an error code for failing tests.

mikaelarguedas commented 6 years ago

Thanks for the pointer, yeah I think that;s the discussion I was thinking of.

In this case "colcon test" isn't able to perform the build or install steps, is that right ? So having an opt-in option on the "test" verb to return an error code on test failure may make more sense here as there is less ambiguity about what failed

dirk-thomas commented 6 years ago

In this case "colcon test" isn't able to perform the build or install steps, is that right ?

Correct, build builds and installs, test only runs the tests.

So having an opt-in option on the "test" verb to return an error code on test failure may make more sense here as there is less ambiguity about what failed

Sure, such an option can be added as a new feature.

I would keep that separate from this patch though. This patch addresses the problem which currently fails our CI builds and is therefore blocking the deployment of colcon on CI.

mikaelarguedas commented 6 years ago

Feature addition ticketed at https://github.com/colcon/colcon-core/issues/21