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

@@ -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

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()

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"""

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():
@@ -38,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
@@ -58,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
@@ -69,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."""
@@ -110,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."""
@@ -187,6 +198,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"""
@@ -196,17 +217,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."""
@@ -218,10 +248,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")

test/functional/test_runner.py

@@ -94,6 +94,7 @@ BASE_SCRIPTS= [
     'mempool_reorg.py',
     'mempool_persist.py',
     'multiwallet.py',
+    'multiwallet.py --usecli',
     'httpbasics.py',
     'multi_rpc.py',
     'proxy_test.py',