[test] functional: allow custom cwd, use tmpdir as default #15415

Sjors commented at 12:00 PM on February 15, 2019: member

Any process launched by bitcoind will have self.datadir as its cwd.

Sjors commented at 12:03 PM on February 15, 2019: member

I use this in #14912 in order to mock a hardware wallet. The functional first generates a signed PSBT which it writes to a file. The test node then calls a mock signer python script, which reads that file. The latter scripts gets it cwd from bitcoind.

Perhaps the tmp root dir of the functional tests runner is a better choice? cc @jnewbery

fanquake added the label Tests on Feb 15, 2019

MarcoFalke commented at 12:14 PM on February 15, 2019: member

Why would you want to do this? Imo you can't rely a user is doing the same. I'd prefer to pass in the absulute paths from outside or specify with a flag that it should be run from relative to the datadir

Sjors commented at 12:26 PM on February 15, 2019: member

@MarcoFalke this only affects the test suite. The mock signer script I created isn't itself part of the test suite, so it has no idea where everything lives on the system.

Note that the real thing (HWI) doesn't care where it's called from. It doesn't need to know where Bitcoin Core lives.

promag commented at 1:30 PM on February 15, 2019: member

The latter scripts gets it cwd from bitcoind.

Why? Is that a requirement?

MarcoFalke commented at 1:56 PM on February 15, 2019: member

If you call that script from bitcoind, you might as well pass the datadir to the script?

Sjors force-pushed on Feb 15, 2019

Sjors commented at 2:45 PM on February 15, 2019: member

@MarcoFalke that would involve something like this:

bitcoind -signer="/.../mock.py --mock_cwd=/tmp/.../node1/..".

That --mock_cwd=/.../ argument would then be passed along to all calls to mock.py and concatenated with the other arguments. That would probably work, but then it deviates from the way -signer should normally be used.

Sjors renamed this:
~~[test] functional: set node cwd to datadir~~
[test] functional: allow custom cwd, use tmpdir as default
on Feb 15, 2019

MarcoFalke commented at 3:09 PM on February 15, 2019: member

You can pass the boost::process::start_dir as datadir in runCommandParseJSON?

in test/functional/test_framework/test_framework.py:125 in 3d323a658c outdated

 120 | @@ -121,6 +121,8 @@ def main(self):
 121 |                              help="The seed to use for assigning port numbers (default: current process id)")
 122 |          parser.add_argument("--coveragedir", dest="coveragedir",
 123 |                              help="Write tested RPC commands into this directory")
 124 | +        parser.add_argument("--cwd", dest="cwd",
 125 | +                            help="Set current working directory for nodes (default: tmpdir)")

MarcoFalke commented at 3:21 PM on February 15, 2019:

NACK, this should be added to the specific test that needs it, not globally

Sjors commented at 3:25 PM on February 15, 2019: member

You can pass the boost::process::start_dir as datadir in runCommandParseJSON?

I don't want to modify the real world behaviour of runCommandParseJSON(). That should just use the cwd of however bitcoind was started, and that generally shouldn't be the datadir.

DrahtBot commented at 3:48 PM on February 15, 2019: 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:

#15419 (qa: Always refresh rotten cache to be out of ibd by MarcoFalke)

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.

Sjors force-pushed on Feb 15, 2019

Sjors commented at 4:54 PM on February 15, 2019: member

I simplified it: it now merely sets the default cwd to the temp dir. Tests that need to override this can pass cwd= via args* to node.start(), which wallet_multiwallet.py already does.

It's now actually a bug fix: try creating a directory wallets in master and then run wallet_multiwallet.py; it will fail.

Sjors force-pushed on Feb 15, 2019

MarcoFalke commented at 5:02 PM on February 15, 2019: member

It's now actually a bug fix: try creating a directory wallets in master and then run wallet_multiwallet.py; it will fail.

Please elaborate. this should not happen

Sjors commented at 5:07 PM on February 15, 2019: member

Before this PR the test suite would launch nodes with the cwd set to wherever you're launching the test suite from. wallet_multiwallet.py then does the following:

self.nodes[0].assert_start_raises_init_error(['-walletdir=wallets'], 'Error: Specified -walletdir "wallets" does not exist')
self.nodes[0].assert_start_raises_init_error(['-walletdir=wallets'], 'Error: Specified -walletdir "wallets" is a relative path', cwd=data_dir())
self.nodes[0].assert_start_raises_init_error(['-walletdir=debug.log'], 'Error: Specified -walletdir "debug.log" is not a directory', cwd=data_dir())

That first test will fail if the directory you launch the test from contains a wallets directory.

This PR fixes that by setting cwd for each node to the temp directory. The second and third command show how to override that default.

The existing behavior of assert_start_raises_init_error is to pass extra arguments like cwd on to the subprocess call, which is a bit of a hack IMO, but this PR doesn't touch that.

in test/functional/test_framework/test_node.py:175 in 9548f93c55 outdated

 171 | @@ -171,7 +172,7 @@ def __getattr__(self, name):
 172 |              assert self.rpc_connected and self.rpc is not None, self._node_msg("Error: no RPC connection")
 173 |              return getattr(self.rpc, name)
 174 |  
 175 | -    def start(self, extra_args=None, *, stdout=None, stderr=None, **kwargs):
 176 | +    def start(self, extra_args=None, cwd=None, *, stdout=None, stderr=None, **kwargs):

MarcoFalke commented at 5:51 PM on February 15, 2019:

style-nit: Please move this to after the , *, , otherwise it will be possible to supply this as non-named arg

in test/functional/test_framework/test_framework.py:473 in 9548f93c55 outdated

 469 | @@ -469,6 +470,7 @@ def _initialize_chain(self):
 470 |                      bitcoin_cli=self.options.bitcoincli,
 471 |                      mocktime=self.mocktime,
 472 |                      coverage_dir=None,
 473 | +                    cwd=self.options.tmpdir

MarcoFalke commented at 5:52 PM on February 15, 2019:

style-nit: Please add a trailing comma, otherwise formatters such as https://github.com/MarcoFalke/yapf-diff will grind on this

MarcoFalke approved

MarcoFalke added this to the milestone 0.18.0 on Feb 15, 2019

Sjors force-pushed on Feb 15, 2019

meshcollider commented at 7:55 PM on February 16, 2019: contributor

utACK https://github.com/bitcoin/bitcoin/commit/2cb2f546ee5efac8bb27a628ffa4bcfe5a7c99de

[test] functional: set cwd of nodes to tmpdir e3e1a5631e

in test/functional/test_framework/test_node.py:175 in 2cb2f546ee outdated

 171 | @@ -171,7 +172,7 @@ def __getattr__(self, name):
 172 |              assert self.rpc_connected and self.rpc is not None, self._node_msg("Error: no RPC connection")
 173 |              return getattr(self.rpc, name)
 174 |  
 175 | -    def start(self, extra_args=None, *, stdout=None, stderr=None, **kwargs):
 176 | +    def start(self, extra_args=None, cwd=None, stdout=None, stderr=None, **kwargs):

MarcoFalke commented at 2:15 PM on February 18, 2019:

    def start(self, extra_args=None, *, cwd=None, stdout=None, stderr=None, **kwargs):

Any reason why you removed the star? It should stay there

Sjors commented at 7:53 AM on February 19, 2019:

Oops, I misread your original comment.

Sjors force-pushed on Feb 19, 2019

laanwj commented at 3:06 PM on February 19, 2019: member

@MarcoFalke I suppose you've retracted your NACK above?

MarcoFalke commented at 3:12 PM on February 19, 2019: member

utACK e3e1a5631e

MarcoFalke commented at 3:13 PM on February 19, 2019: member

@MarcoFalke I suppose you've retracted your NACK above?

Yes, this is a bugfix as pointed out by @Sjors in an earlier comment

MarcoFalke referenced this in commit 3e4fd40753 on Feb 19, 2019

MarcoFalke merged this on Feb 19, 2019

MarcoFalke closed this on Feb 19, 2019

jnewbery commented at 3:33 PM on February 19, 2019: member

utACK e3e1a5631e2456bb40844862ef35c07c1d1c47b0

Sjors deleted the branch on Feb 19, 2019

deadalnix referenced this in commit 8c7b2b836e on Jul 30, 2020

Munkybooty referenced this in commit 8e368a3317 on Sep 5, 2021

Munkybooty referenced this in commit aee2b0d00c on Sep 7, 2021

Munkybooty referenced this in commit 5355564753 on Sep 7, 2021

Munkybooty referenced this in commit 441a2001d1 on Sep 7, 2021

DrahtBot locked this on Dec 16, 2021