Add bitcoin-cli -stdin and -stdinrpcpass functional tests #11125

promag · 2017-08-24T17:47:50Z

This patch adds tests for bitcoin-cli options -stdin (#7550) and -stdinrpcpass #10997.

laanwj · 2017-08-24T18:30:00Z

test/functional/test_framework/test_node.py

We need a way to override what bitcoin-cli to use, akin to BITCOIND environment variable.

Which reminds me, didn't we have another PR to add bitcoin-cli testing support at some point?
(if so, might be better to do it on top of that)

You might be able to hack something up based on TestNodeCLI from current master branch.

This should already be possible with #10798 merged

laanwj · 2017-08-24T18:30:27Z

Concept ACK

jnewbery · 2017-08-24T18:32:17Z

Concept ACK, but I recommend you build on top of #10798 which already adds bitcoin-cli interface to TestNode.

promag · 2017-08-24T20:16:08Z

I'll refactor now that #10798 is merged.

promag · 2017-08-25T03:16:27Z

@jnewbery take a look at f275cf6, it allows to pass -stdin and -stdinrpcpass. Also, TestNodeCLI instances are only created when necessary.

Updated PR title and description accordingly.

jnewbery · 2017-08-25T15:42:45Z

See commit here for an alternative way of doing this: jnewbery@5bfd5e0

A few advantages:

bitcoin-cli command line arguments are passed in as a string and fed through to subprocess, rather than being parsed by python
It avoids the cludgy input magic key
it maintains the -datadir argument, which I think we want to keep (without that, bitcoin-cli won't have access to the rpcport value in the bitcoin.conf file).

I've proposed that as a test for #10871. Let me know what you think.

promag · 2017-08-25T18:02:04Z

It avoids the cludgy input magic key

@jnewbery how do you pass the stdin content?

jnewbery · 2017-08-25T18:08:31Z

ah sorry, I just looked at the commit at f275cf6 and misunderstood how input was used. I'll re-review.

maflcko · 2017-08-25T19:54:56Z

utACK 5b4e3391f6a7dc9e7148eaee05e285f200f46927

laanwj · 2017-08-28T07:49:47Z

test/functional/bitcoin_cli.py

I would prefer to make a temporary, empty datadir here instead of make it use the default. The tests should not touch ~/.bitcoin.

promag · 2017-08-28T07:55:00Z

Agree!

jnewbery

Looks generally good to me. A couple of nits inline.

jnewbery · 2017-08-28T14:21:43Z

test/functional/test_framework/test_node.py

I think you should add an error here. If args is specified, then datadir must be one of the args (otherwise bitcoin-cli will look in ~/.bitcoin).

Another option would be to set datadir=self.datadir if it isn't overridden otherwise. As you say, there is no reason the argument should ever be missing.

I think either approach is fine. Making it an error to not include it in args makes it more explicit to the test writer (ie datadir doesn't get set magically for them).

jnewbery · 2017-08-28T14:50:43Z

test/functional/bitcoin_cli.py

It seems weird to me that input is an argument to the RPC, rather than an argument to the bitcoin-cli instance.

It seems possible that an RPC will have input as a parameter. Rather less likely that bitcoin-cli will add an argument called input.

maflcko · 2017-08-28T19:50:04Z

test/functional/test_framework/test_node.py

Thinking more about this, I think we should keep TestNode.cli to be the corresponding interface to TestNode.rpc. The logic to hack a different datadir can be placed in the test that checks for -stdinrpcpass. This makes the TestNode code easier to understand, as the datadir overwrite is rarely needed... Likely only once?

I also think I prefer keeping TestNode.cli as a class (and making it callable as per jnewbery@5bfd5e0), but I'm happy to be convinced the other way.

@MarcoFalke - I'm not sure I fully understand your comment though. As things currently stand with this PR, the interface for changing datadir is to add an argument to TestNode.cli(). How would you propose to make that different?

In python every member is public, so changing the datadir is as easy as self.nodes[0].cli.datadir = './new' # Overwrite datadir, so we pass in rpcuser and password through stdin

yes, that works. I think I still prefer making TestNodeCLI callable and adding datadir as an argument.

It is not like we need to pass in anything to TestNodeCLI. The datadir change is only needed for the test in this pull.

Have you looked at jnewbery@5bfd5e0 ?
Passing arguments to TestNodeCLI seems like a good way to pass bitcoin-cli command line arguments.

(I'm not arguing forcefully for the change in jnewbery@5bfd5e0 and I'm happy to be shown a different way to do this)

I did not.

This also help with selecting the rpcwallet, so making it callable is more useful than I thought.

ryanofsky · 2017-08-29T18:24:43Z

test/functional/test_framework/test_node.py

In commit "[test] Add support for custom arguments to TestNodeCLI"

You could replace these four lines with input = kwargs.pop("input", None), or change the function signature to def send_cli(self, command, *args, input=None, **kwargs) to get the same result.

Probably though it is better just to pass input to the TestNodeCLI constructor as John suggests.

maflcko · 2017-09-05T19:41:08Z

@promag Are you still working on this. I think what is missing is setting up the temp datadir and addressing @ryanofsky feedback..

promag · 2017-09-06T15:59:23Z

Thanks all for the great feedback. These are the relevant changes:

keep -datadir
input=... moved to .cli(), although -input is not bitcoin-cli argument 🙄
added tests for invalid authentication

jnewbery

A few nits inline. Tested ACK ad730f55b182549c5e95d6cc09be4474a1c3152a

jnewbery · 2017-09-06T19:14:29Z

test/functional/test_framework/util.py

You can remove (returncode is not None) here and (output is not None) below. The only reason I allowed assert_raises_jsonrpc() callers to not specify returncode or output was to be backwards compatible with existing tests which didn't specify those. As far as I'm concerned, tests should always assert return codes and error messages if they're expecting.

(we can probably remove the same from assert_raises_jsonrpc() as well in a separate PR now)

Great, will do.

jnewbery · 2017-09-06T19:15:57Z

test/functional/test_framework/util.py

nit: my personal preference is for imports to be sorted by module name (ie from subprocess... should be between import re and import time)

My bad, that's also my preference.

jnewbery · 2017-09-06T19:17:01Z

test/functional/test_framework/util.py

nit: perhaps assert_raises_process_error()?

No problem.

jnewbery · 2017-09-06T19:19:46Z

test/functional/bitcoin_cli.py

Thanks for teaching me some Portuguese: https://pt.wikipedia.org/wiki/X.P.T.O. 🙂

29e1dfb [test] Add bitcoin-cli -stdin and -stdinrpcpass functional tests (João Barbosa) ce379b4 [test] Replace check_output with low level version (João Barbosa) 232e3e8 [test] Add assert_raises_process_error to assert process errors (João Barbosa) 5c18a84 [test] Add support for custom arguments to TestNodeCLI (João Barbosa) e127494 [test] Improve assert_raises_jsonrpc docstring (João Barbosa) 7696841 Fix style in -stdin and -stdinrpcpass handling (João Barbosa) Pull request description: This patch adds tests for `bitcoin-cli` options `-stdin` (#7550) and `-stdinrpcpass` #10997. Tree-SHA512: fd8133f44876f2b5b41dfd3762b1988598f6b7bf13fb2385ad95876825d9c0b2b896ce4ea6eeb21012158e1f276907f155d37bb967198b609d2d3dddbfa334c1

Github-Pull: bitcoin#11125 Rebased-From: e127494

Github-Pull: bitcoin#11125 Rebased-From: 5c18a84

Github-Pull: bitcoin#11125 Rebased-From: 232e3e8

Github-Pull: bitcoin#11125 Rebased-From: ce379b4

…nal tests 29e1dfb [test] Add bitcoin-cli -stdin and -stdinrpcpass functional tests (João Barbosa) ce379b4 [test] Replace check_output with low level version (João Barbosa) 232e3e8 [test] Add assert_raises_process_error to assert process errors (João Barbosa) 5c18a84 [test] Add support for custom arguments to TestNodeCLI (João Barbosa) e127494 [test] Improve assert_raises_jsonrpc docstring (João Barbosa) 7696841 Fix style in -stdin and -stdinrpcpass handling (João Barbosa) Pull request description: This patch adds tests for `bitcoin-cli` options `-stdin` (bitcoin#7550) and `-stdinrpcpass` bitcoin#10997. Tree-SHA512: fd8133f44876f2b5b41dfd3762b1988598f6b7bf13fb2385ad95876825d9c0b2b896ce4ea6eeb21012158e1f276907f155d37bb967198b609d2d3dddbfa334c1

…nal tests 29e1dfb [test] Add bitcoin-cli -stdin and -stdinrpcpass functional tests (João Barbosa) ce379b4 [test] Replace check_output with low level version (João Barbosa) 232e3e8 [test] Add assert_raises_process_error to assert process errors (João Barbosa) 5c18a84 [test] Add support for custom arguments to TestNodeCLI (João Barbosa) e127494 [test] Improve assert_raises_jsonrpc docstring (João Barbosa) 7696841 Fix style in -stdin and -stdinrpcpass handling (João Barbosa) Pull request description: This patch adds tests for `bitcoin-cli` options `-stdin` (bitcoin#7550) and `-stdinrpcpass` bitcoin#10997. Tree-SHA512: fd8133f44876f2b5b41dfd3762b1988598f6b7bf13fb2385ad95876825d9c0b2b896ce4ea6eeb21012158e1f276907f155d37bb967198b609d2d3dddbfa334c1 remove duplicate method Signed-off-by: Pasta <[email protected]>

promag force-pushed the 2017-08-stdinrpcpass-functional-test branch from bea65ea to 7162087 Compare August 24, 2017 17:59

laanwj added the Tests label Aug 24, 2017

laanwj reviewed Aug 24, 2017

View reviewed changes

promag force-pushed the 2017-08-stdinrpcpass-functional-test branch 2 times, most recently from 9c28558 to 64c89a9 Compare August 25, 2017 03:04

promag changed the title ~~Add bitcoin-cli -stdinrpcpass functional test~~ Add bitcoin-cli -stdin and -stdinrpcpass functional tests Aug 25, 2017

promag force-pushed the 2017-08-stdinrpcpass-functional-test branch from 64c89a9 to 5b4e339 Compare August 25, 2017 03:17

laanwj reviewed Aug 28, 2017

View reviewed changes

jnewbery reviewed Aug 28, 2017

View reviewed changes

maflcko reviewed Aug 28, 2017

View reviewed changes

ryanofsky reviewed Aug 29, 2017

View reviewed changes

promag force-pushed the 2017-08-stdinrpcpass-functional-test branch 3 times, most recently from 95fe4df to 04e0e12 Compare September 6, 2017 15:54

promag force-pushed the 2017-08-stdinrpcpass-functional-test branch from 04e0e12 to ad730f5 Compare September 6, 2017 16:08

jnewbery reviewed Sep 6, 2017

View reviewed changes

promag force-pushed the 2017-08-stdinrpcpass-functional-test branch from ad730f5 to 71a13a5 Compare September 6, 2017 20:28

laanwj self-assigned this Sep 6, 2017

Fix style in -stdin and -stdinrpcpass handling

7696841

promag added 4 commits September 7, 2017 00:35

[test] Add support for custom arguments to TestNodeCLI

5c18a84

[test] Add assert_raises_process_error to assert process errors

232e3e8

[test] Replace check_output with low level version

ce379b4

[test] Add bitcoin-cli -stdin and -stdinrpcpass functional tests

29e1dfb

promag force-pushed the 2017-08-stdinrpcpass-functional-test branch from 71a13a5 to 29e1dfb Compare September 6, 2017 23:38

laanwj merged commit 29e1dfb into bitcoin:master Sep 6, 2017

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

[test] Improve assert_raises_jsonrpc docstring

812c870

Github-Pull: bitcoin#11125 Rebased-From: e127494

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

[test] Add support for custom arguments to TestNodeCLI

e0bfd28

Github-Pull: bitcoin#11125 Rebased-From: 5c18a84

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

[test] Add assert_raises_process_error to assert process errors

e38211f

Github-Pull: bitcoin#11125 Rebased-From: 232e3e8

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

[test] Replace check_output with low level version

2b97b36

Github-Pull: bitcoin#11125 Rebased-From: ce379b4

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

Add bitcoin-cli -stdin and -stdinrpcpass functional tests #11125

Add bitcoin-cli -stdin and -stdinrpcpass functional tests #11125

Uh oh!

Conversation

promag commented Aug 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laanwj Aug 24, 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!

laanwj commented Aug 24, 2017

Uh oh!

jnewbery commented Aug 24, 2017

Uh oh!

promag commented Aug 24, 2017

Uh oh!

promag commented Aug 25, 2017

Uh oh!

jnewbery commented Aug 25, 2017

Uh oh!

promag commented Aug 25, 2017

Uh oh!

jnewbery commented Aug 25, 2017

Uh oh!

maflcko commented Aug 25, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

promag commented Aug 28, 2017

Uh oh!

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

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!

maflcko commented Sep 5, 2017

Uh oh!

promag commented Sep 6, 2017

Uh oh!

jnewbery 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

promag commented Aug 24, 2017 •

edited

Loading

laanwj Aug 24, 2017 •

edited

Loading