Add test coverage for bitcoin-cli multiwallet calls

ryanofsky commented at 12:05 pm on December 21, 2017: member

Lack of test coverage was pointed out by @jnewbery in #11687 (review)

fanquake added the label Tests on Dec 21, 2017

ryanofsky force-pushed on Dec 21, 2017

jonasschnelli commented at 7:13 pm on December 21, 2017: contributor

utACK b67bd3d62c998ad4b91a94c879505544c28b2ba4

laanwj added the label Wallet on Dec 21, 2017

in test/functional/multiwallet.py:23 in 963315dbc7 outdated

16@@ -17,69 +17,78 @@ def set_test_params(self):
17         self.setup_clean_chain = True
18         self.num_nodes = 1
19         self.extra_args = [['-wallet=w1', '-wallet=w2', '-wallet=w3', '-wallet=w']]
20+        self.variants = ["rpc", "cli"]
21 
22     def run_test(self):
23-        assert_equal(set(self.nodes[0].listwallets()), {"w1", "w2", "w3", "w"})
24+        data_dir = lambda *p: os.path.join(self.nodes[0].datadir, 'regtest', *p)

promag commented at 1:32 am on January 4, 2018:

Rebase with #12076 instead?

ryanofsky commented at 2:24 pm on January 4, 2018:

Rebase with #12076 instead?

I think #11687 does a better job of cleaning up this particular test than #12076. But in general my preference is to resolve conflicts when they arise instead of adding unnecessary dependencies between prs to avoid conflicts. Better for prs to be smaller, more targeted, with fewer dependencies when this is possible, because reviewing changes requires more effort than resolving simple merge conflicts.

in test/functional/test_framework/test_framework.py:119 in 7792aee844 outdated

114+            self.variant = variant
115+            self.tmpdir = self.options.tmpdir
116+            if variant:
117+                self.tmpdir = os.path.join(self.options.tmpdir, variant)
118+                os.mkdir(self.tmpdir)
119+            success = self.run_variant()

MarcoFalke commented at 2:04 pm on January 4, 2018:

I’d prefer if this was renamed to status.

Also, we don’t support different statūs for variants, since the test runner has to print one status per test script. Thus, I’d prefer if the result of run_variant was appended to the list status and then (outside the for loop):

All statūs the same => sys.exit(TEST_EXIT_${STATUS})
Different statūs, probably best to exit with failure.

ryanofsky commented at 2:28 pm on January 4, 2018:

Will rename. The behavior you’re describing is what I was intending and what should happen after fixing the bug below (no need for an array/list just to return one status).

in test/functional/test_framework/test_framework.py:120 in 7792aee844 outdated

115+            self.tmpdir = self.options.tmpdir
116+            if variant:
117+                self.tmpdir = os.path.join(self.options.tmpdir, variant)
118+                os.mkdir(self.tmpdir)
119+            success = self.run_variant()
120+            if success == TestStatus.SKIPPED and exit_code == TEST_EXIT_PASSED:

MarcoFalke commented at 2:04 pm on January 4, 2018:

Skipping a test for different variant will result in FAILED instead of SKIPPED, no?

ryanofsky commented at 2:30 pm on January 4, 2018:

Will fix.

MarcoFalke commented at 2:05 pm on January 4, 2018: member

Concept ACK 963315dbc7d020a6569975fd1f75f656937edbb1

promag commented at 2:10 pm on January 4, 2018: member

utACK b67bd3d.

in test/functional/multiwallet.py:20 in 963315dbc7 outdated

16@@ -17,69 +17,78 @@ def set_test_params(self):
17         self.setup_clean_chain = True
18         self.num_nodes = 1
19         self.extra_args = [['-wallet=w1', '-wallet=w2', '-wallet=w3', '-wallet=w']]
20+        self.variants = ["rpc", "cli"]

promag commented at 2:16 pm on January 4, 2018:

BTW, I thought of:

0self.matrix = { mode: ['rpc', 'cli'], foo: ... }

Which would generate all combinations.

ryanofsky commented at 2:36 pm on January 4, 2018:

What is the suggestion? That seems identical to current test except it renames things and adds a foo.

promag commented at 2:50 pm on January 4, 2018:

Sorry if it was not clear. The suggestion is to support combinations. A more decent example:

0self.matrix = { a: ['a1', 'a2'], b: ['b1', 'b2', 'b3'] }

This would call 6 times run_test.

MarcoFalke commented at 2:56 pm on January 4, 2018:

Could be added when needed, i.e. later.

promag commented at 3:00 pm on January 4, 2018:

Sure, I didn’t mean to do it here. Variants is enough in this PR.

ryanofsky commented at 2:41 pm on January 4, 2018: member

@jnewbery it would be good to have feedback on the idea of supporting test variants, and on the locations of log / data files with variants since it may impact utils like combine_logs and test_runner.

Marking WIP, since this needs design feedback and at least one bugfix, and there are other PRs I’d prioritize.

ryanofsky renamed this:
~~Add test coverage for bitcoin-cli multiwallet calls~~
WIP: Add test coverage for bitcoin-cli multiwallet calls
on Jan 4, 2018

Improve TestNodeCLI output parsing

Parse JSONRPCException errors, and avoid JSON decode exception if RPC method
returns a plain string.

fcfb952bca

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.

ca9085afc5

TestNodeCLI batch emulation

Support same get_request and batch methods as AuthServiceProxy

ff9a363ff7

jnewbery commented at 8:55 pm on January 8, 2018: member

I’m a weak concept NACK on the new variants logic. It seems to add a lot of machinery and obscures the test flow logic for just this one test case. Perhaps if there are wider use cases it makes sense to add this, but if it’s just for this test, I don’t think it makes sense.

An alternative approach is to add a new parameter --usecli to the test framework, which could be used by any test (or most of the tests). I’ve made a branch that does that here: https://github.com/jnewbery/bitcoin/tree/pr11970.1. @ryanofsky - let me know what you think.

ryanofsky commented at 9:19 pm on January 8, 2018: member

Branch looks good to me. Do you want to PR it? Happy to close this if so.

promag commented at 9:21 pm on January 8, 2018: member

@jnewbery the test test/functional/address_types.py sounds like a candidate from a matrix type of test. But, as you point out, it isn’t worth losing the simplicity of the test framework.

jnewbery commented at 9:42 pm on January 8, 2018: member

Do you want to PR it?

Do you mind pushing it to this PR? I’m happy to handle any review comments on my commits.

ryanofsky commented at 10:02 pm on January 8, 2018: member

Reset to jnewbery pr11970.1, 963315dbc7d020a6569975fd1f75f656937edbb1 -> f4bad3125af167c2e5cf5dc153aebeff1a5d4ac8, pr/mcli.2 -> pr/mcli.3

ryanofsky force-pushed on Jan 8, 2018

ryanofsky renamed this:
~~WIP: Add test coverage for bitcoin-cli multiwallet calls~~
Add test coverage for bitcoin-cli multiwallet calls
on Jan 8, 2018

ryanofsky commented at 10:08 pm on January 8, 2018: member

ACK f4bad3125af167c2e5cf5dc153aebeff1a5d4ac8

in test/functional/test_framework/test_node.py:77 in c6397758d1 outdated

74@@ -74,8 +75,11 @@ def __init__(self, i, dirname, extra_args, rpchost, timewait, binary, stderr, mo
75 
76     def __getattr__(self, name):
77         """Dispatches any unrecognised messages to the RPC connection."""

promag commented at 10:10 pm on January 8, 2018:

In commit “[tests] allow tests to be run with –usecli”

Improve comment.

jnewbery commented at 10:39 pm on January 8, 2018:

fixed

in test/functional/test_framework/test_framework.py:120 in c6397758d1 outdated

115@@ -113,6 +116,8 @@ def main(self):
116         success = TestStatus.FAILED
117 
118         try:
119+            if self.options.usecli and not self.supports_cli:
120+                raise SkipTest("--usecli specificed but test does not support using CLI")

promag commented at 10:16 pm on January 8, 2018:

Why SkipTest? IMO should fail immediately in the test runner.

jnewbery commented at 10:39 pm on January 8, 2018:

This allows --usecli to be passed to test_runner and any tests that don’t support --usecli will be skipped:

 0./test_runner.py --usecli
 1[...]
 2TEST                           | STATUS    | DURATION
 3
 4abandonconflict.py             | ○ Skipped | 2 s
 5bip65-cltv-p2p.py              | ○ Skipped | 2 s
 6[...]
 7multiwallet.py                 | ✓ Passed  | 4 s
 8multiwallet.py --usecli        | ✓ Passed  | 4 s
 9[...]
10zmq_test.py                    | ○ Skipped | 2 s
11
12ALL                            | ✓ Passed  | 140 s (accumulated) 
13Runtime: 36 s

I have however fixed the typo in the exception message.

promag commented at 10:19 pm on January 8, 2018: member

utACK f4bad31.

[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.

f6ade9ce1a

Allow multiwallet.py to be used with --usecli

Add test coverage for bitcoin-cli multiwallet calls.

a14dbff39e

jnewbery force-pushed on Jan 8, 2018

promag commented at 11:08 pm on January 8, 2018: member

ACK a14dbff.

Running a single also works as expected IMO:

0./test/functional/abandonconflict.py --usecli && echo should not echo
12018-01-08 23:05:51.549000 TestFramework (WARNING): Test Skipped: --usecli specified but test does not support using CLI
22018-01-08 23:05:51.549000 TestFramework (INFO): Stopping nodes
32018-01-08 23:05:51.549000 TestFramework (INFO): Cleaning up
42018-01-08 23:05:51.550000 TestFramework (INFO): Test skipped

jnewbery commented at 10:21 pm on January 11, 2018: member

ACK a14dbff39ea050b74b32bb0f4cbb59f4a9ad3865 (obviously, since I authored it!)

MarcoFalke commented at 10:23 pm on January 12, 2018: member

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3utACK a14dbff39ea050b74b32bb0f4cbb59f4a9ad3865
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQIcBAEBCgAGBQJaWTVJAAoJENLqSFDnUoslFMUP/3Hu8h6cSYR9WEKdLvai2oM2
 7ZSECynk2MWyLH2vuRw0b5Mw/+eEoEgQsd/CTrRsQLJTr0xg6ymFK6FBhiZYM4++k
 8SSG8WtpoMiWokOvkqTMz4v/plLs5oa9t++HYq13L/hVoi0p9oFbUxa/0/dgIDeyP
 9gL+cPkLKN2jYWUy0e9y0KJIWWpscZeuYmKzxxsT6xyjsx9fwfrW2UdWlugx0l3Aa
10nesDNv9T5rXsSNg8MLppTR5o4DcKV2yxXPdQnkg7yh18aRIEY4IdWmUnv1Vqd70l
11bOsvk29Ow0VHqPK0zCO+hOxnRdU5cvWnx5nWN1o5s4R+SyGfLOW8hMDaRgZWcu2B
123xNavosyLf61fBhrZ/FJirPDmx7LBkfkcxuxO9ckvEdSjA3cyPPZyRgDo/rny/Sc
13P63pY4Y5Fa/fD98d3vbTFEIzSSi/4pGEfwxYQiCTMBR5PHP6Gm5i0jfjWtFkvU4M
14fRPj98KyYAwDFhz/2lFD+3dClKg5HQg7Ce84enHTq0uyMjo80CThNLIAE1tTjI/K
15pzFL59PAP6wBOK4C441MZVz7d1cbONQg2Aev1i/iXWRHg3eDjIEnkIsHN9N5OsAW
16D3OnIFG14umFG/02H65BBPiKNH95JPTmodPZPdTF1TgZ1gGfEJ+MLgt2yl9REmMM
17Cwu4THqwjUr1ahfAVdMP
18=nYAd
19-----END PGP SIGNATURE-----

MarcoFalke merged this on Jan 12, 2018

MarcoFalke closed this on Jan 12, 2018

MarcoFalke referenced this in commit b7450cdbd8 on Jan 12, 2018

UdjinM6 referenced this in commit ce2f97c28d on Feb 29, 2020

PastaPastaPasta referenced this in commit cc2cd7291b on Mar 4, 2020

jasonbcox referenced this in commit cc5ceb0d2e on Nov 10, 2020

ckti referenced this in commit 8b12185e22 on Mar 28, 2021

gades referenced this in commit 75723e599e on Jun 30, 2021

DrahtBot locked this on Sep 8, 2021

Add test coverage for bitcoin-cli multiwallet calls #11970