Add test coverage for bitcoin-cli multiwallet calls #11970

ryanofsky · 2017-12-21T12:05:05Z

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

jonasschnelli · 2017-12-21T19:13:20Z

utACK b67bd3d

maflcko · 2018-01-04T14:04:10Z

test/functional/test_framework/test_framework.py

+            if variant:
+                self.tmpdir = os.path.join(self.options.tmpdir, variant)
+                os.mkdir(self.tmpdir)
+            success = self.run_variant()


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.

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

maflcko · 2018-01-04T14:04:41Z

test/functional/test_framework/test_framework.py

+                self.tmpdir = os.path.join(self.options.tmpdir, variant)
+                os.mkdir(self.tmpdir)
+            success = self.run_variant()
+            if success == TestStatus.SKIPPED and exit_code == TEST_EXIT_PASSED:


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

maflcko · 2018-01-04T14:05:46Z

Concept ACK 963315d

promag

utACK b67bd3d.

promag · 2018-01-04T01:32:57Z

test/functional/multiwallet.py


    def run_test(self):
-        assert_equal(set(self.nodes[0].listwallets()), {"w1", "w2", "w3", "w"})
+        data_dir = lambda *p: os.path.join(self.nodes[0].datadir, 'regtest', *p)


Rebase with #12076 instead?

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.

promag · 2018-01-04T14:16:39Z

test/functional/multiwallet.py

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


BTW, I thought of:

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

Which would generate all combinations.

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

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

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

This would call 6 times run_test.

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

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

ryanofsky

@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 · 2018-01-04T14:28:28Z

test/functional/test_framework/test_framework.py

+            if variant:
+                self.tmpdir = os.path.join(self.options.tmpdir, variant)
+                os.mkdir(self.tmpdir)
+            success = self.run_variant()


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

ryanofsky · 2018-01-04T14:30:34Z

test/functional/test_framework/test_framework.py

+                self.tmpdir = os.path.join(self.options.tmpdir, variant)
+                os.mkdir(self.tmpdir)
+            success = self.run_variant()
+            if success == TestStatus.SKIPPED and exit_code == TEST_EXIT_PASSED:


ryanofsky · 2018-01-04T14:36:12Z

test/functional/multiwallet.py

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


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

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

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.

Support same get_request and batch methods as AuthServiceProxy

jnewbery · 2018-01-08T20:55:19Z

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 · 2018-01-08T21:19:17Z

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

promag · 2018-01-08T21:21:39Z

@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 · 2018-01-08T21:42:30Z

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 · 2018-01-08T22:02:21Z

Reset to jnewbery pr11970.1, 963315d -> f4bad31, pr/mcli.2 -> pr/mcli.3

ryanofsky

ACK f4bad31

promag

utACK f4bad31.

promag · 2018-01-08T22:10:50Z

test/functional/test_framework/test_node.py

@@ -74,8 +75,11 @@ def __init__(self, i, dirname, extra_args, rpchost, timewait, binary, stderr, mo

    def __getattr__(self, name):
        """Dispatches any unrecognised messages to the RPC connection."""


In commit "[tests] allow tests to be run with --usecli"

Improve comment.

promag · 2018-01-08T22:16:57Z

test/functional/test_framework/test_framework.py

@@ -113,6 +116,8 @@ def main(self):
        success = TestStatus.FAILED

        try:
+            if self.options.usecli and not self.supports_cli:
+                raise SkipTest("--usecli specificed but test does not support using CLI")


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

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.

Add test coverage for bitcoin-cli multiwallet calls.

promag · 2018-01-08T23:08:31Z

ACK a14dbff.

Running a single also works as expected IMO:

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

jnewbery · 2018-01-11T22:21:05Z

ACK a14dbff (obviously, since I authored it!)

maflcko · 2018-01-12T22:23:23Z

@jnewbery

a14dbff Allow multiwallet.py to be used with --usecli (Russell Yanofsky) f6ade9c [tests] allow tests to be run with --usecli (John Newbery) ff9a363 TestNodeCLI batch emulation (Russell Yanofsky) ca9085a Prevent TestNodeCLI.args mixups (Russell Yanofsky) fcfb952 Improve TestNodeCLI output parsing (Russell Yanofsky) Pull request description: Lack of test coverage was pointed out by @jnewbery in #11687 (comment) Tree-SHA512: 5f10e31abad11a5edab0da4e2515e39547adb6ab9e55e50427ab2eb7ec9a43d6b896b579b15863e5edc9beee7d8bf1c84d9dabd247be0760a1b9ae39e1e8ee02

@jnewbery

a14dbff Allow multiwallet.py to be used with --usecli (Russell Yanofsky) f6ade9c [tests] allow tests to be run with --usecli (John Newbery) ff9a363 TestNodeCLI batch emulation (Russell Yanofsky) ca9085a Prevent TestNodeCLI.args mixups (Russell Yanofsky) fcfb952 Improve TestNodeCLI output parsing (Russell Yanofsky) Pull request description: Lack of test coverage was pointed out by @jnewbery in bitcoin#11687 (comment) Tree-SHA512: 5f10e31abad11a5edab0da4e2515e39547adb6ab9e55e50427ab2eb7ec9a43d6b896b579b15863e5edc9beee7d8bf1c84d9dabd247be0760a1b9ae39e1e8ee02

@jnewbery

a14dbff Allow multiwallet.py to be used with --usecli (Russell Yanofsky) f6ade9c [tests] allow tests to be run with --usecli (John Newbery) ff9a363 TestNodeCLI batch emulation (Russell Yanofsky) ca9085a Prevent TestNodeCLI.args mixups (Russell Yanofsky) fcfb952 Improve TestNodeCLI output parsing (Russell Yanofsky) Pull request description: Lack of test coverage was pointed out by @jnewbery in bitcoin#11687 (comment) Tree-SHA512: 5f10e31abad11a5edab0da4e2515e39547adb6ab9e55e50427ab2eb7ec9a43d6b896b579b15863e5edc9beee7d8bf1c84d9dabd247be0760a1b9ae39e1e8ee02

… such Summary: The option `--usecli` was added to test_framework in [[bitcoin/bitcoin#11970 | PR11970]]. Running the test with this parameter will cause all RPCs to be sent through `bitcoin-cli` rather than directly over http. But the test were not all annotated accordingly. 74 tests were unnecessarily skipped. Explicitely annotating the tests that do not support this option will allow to make this lack of support the exception, and identify these tests clearly as needing to be fixed in the future. This commit has currently no effect, as `self.supports_cli` is still considered `False` by default. This will be changed in the next commit. In addition to the tests skipped in the Core PR, I also found the following tests to not support `--usecli`: - `abc_wallet_standardness`: `TypeError: Object of type 'Decimal' is not JSON serializable` (will be fixed when backporting PR17705) - `abc-magnetic-anomaly-mining`: idem - `p2p_blocksonly`: idem - `rpc_whitelist`: `AttributeError: 'NoneType' object has no attribute 'rfind'` (this test was added after PR17675 and should have been skipped in Core as well) - `abc_p2p_fullblocktest`: `OSError: [Errno 7] Argument list too long: '.../bitcoin-abc/build/src/bitcoin-cli'` - `wallet_groups` : idem This is a backport of Core [[bitcoin/bitcoin#17675 | PR17675]] [1/2] bitcoin/bitcoin@993e38a Test Plan: `test/functional/test_runner.py --usecli` Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Subscribers: deadalnix Differential Revision: https://reviews.bitcoinabc.org/D8316

@jnewbery

a14dbff Allow multiwallet.py to be used with --usecli (Russell Yanofsky) f6ade9c [tests] allow tests to be run with --usecli (John Newbery) ff9a363 TestNodeCLI batch emulation (Russell Yanofsky) ca9085a Prevent TestNodeCLI.args mixups (Russell Yanofsky) fcfb952 Improve TestNodeCLI output parsing (Russell Yanofsky) Pull request description: Lack of test coverage was pointed out by @jnewbery in bitcoin#11687 (comment) Tree-SHA512: 5f10e31abad11a5edab0da4e2515e39547adb6ab9e55e50427ab2eb7ec9a43d6b896b579b15863e5edc9beee7d8bf1c84d9dabd247be0760a1b9ae39e1e8ee02

fanquake added the Tests label Dec 21, 2017

ryanofsky mentioned this pull request Dec 21, 2017

External wallet files #11687

Merged

ryanofsky force-pushed the pr/mcli branch from 46d4641 to 963315d Compare December 21, 2017 13:45

laanwj added the Wallet label Dec 21, 2017

maflcko reviewed Jan 4, 2018

View reviewed changes

promag reviewed Jan 4, 2018

View reviewed changes

ryanofsky commented Jan 4, 2018

View reviewed changes

ryanofsky changed the title ~~Add test coverage for bitcoin-cli multiwallet calls~~ WIP: Add test coverage for bitcoin-cli multiwallet calls Jan 4, 2018

ryanofsky added 3 commits January 8, 2018 15:17

Improve TestNodeCLI output parsing

fcfb952

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

TestNodeCLI batch emulation

ff9a363

Support same get_request and batch methods as AuthServiceProxy

ryanofsky force-pushed the pr/mcli branch from 963315d to f4bad31 Compare January 8, 2018 22:02

ryanofsky changed the title ~~WIP: Add test coverage for bitcoin-cli multiwallet calls~~ Add test coverage for bitcoin-cli multiwallet calls Jan 8, 2018

ryanofsky commented Jan 8, 2018

View reviewed changes

promag reviewed Jan 8, 2018

View reviewed changes

jnewbery and others added 2 commits January 8, 2018 17:35

Allow multiwallet.py to be used with --usecli

a14dbff

Add test coverage for bitcoin-cli multiwallet calls.

jnewbery force-pushed the pr/mcli branch from f4bad31 to a14dbff Compare January 8, 2018 22:42

maflcko merged commit a14dbff into bitcoin:master Jan 12, 2018

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

		@@ -74,8 +75,11 @@ def __init__(self, i, dirname, extra_args, rpchost, timewait, binary, stderr, mo

		def __getattr__(self, name):
		"""Dispatches any unrecognised messages to the RPC connection."""

Add test coverage for bitcoin-cli multiwallet calls #11970

Add test coverage for bitcoin-cli multiwallet calls #11970

Uh oh!

Conversation

ryanofsky commented Dec 21, 2017

Uh oh!

jonasschnelli commented Dec 21, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maflcko Jan 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maflcko commented Jan 4, 2018

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maflcko Jan 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnewbery commented Jan 8, 2018

Uh oh!

ryanofsky commented Jan 8, 2018

Uh oh!

promag commented Jan 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jnewbery commented Jan 8, 2018

Uh oh!

ryanofsky commented Jan 8, 2018

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

promag commented Jan 8, 2018

Uh oh!

jnewbery commented Jan 11, 2018

Uh oh!

maflcko commented Jan 12, 2018

Uh oh!

Uh oh!

maflcko Jan 4, 2018 •

edited

Loading

maflcko Jan 4, 2018 •

edited

Loading

promag commented Jan 8, 2018 •

edited

Loading