test: Remove wallet option from non-wallet tests #26480

maflcko · 2022-11-10T15:38:16Z

The tests have several issues:

Some tests that are wallet-type specific offer the option to run the test with the incompatible type

For example, wallet_dump.py offers --descriptors and on current master fails with JSONRPCException: Invalid public key. After the changes here, it fails with a clear error: unrecognized arguments: --descriptors.

Tests that don't use the wallet at all offer the option to run it with a wallet type. This is confusing and wastes developers time if they are "tricked" into running the test for both wallet types, even though no wallet code is executed at all.

For example, feature_addrman.py will happily accept and run with --descriptors or --legacy-wallet. After the changes here, it no longer silently ignores the flag, but reports a clear error: unrecognized arguments.

The bool is only used to call a public helper, which some tests already do. So use the public helper in all tests consistently and make the confusingly named bool private.

self.descriptors is None when no wallet has been compiled, so it is safe to completely disable the wallet. This change will enhance a future commit.

Review note: The changes are complete, because self.options.descriptors is set to None in parse_args (test_framework.py). A value of None implies -disablewallet, see the previous commit. So if a call to add_wallet_options is missing, it will lead to a test failure when the wallet is compiled in.

maflcko · 2022-11-10T16:24:49Z

The change here would also allow to enforce consistency when running the tests, so that review feedback like #24865 (comment) (or similar) can be detected locally before creating a pull request. Let me know if this is desired (in a follow-up).

DrahtBot · 2022-11-12T00:38:42Z

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	achow101
Concept ACK	luke-jr
Approach ACK	S3RK

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#20892 (tests: Run both descriptor and legacy tests within a single test invocation by achow101)

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.

S3RK · 2022-11-14T08:53:38Z

Concept ACK.

Approach wise I think a subclass for wallet tests (similar to #20892) is cleaner. Creating a subclass enables us to incapsulate wallet specific logic in one place instead of spreading it across test files. I'd like to hear the arguments against creating a new subclass for wallet tests.

achow101 · 2022-11-14T22:09:41Z

Concept ACK

One issue I noticed is that self.options.descriptors's default value does not necessarily match what the test expects. Some tests, such as wallet_avoid_mixing_output_types.py do not work if --descriptors is not specified as it is a descriptor wallet test, but the default value for self.options.descriptors indicates legacy wallet. There are other descriptor wallet only tests which actually do work with this incorrect default value, but that's only because they completely ignore self.options.descriptors.

luke-jr · 2022-11-14T23:23:28Z

Concept ACK. Approach meh considering we already have another place(s) we specify wallet-used/required in tests.

Seems like it would be better to just let test classes define wallet_used or wallet_required = True/False/["legacy"]/["descriptors"] and let the framework deal with the skipping/options/etc.

maflcko · 2022-11-15T09:07:44Z

I'd like to hear the arguments against creating a new subclass for wallet tests.

As Luke points out, there are at least 4 cases, so I don't think a single subclass will help. Maybe you can elaborate?

Some tests, such as wallet_avoid_mixing_output_types.py do not work if --descriptors is not specified as it is a descriptor wallet test, but the default value for self.options.descriptors indicates legacy wallet.

Yes, this is a bug that already exists on master. It can be trivially fixed with the changes here, see the new commit.

Seems like it would be better to just let test classes define ...

This is exactly what this pull is doing, no? Note that there is a difference between the "hard" skip and the "soft" option: Some tests may be mostly non-wallet, but have a small test portion that requires a wallet, but is skipped otherwise. If the wallet isn't compiled in, the test is still run, except for the small wallet portion.

luke-jr · 2022-11-15T20:31:37Z

This is exactly what this pull is doing, no?

This PR seems to leave in place the previous wallet-required/used checks, but adds yet another place wallet-using tests need to explicitly define that they use the wallet.

My suggestion is that tests should only need to define it one place (or at most two, to handle optional vs required)

achow101 · 2022-11-15T20:32:53Z

ACK fa10f19

maflcko · 2022-11-15T20:34:15Z

My suggestion is that tests should only need to define it one place (or at most two, to handle optional vs required)

This is impossible unless the test logic of a few tests is rewritten, so my suggestion would be to first rewrite the test logic and then address your concern.

luke-jr · 2022-11-15T23:22:10Z

This is impossible unless the test logic of a few tests is rewritten

Why? Can't the class just make parse_args's wallet options conditional on self.uses_wallet, and then do the same with skip_if_no_wallet in a refactor?

maflcko · 2022-11-16T08:07:58Z

Sure you can turn the type bool into a tristate type-off, type-soft, and type-hard, but I am not sure if this is an improvement. Soft and hard handling are separate concerns, so it seems better to handle them separately.

S3RK · 2022-11-16T08:14:56Z

test/functional/test_framework/test_framework.py

                self.options.descriptors = False
            elif self.is_sqlite_compiled():
                self.options.descriptors = True
            else:


This else block seems redundant. It sets value to None when it's None already

Correct. This is also the current code on master. My guess is that this was done on purpose, to be able to attach a comment to the None value, which is why I left this as-is for now.

I can replace the ... = None with pass, but I am not sure if this is an improvement.

S3RK · 2022-11-17T08:02:53Z

I'd like to hear the arguments against creating a new subclass for wallet tests.

As Luke points out, there are at least 4 cases, so I don't think a single subclass will help. Maybe you can elaborate?

What I had in mind is to create BitcoinWalletTestFramework that would be required for tests involving a wallet. This subclass should be responsible for handling the arguments and the logic of supported wallet types: legacy, descriptors or both. But this could be done in a follow up and this PR is an improvement over status quo. It's great that add_wallet_option now clearly delineates wallet vs non-wallet tests.

Approach ACK.

maflcko · 2022-11-28T13:32:28Z

Did anyone who commented here (or anyone else) wanted to review this? If not, it can probably be merged

bcb7123 test: add add_wallet_options to TestShell (josibake) Pull request description: following 555519d, `TestShell` now always runs with `-disablewallet`. simple fix is to add `add_wallet_options` to `add_options`; looks like testshell was overlooked when adding in the `add_wallet_options` call to the functional tests in #26480 ACKs for top commit: amitiuttarwar: ACK bcb7123 Tree-SHA512: db554a8b3c8ff5bd10cab9592b316035a92f86a0a0ae8ff914de9687bbbb6fc2235bdf82c4cd40e4071782f8b6edf91aad372e82ed3b826c9d8ae39dbe3dbf57

MacroFake added 2 commits November 10, 2022 10:01

test: Make requires_wallet private

fa68937

The bool is only used to call a public helper, which some tests already do. So use the public helper in all tests consistently and make the confusingly named bool private.

test: Set -disablewallet when no wallet has been compiled

fac8d59

self.descriptors is None when no wallet has been compiled, so it is safe to completely disable the wallet. This change will enhance a future commit.

maflcko mentioned this pull request Nov 10, 2022

tests: Run both descriptor and legacy tests within a single test invocation #20892

Closed

fanquake added the Tests label Nov 10, 2022

maflcko force-pushed the 2211-test-wallet-options- branch from fafe79b to 555519d Compare November 10, 2022 16:19

test: Set default in add_wallet_options if only one type can be chosen

fa10f19

S3RK reviewed Nov 16, 2022

View reviewed changes

achow101 merged commit 8597260 into bitcoin:master Nov 28, 2022

maflcko deleted the 2211-test-wallet-options-💣 branch November 28, 2022 16:25

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Dec 1, 2022

Merge bitcoin#26480: test: Remove wallet option from non-wallet tests

deef63c

josibake mentioned this pull request Dec 12, 2022

test: add add_wallet_options to TestShell #26689

Merged

bitcoin locked and limited conversation to collaborators Nov 28, 2023

test: Remove wallet option from non-wallet tests #26480

test: Remove wallet option from non-wallet tests #26480

Uh oh!

Conversation

maflcko commented Nov 10, 2022

Uh oh!

maflcko commented Nov 10, 2022

Uh oh!

DrahtBot commented Nov 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews

Conflicts

Uh oh!

S3RK commented Nov 14, 2022

Uh oh!

achow101 commented Nov 14, 2022

Uh oh!

luke-jr commented Nov 14, 2022

Uh oh!

maflcko commented Nov 15, 2022

Uh oh!

luke-jr commented Nov 15, 2022

Uh oh!

achow101 commented Nov 15, 2022

Uh oh!

maflcko commented Nov 15, 2022

Uh oh!

luke-jr commented Nov 15, 2022

Uh oh!

maflcko commented Nov 16, 2022

Uh oh!

S3RK Nov 16, 2022

Choose a reason for hiding this comment

Uh oh!

maflcko Nov 16, 2022

Choose a reason for hiding this comment

Uh oh!

maflcko Nov 16, 2022 • 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!

S3RK commented Nov 17, 2022

Uh oh!

maflcko commented Nov 28, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

DrahtBot commented Nov 12, 2022 •

edited

Loading

maflcko Nov 16, 2022 •

edited

Loading