Revert "tests: Support calling add_nodes more than once" #14951

pull MarcoFalke wants to merge 2 commits into bitcoin:master from MarcoFalke:Mf1812-TestrevertAddNodes changing 1 files +17 −3

MarcoFalke commented at 5:09 PM on December 13, 2018: member

Writing tests should be straightforward and with little side-effects as possible.

I don't see how this is needed and can not be achieved with self.num_nodes (and self.extra_args et al.)
Revert "tests: Support calling add_nodes more than once"
This reverts commit 98a1846b00d9c3076d6dcd96244fae6f923e26a0.
faa831102a
MarcoFalke added the label Tests on Dec 13, 2018
jnewbery commented at 5:57 PM on December 13, 2018: member

utACK faa831102a0ce11b673b72081eed159ad0c17159

reasons: #14805 (comment)

sorry @stevenroose !
laanwj commented at 6:36 PM on December 13, 2018: member

~~I'm not sure this is worth reverting, but okay~~

utACK faa831102a0ce11b673b72081eed159ad0c17159 (really confused by Travis here)
jnewbery commented at 7:18 PM on December 13, 2018: member

Can you add a comment to add_nodes() explaining why it should only be called once.
test: add_nodes can only be called once after set_test_params fa4b8c90d3
MarcoFalke commented at 8:10 PM on December 13, 2018: member

Is there a reason other than it being the design we want it to? I added a comment to mention that it should be called once after the parameters for all nodes have been set up.
jnewbery commented at 9:14 PM on December 13, 2018: member

Is there a reason other than it being the design we want it to?

That's really it. Some of the test framework infrastructure relies on the num_nodes not changing. We could change that, but I think declaring the number of nodes at the start of the test is a good thing to do anyway.

in test/functional/test_framework/test_framework.py:298 in fa4b8c90d3

 294 | @@ -292,8 +295,19 @@ def add_nodes(self, num_nodes, extra_args=None, *, rpchost=None, binary=None):
 295 |          assert_equal(len(extra_args), num_nodes)
 296 |          assert_equal(len(binary), num_nodes)
 297 |          for i in range(num_nodes):
 298 | -            numnode = len(self.nodes)
 299 | -            self.nodes.append(TestNode(numnode, get_datadir_path(self.options.tmpdir, numnode), rpchost=rpchost, timewait=self.rpc_timewait, bitcoind=binary[i], bitcoin_cli=self.options.bitcoincli, mocktime=self.mocktime, coverage_dir=self.options.coveragedir, extra_conf=extra_confs[i], extra_args=extra_args[i], use_cli=self.options.usecli))
 300 | +            self.nodes.append(TestNode(

laanwj commented at 9:55 PM on December 13, 2018:

much better readable

stevenroose commented at 10:25 AM on December 14, 2018: contributor

I added a comment to mention that it should be called once after the parameters for all nodes have been set up.

That's what a method setup_nodes would be expected to do, IMO :)

Fair enough. So the correct was is to call add_nodes once and build a list of the binaries, chain names, etc beforehand?
jnewbery commented at 3:41 PM on December 14, 2018: member

So the correct was is to call add_nodes once and build a list of the binaries, chain names, etc beforehand?

Yes. If necessary, the node's arguments and config file can be updated later with TestNode.args and append_config().
MarcoFalke referenced this in commit b53573e5c6 on Dec 14, 2018
MarcoFalke merged this on Dec 14, 2018
MarcoFalke closed this on Dec 14, 2018
MarcoFalke deleted the branch on Dec 14, 2018
Munkybooty referenced this in commit 436e3b4fc3 on Aug 8, 2021
christiancfifi referenced this in commit 496d6091f1 on Aug 24, 2021
christiancfifi referenced this in commit efc7f15f49 on Aug 24, 2021
christiancfifi referenced this in commit 333fd55a92 on Aug 25, 2021
christiancfifi referenced this in commit 22eb00558c on Aug 26, 2021
MarcoFalke locked this on Sep 8, 2021

Contributors

Labels