test: Remove wallet option from non-wallet tests

maflcko commented at 3:38 pm on November 10, 2022: member

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.

test: Make requires_wallet private

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.

fa68937b89

test: Set -disablewallet when no wallet has been compiled

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.

fac8d59d31

fanquake added the label Tests on Nov 10, 2022

test: Remove wallet option from non-wallet tests

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.

555519d082

maflcko force-pushed on Nov 10, 2022

maflcko commented at 4:24 pm on November 10, 2022: member

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

DrahtBot commented at 0:38 am on November 12, 2022: contributor

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 commented at 8:53 am on November 14, 2022: contributor

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 commented at 10:09 pm on November 14, 2022: member

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 commented at 11:23 pm on November 14, 2022: member

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.

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

maflcko commented at 9:07 am on November 15, 2022: member

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 commented at 8:31 pm on November 15, 2022: member

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 commented at 8:32 pm on November 15, 2022: member

ACK fa10f193b54650b3071bc7ee2d90fcfe40a16dc9

maflcko commented at 8:34 pm on November 15, 2022: member

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 commented at 11:22 pm on November 15, 2022: member

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 commented at 8:07 am on November 16, 2022: member

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.

in test/functional/test_framework/test_framework.py:221 in fa10f193b5

220@@ -221,6 +221,7 @@ def parse_args(self):
221             else:

S3RK commented at 8:14 am on November 16, 2022:

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

maflcko commented at 8:26 am on November 16, 2022:

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.

maflcko commented at 8:27 am on November 16, 2022:

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

S3RK commented at 8:02 am on November 17, 2022: contributor

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 commented at 1:32 pm on November 28, 2022: member

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

achow101 merged this on Nov 28, 2022

achow101 closed this on Nov 28, 2022

maflcko deleted the branch on Nov 28, 2022

sidhujag referenced this in commit deef63c88b on Dec 1, 2022

maflcko referenced this in commit 678889e6c6 on Dec 14, 2022

bitcoin locked this on Nov 28, 2023

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

Reviews

Conflicts