From fcfb952bca922682e61c77e59a59f4e7fa6619c7 Mon Sep 17 00:00:00 2001 From: Russell Yanofsky Date: Wed, 20 Dec 2017 18:38:40 -0500 Subject: [PATCH 1/5] Improve TestNodeCLI output parsing Parse JSONRPCException errors, and avoid JSON decode exception if RPC method returns a plain string. --- test/functional/test_framework/test_node.py | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/test/functional/test_framework/test_node.py b/test/functional/test_framework/test_node.py index a9248c764..66f99f301 100755 --- a/test/functional/test_framework/test_node.py +++ b/test/functional/test_framework/test_node.py @@ -10,6 +10,7 @@ import http.client import json import logging import os +import re import subprocess import time @@ -22,6 +23,9 @@ from .util import ( p2p_port, ) +# For Python 3.4 compatibility +JSONDecodeError = getattr(json, "JSONDecodeError", ValueError) + BITCOIND_PROC_WAIT_TIMEOUT = 60 class TestNode(): @@ -222,6 +226,13 @@ class TestNodeCLI(): cli_stdout, cli_stderr = process.communicate(input=self.input) returncode = process.poll() if returncode: + match = re.match(r'error code: ([-0-9]+)\nerror message:\n(.*)', cli_stderr) + if match: + code, message = match.groups() + raise JSONRPCException(dict(code=int(code), message=message)) # Ignore cli_stdout, raise with cli_stderr raise subprocess.CalledProcessError(returncode, self.binary, output=cli_stderr) - return json.loads(cli_stdout, parse_float=decimal.Decimal) + try: + return json.loads(cli_stdout, parse_float=decimal.Decimal) + except JSONDecodeError: + return cli_stdout.rstrip("\n") From ca9085afc53eb20c1fc745ae469e9587a05b7f24 Mon Sep 17 00:00:00 2001 From: Russell Yanofsky Date: Wed, 20 Dec 2017 18:41:12 -0500 Subject: [PATCH 2/5] Prevent TestNodeCLI.args mixups Change TestNodeCLI.__call__() to return a new instance instead of modifying the existing instance. This way, it's possible to create different cli objects that have their own options (for example -rpcwallet options to connect to different wallets), and options set for a single call (`node.cli(options).method(args)`) will no longer leak into future calls. --- test/functional/test_framework/test_node.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/test/functional/test_framework/test_node.py b/test/functional/test_framework/test_node.py index 66f99f301..1723f50eb 100755 --- a/test/functional/test_framework/test_node.py +++ b/test/functional/test_framework/test_node.py @@ -203,9 +203,10 @@ class TestNodeCLI(): def __call__(self, *args, input=None): # TestNodeCLI is callable with bitcoin-cli command-line args - self.args = [str(arg) for arg in args] - self.input = input - return self + cli = TestNodeCLI(self.binary, self.datadir) + cli.args = [str(arg) for arg in args] + cli.input = input + return cli def __getattr__(self, command): def dispatcher(*args, **kwargs): From ff9a363ff70e1b72a1283098e69bbe14d1c16bcc Mon Sep 17 00:00:00 2001 From: Russell Yanofsky Date: Thu, 21 Dec 2017 04:54:43 -0500 Subject: [PATCH 3/5] TestNodeCLI batch emulation Support same get_request and batch methods as AuthServiceProxy --- test/functional/test_framework/test_node.py | 23 ++++++++++++++++++--- 1 file changed, 20 insertions(+), 3 deletions(-) diff --git a/test/functional/test_framework/test_node.py b/test/functional/test_framework/test_node.py index 1723f50eb..fd44bff32 100755 --- a/test/functional/test_framework/test_node.py +++ b/test/functional/test_framework/test_node.py @@ -191,6 +191,16 @@ class TestNode(): p.peer_disconnect() del self.p2ps[:] +class TestNodeCLIAttr: + def __init__(self, cli, command): + self.cli = cli + self.command = command + + def __call__(self, *args, **kwargs): + return self.cli.send_cli(self.command, *args, **kwargs) + + def get_request(self, *args, **kwargs): + return lambda: self(*args, **kwargs) class TestNodeCLI(): """Interface to bitcoin-cli for an individual node""" @@ -209,9 +219,16 @@ class TestNodeCLI(): return cli def __getattr__(self, command): - def dispatcher(*args, **kwargs): - return self.send_cli(command, *args, **kwargs) - return dispatcher + return TestNodeCLIAttr(self, command) + + def batch(self, requests): + results = [] + for request in requests: + try: + results.append(dict(result=request())) + except JSONRPCException as e: + results.append(dict(error=e)) + return results def send_cli(self, command, *args, **kwargs): """Run bitcoin-cli command. Deserializes returned string as python object.""" From f6ade9ce1a679a026c84e5baa9f8595fa2be78a5 Mon Sep 17 00:00:00 2001 From: John Newbery Date: Tue, 11 Jul 2017 13:14:18 -0400 Subject: [PATCH 4/5] [tests] allow tests to be run with --usecli test_framework accepts a new --usecli parameter. Running the test with this parameter will cause all RPCs to be sent through bitcoin-cli rather than directly over http. By default, individual test cases do not support --usecli, and self.supports_cli must be set to True in the set_test_params method. We can make supports_cli default to True in future once we know which tests will fail with use_cli. --- test/functional/create_cache.py | 1 + .../test_framework/test_framework.py | 7 +++++- test/functional/test_framework/test_node.py | 25 +++++++++++++------ 3 files changed, 24 insertions(+), 9 deletions(-) diff --git a/test/functional/create_cache.py b/test/functional/create_cache.py index 4c79814a2..9665c50a9 100755 --- a/test/functional/create_cache.py +++ b/test/functional/create_cache.py @@ -16,6 +16,7 @@ class CreateCache(BitcoinTestFramework): def set_test_params(self): self.num_nodes = 0 + self.supports_cli = True def setup_network(self): pass diff --git a/test/functional/test_framework/test_framework.py b/test/functional/test_framework/test_framework.py index e42f3e60c..5d4f8e672 100755 --- a/test/functional/test_framework/test_framework.py +++ b/test/functional/test_framework/test_framework.py @@ -62,6 +62,7 @@ class BitcoinTestFramework(): self.setup_clean_chain = False self.nodes = [] self.mocktime = 0 + self.supports_cli = False self.set_test_params() assert hasattr(self, "num_nodes"), "Test must set self.num_nodes in set_test_params()" @@ -91,6 +92,8 @@ class BitcoinTestFramework(): help="Location of the test framework config file") parser.add_option("--pdbonfailure", dest="pdbonfailure", default=False, action="store_true", help="Attach a python debugger if test fails") + parser.add_option("--usecli", dest="usecli", default=False, action="store_true", + help="use bitcoin-cli instead of RPC for all commands") self.add_options(parser) (self.options, self.args) = parser.parse_args() @@ -113,6 +116,8 @@ class BitcoinTestFramework(): success = TestStatus.FAILED try: + if self.options.usecli and not self.supports_cli: + raise SkipTest("--usecli specified but test does not support using CLI") self.setup_chain() self.setup_network() self.run_test() @@ -213,7 +218,7 @@ class BitcoinTestFramework(): assert_equal(len(extra_args), num_nodes) assert_equal(len(binary), num_nodes) for i in range(num_nodes): - self.nodes.append(TestNode(i, self.options.tmpdir, extra_args[i], rpchost, timewait=timewait, binary=binary[i], stderr=None, mocktime=self.mocktime, coverage_dir=self.options.coveragedir)) + self.nodes.append(TestNode(i, self.options.tmpdir, extra_args[i], rpchost, timewait=timewait, binary=binary[i], stderr=None, mocktime=self.mocktime, coverage_dir=self.options.coveragedir, use_cli=self.options.usecli)) def start_node(self, i, extra_args=None, stderr=None): """Start a bitcoind""" diff --git a/test/functional/test_framework/test_node.py b/test/functional/test_framework/test_node.py index fd44bff32..589a8f396 100755 --- a/test/functional/test_framework/test_node.py +++ b/test/functional/test_framework/test_node.py @@ -42,7 +42,7 @@ class TestNode(): To make things easier for the test writer, any unrecognised messages will be dispatched to the RPC connection.""" - def __init__(self, i, dirname, extra_args, rpchost, timewait, binary, stderr, mocktime, coverage_dir): + def __init__(self, i, dirname, extra_args, rpchost, timewait, binary, stderr, mocktime, coverage_dir, use_cli=False): self.index = i self.datadir = os.path.join(dirname, "node" + str(i)) self.rpchost = rpchost @@ -62,6 +62,7 @@ class TestNode(): self.args = [self.binary, "-datadir=" + self.datadir, "-server", "-keypool=1", "-discover=0", "-rest", "-logtimemicros", "-debug", "-debugexclude=libevent", "-debugexclude=leveldb", "-mocktime=" + str(mocktime), "-uacomment=testnode%d" % i] self.cli = TestNodeCLI(os.getenv("BITCOINCLI", "bitcoin-cli"), self.datadir) + self.use_cli = use_cli self.running = False self.process = None @@ -73,9 +74,12 @@ class TestNode(): self.p2ps = [] def __getattr__(self, name): - """Dispatches any unrecognised messages to the RPC connection.""" - assert self.rpc_connected and self.rpc is not None, "Error: no RPC connection" - return getattr(self.rpc, name) + """Dispatches any unrecognised messages to the RPC connection or a CLI instance.""" + if self.use_cli: + return getattr(self.cli, name) + else: + assert self.rpc_connected and self.rpc is not None, "Error: no RPC connection" + return getattr(self.rpc, name) def start(self, extra_args=None, stderr=None): """Start the node.""" @@ -114,10 +118,13 @@ class TestNode(): raise AssertionError("Unable to connect to bitcoind") def get_wallet_rpc(self, wallet_name): - assert self.rpc_connected - assert self.rpc - wallet_path = "wallet/%s" % wallet_name - return self.rpc / wallet_path + if self.use_cli: + return self.cli("-rpcwallet={}".format(wallet_name)) + else: + assert self.rpc_connected + assert self.rpc + wallet_path = "wallet/%s" % wallet_name + return self.rpc / wallet_path def stop_node(self): """Stop the node.""" @@ -210,6 +217,7 @@ class TestNodeCLI(): self.binary = binary self.datadir = datadir self.input = None + self.log = logging.getLogger('TestFramework.bitcoincli') def __call__(self, *args, input=None): # TestNodeCLI is callable with bitcoin-cli command-line args @@ -240,6 +248,7 @@ class TestNodeCLI(): if named_args: p_args += ["-named"] p_args += [command] + pos_args + named_args + self.log.debug("Running bitcoin-cli command: %s" % command) process = subprocess.Popen(p_args, stdin=subprocess.PIPE, stdout=subprocess.PIPE, stderr=subprocess.PIPE, universal_newlines=True) cli_stdout, cli_stderr = process.communicate(input=self.input) returncode = process.poll() From a14dbff39ea050b74b32bb0f4cbb59f4a9ad3865 Mon Sep 17 00:00:00 2001 From: Russell Yanofsky Date: Wed, 20 Dec 2017 18:37:34 -0500 Subject: [PATCH 5/5] Allow multiwallet.py to be used with --usecli Add test coverage for bitcoin-cli multiwallet calls. --- test/functional/multiwallet.py | 48 +++++++++++++++++++--------------- test/functional/test_runner.py | 1 + 2 files changed, 28 insertions(+), 21 deletions(-) diff --git a/test/functional/multiwallet.py b/test/functional/multiwallet.py index d0c40e544..12d9e9f48 100755 --- a/test/functional/multiwallet.py +++ b/test/functional/multiwallet.py @@ -17,9 +17,16 @@ class MultiWalletTest(BitcoinTestFramework): self.setup_clean_chain = True self.num_nodes = 1 self.extra_args = [['-wallet=w1', '-wallet=w2', '-wallet=w3', '-wallet=w']] + self.supports_cli = True def run_test(self): - assert_equal(set(self.nodes[0].listwallets()), {"w1", "w2", "w3", "w"}) + node = self.nodes[0] + + data_dir = lambda *p: os.path.join(node.datadir, 'regtest', *p) + wallet_dir = lambda *p: data_dir('wallets', *p) + wallet = lambda name: node.get_wallet_rpc(name) + + assert_equal(set(node.listwallets()), {"w1", "w2", "w3", "w"}) self.stop_node(0) @@ -27,39 +34,38 @@ class MultiWalletTest(BitcoinTestFramework): self.assert_start_raises_init_error(0, ['-wallet=w1', '-wallet=w1'], 'Error loading wallet w1. Duplicate -wallet filename specified.') # should not initialize if wallet file is a directory - wallet_dir = os.path.join(self.options.tmpdir, 'node0', 'regtest', 'wallets') - os.mkdir(os.path.join(wallet_dir, 'w11')) + os.mkdir(wallet_dir('w11')) self.assert_start_raises_init_error(0, ['-wallet=w11'], 'Error loading wallet w11. -wallet filename must be a regular file.') # should not initialize if one wallet is a copy of another - shutil.copyfile(os.path.join(wallet_dir, 'w2'), os.path.join(wallet_dir, 'w22')) + shutil.copyfile(wallet_dir('w2'), wallet_dir('w22')) self.assert_start_raises_init_error(0, ['-wallet=w2', '-wallet=w22'], 'duplicates fileid') # should not initialize if wallet file is a symlink - os.symlink(os.path.join(wallet_dir, 'w1'), os.path.join(wallet_dir, 'w12')) + os.symlink(wallet_dir('w1'), wallet_dir('w12')) self.assert_start_raises_init_error(0, ['-wallet=w12'], 'Error loading wallet w12. -wallet filename must be a regular file.') # should not initialize if the specified walletdir does not exist self.assert_start_raises_init_error(0, ['-walletdir=bad'], 'Error: Specified -walletdir "bad" does not exist') # should not initialize if the specified walletdir is not a directory - not_a_dir = os.path.join(wallet_dir, 'notadir') + not_a_dir = wallet_dir('notadir') open(not_a_dir, 'a').close() - self.assert_start_raises_init_error(0, ['-walletdir='+not_a_dir], 'Error: Specified -walletdir "' + not_a_dir + '" is not a directory') + self.assert_start_raises_init_error(0, ['-walletdir=' + not_a_dir], 'Error: Specified -walletdir "' + not_a_dir + '" is not a directory') # if wallets/ doesn't exist, datadir should be the default wallet dir - wallet_dir2 = os.path.join(self.options.tmpdir, 'node0', 'regtest', 'walletdir') - os.rename(wallet_dir, wallet_dir2) + wallet_dir2 = data_dir('walletdir') + os.rename(wallet_dir(), wallet_dir2) self.start_node(0, ['-wallet=w4', '-wallet=w5']) - assert_equal(set(self.nodes[0].listwallets()), {"w4", "w5"}) - w5 = self.nodes[0].get_wallet_rpc("w5") + assert_equal(set(node.listwallets()), {"w4", "w5"}) + w5 = wallet("w5") w5.generate(1) self.stop_node(0) # now if wallets/ exists again, but the rootdir is specified as the walletdir, w4 and w5 should still be loaded - os.rename(wallet_dir2, wallet_dir) - self.start_node(0, ['-wallet=w4', '-wallet=w5', '-walletdir=' + os.path.join(self.options.tmpdir, 'node0', 'regtest')]) - assert_equal(set(self.nodes[0].listwallets()), {"w4", "w5"}) - w5 = self.nodes[0].get_wallet_rpc("w5") + os.rename(wallet_dir2, wallet_dir()) + self.start_node(0, ['-wallet=w4', '-wallet=w5', '-walletdir=' + data_dir()]) + assert_equal(set(node.listwallets()), {"w4", "w5"}) + w5 = wallet("w5") w5_info = w5.getwalletinfo() assert_equal(w5_info['immature_balance'], 50) @@ -67,11 +73,11 @@ class MultiWalletTest(BitcoinTestFramework): self.start_node(0, self.extra_args[0]) - w1 = self.nodes[0].get_wallet_rpc("w1") - w2 = self.nodes[0].get_wallet_rpc("w2") - w3 = self.nodes[0].get_wallet_rpc("w3") - w4 = self.nodes[0].get_wallet_rpc("w") - wallet_bad = self.nodes[0].get_wallet_rpc("bad") + w1 = wallet("w1") + w2 = wallet("w2") + w3 = wallet("w3") + w4 = wallet("w") + wallet_bad = wallet("bad") w1.generate(1) @@ -79,7 +85,7 @@ class MultiWalletTest(BitcoinTestFramework): assert_raises_rpc_error(-18, "Requested wallet does not exist or is not loaded", wallet_bad.getwalletinfo) # accessing wallet RPC without using wallet endpoint fails - assert_raises_rpc_error(-19, "Wallet file not specified", self.nodes[0].getwalletinfo) + assert_raises_rpc_error(-19, "Wallet file not specified", node.getwalletinfo) # check w1 wallet balance w1_info = w1.getwalletinfo() diff --git a/test/functional/test_runner.py b/test/functional/test_runner.py index 428a18be8..458c296d0 100755 --- a/test/functional/test_runner.py +++ b/test/functional/test_runner.py @@ -92,6 +92,7 @@ BASE_SCRIPTS= [ 'mempool_reorg.py', 'mempool_persist.py', 'multiwallet.py', + 'multiwallet.py --usecli', 'httpbasics.py', 'multi_rpc.py', 'proxy_test.py',