Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from Feb 19, 2019

Conversation

Projects
None yet
8 participants
@Sjors
Copy link
Member

commented Feb 15, 2019

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

@Sjors

This comment has been minimized.

Copy link
Member Author

commented Feb 15, 2019

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 fanquake added the Tests label Feb 15, 2019

@Sjors Sjors referenced this pull request Feb 15, 2019

Open

[WIP] External signer support (e.g. hardware wallet) #14912

0 of 2 tasks complete
@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Feb 15, 2019

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

This comment has been minimized.

Copy link
Member Author

commented Feb 15, 2019

@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

This comment has been minimized.

Copy link
Member

commented Feb 15, 2019

The latter scripts gets it cwd from bitcoind.

Why? Is that a requirement?

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Feb 15, 2019

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

@Sjors Sjors force-pushed the Sjors:2019/02/test_cwd branch from 2fc2f3e to 3d323a6 Feb 15, 2019

@Sjors

This comment has been minimized.

Copy link
Member Author

commented Feb 15, 2019

@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 Sjors changed the title [test] functional: set node cwd to datadir [test] functional: allow custom cwd, use tmpdir as default Feb 15, 2019

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Feb 15, 2019

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

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

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Feb 15, 2019

Member

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

@Sjors

This comment has been minimized.

Copy link
Member Author

commented Feb 15, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Feb 15, 2019

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 Sjors force-pushed the Sjors:2019/02/test_cwd branch from 3d323a6 to 6496bc9 Feb 15, 2019

@Sjors

This comment has been minimized.

Copy link
Member Author

commented Feb 15, 2019

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 Sjors force-pushed the Sjors:2019/02/test_cwd branch from 6496bc9 to 9548f93 Feb 15, 2019

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Feb 15, 2019

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

This comment has been minimized.

Copy link
Member Author

commented Feb 15, 2019

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.

Show resolved Hide resolved test/functional/test_framework/test_node.py Outdated
Show resolved Hide resolved test/functional/test_framework/test_framework.py Outdated

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

@Sjors Sjors force-pushed the Sjors:2019/02/test_cwd branch from 9548f93 to 2cb2f54 Feb 15, 2019

@meshcollider

This comment has been minimized.

Copy link
Member

commented Feb 16, 2019

utACK 2cb2f54

@Sjors Sjors force-pushed the Sjors:2019/02/test_cwd branch from b828ff7 to e3e1a56 Feb 19, 2019

@laanwj

This comment has been minimized.

Copy link
Member

commented Feb 19, 2019

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

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Feb 19, 2019

utACK e3e1a56

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Feb 19, 2019

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

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

MarcoFalke added a commit to MarcoFalke/bitcoin that referenced this pull request Feb 19, 2019

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

e3e1a56 [test] functional: set cwd of nodes to tmpdir (Sjors Provoost)

Pull request description:

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

Tree-SHA512: 0b311643bb96c7dc2f693774620173243b3add40bf373284695af2f0071823b23485289fd2ffe152b7f63e0bfe989b16720077cfc2ce33905f9b8e7f2630f3c0

@MarcoFalke MarcoFalke merged commit e3e1a56 into bitcoin:master Feb 19, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jnewbery

This comment has been minimized.

Copy link
Member

commented Feb 19, 2019

utACK e3e1a56

@Sjors Sjors deleted the Sjors:2019/02/test_cwd branch Feb 19, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.