Fix example of add_code_coverage() in README.md

[leunMar]

Only a minor fix, but it helps a lot.

[mikeferguson]

So this is clearly right -- but it breaks things slightly on my system. Are you using catkin_make or catkin tools?

I'm using catkin_make, and it barfs because it runs the cleanup() later and so my .gcda files seem to be deleted... I found the following change made things work again:

diff --git a/cmake/Modules/CodeCoverage.cmake b/cmake/Modules/CodeCoverage.cmake
index e6986e0..5d2c76b 100644
--- a/cmake/Modules/CodeCoverage.cmake
+++ b/cmake/Modules/CodeCoverage.cmake
@@ -132,7 +132,6 @@ function(ADD_CODE_COVERAGE)
         # Create baseline to make sure untouched files show up in the report
         COMMAND ${LCOV_PATH} -c -i -d . -o ${Coverage_NAME}.base
         WORKING_DIRECTORY ${PROJECT_BINARY_DIR}
-        DEPENDS ${Coverage_DEPENDENCIES}
         COMMENT "Resetting code coverage counters to zero."
     )
 
@@ -147,7 +146,7 @@ function(ADD_CODE_COVERAGE)
         COMMAND ${CMAKE_COMMAND} -E remove ${Coverage_NAME}.base ${Coverage_NAME}.total
 
         WORKING_DIRECTORY ${PROJECT_BINARY_DIR}
-        DEPENDS ${Coverage_NAME}_cleanup
+        DEPENDS ${Coverage_NAME}_cleanup ${Coverage_DEPENDENCIES}
         DEPENDS _run_tests_${PROJECT_NAME}
         COMMENT "Processing code coverage counters and generating report."
     )

[leunMar]

I used catkin_make. I just stumbled upon it because I used DEPENDSand it didn't build the dependencies first.
Using a header-only package, I require the tests to be built before the baseline is created. I could achieve that by specifying DEPENDENCIES tests as described here:

code_coverage/cmake/Modules/CodeCoverage.cmake

Line 111 in ad76d74

# DEPENDENCIES testrunner # Dependencies to build first

However, with the your change this would not be solved.

[mikeferguson]

Ok, I'm going to merge this -- but then also open a PR to put in a slightly modified version of the other change I mention. The reasoning here (for anyone who stumbles upon this):

• if our "cleanup" command depends on "tests" - it will be run quite late in the process - which, on several of my packages appears to cause the cleanup to run AFTER the tests are done running. There is no need for cleanup to depend on tests though - since all it does is remove the old files before we re-run the tests.
• adding the dependency to the main "coverage" command will still make sure that dependencies are built before we actually do the code coverage stuff (it doesn't make any guarantees about the running of the tests).
• unfortunately, we can't absolutely guarantee that "test" will be finished before "run_tests_PACKAGE_NAME" - but it appears based on discussions in run_tests_TARGET dependency on tests ros/catkin#850 that we can be sure the test targets we actually will end up running will be built. There also appears to be no mechanism in cmake to add a dependency to a custom command after it is created. My takeaway here is that we really don't need to add a dependency on "tests" anyways, since we should already get that from the dependency on run_tests_{PACKAGE_NAME}.
• There is also no guarantee that cleanup actually runs before the tests -- we're all just getting very lucky. I'll take it for now.

[leunMar]

First of all, thanks for your comments.

My takeaway here is that we really don't need to add a dependency on "tests" anyway

This is good news. Imho anyone who does not need (or want) the cleanup command to depend on the tests target, can simply omit it in the invocation of add_code_coverage, right?

There is no need for cleanup to depend on tests though - since all it does is remove the old files before we re-run the tests.

The cleanup comand also creates the "baseline" tracefile and that is why I need the tests to be built. So I would suggest to keep the code as it is and remove the line from the ReadMe completely.