From cc2cd7291b3e67fdbbc13bc03c9a14d92532136b Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Fri, 12 Jan 2018 17:24:36 -0500 Subject: [PATCH] Merge #11970: Add test coverage for bitcoin-cli multiwallet calls a14dbff39e Allow multiwallet.py to be used with --usecli (Russell Yanofsky) f6ade9ce1a [tests] allow tests to be run with --usecli (John Newbery) ff9a363ff7 TestNodeCLI batch emulation (Russell Yanofsky) ca9085afc5 Prevent TestNodeCLI.args mixups (Russell Yanofsky) fcfb952bca Improve TestNodeCLI output parsing (Russell Yanofsky) Pull request description: Lack of test coverage was pointed out by @jnewbery in https://github.com/bitcoin/bitcoin/pull/11687#discussion_r158133900 Tree-SHA512: 5f10e31abad11a5edab0da4e2515e39547adb6ab9e55e50427ab2eb7ec9a43d6b896b579b15863e5edc9beee7d8bf1c84d9dabd247be0760a1b9ae39e1e8ee02 --- test/functional/create_cache.py | 1 + test/functional/multiwallet.py | 48 +++++++------ .../test_framework/test_framework.py | 7 +- test/functional/test_framework/test_node.py | 68 +++++++++++++++---- test/functional/test_runner.py | 1 + 5 files changed, 88 insertions(+), 37 deletions(-) diff --git a/test/functional/create_cache.py b/test/functional/create_cache.py index 7d4d1a529b..0909847611 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/multiwallet.py b/test/functional/multiwallet.py index c71622f3b6..41df1a7c85 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'], 500) @@ -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_framework/test_framework.py b/test/functional/test_framework/test_framework.py index b7d1f6af3c..eeeff821aa 100755 --- a/test/functional/test_framework/test_framework.py +++ b/test/functional/test_framework/test_framework.py @@ -73,6 +73,7 @@ class BitcoinTestFramework(object): 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()" @@ -102,6 +103,8 @@ class BitcoinTestFramework(object): 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() @@ -124,6 +127,8 @@ class BitcoinTestFramework(object): 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() @@ -247,7 +252,7 @@ class BitcoinTestFramework(object): assert_equal(len(binary), num_nodes) old_num_nodes = len(self.nodes) for i in range(num_nodes): - self.nodes.append(TestNode(old_num_nodes + i, self.options.tmpdir, extra_args[i], rpchost, timewait=timewait, binary=binary[i], stderr=stderr, mocktime=self.mocktime, coverage_dir=self.options.coveragedir)) + self.nodes.append(TestNode(old_num_nodes + i, self.options.tmpdir, extra_args[i], rpchost, timewait=timewait, binary=binary[i], stderr=stderr, 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 dashd""" diff --git a/test/functional/test_framework/test_node.py b/test/functional/test_framework/test_node.py index c02d17fbb7..48adb33155 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 @@ -23,6 +24,9 @@ from .util import ( p2p_port, ) +# For Python 3.4 compatibility +JSONDecodeError = getattr(json, "JSONDecodeError", ValueError) + BITCOIND_PROC_WAIT_TIMEOUT = 60 class TestNode(): @@ -39,7 +43,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 @@ -59,6 +63,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", "dash-cli"), self.datadir) + self.use_cli = use_cli # Don't try auto backups (they fail a lot when running tests) self.args.append("-createwalletbackups=0") @@ -73,9 +78,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.""" @@ -116,10 +124,13 @@ class TestNode(): raise AssertionError("Unable to connect to dashd") 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, wait=0): """Stop the node.""" @@ -195,6 +206,16 @@ class TestNode(): p.connection.disconnect_node() 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""" @@ -204,17 +225,26 @@ 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 - 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): - 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.""" @@ -226,10 +256,18 @@ 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() 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") diff --git a/test/functional/test_runner.py b/test/functional/test_runner.py index 595918f8f0..33b7775fb9 100755 --- a/test/functional/test_runner.py +++ b/test/functional/test_runner.py @@ -99,6 +99,7 @@ BASE_SCRIPTS= [ 'mempool_reorg.py', 'mempool_persist.py', 'multiwallet.py', + 'multiwallet.py --usecli', 'httpbasics.py', 'multi_rpc.py', 'proxy_test.py',