[tests] Add basic coverage reporting for RPC tests #6804

jamesob commented at 7:16 AM on October 11, 2015: member

After the missed-unobfuscation snafu of #6777, I realized that we don't have comprehensive coverage of the RPC commands, nor do we have a method of determining which RPC calls are or aren't covered. I'd like to start writing more RPC tests, but first I'd like to have an easy way of figuring out which tests to write.

In this PR I've provided a basic method of getting a binary coverage report for each RPC command, which should be useful to guide adding tests until we have full coverage of the RPC interface. After we're exercising every RPC command, we can possibly gate builds on that full-coverage invariant using a mechanism like this, if that's preferable.

The report looks as follows:

 $ ./qa/pull-tester/rpc-tests.py --coverage

Initialized coverage directory at /tmp/coverageAHqCbm
Running testscript wallet.py...
Initializing test directory /tmp/testSAwNV3

[ tests run... ]

Tests successful
Uncovered RPC commands:
  - dumpprivkey
  - estimatefee
  - estimatepriority
  - getaccount
  - getaddednodeinfo
  - getaddressesbyaccount
  - getblockchaininfo
  - getblockheader
  - getblocktemplate
  - getconnectioncount
  - getdifficulty
  - getgenerate
  - getinfo
  - getmempoolinfo
  - getmininginfo
  - getnettotals
  - getnetworkhashps
  - getrawchangeaddress
  - getreceivedbyaccount
  - getreceivedbyaddress
  - gettxout
  - gettxoutsetinfo
  - getunconfirmedbalance
  - importprivkey
  - keypoolrefill
  - listaccounts
  - listaddressgroupings
  - listlockunspent
  - listreceivedbyaccount
  - listreceivedbyaddress
  - listsinceblock
  - lockunspent
  - move
  - ping
  - prioritisetransaction
  - setaccount
  - setgenerate
  - signmessage
  - submitblock
  - verifychain
  - verifymessage

Cleaning up coverage data

jamesob force-pushed on Oct 11, 2015

MarcoFalke commented at 8:05 AM on October 12, 2015: member

Is it required to have the coverage tempdir? Couldn't this be done in memory without disk io?

jamesob commented at 3:37 PM on October 12, 2015: member

@MarcoFalke I can't see how that would work given that we call the individual test scripts in separate subprocesses, but agree that would be preferable if possible.

jamesob force-pushed on Oct 12, 2015

jamesob commented at 4:54 PM on October 12, 2015: member

Yargh... build failure due to #6778. Going to try rerunning.

jamesob closed this on Oct 12, 2015

jamesob reopened this on Oct 12, 2015

laanwj added the label Tests on Oct 13, 2015

laanwj commented at 9:47 AM on October 13, 2015: member

Concept ACK, although there seems to be overlap (at least in purpose) with #6813

jamesob commented at 6:11 PM on October 22, 2015: member

Merge conflict resolved here.

jamesob force-pushed on Oct 22, 2015

jamesob commented at 7:21 PM on October 22, 2015: member

Rebased (thanks again @MarcoFalke).

jamesob commented at 8:09 PM on November 2, 2015: member

@laanwj @dexX7 how are you two feeling about this PR? I still think it'd be useful, but if you have reservations I'm happy to close it out for now.

dexX7 commented at 1:52 PM on November 3, 2015: contributor

I personally have no preference to be honest. I think it could be handy, if new users dive into Bitcoin Core and start with adding additional tests, and I also like the cleanup and documentation. Then again, it's unclear, whether users really are going to use it. Mini-ACK from my side.

jamesob commented at 1:54 PM on November 3, 2015: member

@dexX7 yeah, apologies for conflating some of the cleanup & doc with this feature. Would this PR be more useful if we enabled it by default for travis builds?

dexX7 commented at 1:56 PM on November 3, 2015: contributor

That's a very interesting idea!

jamesob commented at 2:42 PM on November 3, 2015: member

The Travis build now reports coverage by default:

selection_041

dexX7 commented at 2:43 PM on November 3, 2015: contributor

That's pretty cool imho: https://travis-ci.org/bitcoin/bitcoin/jobs/89011588#L3943

edit: you beat me, hehe. :)

in qa/rpc-tests/test_framework/authproxy.py:None in 1fb9e78495 outdated

  66 | @@ -67,7 +67,18 @@ def EncodeDecimal(o):
  67 |  class AuthServiceProxy(object):

laanwj commented at 4:38 PM on November 3, 2015:

Instead of changing authproxy, I'd prefer if this was an object that wrapped an authproxy, and logs and passes on all the calls. I think it could be done that way

jamesob commented at 3:17 PM on November 5, 2015:

Good suggestion. I've started working on the implementation of this, and I think it's going to simplify things a bit.

laanwj commented at 4:39 PM on November 3, 2015: member

The overview of untested RPC commands is confronting. We should definitely have it.

MarcoFalke commented at 4:47 PM on November 3, 2015: member

Nit: Would it be hard to unfiddle into two commits?

Cleanup
New feature

Regardless of that: Concept ACK, I think we could make it fail as soon as all commands are covered.

jamesob force-pushed on Nov 5, 2015

jamesob commented at 3:34 PM on November 5, 2015: member

@MarcoFalke, going forward I'll certainly be more diligent about separating cleanup changes from new functionality -- unfortunately I think in this case it'll be tough to tease out the distinction between cleanup and coverage here, as there isn't a clean way to do it in terms of git chunks, so I'd have to basically rewrite the PR with that separation in mind. This is test code, and so the commit separability doesn't seem to me to be as important as in other parts of the system (though I understand in general this is what we should aim for). That said, if you think it's important I'd be happy to try.

jamesob force-pushed on Nov 5, 2015

in qa/rpc-tests/test_framework/authproxy.py:None in 37f44813f5 outdated

  68 | @@ -69,7 +69,7 @@ class AuthServiceProxy(object):
  69 |  
  70 |      def __init__(self, service_url, service_name=None, timeout=HTTP_TIMEOUT, connection=None):
  71 |          self.__service_url = service_url
  72 | -        self.__service_name = service_name
  73 | +        self._service_name = service_name

jamesob commented at 4:09 PM on November 5, 2015:

@laanwj note here that unfortunately I wasn't able to completely avoid changing authproxy.py: in order to use composition over inheritance in coverage.AuthServiceProxyWrapper (which I think is preferable), we have to make this attribute un-name-mangled, i.e. accessible outside of the class itself.

jamesob commented at 4:31 PM on November 5, 2015: member

Aside from any further feedback, I'm happy with where this is.

jamesob commented at 7:11 PM on November 9, 2015: member

Ping here -- any further feedback?

in qa/rpc-tests/test_framework/test_framework.py:None in 37f44813f5 outdated

  12 | @@ -13,8 +13,9 @@
  13 |  import tempfile
  14 |  import traceback
  15 |  
  16 | +from . import util
  17 | +from . import coverage
  18 |  from authproxy import AuthServiceProxy, JSONRPCException
  19 | -from util import *

MarcoFalke commented at 7:33 PM on November 9, 2015:

Nit: is the required? (Makes diff larger)

laanwj commented at 5:38 PM on November 11, 2015:

In principle I agree with getting rid of cases of import *, but what I'd propose is to explicitly import the symbols that are used:

from util import initialize_chain, start_nodes, # etc

Using the full name explicitly in every use is a) more typing b) generates a large diff (as @MarcoFalke already says)

jgarzik commented at 5:45 PM on November 11, 2015:

Indeed; explicitly importing symbols reduces namespace pollution and slightly reduces chances for odd bugs.

jamesob commented at 5:55 PM on November 11, 2015:

I can file a cleanup PR afterwards doing this more broadly; would you guys prefer I (i) revert this diff here, (ii) import the symbols explicitly instead of the util namespace, or (iii) leave as-is for now?

laanwj commented at 5:58 PM on November 11, 2015:

I'd prefer (ii), but if you want to revert this and do it in another PR it's fine with me too. I don't like (iii) because it implies changing this now and changing this (partially) back again later.

jamesob commented at 6:12 PM on November 11, 2015:

:+1: I'll do (ii) sometime tonight.

jamesob commented at 6:19 PM on November 11, 2015:

FWIW I'd argue that the existing method of importing the util namespace rather than particular symbols results in less pollution, but this is a small thing.

MarcoFalke commented at 7:37 PM on November 9, 2015: member

utACK 37f4481

laanwj commented at 6:29 PM on November 11, 2015: member

ACK after squashing into one commit

Add basic coverage reporting for RPC tests

Thanks to @MarcoFalke @dexX7 @laanwj for review.

b5cbd396ca

jamesob force-pushed on Nov 11, 2015

MarcoFalke commented at 7:09 PM on November 11, 2015: member

reACK b5cbd39

jamesob commented at 7:09 AM on November 12, 2015: member

Squashed.

laanwj merged this on Nov 12, 2015

laanwj closed this on Nov 12, 2015

laanwj referenced this in commit 5fcc14ee05 on Nov 12, 2015

zkbot referenced this in commit b2ab91b032 on Mar 24, 2020

zkbot referenced this in commit 38755ebc22 on Mar 24, 2020

zkbot referenced this in commit da12da80c4 on Apr 2, 2020

zkbot referenced this in commit 1be7250db9 on Apr 3, 2020

zkbot referenced this in commit 025bd44543 on Nov 21, 2020

zkbot referenced this in commit 7a0a268054 on Dec 2, 2020

zkbot referenced this in commit c8896f9907 on Dec 2, 2020

DrahtBot locked this on Sep 8, 2021