test: remove strict restrictions on rpc

polespinasa commented at 6:43 pm on March 25, 2025: contributor

Removed the wallet restrictions for rpc_deprecated.py and added specific test case for the current deprecated rpc. skip_test_if_missing_module will skip the whole test when the wallet is missing, even if a part of the test is non-wallet related. This PR ensures that other tests not related to wallet can be ran and only this specific test will be skipped if there’s no wallet

For more context check #31278 (review)

DrahtBot commented at 6:43 pm on March 25, 2025: contributor

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

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32139.

Reviews

See the guideline for information on the review process. A summary of reviews will appear here.

Conflicts

No conflicts as of last run.

DrahtBot added the label Tests on Mar 25, 2025

polespinasa renamed this:
~~test: remove strict restrictions~~
test: remove strict restrictions on rpc_deprecated test
on Mar 25, 2025

in test/functional/rpc_deprecated.py:31 in a37547c875 outdated

26@@ -31,8 +27,13 @@ def run_test(self):
27         # at least one other functional test that still tests the RPCs
28         # functionality using the respective -deprecatedrpc flag.
29 
30-        self.log.info("Test settxfee RPC")
31-        assert_raises_rpc_error(-32, 'settxfee is deprecated and will be fully removed in v31.0.', self.nodes[0].rpc.settxfee, 0.01)
32+        self.log.info("Tests for deprecated RPC methods (if any)")
33+        # self.log.info("Currently no tests for deprecated RPC methods")

yancyribbens commented at 7:24 pm on March 25, 2025:

did you intend to leave this commented code?

polespinasa commented at 7:34 pm on March 25, 2025:

yes, here the conversation #31278 (review) I have modified the comments a bit to, I hope, make it clearer.

maflcko commented at 11:42 am on March 27, 2025:

My suggestion to use “(if any)” was to be able to just leave everything in the skeleton as-is forever. In the future only new test cases are added or removed. This should also reduce merge conflicts.

The suggestion was, but I am not sure if it was self-explanatory: (If not, maybe add a comment to not remove or otherwise touch the log line)

0self.log.info("Tests for deprecated RPC methods (if any)")
1
2if self.is_wallet_compiled():
3  self.log.info("Tests for deprecated wallet-related RPC methods (if any)")

polespinasa commented at 12:14 pm on March 27, 2025:

Oh so you suggest to leave the self.is_wallet_compiled() conditional as a skeleton for other future tests? And leave the log “…if any…” always not commented, so it’s still valid with or without tests? If so, I like the idea will apply.

polespinasa force-pushed on Mar 25, 2025

in test/functional/rpc_deprecated.py:35 in 983b035b1f outdated

32 
33-        self.log.info("Test settxfee RPC")
34-        assert_raises_rpc_error(-32, 'settxfee is deprecated and will be fully removed in v31.0.', self.nodes[0].rpc.settxfee, 0.01)
35+        if self.is_wallet_compiled():
36+            self.log.info("Test settxfee RPC deprecation")
37+            self.nodes[0].createwallet("settxfeerpc")

rkrux commented at 2:42 pm on March 26, 2025:

Is creating wallet necessary? It didn’t seem to be earlier.

polespinasa commented at 2:46 pm on March 26, 2025:

Seems to, if the wallet is not created the following error is thrown on the assert:

No wallet is loaded. Load a wallet using loadwallet or create a new one with createwallet. (Note: A default wallet is no longer automatically created) (-18)

rkrux commented at 3:07 pm on March 26, 2025:

Interesting, I was wondering why, ended up tracking down the conditional, makes sense now.

https://github.com/bitcoin/bitcoin/blob/c0b7159de47592c95c6db061de682b4af4058102/test/functional/test_framework/test_framework.py#L470-L471

which is due to:

https://github.com/bitcoin/bitcoin/blob/c0b7159de47592c95c6db061de682b4af4058102/test/functional/test_framework/test_framework.py#L158-L160

rkrux commented at 3:18 pm on March 26, 2025:

The skip_if_no_wallet function updates the internal state of the BitcoinTestFramework class, which I wouldn’t have guessed just by its name.

maflcko commented at 3:23 pm on March 26, 2025:

I guess it could make sense to require explicit setup of any wallets in tests, but this can be done in a follow-up, probably best after the bdb removal

rkrux commented at 2:42 pm on March 26, 2025: contributor

remove strict restrictions 983b035

I would suggest updating the commit message according to https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#committing-patches

The PR description contains useful information that can be used to some extent.

polespinasa force-pushed on Mar 26, 2025

test: remove strict restrictions on rpc_deprecated

Removed the wallet restrictions for rpc_deprecated.py and added specific test case for the current deprecated rpc.
skip_test_if_missing_module will skip the whole test when the wallet is missing, even if a part of the test is non-wallet related.

aa18485120

polespinasa force-pushed on Mar 26, 2025

test: remove strict restrictions on rpc_deprecated test #32139

Code Coverage & Benchmarks

Reviews

Conflicts