Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Coverage #682

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Coverage #682

wants to merge 4 commits into from

Conversation

marcusmueller
Copy link
Member

This adds support for Coverage reporting.

Requirements:

  • gcc or clang
  • gcov
  • gcovr

Usage:

mkdir build-coverage && cd build-coverage
cmake -DCMAKE_BUILD_TYPE=Coverage ..
make -j
make coverage
firefox coverage/index.html

Demo of the result on my oldish desktop machine: https://doppler.inpha.se/coverage/

From https://github.com/bilke/cmake-modules commit 877bab9dd1b17468c5d939cacaa2ad7ba99d1977

Signed-off-by: Marcus Müller <[email protected]>
Requirements:

- gcc or clang
- gcov
- gcovr

Usage:

```shell
mkdir build-coverage && cd build-coverage
cmake -DCMAKE_BUILD_TYPE=Coverage ..
make -j
make coverage
firefox coverage/index.html
```

Signed-off-by: Marcus Müller <[email protected]>
This makes clang-16 builds possible.

Signed-off-by: Marcus Müller <[email protected]>
@marcusmueller
Copy link
Member Author

The patch to CodeCoverage.cmake is something I'm trying to upstream: bilke/cmake-modules#82

Copy link
Contributor

@jdemel jdemel left a comment

Choose a reason for hiding this comment

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

LGTM. It's unfortunate that we can't include thie CodeCoverage.cmake but need to copy it.

Could you add a reference to the origin of the CodeCoverage.cmake file? This might make updates easier.

@marcusmueller
Copy link
Member Author

@jdemel done, but you won't be updating this anytime soon.

@jdemel
Copy link
Contributor

jdemel commented Nov 7, 2023

@jdemel done, but you won't be updating this anytime soon.

That is actually a good reason to put the info in there.

Copy link
Member

@noc0lour noc0lour left a comment

Choose a reason for hiding this comment

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

The direct HTML output is nice, but actually the regular coverage info can be post-processed with tools to give you HTML as you wish. Therefore it would be better if we would produce regular coverage output and not HTML.

Would it be possible for you to add a github workflow which will run and produce codecov artifacts and uploads them with codecov-action so we can hook it into codecov.io?

########################################################################
# Setup Coverage
########################################################################
if(${CMAKE_BUILD_TYPE} STREQUAL "Coverage")
Copy link
Member

Choose a reason for hiding this comment

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

With the CodeCoverage.cmake setting the Build_Type to anything other than Debug results in a warning. Either removing that check from there or leaving the CMAKE_BUILD_TYPE alone and just adding a switch to enable coverage would be an option.

Copy link
Member Author

Choose a reason for hiding this comment

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

this is honestly just a mirror of what GR does; looking back, yes, having extra flags instead of a build type of its own would have been a worthwhile option, but I foreseeable won't have time to implement that.

# modifications as done in the past for GNU Radio.
include(CodeCoverage)
append_coverage_compiler_flags()
setup_target_for_coverage_gcovr_html(
Copy link
Member

Choose a reason for hiding this comment

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

Why only html output and why gcovr? IIRC lcov is the more mature and complete tool. Allowing regular coverage output we can hook that into codecov.io pipelines and add information to pullrequests and pushes to track coverage as we go.

Copy link
Member Author

Choose a reason for hiding this comment

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

gcov vs lcov:

TBH, because I looked into lcov first, couldn't make it work, and then resorted to doing what was possible within limited time :)

html:

because I wanted this to be immediately useful for insights, and hence jumped on a minimum effort way of getting readable output. Proper CI integration, including things like complaining when you add code that is specifically not well-covered, is something that needs to come – but having this seemed better than not having it.

Copy link
Member Author

Choose a reason for hiding this comment

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

gcov vs lcov:

TBH, because I looked into lcov first, couldn't make it work, and then resorted to doing what was possible within limited time :)

html:

because I wanted this to be immediately useful for insights, and hence jumped on a minimum effort way of getting readable output. Proper CI integration, including things like complaining when you add code that is specifically not well-covered, is something that needs to come – but having this seemed better than not having it.

@noc0lour
Copy link
Member

noc0lour commented Jan 2, 2024

@marcusmueller If you don't mind I'll have a crack at it to make it useful for user output & ingestion into statistics/CI pipeline. Because with the way it is right now it will not be immediately useful to integrate...

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.

3 participants