test: Use rpc_deprecated only for testing deprecation #31977

pull fjahr wants to merge 1 commits into bitcoin:master from fjahr:2025-03-dep changing 1 files +11 −11
  1. fjahr commented at 9:05 pm on March 3, 2025: contributor
    The comment in functional/rpc_deprecated.py says “This test should be used to verify correct behaviour of deprecated RPC methods with and without the -deprecatedrpc flags.” I think we can get rid of the “with” part since we can assume that every deprecated RPC is already tested in at least one other functional test. (I didn’t look but I could verify in our coverage if someone has doubts about that.) In order for this test to continue working, the flag will need to be used there. Otherwise this seems to prescribe copy+pasting a basic test from another file and I don’t see a good reason for that.
  2. DrahtBot commented at 9:05 pm on March 3, 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/31977.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK janb84, polespinasa, maflcko
    Concept ACK musaHaruna
    Stale ACK rkrux

    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:

    • #31278 (wallet, rpc: deprecate settxfee and paytxfee by polespinasa)

    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.

  3. DrahtBot added the label Tests on Mar 3, 2025
  4. polespinasa commented at 10:53 am on March 4, 2025: none

    Concept ACK.

    No need to double test a functionality.

  5. in test/functional/rpc_deprecated.py:22 in c5e384de6c outdated 
    25         # self.log.info("Test generate RPC")
26         # assert_raises_rpc_error(-32, 'The wallet generate rpc method is deprecated', self.nodes[0].rpc.generate, 1)
27-        # self.generate(self.nodes[1], 1)
28+        #
29+        # Please ensure that the -depgrecatedrpc flag for this RPC method is
30+        # used in anther test where the RPC functionality is actually tested.
    rkrux commented at 2:24 pm on March 4, 2025:
    Couple typos depgrecatedrpc anther
    fjahr commented at 4:54 pm on March 4, 2025:
    fixed
  6. in test/functional/rpc_deprecated.py:16 in c5e384de6c outdated 
    14+        self.extra_args = [[]]
15 
16     def run_test(self):
17         # This test should be used to verify correct behaviour of deprecated
18-        # RPC methods with and without the -deprecatedrpc flags. For example:
19+        # RPC methods without the -deprecatedrpc flag. For example:
    rkrux commented at 2:45 pm on March 4, 2025:

    This is the final comment after this change:

    This test should be used to verify correct behaviour of deprecated RPC methods without the -deprecatedrpc flag.

    I think it can be reworded to be more explicit in the intention. I’m assuming the deprecated RPCs will just throw errors, please correct me if I’m wrong.

    0This test should be used to verify the errors of the currently deprecated RPC methods (without the -deprecatedrpc flag) until such RPCs are fully removed.

    Suggesting this because first time I have looked at this test and I found it a little strange that it doesn’t test any RPCs at the moment until I saw the file commit history.

    fjahr commented at 4:54 pm on March 4, 2025:
    Added, thanks!
  7. in test/functional/rpc_deprecated.py:24 in c5e384de6c outdated 
    27-        # self.generate(self.nodes[1], 1)
28+        #
29+        # Please ensure that the -depgrecatedrpc flag for this RPC method is
30+        # used in anther test where the RPC functionality is actually tested.
31 
32         self.log.info("No tested deprecated RPC methods")
    rkrux commented at 2:45 pm on March 4, 2025:

    Nit:

    0 self.log.info("No tested currently deprecated RPC methods")
    fjahr commented at 4:54 pm on March 4, 2025:
    Changed this but with slightly different wording
  8. rkrux commented at 2:49 pm on March 4, 2025: contributor

    Concept ACK c5e384de6c0b92692021272a7464cd26a54d298c

    New to this test, suggested some verbiage change. PLMK if it doesn’t fit.

  9. fjahr force-pushed on Mar 4, 2025
  10. fjahr commented at 4:55 pm on March 4, 2025: contributor
    Addressed the comments by @rkrux , thanks!
  11. in test/functional/rpc_deprecated.py:10 in b1c80fab0c outdated 
     6@@ -7,23 +7,22 @@
 7 
 8 class DeprecatedRpcTest(BitcoinTestFramework):
 9     def set_test_params(self):
10-        self.num_nodes = 2
11+        self.num_nodes = 1
    rkrux commented at 9:08 am on March 5, 2025:
    1 node fewer is an additional benefit.
  12. in test/functional/rpc_deprecated.py:17 in b1c80fab0c outdated 
    16     def run_test(self):
17-        # This test should be used to verify correct behaviour of deprecated
18-        # RPC methods with and without the -deprecatedrpc flags. For example:
19+        # This test should be used to verify the errors of the currently
20+        # deprecated RPC methods (without the -deprecatedrpc flag) until
21+        # such RPCs are fully removed.
    rkrux commented at 9:08 am on March 5, 2025:
    Missed the For example part. In case retouched, fine otherwise.
    fjahr commented at 2:00 pm on March 5, 2025:
    Right, will add it back if I retouch.
    fjahr commented at 9:48 pm on March 14, 2025:
    Added it back now
  13. rkrux approved
  14. rkrux commented at 9:13 am on March 5, 2025: contributor

    since we can assume that every RPC is already tested

    In the PR description, it can be reworded to say ... that every deprecated RPC is ... in order to narrow down its intent.

    Thanks for addressing the comments, ACK b1c80fab0c026a40f2901958abdeedb9dd36c30d

  15. DrahtBot requested review from polespinasa on Mar 5, 2025
  16. in test/functional/rpc_deprecated.py:1 in b1c80fab0c
    polespinasa commented at 10:12 am on March 11, 2025:
    nit: update copyright
    fjahr commented at 9:48 pm on March 14, 2025:
    done
  17. polespinasa commented at 10:12 am on March 11, 2025: none
    ACK b1c80fab0c026a40f2901958abdeedb9dd36c30d
  18. in test/functional/rpc_deprecated.py:23 in b1c80fab0c outdated 
    27         # self.log.info("Test generate RPC")
28         # assert_raises_rpc_error(-32, 'The wallet generate rpc method is deprecated', self.nodes[0].rpc.generate, 1)
29-        # self.generate(self.nodes[1], 1)
30+        #
31+        # Please ensure that the -deprecatedrpc flag for this RPC method is
32+        # used in another test where the RPC functionality is actually tested.
    janb84 commented at 11:40 am on March 12, 2025:

    nit: Needed to read this comment several times before understanding, suggested small change for clarity.

    0        # Please ensure that the -deprecatedrpc flag for all the RPC methods
1        # mentioned above is used in their respective functional tests, 
2        # where the RPC functionality is actually tested.
    fjahr commented at 9:49 pm on March 14, 2025:
    I have reworked that part a bit, I hope it’s clearer now.
  19. janb84 commented at 11:43 am on March 12, 2025: contributor

    Concept ACK b1c80fa

    Suggested small comment change for clarity, feel free to ignore.

  20. musaHaruna commented at 5:40 pm on March 12, 2025: none
    Concept ACK b1c80fa New to this test, please correct me know if am wrong, after checking the history of the file, it seems that it has primarily been used to test deprecated RPCs both with and without the -deprecatedrpc flag. This change proposes to focus exclusively on testing the behavior of deprecated RPCs without the flag, as other tests already cover the cases where the -deprecatedrpc flag is used.
  21. test: Use rpc_deprecated only for testing deprecation 2819c51482
  22. fjahr force-pushed on Mar 14, 2025
  23. fjahr commented at 9:50 pm on March 14, 2025: contributor

    Addressed the comments now since there were a few that made sense to fix.

    New to this test, please correct me know if am wrong, after checking the history of the file, it seems that it has primarily been used to test deprecated RPCs both with and without the -deprecatedrpc flag. This change proposes to focus exclusively on testing the behavior of deprecated RPCs without the flag, as other tests already cover the cases where the -deprecatedrpc flag is used.

    Yes, that’s correct!

  24. janb84 commented at 10:37 am on March 15, 2025: contributor
    re ACK 2819c51 Thank you for rewriting the comment - the intention is much easier to understand now (IMO)
  25. DrahtBot requested review from polespinasa on Mar 15, 2025
  26. DrahtBot requested review from rkrux on Mar 15, 2025
  27. DrahtBot requested review from musaHaruna on Mar 15, 2025
  28. polespinasa commented at 10:55 am on March 15, 2025: none
    reACK 2819c514825577dc8d73690882f25117d412ed9f
  29. maflcko commented at 11:39 am on March 15, 2025: member
    lgtm ACK 2819c514825577dc8d73690882f25117d412ed9f
  30. fanquake merged this on Mar 16, 2025
  31. fanquake closed this on Mar 16, 2025


fjahr DrahtBot polespinasa rkrux janb84 musaHaruna maflcko


rkrux musaHaruna

Labels
Tests

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2025-03-29 06:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me