Details

Reviewers

Group Reviewers

Restricted Project

Commits

rSTAGING100c22d068f4: [CMAKE] Add support for generating test coverage reports
rABC100c22d068f4: [CMAKE] Add support for generating test coverage reports

Summary

This is inspired by the autotools make cov feature, but is not
identical.

Main differences with core:

The way autotools coverage works requires the user to run the build

before calling the cov target, in order to build the baseline coverage
data. With cmake, the all target is built to regenerate the coverage
baseline data.

The coverage target is associated to the test target. So `ninja

coverage-check` build a coverage report for the check target, `ninja
coverage-check-functional` for the check-functional target, etc.
To get a global coverage, one can use ninja coverage-check-all or
ninja coverage-check-extended. It also avoids adding switch at build
time to run the extended tests (what autotools does).

Note: the -DUSE_COVERAGE=1 from core is not ported as it is no longer
in use since D5047.

For now only the association of lcov, gcov and genhtml is
supported (llvm-cov requires a wrapper to work as gcov).

Test Plan

cmake -GNinja .. -DCMAKE_BUILD_TYPE=Release -DENABLE_COVERAGE=ON

Should display a warning message: It is advised to not enforce CMAKE_BUILD_TYPE to get the best coverage results

cmake -GNinja .. \
  -DCMAKE_C_COMPILER=gcc \
  -DCMAKE_CXX_COMPILER=g++ \
  -DENABLE_COVERAGE=ON \
  -DENABLE_BRANCH_COVERAGE=ON
ninja coverage-check-all

Be patient, it takes a long time to complete.
Open the generated check-all.coverage/index.html and navigate through
the coverage report.

Try to run a few other coverage targets, like:

ninja coverage-check

ninja coverage-check-functional

With secp256k1 standalone:

cmake -GNinja .. \
  -DCMAKE_C_COMPILER=gcc \
  -DSECP256K1_ENABLE_COVERAGE=ON \
  -DSECP256K1_ENABLE_BRANCH_COVERAGE=ON
ninja coverage-check-secp256k1

Diff Detail

Repository

rABC Bitcoin ABC

Branch

cmake_coverage

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 10250
Build 18322: Default Diff Build & Tests
Build 18321: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

deadalnix added inline comments.Mar 19 2020, 00:16

src/secp256k1/CMakeLists.txt
335 ↗	(On Diff #16931)	I don't think that this is correct. You want VERIFY enabled for tests no matter if there is coverage or not. Is there something I'm missing?

This revision now requires changes to proceed.Mar 19 2020, 00:16

Yes in the end I require an option because I need to be able to branch on coverage at configure time (or at build time but on properties that don't support generator expressions yet).
I use the build type only as a convenience to set the optimization flags and make the result configuration agnostic. If you are uncomfortable with this solution I can override the C/CXX flags directly.

src/secp256k1/CMakeLists.txt
335 ↗	(On Diff #16931)	Honestly I don't know. This has been added in https://github.com/bitcoin-core/secp256k1/pull/428 but there is no comment on why this is required. My bet is that it will decrease coverage statistics when there is no failure, because the check failed branch is never covered. Note that I am missing the COVERAGE definition (will update), that undefines `VERIFY_CHECK` as well: from util.h /* Like assert(), but when VERIFY is defined, and side-effect safe. */ #if defined(COVERAGE) #define VERIFY_CHECK(check) #define VERIFY_SETUP(stmt) #elif defined(VERIFY) #define VERIFY_CHECK CHECK #define VERIFY_SETUP(stmt) do { stmt; } while(0) #else #define VERIFY_CHECK(cond) do { (void)(cond); } while(0) #define VERIFY_SETUP(stmt) #endif Not sure what to do here, should I let VERIFY as standard and have different result than autotools ?

Add the COVERAGE definition to secp256k1.

Build Bitcoin-ABC / Diffs / Diff Testing (build-diff) started.

Build Bitcoin-ABC / Diffs / Diff Testing (build-diff) passed.

Harbormaster completed remote builds in B9883: Diff 17034.Mar 19 2020, 10:44

deadalnix requested changes to this revision.Mar 19 2020, 13:24

deadalnix added inline comments.

CMakeLists.txt
83 ↗	(On Diff #17034)	Doesn't this interfere with secp256k1's coverage?
cmake/modules/TestSuite.cmake
31 ↗	(On Diff #17034)	Why not inconditionally include sooner? I'm not 100% but cmake may well be processing this again and again every time it is included.
src/secp256k1/CMakeLists.txt
28 ↗	(On Diff #17034)	Is there a reason not to use OverrideInitFlags ?
329 ↗	(On Diff #17034)	Add a comment explaining why.

This revision now requires changes to proceed.Mar 19 2020, 13:24

The diff is actually not the one I expected, I should have done something wrong. The previous one was correct though.

CMakeLists.txt
83 ↗	(On Diff #17034)	It does unless you run from the standalone secp256k1 project (which doesn't have this file), in which case you get secp256k1's coverage only which is what you want.
cmake/modules/TestSuite.cmake
31 ↗	(On Diff #17034)	Actually this is a bug, there should be no `if(CMAKE_BUILD_TYPE STREQUAL "Coverage")` anywhere. I should have failed my rebase somehow, will update.
src/secp256k1/CMakeLists.txt
28 ↗	(On Diff #17034)	secp256k1 doesn't use the override mechanism, as the initial reason for using it was removing the NDEBUG which is not an issue for secp256k1. That said I don't see any reason why it can't be done, but I'll better make it another diff.
329 ↗	(On Diff #17034)	Will do.

Add a comment as to why VERIFY shouldn't be defined with coverage.

Build Bitcoin-ABC / Diffs / Diff Testing (build-diff) started.

Build Bitcoin-ABC / Diffs / Diff Testing (build-diff) passed.

Harbormaster completed remote builds in B9884: Diff 17035.Mar 19 2020, 14:01

deadalnix requested changes to this revision.Mar 19 2020, 14:10

deadalnix added inline comments.

CMakeLists.txt
69 ↗	(On Diff #17035)	You decided this was a build type.
83 ↗	(On Diff #17035)	If it interfers, then it probably worth reconsidering.
doc/developer-notes.md
299 ↗	(On Diff #17035)	This seems to be out of date. Why disable CCACHE ?
src/secp256k1/CMakeLists.txt
9 ↗	(On Diff #17035)	You made the decision that Coverage was a build type. This is inconsistent with that decision.

This revision now requires changes to proceed.Mar 19 2020, 14:10

Fabien edited the test plan for this revision. (Show Details)Apr 13 2020, 11:46

Remove all reference to CMAKE_BUILD_TYPE.
Remove disabling ccache from the doc.
Include secp256k1 and seeder into coverage, as we run tests for them.

Build Bitcoin-ABC / Diffs / Diff Testing (build-diff) started.

Fabien edited the summary of this revision. (Show Details)Apr 13 2020, 12:09

Fabien added a parent revision: D5705: [CMAKE] Allow to override the optimization level.

Remove the OverrideInitFlags change, no longer needed.

Build Bitcoin-ABC / Diffs / Diff Testing (build-diff) started.

Harbormaster completed remote builds in B10183: Diff 18752.Apr 13 2020, 12:19

Build Bitcoin-ABC / Diffs / Diff Testing (build-diff) passed.

Harbormaster completed remote builds in B10184: Diff 18753.Apr 13 2020, 12:27

Will squash with D5705.

Fabien edited the summary of this revision. (Show Details)Apr 13 2020, 13:23

Fabien removed a parent revision: D5705: [CMAKE] Allow to override the optimization level.

Squashed D5705

Build Bitcoin-ABC / Diffs / Diff Testing (build-diff) started.

Build Bitcoin-ABC / Diffs / Diff Testing (build-diff) passed.

Harbormaster completed remote builds in B10186: Diff 18758.Apr 13 2020, 13:45

I think there is a problem with the way you handle the optimization, and various nits, but overall this looks solid.

CMakeLists.txt
77 ↗	(On Diff #18758)	See comment in libsecp256k1
src/secp256k1/CMakeLists.txt
27 ↗	(On Diff #18758)	glob is usually a bad idea due to dependency management. See for instance qt mocks that need to run for every build, because dependencies for them cannot be tracked.
30 ↗	(On Diff #18758)	This is a bit of a problem because if I ask an optimized build, I should get one. It's okay for this to change defaults, but not superimpose user settings.
src/secp256k1/README.md
136 ↗	(On Diff #18758)	You have twice the same option here.
test/CMakeLists.txt
73 ↗	(On Diff #18758)	Does this work at all? I don't think anything in the test plan related to this.

Fabien added inline comments.Apr 15 2020, 17:12

src/secp256k1/CMakeLists.txt
27 ↗	(On Diff #18758)	I agree with the general case, but in this case there is no dependency to track. I can just list the files if you're more comfortable with this solution, this is equivalent (and there are only 8 files), you tell me.
30 ↗	(On Diff #18758)	I had the same questioning, this is also one of the reason the initial version used a coverage build type. If you ask for an optimized build and coverage, you're basically asking for something that can't work (or badly, since the coverage result will still be generated but hard to understand). I don't have a strong opinion of which option should have priority. Maybe a tradeoff would be to set some warning if the used specified a build type different from Debug with coverage enabled ? Otherwise I can just filter and let the user do what he wants, only overriding when there is no build type explicitly set. I don't have a strong opinion on what is a better UX here.
test/CMakeLists.txt
73 ↗	(On Diff #18758)	Yes, it does. I just didn't put all the combinations in the test plan. Here is an example output (was generated before I added back secp256k1 and seeder, so they are missing) for the extended functional tests with upgrade activated:

Fabien edited the test plan for this revision. (Show Details)Apr 15 2020, 17:13

Update doc

Build Bitcoin-ABC / Diffs / Diff Testing (build-diff) started.

Build Bitcoin-ABC / Diffs / Diff Testing (build-diff) passed.

Harbormaster completed remote builds in B10234: Diff 18831.Apr 15 2020, 17:38

deadalnix requested changes to this revision.Apr 16 2020, 13:37

deadalnix added inline comments.

src/secp256k1/CMakeLists.txt
27 ↗	(On Diff #18758)	Listing them is an option. But maybe even better, is it possible to pass a glob to the script that generate the coverage report so it can exclude these files? After all, there is no point having cmake track this.
30 ↗	(On Diff #18758)	Well I know, but in that case, you'd want to have some error message or something, not doing something random that do not match what the user asked. Maybe making coverage a build type is the way to go, but then you actually want to commit to that design decision, not do something where it's half a config, half a build type, like you previously did.
test/CMakeLists.txt
73 ↗	(On Diff #18758)	I'm really puzzled that this works at all.

This revision now requires changes to proceed.Apr 16 2020, 13:37

Fabien added inline comments.Apr 16 2020, 14:52

src/secp256k1/CMakeLists.txt
27 ↗	(On Diff #18758)	The script does not handle glob like pattern but searches for the string, so passing `${CMAKE_CURRENT_SOURCE_DIR}/src/bench` should work, and is cleaner. Good idea !
30 ↗	(On Diff #18758)	This is actually not possible to have it be a build type without also having an option (due to cmake limitations), so I'd better stick with this option solution. This is less elegant but at least it works. I will find a solution to make it more user friendly.

Fabien edited the test plan for this revision. (Show Details)Apr 16 2020, 15:01

Pass secp256k1 bench files directly to the filter with no glob.

The optimization level is overriden only when there is no user defined build type, and displays a warning otherwise.

Build Bitcoin-ABC / Diffs / Diff Testing (build-diff) started.

Build Bitcoin-ABC / Diffs / Diff Testing (build-diff) passed.

Harbormaster completed remote builds in B10250: Diff 18866.Apr 16 2020, 16:02

I think there is a problem with the way you handle the optimization, and various nits, but overall this looks solid. Accepting for now, but please keep in mind the optimization thing need to be sorted out.

This revision is now accepted and ready to land.Apr 17 2020, 00:28

Closed by commit rABC100c22d068f4: [CMAKE] Add support for generating test coverage reports (authored by Fabien). · Explain WhyApr 17 2020, 07:08

This revision was automatically updated to reflect the committed changes.

Fabien added a commit: rABC100c22d068f4: [CMAKE] Add support for generating test coverage reports.

Fabien added a commit: rSTAGING100c22d068f4: [CMAKE] Add support for generating test coverage reports.May 5 2020, 19:00

[CMAKE] Add support for generating test coverage reports
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 18866

CMakeLists.txt

cmake/modules/AddCompilerFlags.cmake

cmake/modules/Coverage.cmake

cmake/modules/TestSuite.cmake

cmake/templates/CoverageBaseline.sh.in

cmake/templates/CoverageTest.sh.in

doc/developer-notes.md

src/secp256k1/CMakeLists.txt

src/secp256k1/README.md

src/secp256k1/src/libsecp256k1-config.h.cmake.in

test/CMakeLists.txt

[CMAKE] Add support for generating test coverage reportsClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 18866

CMakeLists.txt

cmake/modules/AddCompilerFlags.cmake

cmake/modules/Coverage.cmake

cmake/modules/TestSuite.cmake

cmake/templates/CoverageBaseline.sh.in

cmake/templates/CoverageTest.sh.in

doc/developer-notes.md

src/secp256k1/CMakeLists.txt

src/secp256k1/README.md

src/secp256k1/src/libsecp256k1-config.h.cmake.in

test/CMakeLists.txt

[CMAKE] Add support for generating test coverage reports
ClosedPublic
Actions

Revision Contents
Changeset List