Support gathering code coverage data for RPC tests with lcov #6813

Merged
merged 6 commits into from Oct 26, 2015

Conversation

Projects
None yet
7 participants
@dexX7
Contributor

dexX7 commented Oct 12, 2015

Bitcoin Core has a nice way to gather code coverage data, though it only runs the unit tests and (optionally) the BitcoinJ tests.

With this update, the RPC tests (via qa/pull-tester/rpc-tests.py) are also executed, when gathering code coverage data with make cov.

When cleaning, there were several leftovers, and only the coverage related files in src/ were removed, while the ones in the other dirs remained. The leftovers from tests are now also removed, whereby qa/tmp/ is related to the BitcoinJ tests, and cache/ is related to RPC tests.

Because Python is used to run the RPC tests, it is explicitly checked, whether Python is available.

The configuration option --enable-extended-rpc-tests may be used to enable extended RPC tests, and the configuration option --enable-comparison-tool-reorg-tests may be used to enable extended tests via BitcoinJ. Note that the extended tests can take some time.


How to:

Generating coverage data requires lcov, which may be installed with:

sudo apt-get install lcov

There are a few configuration options, but no further setup is necessary.

To include the BitcoinJ tests, get the test tool:

TOOL_URL=https://github.com/theuni/bitcoind-comparisontool/raw/master/pull-tests-8c6666f.jar
TOOL_HASH=a865332b3827abcde684ab79f5f43c083b0b6a4c97ff5508c79f29fee24f11cd
wget $TOOL_URL -O ./share/BitcoindComparisonTool.jar
echo "$TOOL_HASH  ./share/BitcoindComparisonTool.jar" | shasum --algorithm 256 --check

The coverage data can then be gathered with:

./autogen.sh
./configure --enable-lcov --with-comparison-tool=./share/BitcoindComparisonTool.jar
make
make cov

It runs the tests and generates two HTML reports:

  • test_bitcoin.coverage/index.html
  • total.coverage/index.html

Example report:

(only line and function coverage, no branch coverage, without extended tests)

@fanquake

View changes

.gitignore
@@ -88,12 +88,16 @@ qrc_*.cpp
# Qt creator
*.pro.user
+# NetBeans

This comment has been minimized.

@fanquake

fanquake Oct 13, 2015

Member

Ideally this should be ignored on the dev machine.

@fanquake

fanquake Oct 13, 2015

Member

Ideally this should be ignored on the dev machine.

This comment has been minimized.

@dexX7

dexX7 Oct 13, 2015

Contributor

I'm very open to removing it (or other parts), but the precedence was basically the ignore of the Qt Creator project files.

@dexX7

dexX7 Oct 13, 2015

Contributor

I'm very open to removing it (or other parts), but the precedence was basically the ignore of the Qt Creator project files.

This comment has been minimized.

@laanwj

laanwj Oct 20, 2015

Member

I'm very open to removing it (or other parts), but the precedence was basically the ignore of the Qt Creator project files.

Yes, those should go too (also as we haven't supported qt creator builds since 0.9).

@laanwj

laanwj Oct 20, 2015

Member

I'm very open to removing it (or other parts), but the precedence was basically the ignore of the Qt Creator project files.

Yes, those should go too (also as we haven't supported qt creator builds since 0.9).

This comment has been minimized.

@dexX7

dexX7 Oct 22, 2015

Contributor

I'm currently not at home, but later I can edit and remove the lines. But just for my understanding: isn't it pretty common to ignore project/editor specific files via the .gitignore?

As for the change:

- # Qt creator
- *.pro.user
- 
- # NetBeans
- nbproject/
- 

Anything else?

@dexX7

dexX7 Oct 22, 2015

Contributor

I'm currently not at home, but later I can edit and remove the lines. But just for my understanding: isn't it pretty common to ignore project/editor specific files via the .gitignore?

As for the change:

- # Qt creator
- *.pro.user
- 
- # NetBeans
- nbproject/
- 

Anything else?

This comment has been minimized.

@laanwj

laanwj Oct 23, 2015

Member

In closed-source environments in which everyone uses the same IDE that is common. In open source software, where everyone uses their own editors/IDE/tools, it is less common. Only you know what files your editor produces and this may change from version to version.
The canonical way to do this is thus to create your local gitignore. Add this to ~/.gitconfig:

[core]
    excludesfile = /home/.../.gitignore_global

Then put your favourite tool's excrement filenames in that file :-)
Another option is to use per-project .git/info/exclude. These are not committed either.

@laanwj

laanwj Oct 23, 2015

Member

In closed-source environments in which everyone uses the same IDE that is common. In open source software, where everyone uses their own editors/IDE/tools, it is less common. Only you know what files your editor produces and this may change from version to version.
The canonical way to do this is thus to create your local gitignore. Add this to ~/.gitconfig:

[core]
    excludesfile = /home/.../.gitignore_global

Then put your favourite tool's excrement filenames in that file :-)
Another option is to use per-project .git/info/exclude. These are not committed either.

This comment has been minimized.

This comment has been minimized.

@dexX7

dexX7 Oct 24, 2015

Contributor

I removed the entry for Netbeans.

@dexX7

dexX7 Oct 24, 2015

Contributor

I removed the entry for Netbeans.

@laanwj laanwj added the Tests label Oct 13, 2015

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Oct 13, 2015

Member

Concept ACK - how does this relate to #6804 ?

Member

laanwj commented Oct 13, 2015

Concept ACK - how does this relate to #6804 ?

@dexX7

This comment has been minimized.

Show comment
Hide comment
@dexX7

dexX7 Oct 13, 2015

Contributor

It's not yet clear to me where #6804 is going, so this may be complementary. #6804 seems like a very easy and fast way to get an understanding of what's covered by the RPC tests, while this PR is more heavy. Given that the lcov testing already exists, extending it to include RPC tests seems nevertheless reasonable in my opinion.

Contributor

dexX7 commented Oct 13, 2015

It's not yet clear to me where #6804 is going, so this may be complementary. #6804 seems like a very easy and fast way to get an understanding of what's covered by the RPC tests, while this PR is more heavy. Given that the lcov testing already exists, extending it to include RPC tests seems nevertheless reasonable in my opinion.

@jamesob

This comment has been minimized.

Show comment
Hide comment
@jamesob

jamesob Oct 13, 2015

Member

Agree that this is compatible with #6804; I think this sort of "industrial strength" coverage report is absolutely worth having, but an expedient and simple view into which (if any) RPC functions are going untested is valuable also. Lcov certainly provides a more exact notion of what is tested, but it takes a bit more work to tease out which RPC functions are untested; the simple binary coverage provided in #6804 answers that question immediately and unambiguously, despite being primitive.

#6804 may also make it easier to gate builds down the road based on RPC coverage -- if the list of untested RPC functions calculated is non-empty, we fail. I think this would be nice to do once we have tests in place for each RPC function, It's not immediately apparent to me how we'd do this with just lcov.

So again, I think both are valuable and compatible. What do you think, @dexX7 @laanwj?

Member

jamesob commented Oct 13, 2015

Agree that this is compatible with #6804; I think this sort of "industrial strength" coverage report is absolutely worth having, but an expedient and simple view into which (if any) RPC functions are going untested is valuable also. Lcov certainly provides a more exact notion of what is tested, but it takes a bit more work to tease out which RPC functions are untested; the simple binary coverage provided in #6804 answers that question immediately and unambiguously, despite being primitive.

#6804 may also make it easier to gate builds down the road based on RPC coverage -- if the list of untested RPC functions calculated is non-empty, we fail. I think this would be nice to do once we have tests in place for each RPC function, It's not immediately apparent to me how we'd do this with just lcov.

So again, I think both are valuable and compatible. What do you think, @dexX7 @laanwj?

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Oct 13, 2015

Member

Concept ACK.
What about linking Travis builds with Coveralls (example: https://coveralls.io/github/jonasschnelli/libbtc)?

Not saying that high test coverage stands for quality. But it is a good indicator for people writing new code that they should write unit tests...

Member

jonasschnelli commented Oct 13, 2015

Concept ACK.
What about linking Travis builds with Coveralls (example: https://coveralls.io/github/jonasschnelli/libbtc)?

Not saying that high test coverage stands for quality. But it is a good indicator for people writing new code that they should write unit tests...

@dexX7

This comment has been minimized.

Show comment
Hide comment
@dexX7

dexX7 Oct 15, 2015

Contributor

@jamesob: based on what you posted in #6804, the list of uncovered RPCs is pretty large, and I agree, it's nice to have a quick overview. At some point it probably becomes less useful, if a binary outcome doesn't provide further insight (e.g. once all RPCs are rudimentary covered).

@jonasschnelli: this is an interesting idea. Some time ago I actually tried to combine make cov and cpp-coveralls, but I kinda failed, and unfortunally didn't dig deeper.

On a slightly related note: it looks like coveralls is run two times in your repo (Linux+clang, Linux+gcc), and if this is an issue for you, you may look into a custom script deployment, which supports expressive conditionals (I used it to deploy docs).

One question that arrises is about the extra build time, if coverage data were gathered via Travis + Coveralls. I haven't timed it, but I have the impressionen it takes significantly longer to go though the process, so this would be something to consider. I'd like, if it were nicely integrated though.

Contributor

dexX7 commented Oct 15, 2015

@jamesob: based on what you posted in #6804, the list of uncovered RPCs is pretty large, and I agree, it's nice to have a quick overview. At some point it probably becomes less useful, if a binary outcome doesn't provide further insight (e.g. once all RPCs are rudimentary covered).

@jonasschnelli: this is an interesting idea. Some time ago I actually tried to combine make cov and cpp-coveralls, but I kinda failed, and unfortunally didn't dig deeper.

On a slightly related note: it looks like coveralls is run two times in your repo (Linux+clang, Linux+gcc), and if this is an issue for you, you may look into a custom script deployment, which supports expressive conditionals (I used it to deploy docs).

One question that arrises is about the extra build time, if coverage data were gathered via Travis + Coveralls. I haven't timed it, but I have the impressionen it takes significantly longer to go though the process, so this would be something to consider. I'd like, if it were nicely integrated though.

@jamesob

This comment has been minimized.

Show comment
Hide comment
@jamesob

jamesob Oct 15, 2015

Member

@dexX7 the console output itself certainly becomes less useful after all RPCs are covered, but I think the lasting value is the ability to easily fail a build if someone introduces a new RPC without writing tests that exercise it as a client.

Anyway, I'll fully defer to you and @laanwj as to whether or not we should proceed with #6804 as well. You guys have much more experience in this ecosystem than I do!

Member

jamesob commented Oct 15, 2015

@dexX7 the console output itself certainly becomes less useful after all RPCs are covered, but I think the lasting value is the ability to easily fail a build if someone introduces a new RPC without writing tests that exercise it as a client.

Anyway, I'll fully defer to you and @laanwj as to whether or not we should proceed with #6804 as well. You guys have much more experience in this ecosystem than I do!

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Oct 22, 2015

Member

ACK. Works for me.

Member

laanwj commented Oct 22, 2015

ACK. Works for me.

dexX7 added some commits May 27, 2015

Remove coverage and test related files, when cleaning up
Until now there were quite a few leftovers, and only the coverage
related files in `src/` were cleaned, while the ones in the other dirs
remained. `qa/tmp/` is related to the BitcoinJ tests, and `cache/` is
related to RPC tests.
Require Python for RPC tests, when using lcov
Because Python is (going to be) used to run the RPC tests, when
gathering coverage data with lcov, it is explicitly checked, whether
Python is really available.
Add config option to enable extended RPC tests for code coverage
When using lcov to gather code coverage data, the configuration option
`--enable-extended-rpc-tests` may be used to enable extended RPC tests.
Run extended BitcoinJ tests for coverage based on config
The configuration option `--enable-comparison-tool-reorg-tests` may be
used to enable extended tests via BitcoinJ also for coverage testing.
Support gathering of code coverage data for RPC tests
The RPC tests (via `qa/pull-tester/rpc-tests.py`) are now executed,
when gathering code coverage data, for example with `make cov`.

Generating coverage data requires `lcov`, which can installed with:

    sudo apt-get install lcov

To also use the BitcoinJ tests, get the test tool:

    TOOL_URL=https://github.com/theuni/bitcoind-comparisontool/raw/master/pull-tests-8c6666f.jar
    TOOL_HASH=a865332b3827abcde684ab79f5f43c083b0b6a4c97ff5508c79f29fee24f11cd
    wget $TOOL_URL -O ./share/BitcoindComparisonTool.jar
    echo "$TOOL_HASH  ./share/BitcoindComparisonTool.jar" | shasum --algorithm 256 --check

The coverage data can be generated with:

    ./autogen.sh
    ./configure --enable-lcov --with-comparison-tool=./share/BitcoindComparisonTool.jar
    make
    make cov

Optionally the options `--enable-extended-rpc-tests` and
`--enable-comparison-tool-reorg-tests` may be used to enable more time
consuming tests.

It then runs the tests and generates two HTML reports:

 - test_bitcoin.coverage/index.html
 - total.coverage/index.html

@laanwj laanwj merged commit d80e3cb into bitcoin:master Oct 26, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Oct 26, 2015

Merge pull request #6813
d80e3cb Support gathering of code coverage data for RPC tests (dexX7)
e3b5e6c Run extended BitcoinJ tests for coverage based on config (dexX7)
45d4ff0 Add config option to enable extended RPC tests for code coverage (dexX7)
8e3a27b Require Python for RPC tests, when using lcov (dexX7)
d425877 Remove coverage and test related files, when cleaning up (dexX7)
4d2a926 Ignore coverage data related and temporary test files (dexX7)
@Sjors

This comment has been minimized.

Show comment
Hide comment
@Sjors

Sjors Nov 22, 2017

Member

@jonasschnelli wrote:

What about linking Travis builds with Coveralls

I just noticed #11680 which reminded me of this. Is it worth making an issue / PR?

Member

Sjors commented Nov 22, 2017

@jonasschnelli wrote:

What about linking Travis builds with Coveralls

I just noticed #11680 which reminded me of this. Is it worth making an issue / PR?

@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke Nov 22, 2017

Member

I am running nightly reports: https://marcofalke.github.io/btc_cov/

Member

MarcoFalke commented Nov 22, 2017

I am running nightly reports: https://marcofalke.github.io/btc_cov/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment