bilke / cmake-modules

Additional CMake functionality. Most of the modules are from Ryan Pavlik (https://github.com/rpavlik/cmake-modules)
Boost Software License 1.0
547 stars 214 forks source link

CodeCoverage: Some enhancements to reporting, tooling #39

Closed ferdnyc closed 4 years ago

ferdnyc commented 4 years ago

This adds a few enhancements to CodeCoverage.cmake that address issues I hit while integrating it into our build tree. Specifically:

That last one makes me feel a bit better about the tool's housekeeping. Though TBH, I'd really love to be able to set the gcov output files as GENERATED so that CMake will clean those up as well. I just can't seem to find a reasonable way to do that, without resorting to ugly globbing or the like.

ferdnyc commented 4 years ago

Turns out you can't use generator expressions in regular set() commands, nor in the COMMAND argument of an add_custom_target(). Replaced with traditional if() logic.

TomzBench commented 4 years ago

Does this fix COVERAGE_LCOV_EXCLUDES?

ferdnyc commented 4 years ago

@TomzBench Not sure what you mean by "fix", sorry, because with this change I haven't had any need to use manual excludes. It might affect how they're processed, but I haven't tested it.

TomzBench commented 4 years ago

@ferdnyc I would like to exclude some directories from my tests however the exclude function doesn't seem to work. For example:

APPEND_COVERAGE_COMPILER_FLAGS()
set(COVERAGE_LCOV_EXCLUDES 'examples/*')
SETUP_TARGET_FOR_COVERAGE_LCOV(...)

My reports show 0% test coverage for my "examples". I also have some files in my src folder I would like to exclude as I copied them from other projects and they have their own test's

ferdnyc commented 4 years ago

Ah, gotcha.

First, my setup: I'm building in a build directory inside a cloned repo root, with source files located in (relative to the build directory) ../src/, ../include/, and ../tests/.

If I specify a subdirectory of the source tree by full absolute path, then yes, COVERAGE_LCOV_EXCLUDES will correctly exclude the files specified.

So, if I'm building in /home/a/b/c/project/build/, to exclude files in ../src/somedir/ my ../CMakeLists.txt would include:

if (ENABLE_COVERAGE)
  set(COVERAGE_LCOV_EXCLUDES "/home/a/b/c/project/src/somedir/*")
  setup_target_for_coverage_lcov(
    NAME coverage
    EXECUTABLE test-coverage
    DEPENDENCIES test-coverage)
endif()

You can of course use "${PROJECT_SOURCE_DIR}/src/somedir/*" instead, to make the config more portable. Just as long as the value expands to a full path. Things like "src/somedir/*" or "../src/somedir/*" won't work, even with this change.

ferdnyc commented 4 years ago

Hmmm, but I didn't add a -b argument to the --remove command, let me see what happens when I do...

ferdnyc commented 4 years ago

Nope, no difference. Has to be absolute paths.

I suspect that worked even before this change, since I was already able to manually exclude external files by specifying their full path. It was just tedious because the wildcard exclusions weren't recursive, so I had to list every individual directory.

The reason for adding the -b argument is that it makes it possible to exclude all external files (system includes) by simply doing this:


if (ENABLE_COVERAGE)
  setup_target_for_coverage_lcov(
    NAME coverage
    LCOV_ARGS "--no-external"
    EXECUTABLE test-coverage
    DEPENDENCIES test-coverage)
endif()
TomzBench commented 4 years ago

Ah! Yes, the absolute path works for me. Much better now. I don't mind specifying full path. Just wasn't aware of that requirement. The instructions use relative paths! Maybe we can put that in the instruction steps.

ferdnyc commented 4 years ago

The instructions use relative paths! Maybe we can put that in the instruction steps.

Yeah, that seems sensible. Commit pushed to use ${PROJECT_SOURCE_DIR} in the introductory comment examples. (Actually, one of each — one uses ${PROJECT_SOURCE_DIR}, the other an explicit absolute path.)

And another to lowercase the function names, in accordance with modern CMake norms.

ferdnyc commented 4 years ago

I just pushed some pretty major enhancements to this PR, I'll update the initial comment with details.

bilke commented 4 years ago

It works on our project. Thanks a lot for this contribution!

TomzBench commented 4 years ago

Hello - one more suggestion!

The codecoverage script seems to delete the ${PROJECT_NAME}.info.cleaned cruft from the build directory. However this file is needed when using 3rd party pipline service like codecove or coveralls. So maybe it is a good idea to not remove that file?

Anyway very nice tool thank you.

ferdnyc commented 4 years ago

@TomzBench Is the .cleaned file needed by Coveralls? It's just the .info file with some data removed for the report (hence .info.cleaned), and the original, more-complete .info file is still there.

The deletion of the .base and .total files I could see as being an issue for Coveralls, though. Maybe those should be moved to BYPRODUCTS as well (so they'll be deleted by make clean), instead of forcibly deleting them immediately after the report is generated.

ferdnyc commented 4 years ago

(I mean, the .info.cleaned file can be made a BYPRODUCTS as well, if it truly is useful. I'm just surprised that it would be needed for anything.)

TomzBench commented 4 years ago

@ferdnyc sorry for delay.

Yes I use it for codecov. The cleaned file is needed if you use COVERAGE_LCOV_EXCLUDES. Locally on my machine my report is correct. But when I send the .info to codecov I get the report with out any filtering of source files...

ferdnyc commented 4 years ago

@TomzBench Ah, I see!

It looks like you can do the same filtering on the Codecov end using their .yaml file...

But I'd been meaning to file another PR on this anyway. (I missed the fact that the final output of the gcovr XML target was a single ${Coverage_NAME}.xml file, not a directory called ${Coverage_NAME}, so the cleanup commands for that target are wrong.)

Given the filtering, it makes sense to me that the .info.cleaned file should be preserved, either instead of or in addition to the unfiltered .info file. (Anyone have any preference on which?)

I'll add it in.