[tests] Introduce TestNode by jnewbery · Pull Request #10711 · bitcoin/bitcoin

jnewbery · 2017-06-30T13:54:46Z

Continues #10082

TestNode is a class responsible for all state related to a bitcoind node
under test. It stores local state, is responsible for tracking the
bitcoind process and delegates unrecognised messages to the RPC
connection.

This commit changes start_nodes and stop_nodes to start and stop the
bitcoind nodes in parallel, making test setup and teardown much faster.

On my vm, this changeset reduces total test_runner runtime for the base set of tests
(including building the cache) from 263s to 195s (a 25% speedup). Note that the time
reported by test_runner does not include time spent building the cache:

with TestNode:

→ date +"%T" ; ./test_runner.py -q ; date +"%T"
12:48:04
..................................................................................................................................................................................................................................................................................................................................
TEST                           | STATUS    | DURATION

abandonconflict.py             | ✓ Passed  | 12 s
bip68-112-113-p2p.py           | ✓ Passed  | 19 s
blockchain.py                  | ✓ Passed  | 8 s 
bumpfee.py                     | ✓ Passed  | 13 s
decodescript.py                | ✓ Passed  | 3 s 
disablewallet.py               | ✓ Passed  | 3 s 
disconnect_ban.py              | ✓ Passed  | 6 s 
fundrawtransaction.py          | ✓ Passed  | 37 s
getchaintips.py                | ✓ Passed  | 4 s 
httpbasics.py                  | ✓ Passed  | 3 s 
import-rescan.py               | ✓ Passed  | 4 s 
importmulti.py                 | ✓ Passed  | 6 s 
importprunedfunds.py           | ✓ Passed  | 3 s 
invalidblockrequest.py         | ✓ Passed  | 4 s 
invalidtxrequest.py            | ✓ Passed  | 4 s 
keypool.py                     | ✓ Passed  | 7 s 
listsinceblock.py              | ✓ Passed  | 4 s 
listtransactions.py            | ✓ Passed  | 33 s
mempool_limit.py               | ✓ Passed  | 4 s 
mempool_persist.py             | ✓ Passed  | 15 s
mempool_reorg.py               | ✓ Passed  | 4 s 
mempool_resurrect_test.py      | ✓ Passed  | 3 s 
mempool_spendcoinbase.py       | ✓ Passed  | 3 s 
merkle_blocks.py               | ✓ Passed  | 3 s 
multi_rpc.py                   | ✓ Passed  | 4 s 
net.py                         | ✓ Passed  | 3 s 
nulldummy.py                   | ✓ Passed  | 3 s 
p2p-compactblocks.py           | ✓ Passed  | 28 s
p2p-fullblocktest.py           | ✓ Passed  | 126 s
p2p-leaktests.py               | ✓ Passed  | 8 s 
p2p-mempool.py                 | ✓ Passed  | 3 s 
p2p-segwit.py                  | ✓ Passed  | 59 s
p2p-versionbits-warning.py     | ✓ Passed  | 8 s 
preciousblock.py               | ✓ Passed  | 3 s 
prioritise_transaction.py      | ✓ Passed  | 5 s 
proxy_test.py                  | ✓ Passed  | 3 s 
rawtransactions.py             | ✓ Passed  | 9 s 
receivedby.py                  | ✓ Passed  | 19 s
reindex.py                     | ✓ Passed  | 12 s
rest.py                        | ✓ Passed  | 9 s 
rpcnamedargs.py                | ✓ Passed  | 3 s 
segwit.py                      | ✓ Passed  | 7 s 
sendheaders.py                 | ✓ Passed  | 24 s
signmessages.py                | ✓ Passed  | 3 s 
signrawtransactions.py         | ✓ Passed  | 3 s 
txn_clone.py                   | ✓ Passed  | 4 s 
txn_doublespend.py --mineblock | ✓ Passed  | 4 s 
uptime.py                      | ✓ Passed  | 3 s 
wallet-accounts.py             | ✓ Passed  | 3 s 
wallet-dump.py                 | ✓ Passed  | 7 s 
wallet-encryption.py           | ✓ Passed  | 8 s 
wallet-hd.py                   | ✓ Passed  | 15 s
wallet.py                      | ✓ Passed  | 31 s
walletbackup.py                | ✓ Passed  | 104 s
zapwallettxes.py               | ✓ Passed  | 9 s 
zmq_test.py                    | ○ Skipped | 0 s 

ALL                            | ✓ Passed  | 735 s (accumulated)
Runtime: 189 s

12:51:19

master:

→ date +"%T" ; ./test_runner.py -q ; date +"%T"
12:40:13
..........................................................................................................................................................................................................................................................................................................................................................................................................................................
TEST                           | STATUS    | DURATION

abandonconflict.py             | ✓ Passed  | 15 s
bip68-112-113-p2p.py           | ✓ Passed  | 19 s
blockchain.py                  | ✓ Passed  | 8 s
bumpfee.py                     | ✓ Passed  | 20 s
decodescript.py                | ✓ Passed  | 3 s
disablewallet.py               | ✓ Passed  | 3 s
disconnect_ban.py              | ✓ Passed  | 8 s
fundrawtransaction.py          | ✓ Passed  | 36 s
getchaintips.py                | ✓ Passed  | 11 s
httpbasics.py                  | ✓ Passed  | 7 s
import-rescan.py               | ✓ Passed  | 16 s
importmulti.py                 | ✓ Passed  | 10 s
importprunedfunds.py           | ✓ Passed  | 5 s
invalidblockrequest.py         | ✓ Passed  | 4 s
invalidtxrequest.py            | ✓ Passed  | 3 s
keypool.py                     | ✓ Passed  | 7 s
listsinceblock.py              | ✓ Passed  | 11 s
listtransactions.py            | ✓ Passed  | 37 s
mempool_limit.py               | ✓ Passed  | 4 s
mempool_persist.py             | ✓ Passed  | 23 s
mempool_reorg.py               | ✓ Passed  | 7 s
mempool_resurrect_test.py      | ✓ Passed  | 3 s
mempool_spendcoinbase.py       | ✓ Passed  | 3 s
merkle_blocks.py               | ✓ Passed  | 10 s
multi_rpc.py                   | ✓ Passed  | 6 s
net.py                         | ✓ Passed  | 6 s
nulldummy.py                   | ✓ Passed  | 3 s
p2p-compactblocks.py           | ✓ Passed  | 30 s
p2p-fullblocktest.py           | ✓ Passed  | 126 s
p2p-leaktests.py               | ✓ Passed  | 8 s
p2p-mempool.py                 | ✓ Passed  | 3 s
p2p-segwit.py                  | ✓ Passed  | 62 s
p2p-versionbits-warning.py     | ✓ Passed  | 8 s
preciousblock.py               | ✓ Passed  | 8 s
prioritise_transaction.py      | ✓ Passed  | 7 s
proxy_test.py                  | ✓ Passed  | 10 s
rawtransactions.py             | ✓ Passed  | 15 s
receivedby.py                  | ✓ Passed  | 28 s
reindex.py                     | ✓ Passed  | 12 s
rest.py                        | ✓ Passed  | 12 s
rpcnamedargs.py                | ✓ Passed  | 3 s
segwit.py                      | ✓ Passed  | 12 s
sendheaders.py                 | ✓ Passed  | 26 s
signmessages.py                | ✓ Passed  | 3 s
signrawtransactions.py         | ✓ Passed  | 3 s
txn_clone.py                   | ✓ Passed  | 10 s
txn_doublespend.py --mineblock | ✓ Passed  | 10 s
uptime.py                      | ✓ Passed  | 3 s
wallet-accounts.py             | ✓ Passed  | 3 s
wallet-dump.py                 | ✓ Passed  | 6 s
wallet-encryption.py           | ✓ Passed  | 8 s
wallet-hd.py                   | ✓ Passed  | 18 s
wallet.py                      | ✓ Passed  | 69 s
walletbackup.py                | ✓ Passed  | 130 s
zapwallettxes.py               | ✓ Passed  | 15 s
zmq_test.py                    | ○ Skipped | 0 s

ALL                            | ✓ Passed  | 936 s (accumulated)
Runtime: 242 s

12:44:36

jnewbery · 2017-06-30T16:04:04Z

Wallet tests were failing on Travis because bitcoind startup was timing out. Increasing timeout from 2.5s to 10s for Travis.

meshcollider · 2017-07-04T06:28:13Z

Concept ACK e538dbf, looks good but haven't tested or gone through thoroughly

laanwj · 2017-07-10T15:44:58Z

The speed-up is nice, also like the idea of launching nodes in parallel. Concept ACK.

jtimon · 2017-07-10T18:10:40Z

test/functional/test_framework/util.py

Remove this?

thanks. Removed

jnewbery · 2017-07-11T14:12:17Z

fixed @jtimon's review comment and rebased.

jnewbery · 2017-07-24T15:35:23Z

rebased with TestNode changes for multiwallet.

maflcko · 2017-08-08T22:04:22Z

Needs rebase

jnewbery · 2017-08-09T20:49:39Z

rebased

@MarcoFalke @kallewoof @jimmysong @NicolasDorier I'd love some review of this if any of you have time.

jimmysong

utACK. Couple of nits and one thing I don't get.

jimmysong · 2017-08-09T21:06:59Z

test/functional/test_framework/test_node.py

This is a little harder to read than something like this:

for attempt in range(40): ... time.sleep(0.25)

Thanks. changed

jimmysong · 2017-08-09T21:07:40Z

test/functional/test_framework/test_node.py

no need for as e

jimmysong · 2017-08-09T21:12:13Z

test/functional/test_framework/test_node.py

What does this do? wallet_path is for sure a string, but how do you divide the rpc object by a string?

By magic :)

Take a look at

bitcoin/test/functional/test_framework/authproxy.py

Line 195 in e526ca6

def __truediv__(self, relative_uri):

and

bitcoin/test/functional/test_framework/coverage.py

Line 59 in e526ca6

def __truediv__(self, relative_uri):

The interface here is supposed to be similar to Python's pathlib

This functionality was added to support the multiwallet endpoints in 979d0b8 . Take a look at the multiwallet.py test to see how it's currently used.

NicolasDorier · 2017-08-10T00:40:52Z

utACK 2d189c89f6ecdb846a046bdd57883e190df7b3bf I am a big fan of this idea, it makes writing tests so much easier.

kallewoof

utACK with one nit.

kallewoof · 2017-08-10T02:20:57Z

test/functional/test_framework/test_framework.py

Why not allow None for extra_args? I'm generally wary of passing in objects as defaults, as if those default objects are ever altered, the altered version will be passed as the default in future calls.

E.g. try the following snippet:

def foo(x=[]): x.append(5) print(x) foo() foo() foo()

You will get as output

[5] [5, 5] [5, 5, 5]

wow I did not know.

But even if it was not the case, I think that the user should be able to pass null (Nothing), when he means "default value".

This code can be easily fixed by keeping the default value to Nothing and first line, checking if extra_args is nothing, if that is the case, init extra_args to an empty array.

Totally didn't catch that, but I've run into this in the past and it's pretty nasty to debug unless you know to look for it. I agree with the others, put this back to None.

null in Python is None, so yeah, I think None should be the default here and just set it to [] if it's None at the start.

Nice. Thanks @kallewoof , I wasn't aware of that, but it makes perfect sense.

Changed the default argument to None as suggested.

maflcko

utACK cb8579b, but needs some nits and apparent regressions fixed.

maflcko · 2017-08-15T13:39:34Z

test/functional/multiwallet.py

+        w1 = self.nodes[0].get_wallet_rpc("w1")
+        w2 = self.nodes[0].get_wallet_rpc("w2")
+        w3 = self.nodes[0].get_wallet_rpc("w3")
+        wallet_bad = self.nodes[0].get_wallet_rpc("wallet/bad")


nit: Should be get_wallet_rpc('bad') to qualify for refactor-only.

maflcko · 2017-08-15T14:11:21Z

test/functional/test_framework/test_node.py

+
+    def start(self):
+        """Start the node."""
+        # import pdb; pdb.set_trace()


nit: debug artifact

maflcko · 2017-08-15T14:19:45Z

test/functional/test_framework/test_framework.py

-        return rpcs
+
+        if self.options.coveragedir is not None:
+            coverage.write_all_rpc_commands(self.options.coveragedir, node.rpc)


missing for node in nodes?

maflcko · 2017-08-15T14:24:21Z

test/functional/test_framework/test_framework.py

-        assert not self.bitcoind_processes.values()  # All connections must be gone now
+        for node in self.nodes:
+            node.stop_node()
+        # All connections must be gone now


nit: remove outdated comment

maflcko · 2017-08-15T14:33:01Z

test/functional/test_framework/test_node.py

+                self.rpc_connected = True
+                self.url = self.rpc.url
+                self.log.debug("RPC successfully started")
+                return True


nit: No need to return a value if it is never used. Can be replaced by a plain return.

maflcko · 2017-08-15T14:48:33Z

test/functional/test_framework/test_framework.py

        try:
            for i in range(num_nodes):
-                rpcs.append(self.start_node(i, dirname, extra_args[i], rpchost, timewait=timewait, binary=binary[i]))
+                nodes.append(TestNode(i, dirname, extra_args[i], rpchost, timewait=timewait, binary=binary[i], stderr=None, mocktime=self.mocktime, coverage_dir=self.options.coveragedir))


stderr must be set to not None. Otherwise, we will ignore any output to stderr, no?

This is the same as the existing behaviour. start_nodes() currently calls start_node() without the stderr argument set, so it defaults to None.

This could be changed in a future PR.

TestNode is a class responsible for all state related to a bitcoind node under test. It stores local state, is responsible for tracking the bitcoind process and delegates unrecognised messages to the RPC connection. This commit changes start_nodes and stop_nodes to start and stop the bitcoind nodes in parallel, making test setup and teardown much faster.

jnewbery · 2017-08-15T17:14:12Z

Thanks for the review @MarcoFalke - I've addressed all your nits (except the stderr in start_nodes() which I think maintains existing behaviour).

I've updated and squashed the commits. Unsquashed history is here: https://github.com/jnewbery/bitcoin/tree/pr10711.1 if you just want to see what's changed.

maflcko · 2017-08-15T21:32:46Z

re-utACK 7897338, thanks for addressing the feedback.

I keep a local copy of all commits, so no need to provide branches of the history for me.

c6ec435 [tests] Add bitcoin_cli.py test script (John Newbery) b23549f [tests] add TestNodeCLI class for calling bitcoin-cli for a node (John Newbery) Pull request description: We don't test bitcoin-cli at all. That means that we can miss inconsistencies between the bitcoin-cli client and the RPC interface, such as #10698 and #10747. It also means that the various bitcoin-cli options and features are untested and regressions could be silently introduced. Let's fix that. This PR adds bitcoin-cli testing in the python functional test_framework: 1. Add a bitcoin_cli.py test script that tests bitcoin-cli. At the moment it only tests that the result of `getinfo` is the same if you run it as an RPC or through bitcoin-cli, but can easily be extended to test additional bitcoin-cli features **EDIT: `--usecli` option is moved to a separate PR. This PR now only covers the bitcoin_cli.py test.** 2. ~Add a `--usecli` option to the test framework. This changes the test to use bitcoin-cli for all RPC calls instead of using direct HTTP requests. This is somewhat experimental. It works for most tests, but there are some cases where it can't work transparently because:~ - ~the testcase is asserting on a specific error code, and bitcoin-cli returns a different error code from the direct RPC~ - ~we're sending a very large RPC request (eg `submitblock`) and it can't be serialized into a shell bitcoin-cli call.~ ~I think that even though `--usecli` doesn't work on all tests, it's still a useful experimental feature. Future potential enhancements:~ - ~enhance the framework to automatically skip tests that are known to fail with bitcoin-cli if the `--usecli` option is used.~ - ~run a subset of tests in Travis with `-usecli`~ This builds on and requires the `TestNode` PR #10711 . As an aside, this is a good demonstration of how tidy it is to add additional features/interfaces now that test node logic/state is encapsulated in a TestNode class. Addresses #10791 Tree-SHA512: a1e6be12e8e007f6f67b3d3bbcd142d835787300831eb38e6027a1ad25ca9d79c4bc99a41b19e31ee95205cba1b3b2d21a688b5909316aad70bfc2b4eb6d8a52

TestNode is a class responsible for all state related to a bitcoind node under test. It stores local state, is responsible for tracking the bitcoind process and delegates unrecognised messages to the RPC connection. This commit changes start_nodes and stop_nodes to start and stop the bitcoind nodes in parallel, making test setup and teardown much faster. Github-Pull: bitcoin#10711 Rebased-From: 7897338

c6ec435 [tests] Add bitcoin_cli.py test script (John Newbery) b23549f [tests] add TestNodeCLI class for calling bitcoin-cli for a node (John Newbery) Pull request description: We don't test bitcoin-cli at all. That means that we can miss inconsistencies between the bitcoin-cli client and the RPC interface, such as bitcoin#10698 and bitcoin#10747. It also means that the various bitcoin-cli options and features are untested and regressions could be silently introduced. Let's fix that. This PR adds bitcoin-cli testing in the python functional test_framework: 1. Add a bitcoin_cli.py test script that tests bitcoin-cli. At the moment it only tests that the result of `getinfo` is the same if you run it as an RPC or through bitcoin-cli, but can easily be extended to test additional bitcoin-cli features **EDIT: `--usecli` option is moved to a separate PR. This PR now only covers the bitcoin_cli.py test.** 2. ~Add a `--usecli` option to the test framework. This changes the test to use bitcoin-cli for all RPC calls instead of using direct HTTP requests. This is somewhat experimental. It works for most tests, but there are some cases where it can't work transparently because:~ - ~the testcase is asserting on a specific error code, and bitcoin-cli returns a different error code from the direct RPC~ - ~we're sending a very large RPC request (eg `submitblock`) and it can't be serialized into a shell bitcoin-cli call.~ ~I think that even though `--usecli` doesn't work on all tests, it's still a useful experimental feature. Future potential enhancements:~ - ~enhance the framework to automatically skip tests that are known to fail with bitcoin-cli if the `--usecli` option is used.~ - ~run a subset of tests in Travis with `-usecli`~ This builds on and requires the `TestNode` PR bitcoin#10711 . As an aside, this is a good demonstration of how tidy it is to add additional features/interfaces now that test node logic/state is encapsulated in a TestNode class. Addresses bitcoin#10791 Tree-SHA512: a1e6be12e8e007f6f67b3d3bbcd142d835787300831eb38e6027a1ad25ca9d79c4bc99a41b19e31ee95205cba1b3b2d21a688b5909316aad70bfc2b4eb6d8a52

jnewbery mentioned this pull request Jun 30, 2017

[tests] Demostration/concept: Add a TestNode class to test_Framework.py #10082

Closed

6 tasks

fanquake added the Tests label Jun 30, 2017

jtimon reviewed Jul 10, 2017

View reviewed changes

jnewbery force-pushed the testnode2 branch from dcc0ed7 to 56b2301 Compare July 11, 2017 14:11

jnewbery mentioned this pull request Jul 11, 2017

[tests] [utils] test bitcoin-cli #10798

Merged

jnewbery force-pushed the testnode2 branch 2 times, most recently from 16b9588 to 997680c Compare July 24, 2017 15:35

jnewbery force-pushed the testnode2 branch from 997680c to e25b0cf Compare August 9, 2017 20:48

jimmysong reviewed Aug 9, 2017

View reviewed changes

jnewbery force-pushed the testnode2 branch from e25b0cf to 2d189c8 Compare August 9, 2017 22:01

kallewoof approved these changes Aug 10, 2017

View reviewed changes

jnewbery force-pushed the testnode2 branch 2 times, most recently from db74b1a to cb8579b Compare August 14, 2017 16:17

maflcko suggested changes Aug 15, 2017

View reviewed changes

jnewbery force-pushed the testnode2 branch from aacc234 to 7897338 Compare August 15, 2017 17:12

maflcko merged commit 7897338 into bitcoin:master Aug 15, 2017

jnewbery deleted the testnode2 branch August 15, 2017 21:44

maflcko mentioned this pull request Nov 22, 2017

qa: Add multiwallet prefix test #11743

Merged

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Conversation

jnewbery commented Jun 30, 2017

Uh oh!

jnewbery commented Jun 30, 2017

Uh oh!

meshcollider commented Jul 4, 2017

Uh oh!

laanwj commented Jul 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnewbery commented Jul 11, 2017

Uh oh!

jnewbery commented Jul 24, 2017

Uh oh!

maflcko commented Aug 8, 2017

Uh oh!

jnewbery commented Aug 9, 2017

Uh oh!

jimmysong left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NicolasDorier commented Aug 10, 2017

Uh oh!

kallewoof left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NicolasDorier Aug 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

laanwj commented Jul 10, 2017 •

edited

Loading

NicolasDorier Aug 10, 2017 •

edited

Loading