TestNode tidyups #11121

jnewbery · 2017-08-23T20:53:24Z

Some additional tidyups after the introduction of TestNode:

commit 1 makes TestNode use the correct rpc timeout. This should have been included in [tests] fix timeout issues from TestNode #11077
commit 2 separates add_node() from start_node() as originally discussed here: Move stop/start functions from utils.py into BitcoinTestFramework #10556 (comment) with @kallewoof . The test writer no longer needs to assign to self.nodes when starting/stopping nodes.
commit 3 adds a set_test_params() method, so individual tests don't need to override __init__() and call super().__init__()

maflcko

Concept ACK after quick glance

maflcko · 2017-08-23T21:24:18Z

test/functional/txn_clone.py

I prefer to explicitly set the network topology and initial state of the chain.

Faint agreement. It's easy to forget and start up 4 nodes when you only really need 2, for example. Not seeing the number of nodes makes that even harder to remember.

Sounds good. I've added a new commit that makes setting self.num_nodes mandatory. I haven't done the same for self.setup_clean_chain, since I think it makes more sense for that to have a default. If people feel strongly, it can be added in a future commit.

maflcko · 2017-08-23T21:26:51Z

test/functional/test_framework/test_framework.py

should raise RuntimeError('Not implemented') to enforce the docstring.

kallewoof

utACK 06bde74f5ec8660975275f8594aee3a99064e327

kallewoof · 2017-08-24T07:53:13Z

test/functional/test_framework/test_framework.py

Since self knows about self.options.tmpdir, and all calls to self.add_nodes passes self.options.tmpdir as dirname argument, it seems clean to just give it a default None and to set to self.options.tmpdir if None in add_nodes.
Or perhaps remove it entirely and always use self.options.tmpdir. Will it ever be anything else?

Good point. Thanks! Added this in a new commit.

kallewoof · 2017-08-24T07:53:55Z

test/functional/test_framework/test_node.py

extra_args=None, stderr=None maybe?

TestNode.start is only ever called by TestFramework.start_node, which has its own default for extra_args, so not strictly necessary, but I agree having a default here makes it clearer. Added.

kallewoof · 2017-08-24T07:55:57Z

test/functional/txn_clone.py

Faint agreement. It's easy to forget and start up 4 nodes when you only really need 2, for example. Not seeing the number of nodes makes that even harder to remember.

kallewoof · 2017-08-24T08:00:35Z

test/functional/wallet.py

Why does it not just add_nodes(3, ..) here and then add/start/connect the fourth node below? That would remove a lot of necessary [self.nodes[0:3]] stuff.

Because there's not currently a method to add a single node. The model after this PR is to add all nodes at the top of the test and then start/stop them as required during the test.

We could potentially add an add_node() method to be called during the test, but lets leave that for a future PR.

jnewbery · 2017-08-24T16:36:36Z

@MarcoFalke @kallewoof - thanks for the review. I think I've responded to all your comments.

New commits pushed, and rebased on master.

kallewoof · 2017-08-25T01:04:03Z

utACK df824a9

promag

Have you thought of not having any node started automatically? There are some tests that start with stop_node(0)..

So eachrun_test(self) the nodes are explicitly started, num_nodes can go away, even setup_clean_chain would be a start_node argument.

promag · 2017-08-25T14:05:41Z

test/functional/assumevalid.py

It seems there are more setup_clean_chain = True so why not make it the default?

I haven't made any changes to setup_clean_chain. See earlier comment:

I haven't done the same for self.setup_clean_chain, since I think it makes more sense for that to have a default. If people feel strongly, it can be added in a future commit.

Feel free to open a PR to change/remove the default if you think that's necessary. I don't think it's required for this PR.

jnewbery · 2017-08-25T15:23:52Z

Thanks for the reivew @promag

Have you thought of not having any node started automatically? There are some tests that start with stop_node(0)

Tests that don't need their nodes to start should override the setup_network() or setup_nodes() methods. The vast majority of tests require nodes to be started at the top of the test, so that's the default.

setup_clean_chain would be a start_node argument.

setup_clean_chain is a property of the pre-mined chain. I don't think we should change it to be a property of the individual node (if you disagree please open a separate PR to demonstrate the change).

maflcko · 2017-08-30T10:40:59Z

test/functional/bumpfee.py

This is already done by setup_network.

Same for the line below.

L41 stops node 1 (the encryptwallet RPC shuts down the node), so I believe all the p2p connections are lost and we need to reconnect the nodes after restarting node 1.

I've tested removing these two lines and the test fails (the next call to sync_all() fails)

Sorry, missed that the node shuts down.

maflcko

utACK df824a92121eb68849e474e87098a441d4e52926

maflcko · 2017-09-01T15:23:58Z

test/functional/pruning.py

You are removing timewait=900 from all calls but the first. I assume this does not cause issues?

rpc_timeout is a property of TestNode set when instantiating the python object, not when starting up the bitcoind node.

maflcko · 2017-09-01T15:40:02Z

test/functional/wallet.py

debug statement?

derp. Fixed

maflcko · 2017-09-01T15:40:11Z

test/functional/wallet.py

debug statement?

maflcko · 2017-09-01T15:42:37Z

test/functional/wallet.py

why not self.nodes[:3]?

maflcko · 2017-09-01T15:42:42Z

test/functional/wallet.py

why not self.nodes[:3]?

maflcko · 2017-09-01T15:56:55Z

test/functional/zapwallettxes.py

why is this changed in a refactoring commit?

Separates the act of creating a TestNode object from starting the node. The test_framework now keeps track of its list of TestNodes, and test writers can call start_node() and stop_node() without having to update the self.nodes list.

Almost all test scripts currently need to override the __init__() method. When they do that they need to call into super().__init__() as the base class does some generic initialization. This commit makes the base class __init__() call into set_test_params() method. Individual test cases can override set_test_params() to setup their test parameters.

jnewbery · 2017-09-01T16:32:26Z

Thanks @MarcoFalke . Review comments addressed.

maflcko · 2017-09-01T16:44:22Z

re-utACK 7148b74

@kallewoof

7148b74 [tests] Functional tests must explicitly set num_nodes (John Newbery) 5448a14 [tests] don't override __init__() in individual tests (John Newbery) 6cf094a [tests] Avoid passing around member variables in test_framework (John Newbery) 36b6268 [tests] TestNode: separate add_node from start_node (John Newbery) be2a2ab [tests] fix - use rpc_timeout as rpc timeout (John Newbery) Pull request description: Some additional tidyups after the introduction of TestNode: - commit 1 makes TestNode use the correct rpc timeout. This should have been included in #11077 - commit 2 separates `add_node()` from `start_node()` as originally discussed here: #10556 (comment) with @kallewoof . The test writer no longer needs to assign to `self.nodes` when starting/stopping nodes. - commit 3 adds a `set_test_params()` method, so individual tests don't need to override `__init__()` and call `super().__init__()` Tree-SHA512: 0adb030623b96675b5c29e2890ce99ccd837ed05f721d0c91b35378c5ac01b6658174aac12f1f77402e1d38b61f39b3c43b4df85c96952565dde1cda05b0db84

3918d93 [tests] fixups from set_test_params() (John Newbery) Pull request description: #11121 had a silent merge conflict in `bitcoin_cli.py`. This fixes it. Also fixes a comment in `example_test.py` Tree-SHA512: f22a645c51c9aeda005526338ad6f2ee07f2bab172847fc54f51fecf1c79e778970be61e5e637d4e0aba3a02e5aba0737b0b57f1bc11a514a1acd94c221b54d6

@MarcoFalke

8fdb6f9 [tests] fixup dbcrash interaction with add_nodes() (John Newbery) Pull request description: Another conflict with #11121. Apologies - this is entirely my fault. I didn't run the extended test suite after rebasing on master. @MarcoFalke @sdaftuar Tree-SHA512: eefce1d1c63dc4a63c5e030a541e046ad4832e8a709c0a8aad40ffdc4712b2065486778b406dfa57cfd34e66db86064278ee3fea8f2c2afd2390772875e6fa3e

…nt future similar errors f97ab35 qa: Fix bug introduced in p2p-segwit.py (Suhas Daftuar) a782042 qa: Treat mininode p2p exceptions as fatal (Suhas Daftuar) Pull request description: #11121 inadvertently broke the constructor for the `TestNode()` object in `p2p-segwit.py`, silently breaking at least one of the tests. Although the python code was raising exceptions due to a `TestNode()` object not existing (or having the right type), mininode was masking these from anyone running the test through the test_runner (like travis), because it catches all exceptions during message delivery and just prints a log message and continues. Such "graceful" handling of errors is almost certainly something we don't want in our test suite, so the first commit here attempts to prevent that type of failure from ever being masked. The second commit fixes the particular bug in `p2p-segwit.py`. Tree-SHA512: b6646e3cb1e05c35c28e8899c44104bf2e2d0384643ca87042ab7f6ec0960d89f5bf25a7b95bab6e32d401c20a6018226160500f6ddceb923e81ffb04adb4f2f

Github-Pull: bitcoin#11121 Rebased-From: be2a2ab

Separates the act of creating a TestNode object from starting the node. The test_framework now keeps track of its list of TestNodes, and test writers can call start_node() and stop_node() without having to update the self.nodes list. Github-Pull: bitcoin#11121 Rebased-From: 36b6268

Github-Pull: bitcoin#11121 Rebased-From: 6cf094a

Almost all test scripts currently need to override the __init__() method. When they do that they need to call into super().__init__() as the base class does some generic initialization. This commit makes the base class __init__() call into set_test_params() method. Individual test cases can override set_test_params() to setup their test parameters. Github-Pull: bitcoin#11121 Rebased-From: 5448a14

Github-Pull: bitcoin#11121 Rebased-From: 7148b74

@kallewoof

7148b74 [tests] Functional tests must explicitly set num_nodes (John Newbery) 5448a14 [tests] don't override __init__() in individual tests (John Newbery) 6cf094a [tests] Avoid passing around member variables in test_framework (John Newbery) 36b6268 [tests] TestNode: separate add_node from start_node (John Newbery) be2a2ab [tests] fix - use rpc_timeout as rpc timeout (John Newbery) Pull request description: Some additional tidyups after the introduction of TestNode: - commit 1 makes TestNode use the correct rpc timeout. This should have been included in bitcoin#11077 - commit 2 separates `add_node()` from `start_node()` as originally discussed here: bitcoin#10556 (comment) with @kallewoof . The test writer no longer needs to assign to `self.nodes` when starting/stopping nodes. - commit 3 adds a `set_test_params()` method, so individual tests don't need to override `__init__()` and call `super().__init__()` Tree-SHA512: 0adb030623b96675b5c29e2890ce99ccd837ed05f721d0c91b35378c5ac01b6658174aac12f1f77402e1d38b61f39b3c43b4df85c96952565dde1cda05b0db84

3918d93 [tests] fixups from set_test_params() (John Newbery) Pull request description: bitcoin#11121 had a silent merge conflict in `bitcoin_cli.py`. This fixes it. Also fixes a comment in `example_test.py` Tree-SHA512: f22a645c51c9aeda005526338ad6f2ee07f2bab172847fc54f51fecf1c79e778970be61e5e637d4e0aba3a02e5aba0737b0b57f1bc11a514a1acd94c221b54d6

@MarcoFalke

8fdb6f9 [tests] fixup dbcrash interaction with add_nodes() (John Newbery) Pull request description: Another conflict with bitcoin#11121. Apologies - this is entirely my fault. I didn't run the extended test suite after rebasing on master. @MarcoFalke @sdaftuar Tree-SHA512: eefce1d1c63dc4a63c5e030a541e046ad4832e8a709c0a8aad40ffdc4712b2065486778b406dfa57cfd34e66db86064278ee3fea8f2c2afd2390772875e6fa3e

jnewbery force-pushed the test_node_tidyups branch from d3c2ea2 to 06bde74 Compare August 23, 2017 20:54

fanquake added the Tests label Aug 23, 2017

maflcko reviewed Aug 23, 2017

View reviewed changes

kallewoof reviewed Aug 24, 2017

View reviewed changes

jnewbery force-pushed the test_node_tidyups branch from 06bde74 to e67f8d9 Compare August 24, 2017 16:35

jnewbery force-pushed the test_node_tidyups branch from e67f8d9 to df824a9 Compare August 24, 2017 18:14

promag reviewed Aug 25, 2017

View reviewed changes

maflcko reviewed Aug 30, 2017

View reviewed changes

maflcko reviewed Sep 1, 2017

View reviewed changes

jnewbery added 5 commits September 1, 2017 12:25

[tests] fix - use rpc_timeout as rpc timeout

be2a2ab

[tests] TestNode: separate add_node from start_node

36b6268

Separates the act of creating a TestNode object from starting the node. The test_framework now keeps track of its list of TestNodes, and test writers can call start_node() and stop_node() without having to update the self.nodes list.

[tests] Avoid passing around member variables in test_framework

6cf094a

[tests] Functional tests must explicitly set num_nodes

7148b74

jnewbery force-pushed the test_node_tidyups branch from df824a9 to 7148b74 Compare September 1, 2017 16:26

maflcko merged commit 7148b74 into bitcoin:master Sep 1, 2017

This was referenced Sep 1, 2017

[tests] fixups from set_test_params() #11215

Merged

[RPC][mempool]: Add savemempool RPC #11099

Merged

jnewbery mentioned this pull request Sep 4, 2017

[tests] fixup dbcrash interaction with add_nodes() #11230

Merged

sdaftuar mentioned this pull request Sep 13, 2017

[qa] Fix error introduced into p2p-segwit.py, and prevent future similar errors #11319

Merged

maflcko pushed a commit to maflcko/bitcoin-core that referenced this pull request Oct 3, 2017

[tests] fix - use rpc_timeout as rpc timeout

11a5992

Github-Pull: bitcoin#11121 Rebased-From: be2a2ab

maflcko pushed a commit to maflcko/bitcoin-core that referenced this pull request Oct 3, 2017

[tests] Avoid passing around member variables in test_framework

bb5e7cb

Github-Pull: bitcoin#11121 Rebased-From: 6cf094a

maflcko pushed a commit to maflcko/bitcoin-core that referenced this pull request Oct 3, 2017

[tests] Functional tests must explicitly set num_nodes

82bf6fc

Github-Pull: bitcoin#11121 Rebased-From: 7148b74

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

TestNode tidyups #11121

TestNode tidyups #11121

Uh oh!

Conversation

jnewbery commented Aug 23, 2017

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!

kallewoof 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!

jnewbery commented Aug 24, 2017

Uh oh!

kallewoof commented Aug 25, 2017

Uh oh!

promag 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!

jnewbery commented Aug 25, 2017

Uh oh!

maflcko Aug 30, 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!

maflcko left a comment • 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!

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

maflcko Aug 30, 2017 •

edited

Loading

maflcko left a comment •

edited

Loading