tests: Support calling add_nodes more than once #14805

stevenroose commented at 11:02 AM on November 26, 2018: contributor

Ran into this while writing a multi-chain test for Elements where I call this method more than once.

tests: Support calling add_nodes more than once 98a1846b00

fanquake added the label Tests on Nov 26, 2018

in test/functional/test_framework/test_framework.py:296 in 98a1846b00

 291 | @@ -292,7 +292,8 @@ def add_nodes(self, num_nodes, extra_args=None, *, rpchost=None, binary=None):
 292 |          assert_equal(len(extra_args), num_nodes)
 293 |          assert_equal(len(binary), num_nodes)
 294 |          for i in range(num_nodes):
 295 | -            self.nodes.append(TestNode(i, get_datadir_path(self.options.tmpdir, i), 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))
 296 | +            numnode = len(self.nodes)
 297 | +            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))

promag commented at 11:56 AM on November 26, 2018:

nit, we could return the added nodes.

promag commented at 11:56 AM on November 26, 2018: member

LGTM, restarted travis.

DrahtBot commented at 12:33 PM on November 26, 2018: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#14519 (tests: add utility to easily profile node performance with perf by jamesob)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

in test/functional/test_framework/test_framework.py:294 in 98a1846b00

 291 | @@ -292,7 +292,8 @@ def add_nodes(self, num_nodes, extra_args=None, *, rpchost=None, binary=None):
 292 |          assert_equal(len(extra_args), num_nodes)
 293 |          assert_equal(len(binary), num_nodes)
 294 |          for i in range(num_nodes):

jimmysong commented at 10:26 PM on November 26, 2018:

if you're not going to use i, this is better:

for _ in range(num_nodes):

promag commented at 10:43 PM on November 26, 2018:

It's used.

laanwj commented at 2:00 PM on December 13, 2018: member

LGTM, though we don't need it at the moment, this is a sufficiently small and useful change I don't see any reason to consider not merging it. utACK 98a1846b00d9c3076d6dcd96244fae6f923e26a0

laanwj merged this on Dec 13, 2018

laanwj closed this on Dec 13, 2018

laanwj referenced this in commit 914faf1784 on Dec 13, 2018

jnewbery commented at 4:00 PM on December 13, 2018: member

(sorry for the post-merge review. I only just saw this)

This isn't really safe and I suggest we revert it. add_nodes was designed to only be called once at the top of the test. The idea is that the number of required nodes is declared up front, and each node can then be stopped/started during the test.

We could relax that restriction if necessary, but I don't think it is necessary. Issues with this implementation:

_initialize_chain() and _initialize_chain_clean() both use the num_nodes property to create each node's directory with chainstate and wallet at the start of the test. If an extra node is added later with add_nodes() then it won't have this preconfigured directory, which will probably be unexpected and difficult to debug.
if add_nodes() is called later in the test with the num_nodes argument set to >1, each of the additional new nodes will have the same index (see self.nodes.append(TestNode(numnode,...) @stevenroose. I don't think you need this for your test in https://github.com/ElementsProject/elements/pull/458. You can declare the num_nodes for your test, override the setup_nodes() method to only start the ones you need at the beginning of the test, then update node.args for the nodes you want to update the arguments for before starting them.

Apologies - I should have commented the add_nodes() method more thoroughly when I added it.

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

Yeah, without documentation, the name add_nodes suggests that you can "add" nodes with it, implying it should be able to be called more than once.

if add_nodes() is called later in the test with the num_nodes argument set to >1, each of the additional new nodes will have the same index (see self.nodes.append(TestNode(numnode,...)

That is not true, that is exactly the issue that I solved.

So yeah, regarding your first point, I setup the num_nodes variable upfront and then add the nodes one by one, because they are all (4) different.

jnewbery commented at 3:38 PM on December 14, 2018: member

That is not true, that is exactly the issue that I solved.

Apologies, yes - you're right.

I believe the first point is still a problem.

pravblockc referenced this in commit 4ce12adffa on Aug 2, 2021

pravblockc referenced this in commit 1bd1a23359 on Aug 3, 2021

MarcoFalke locked this on Sep 8, 2021