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

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

Merged
merged 1 commit into from Nov 12, 2015

Conversation

Projects
None yet
5 participants
@jamesob
Member

jamesob commented Oct 11, 2015

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
@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke Oct 12, 2015

Member

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

Member

MarcoFalke commented Oct 12, 2015

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

@jamesob

This comment has been minimized.

Show comment
Hide comment
@jamesob

jamesob Oct 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.

Member

jamesob commented Oct 12, 2015

@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

This comment has been minimized.

Show comment
Hide comment
@jamesob

jamesob Oct 12, 2015

Member

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

Member

jamesob commented Oct 12, 2015

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

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Oct 13, 2015

Member

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

Member

laanwj commented Oct 13, 2015

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

@jamesob

This comment has been minimized.

Show comment
Hide comment
@jamesob

jamesob Oct 22, 2015

Member

Merge conflict resolved here.

Member

jamesob commented Oct 22, 2015

Merge conflict resolved here.

@jamesob

This comment has been minimized.

Show comment
Hide comment
@jamesob

jamesob Oct 22, 2015

Member

Rebased (thanks again @MarcoFalke).

Member

jamesob commented Oct 22, 2015

Rebased (thanks again @MarcoFalke).

@jamesob

This comment has been minimized.

Show comment
Hide comment
@jamesob

jamesob Nov 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.

Member

jamesob commented Nov 2, 2015

@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

This comment has been minimized.

Show comment
Hide comment
@dexX7

dexX7 Nov 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.

Contributor

dexX7 commented Nov 3, 2015

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

This comment has been minimized.

Show comment
Hide comment
@jamesob

jamesob Nov 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?

Member

jamesob commented Nov 3, 2015

@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

This comment has been minimized.

Show comment
Hide comment
@dexX7

dexX7 Nov 3, 2015

Contributor

That's a very interesting idea!

Contributor

dexX7 commented Nov 3, 2015

That's a very interesting idea!

@jamesob

This comment has been minimized.

Show comment
Hide comment
@jamesob

jamesob Nov 3, 2015

Member

The Travis build now reports coverage by default:

selection_041

Member

jamesob commented Nov 3, 2015

The Travis build now reports coverage by default:

selection_041

@dexX7

This comment has been minimized.

Show comment
Hide comment
@dexX7

dexX7 Nov 3, 2015

Contributor

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

edit: you beat me, hehe. :)

Contributor

dexX7 commented Nov 3, 2015

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

edit: you beat me, hehe. :)

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

This comment has been minimized.

@laanwj

laanwj Nov 3, 2015

Member

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

@laanwj

laanwj Nov 3, 2015

Member

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

This comment has been minimized.

@jamesob

jamesob Nov 5, 2015

Member

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

@jamesob

jamesob Nov 5, 2015

Member

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

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Nov 3, 2015

Member

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

Member

laanwj commented Nov 3, 2015

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

@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke Nov 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.

Member

MarcoFalke commented Nov 3, 2015

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

This comment has been minimized.

Show comment
Hide comment
@jamesob

jamesob Nov 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.

Member

jamesob commented Nov 5, 2015

@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.

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

This comment has been minimized.

@jamesob

jamesob Nov 5, 2015

Member

@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

jamesob Nov 5, 2015

Member

@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

This comment has been minimized.

Show comment
Hide comment
@jamesob

jamesob Nov 5, 2015

Member

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

Member

jamesob commented Nov 5, 2015

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

@jamesob

This comment has been minimized.

Show comment
Hide comment
@jamesob

jamesob Nov 9, 2015

Member

Ping here -- any further feedback?

Member

jamesob commented Nov 9, 2015

Ping here -- any further feedback?

@MarcoFalke

View changes

Show outdated Hide outdated qa/rpc-tests/test_framework/test_framework.py
from authproxy import AuthServiceProxy, JSONRPCException
from util import *

This comment has been minimized.

@MarcoFalke

MarcoFalke Nov 9, 2015

Member

Nit: is the required? (Makes diff larger)

@MarcoFalke

MarcoFalke Nov 9, 2015

Member

Nit: is the required? (Makes diff larger)

This comment has been minimized.

@laanwj

laanwj Nov 11, 2015

Member

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)

@laanwj

laanwj Nov 11, 2015

Member

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)

This comment has been minimized.

@jgarzik

jgarzik Nov 11, 2015

Contributor

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

@jgarzik

jgarzik Nov 11, 2015

Contributor

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

This comment has been minimized.

@jamesob

jamesob Nov 11, 2015

Member

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?

@jamesob

jamesob Nov 11, 2015

Member

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?

This comment has been minimized.

@laanwj

laanwj Nov 11, 2015

Member

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.

@laanwj

laanwj Nov 11, 2015

Member

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.

This comment has been minimized.

@jamesob

jamesob Nov 11, 2015

Member

👍 I'll do (ii) sometime tonight.

@jamesob

jamesob Nov 11, 2015

Member

👍 I'll do (ii) sometime tonight.

This comment has been minimized.

@jamesob

jamesob Nov 11, 2015

Member

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.

@jamesob

jamesob Nov 11, 2015

Member

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

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke Nov 9, 2015

Member

utACK 37f4481

Member

MarcoFalke commented Nov 9, 2015

utACK 37f4481

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Nov 11, 2015

Member

ACK after squashing into one commit

Member

laanwj commented Nov 11, 2015

ACK after squashing into one commit

@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke
Member

MarcoFalke commented Nov 11, 2015

reACK b5cbd39

@jamesob

This comment has been minimized.

Show comment
Hide comment
@jamesob

jamesob Nov 12, 2015

Member

Squashed.

Member

jamesob commented Nov 12, 2015

Squashed.

@laanwj laanwj merged commit b5cbd39 into bitcoin:master Nov 12, 2015

1 check passed

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

laanwj added a commit that referenced this pull request Nov 12, 2015

Merge pull request #6804
b5cbd39 Add basic coverage reporting for RPC tests (James O'Beirne)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment