test: add coverage for -rpcwallet cli option

jonatack commented at 4:39 pm on April 21, 2020: member

The bitcoin-cli -rpcwallet= option is an essential RPC/CLI option when more than one wallet is loaded (see bitcoin-cli -help | grep -A5 rpcwallet or src/bitcoin-cli.cpp::L61) and it currently has no test coverage.

It is not only used by users, but also by the test framework and ~10 test files via get_wallet_rpc().

This PR adds coverage, while simultaneously improving the -getinfo coverage when multiple wallets are loaded. This is similar to the test coverage that would be added in #18594.

DrahtBot added the label Tests on Apr 21, 2020

in test/functional/interface_bitcoin_cli.py:128 in 0e64fb6d87 outdated

124@@ -85,7 +125,7 @@ def run_test(self):
125         self.nodes[0].wait_for_cookie_credentials()  # ensure cookie file is available to avoid race condition
126         blocks = self.nodes[0].cli('-rpcwait').send_cli('getblockcount')
127         self.nodes[0].wait_for_rpc_connection()
128-        assert_equal(blocks, BLOCKS)
129+        assert_equal(blocks, BLOCKS + 1 if self.is_wallet_compiled() else BLOCKS)

MarcoFalke commented at 5:22 pm on April 21, 2020:

Could mine the block in the else branch as well, so that subsequent tests don’t depend on conditionals?

jonatack commented at 5:33 pm on April 21, 2020:

thanks for reviewing – done

in test/functional/interface_bitcoin_cli.py:97 in 0e64fb6d87 outdated

92+            self.log.info("Test -getinfo with multiple wallets loaded returns no balance")
93+            assert_equal(set(self.nodes[0].listwallets()), set(wallets))
94+            assert 'balance' not in self.nodes[0].cli('-getinfo').send_cli().keys()
95+
96+            self.log.info("Test -getinfo with multiple wallets and -rpcwallet returns specified wallet balance")
97+            for i in range(len(wallets) - 1):

jonatack commented at 5:27 pm on April 21, 2020:

This should be for i in range(len(wallets)):

jonatack force-pushed on Apr 21, 2020

DrahtBot commented at 5:41 pm on April 21, 2020: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#18594 (cli: display multiwallet balances in -getinfo by jonatack)
#18453 (rpc, cli: add multiwallet balances rpc and use it in -getinfo by jonatack)

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.

in test/functional/interface_bitcoin_cli.py:79 in 5835c4ad60 outdated

72@@ -72,8 +73,48 @@ def run_test(self):
73             assert_equal(cli_get_info['paytxfee'], wallet_info['paytxfee'])
74             assert_equal(cli_get_info['relayfee'], network_info['relayfee'])
75             assert_equal(self.nodes[0].cli.getwalletinfo(), wallet_info)
76+
77+            # Setup to test -getinfo and -rpcwallet= with multiple wallets.
78+            wallets = ['', 'Encrypted', 'secret']
79+            amounts = [Decimal('59.999928'), Decimal(9), Decimal(31)]

robot-visions commented at 5:59 pm on April 24, 2020:

Just curious, how did you get this value 59.999928 (did you print out the amount to check, or is there a known fee that’s listed somewhere)?

jonatack commented at 7:38 pm on April 24, 2020:

Yes, I actually did that while writing tests for #18594 which most of this code comes from; -getinfo in that PR outputs all the wallet names and balances and I was just printing the output.

in test/functional/interface_bitcoin_cli.py:85 in 5835c4ad60 outdated

80+            self.nodes[0].createwallet(wallet_name=wallets[1])
81+            self.nodes[0].createwallet(wallet_name=wallets[2])
82+            w1 = self.nodes[0].get_wallet_rpc(wallets[0])
83+            w2 = self.nodes[0].get_wallet_rpc(wallets[1])
84+            w3 = self.nodes[0].get_wallet_rpc(wallets[2])
85+            w1.walletpassphrase(password, 60)

robot-visions commented at 6:00 pm on April 24, 2020:

Just as a paranoid check, could increasing the timeout here reduce the chance of a flaky test (I’m not familiar enough with the CI framework to answer this definitively)?

jonatack commented at 7:32 pm on April 24, 2020:

I arbitrarily used the default rpc_timeout. But you’re right to mention it; the magic value is kind of sucky and non-informative. Changed it to:

0-            w1.walletpassphrase(password, 60)
1+            w1.walletpassphrase(password, self.rpc_timeout)

robot-visions commented at 6:01 pm on April 24, 2020: contributor

ACK 5835c4ad60f1c6507ac3953210e68ae6fd8cc81f

Passes for me locally.

I remember doing a lot of this manually for your PR Review Club session a few weeks ago. Thanks for adding all this as a test!

jonatack force-pushed on Apr 24, 2020

jonatack commented at 7:46 pm on April 24, 2020: member

Thanks @robot-visions for reviewing. I replaced the timeout magic value of 60 with self.rpc_timeout. IDK if there is a better value to use.

test: add coverage for -rpcwallet cli option

and add -getinfo coverage with multiple wallets loaded.

2495110012

jonatack force-pushed on Apr 24, 2020

robot-visions commented at 7:59 pm on April 24, 2020: contributor

ACK 2495110012fc0cb7142a4536791dd79da5cc3e44

Tests still pass. Thanks for updating!

MarcoFalke commented at 10:37 pm on April 24, 2020: member

Thanks for writing tests

MarcoFalke merged this on Apr 24, 2020

MarcoFalke closed this on Apr 24, 2020

in test/functional/interface_bitcoin_cli.py:98 in 2495110012

93+            assert_equal(set(self.nodes[0].listwallets()), set(wallets))
94+            assert 'balance' not in self.nodes[0].cli('-getinfo').send_cli().keys()
95+
96+            self.log.info("Test -getinfo with multiple wallets and -rpcwallet returns specified wallet balance")
97+            for i in range(len(wallets)):
98+                cli_get_info = self.nodes[0].cli('-getinfo').send_cli('-rpcwallet={}'.format(wallets[i]))

MarcoFalke commented at 10:44 pm on April 24, 2020:

nit: rcpwallet is an option, not a command

jonatack commented at 10:48 pm on April 24, 2020:

Thanks, will update in #18594 or next opportunity.

jonatack commented at 6:34 pm on May 22, 2020:

rpcwallet is an option, not a command

Tucked the cli option vs command test fixups into the last commit of #18594 adding tests for that PR.

jonatack deleted the branch on Apr 24, 2020

sidhujag referenced this in commit a1abc553ae on Apr 25, 2020

luke-jr referenced this in commit 070c59f532 on Jun 9, 2020

luke-jr referenced this in commit 62d0fd4df5 on Jun 13, 2020

luke-jr referenced this in commit 094150c9b0 on Jun 14, 2020

Fabcien referenced this in commit efe531436f on Jan 20, 2021

DrahtBot locked this on Feb 15, 2022

test: add coverage for -rpcwallet cli option #18724

Conflicts