test: fix RPC coverage check #29387

pull BrandonOdiwuor wants to merge 2 commits into bitcoin:master from BrandonOdiwuor:test-coverage changing 4 files +15 −3
  1. BrandonOdiwuor commented at 6:16 pm on February 5, 2024: contributor

    Fixes #27593

    Currently, the RPC coverage check in functional tests doesn’t include a list of all RPCs. This fix enables wallet RPCs to be included in the rpc_interface.txt used in the coverage check

    This PR Reverses part of the changes on PR #16042 - Commit https://github.com/bitcoin/bitcoin/pull/16042/commits/fa473303972b7dad600d949dc9b303d8136cb7e7#diff-4a04bc0b355c780033960e8c261ee9b6d3c452897e1dcd88a15d272512266c76R486 to speed up cache_creation by disabling the wallet

    Update:

    • I have added basic abortrescan RPC test (commit - eb085603af2a5da954cf7f95c7f77e173585e878) to fix the failing CI due to no coverage
  2. DrahtBot commented at 6:16 pm on February 5, 2024: contributor

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

    Code Coverage

    For detailed information about the code coverage, see the test coverage report.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    Concept ACK epiccurious, hernanmarino
    Stale ACK maflcko

    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:

    • #29335 (test: Handle functional test disk-full error by BrandonOdiwuor)

    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 Feb 5, 2024
  4. in test/functional/test_framework/test_framework.py:832 in 156c1a25fc outdated 
    813@@ -814,7 +814,7 @@ def _initialize_chain(self):
814                     cache_node_dir,
815                     chain=self.chain,
816                     extra_conf=["bind=127.0.0.1"],
817-                    extra_args=['-disablewallet'],
    jo-elimu commented at 4:36 am on February 6, 2024:
    @BrandonOdiwuor What does the -disablewallet argument mean?
    BrandonOdiwuor commented at 3:40 pm on February 6, 2024:
    you can follow the conversation under the issue: https://github.com/bitcoin/bitcoin/issues/27593
  5. in test/functional/test_framework/test_framework.py:832 in 156c1a25fc outdated 
    813@@ -814,7 +814,7 @@ def _initialize_chain(self):
814                     cache_node_dir,
815                     chain=self.chain,
816                     extra_conf=["bind=127.0.0.1"],
817-                    extra_args=['-disablewallet'],
818+                    extra_args=[],
    maflcko commented at 7:55 am on February 6, 2024:
    You can remove this? Also, it would be good to mention the commit that this reverts. (It may be one of mine)
    epiccurious commented at 12:17 pm on February 6, 2024:

    The previous commit per git blame is test: Speed up cache creation dated May 17, 2019 by @maflcko.

    -disablewallet arg was added earlier in qa: Premine to deterministic address with -disablewallet dated Sep 10, 2018 also by maflcko.

    BrandonOdiwuor commented at 3:43 pm on February 6, 2024:
    @maflcko extra_args parameter cannot be removed since it’s a named parameter, unless the TestNode.__init__(...) signature is changed
  6. maflcko commented at 9:29 am on February 6, 2024: member
    Also, it doesn’t seem to work, because the CI should fail, because the abortrescan rpc is uncovered
  7. BrandonOdiwuor commented at 2:23 pm on February 6, 2024: contributor

    @maflcko this is because self.options.descriptors == None in TestNode(... , descriptors=self.options.descriptors, ...) hence the disable_wallet flag is added when the TestNode is instantiated disabling the wallet RPCs just before the first start_node() is invoked

    https://github.com/bitcoin/bitcoin/blob/4de84557d6d1f53255ab19f27c8b6ea0a712934a/test/functional/test_framework/test_node.py#L111-L112

  8. BrandonOdiwuor force-pushed on Feb 6, 2024
  9. BrandonOdiwuor requested review from maflcko on Feb 6, 2024
  10. BrandonOdiwuor requested review from epiccurious on Feb 6, 2024
  11. BrandonOdiwuor marked this as ready for review on Feb 6, 2024
  12. in test/functional/test_framework/test_framework.py:813 in a4498d3add outdated 
    806@@ -807,14 +807,21 @@ def _initialize_chain(self):
807         if not os.path.isdir(cache_node_dir):
808             self.log.debug("Creating cache directory {}".format(cache_node_dir))
809 
810+            # Do not -disablewallet when in coverage mode to enable wallet RPCs to be checked
811+            if self.options.coveragedir is not None:
812+                extra_args = []
813+                self.options.descriptors = True # since `None` will -disablewallet in TestNode(...)
    maflcko commented at 3:48 pm on February 6, 2024:
    Why not add this to create_cache.py properly, like in all other wallet tests?
    BrandonOdiwuor commented at 11:04 am on February 7, 2024:
    fixed
  13. DrahtBot added the label CI failed on Feb 6, 2024
  14. BrandonOdiwuor force-pushed on Feb 7, 2024
  15. BrandonOdiwuor requested review from maflcko on Feb 7, 2024
  16. in test/functional/test_framework/test_framework.py:811 in 1806de015e outdated 
    806@@ -807,14 +807,17 @@ def _initialize_chain(self):
807         if not os.path.isdir(cache_node_dir):
808             self.log.debug("Creating cache directory {}".format(cache_node_dir))
809 
810+            # Do not -disablewallet when in coverage mode to enable wallet RPCs to be checked
811+            extra_args = [] if self.options.coveragedir is not None else ['-disablewallet']
    maflcko commented at 11:45 am on February 7, 2024:

    Is this needed? Why not:

    0            extra_args = []
    BrandonOdiwuor commented at 12:14 pm on February 7, 2024:
    fixed
  17. in test/functional/test_runner.py:591 in 1806de015e outdated 
    587@@ -588,7 +588,7 @@ def run_tests(*, test_list, src_dir, build_dir, tmpdir, jobs=1, enable_coverage=
588     if len(test_list) > 1 and jobs > 1:
589         # Populate cache
590         try:
591-            subprocess.check_output([sys.executable, tests_dir + 'create_cache.py'] + flags + ["--tmpdir=%s/cache" % tmpdir])
592+            subprocess.check_output([sys.executable, tests_dir + 'create_cache.py'] + flags + ["--tmpdir=%s/cache" % tmpdir] + ["--descriptors"])
    maflcko commented at 11:46 am on February 7, 2024:
    nit: Could use f-string instead of % when touching this line?
    BrandonOdiwuor commented at 12:13 pm on February 7, 2024:
    fixed
  18. BrandonOdiwuor force-pushed on Feb 7, 2024
  19. BrandonOdiwuor force-pushed on Feb 7, 2024
  20. BrandonOdiwuor requested review from maflcko on Feb 7, 2024
  21. in test/functional/test_runner.py:591 in 9e28a83965 outdated 
    587@@ -588,7 +588,7 @@ def run_tests(*, test_list, src_dir, build_dir, tmpdir, jobs=1, enable_coverage=
588     if len(test_list) > 1 and jobs > 1:
589         # Populate cache
590         try:
591-            subprocess.check_output([sys.executable, tests_dir + 'create_cache.py'] + flags + ["--tmpdir=%s/cache" % tmpdir])
592+            subprocess.check_output([sys.executable, tests_dir + 'create_cache.py'] + flags + [f"--tmpdir={tmpdir}/cache"] + ["--descriptors"])
    maflcko commented at 12:16 pm on February 7, 2024: 
    0            subprocess.check_output([sys.executable, tests_dir + 'create_cache.py'] + flags + [f"--tmpdir={tmpdir}/cache", "--descriptors"])
    BrandonOdiwuor commented at 12:28 pm on February 7, 2024:
    fixed
  22. maflcko approved
  23. maflcko commented at 12:16 pm on February 7, 2024: member
    Thanks. lgtm
  24. BrandonOdiwuor force-pushed on Feb 7, 2024
  25. BrandonOdiwuor requested review from maflcko on Feb 7, 2024
  26. jo-elimu changes_requested
  27. jo-elimu commented at 1:29 pm on February 8, 2024: none

    @BrandonOdiwuor There is a CI error:

    0Uncovered RPC commands:
1  - abortrescan
  28. BrandonOdiwuor commented at 1:43 pm on February 8, 2024: contributor
    @jo-elimu that’s the error the fix was to catch. Check the issue
  29. maflcko commented at 1:52 pm on February 8, 2024: member
    In any case, a pull request with a failing CI can not be merged. Someone will need to add coverage for the RPC, here, or in a different pull.
  30. epiccurious commented at 2:53 pm on February 9, 2024: none
    Concept ACK 7dc0442ff5013827c051c8ff50c8fd8aa9440d7c.
  31. BrandonOdiwuor force-pushed on Feb 10, 2024
  32. BrandonOdiwuor force-pushed on Feb 10, 2024
  33. BrandonOdiwuor commented at 8:43 am on February 10, 2024: contributor

    @maflcko I have added a basic test for the abortrescan RPC and TODO for adding a test when rescanblockchain is running

    I cannot add the second test since I’m unable to get abortrescan to execute when a rescan is ongoing Screenshot from 2024-02-10 11-39-11

  34. BrandonOdiwuor force-pushed on Feb 10, 2024
  35. BrandonOdiwuor force-pushed on Feb 10, 2024
  36. DrahtBot removed the label CI failed on Feb 10, 2024
  37. in test/functional/wallet_abortrescan.py:29 in 25042f2307 outdated 
    24+        node_0.createwallet(wallet_name="w_0")
25+        w_0 = self.nodes[0].get_wallet_rpc("w_0")
26+
27+        self.log.info('Testing abortrescan when no rescan is in progress')
28+        assert_equal(w_0.getwalletinfo()['scanning'], False)
29+        assert_equal(w_0.abortrescan(), False)
    maflcko commented at 10:19 am on February 13, 2024:
    Not sure if a new test file with all the overhead (starting a node) is needed to call a single RPC. Seems easier to just call the RPC in an existing test
  38. BrandonOdiwuor force-pushed on Feb 14, 2024
  39. in test/functional/wallet_transactiontime_rescan.py:147 in e49765c891 outdated 
    143@@ -144,6 +144,11 @@ def run_test(self):
144         restorewo_wallet.importaddress(wo2, rescan=False)
145         restorewo_wallet.importaddress(wo3, rescan=False)
146 
147+        # Testing abortrescan() when no rescanblockchain in progress
148+        self.log.info('Testing abortrescan when no rescan is in progress')
    maflcko commented at 12:15 pm on February 14, 2024: 
    0
1        self.log.info('Testing abortrescan when no rescan is in progress')

    No need to type the same thing twice

    BrandonOdiwuor commented at 12:18 pm on February 14, 2024:
    fixed
  40. BrandonOdiwuor force-pushed on Feb 14, 2024
  41. BrandonOdiwuor requested review from maflcko on Feb 14, 2024
  42. maflcko commented at 12:44 pm on February 14, 2024: member
    lgtm ACK fe5919d7cec62c31367be456a0dc5dcf094efbe2
  43. DrahtBot removed review request from maflcko on Feb 14, 2024
  44. DrahtBot removed review request from epiccurious on Feb 14, 2024
  45. DrahtBot requested review from epiccurious on Feb 14, 2024
  46. fanquake commented at 1:54 pm on February 14, 2024: member

    This still seems broken. i.e Running --legacy-wallet tests directly will no-longer work:

     0test/functional/test_runner.py wallet_createwallet.py --legacy-wallet
 1Temporary test directory at /tmp/test_runner_₿_🏃_20240214_135022
 2Running Unit Tests for Test Framework Modules
 3.....................
 4----------------------------------------------------------------------
 5Ran 21 tests in 25.689s
 6
 7OK
 8usage: create_cache.py [options]
 9create_cache.py: error: argument --descriptors: not allowed with argument --legacy-wallet
10Traceback (most recent call last):
11  File "/root/ci_scratch/test/functional/test_runner.py", line 902, in <module>
12    main()
13  File "/root/ci_scratch/test/functional/test_runner.py", line 536, in main
14    run_tests(
15  File "/root/ci_scratch/test/functional/test_runner.py", line 593, in run_tests
16    subprocess.check_output([sys.executable, tests_dir + 'create_cache.py'] + flags + [f"--tmpdir={tmpdir}/cache", "--descriptors"])
17  File "/usr/lib64/python3.12/subprocess.py", line 466, in check_output
18    return run(*popenargs, stdout=PIPE, timeout=timeout, check=True,
19           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
20  File "/usr/lib64/python3.12/subprocess.py", line 571, in run
21    raise CalledProcessError(retcode, process.args,
22subprocess.CalledProcessError: Command '['/usr/bin/python3', '/root/ci_scratch/test/functional/create_cache.py', '--cachedir=/root/ci_scratch/test/cache', '--legacy-wallet', '--configfile=/root/ci_scratch/test/functional/../config.ini', '--tmpdir=/tmp/test_runner_₿_🏃_20240214_135022/cache', '--descriptors']' returned non-zero exit status 2.
  47. DrahtBot removed review request from epiccurious on Feb 14, 2024
  48. DrahtBot requested review from epiccurious on Feb 14, 2024
  49. BrandonOdiwuor force-pushed on Feb 21, 2024
  50. BrandonOdiwuor commented at 2:42 pm on February 21, 2024: contributor
    @fanquake fixed by disabling --descriptors flag when running create_cache.py if --legacy-wallet flag is provided
  51. BrandonOdiwuor requested review from maflcko on Feb 21, 2024
  52. maflcko commented at 3:14 pm on February 21, 2024: member
    Can you test what happens when the wallet is completely disabled in configure?
  53. BrandonOdiwuor commented at 6:33 am on February 22, 2024: contributor

    @maflcko When wallet is disabled these RPS are reported as uncovered;

    Screenshot from 2024-02-22 09-26-27

  54. maflcko commented at 9:51 am on February 22, 2024: member
    Why would they? If the RPCs are not compiled into the binary, how would the test know they exist?
  55. BrandonOdiwuor commented at 9:04 am on February 23, 2024: contributor

    @maflcko I was responding to this request

    Can you test what happens when the wallet is completely disabled in configure? #29387 (comment)

  56. DrahtBot added the label Needs rebase on Feb 26, 2024
  57. BrandonOdiwuor force-pushed on Feb 26, 2024
  58. DrahtBot removed the label Needs rebase on Feb 26, 2024
  59. DrahtBot added the label CI failed on Mar 6, 2024
  60. DrahtBot removed the label CI failed on Mar 12, 2024
  61. hernanmarino commented at 11:01 pm on April 12, 2024: contributor
    Concept ACK , but still a little concerned about @maflcko last comment ( #29387 (comment) )
  62. maflcko commented at 8:55 am on April 18, 2024: member
    I just tried this myself with --disable-wallet, then call help, and the wallet RPCs were not printed. write_all_rpc_commands also uses help(), so I wonder how you achieved this outcome.
  63. BrandonOdiwuor commented at 1:34 pm on April 19, 2024: contributor

    @maflcko @hernanmarino sorry I misunderstood #29387 (comment) earlier

    When the wallet is disabled in the configure, the wallet RPCs are not included in the rpc_interface.txt list, hence not part of the coverage check

    (example below from my test) rpc_interface.txt

  64. BrandonOdiwuor requested review from hernanmarino on Apr 22, 2024
  65. DrahtBot added the label Needs rebase on Apr 26, 2024
  66. test: add abortrescan RPC test 415f2b4975
  67. test: fix RPC coverage check dcd28c8d26
  68. BrandonOdiwuor force-pushed on Apr 29, 2024
  69. DrahtBot removed the label Needs rebase on Apr 29, 2024
  70. DrahtBot added the label Needs rebase on May 13, 2024
  71. DrahtBot commented at 9:22 am on May 13, 2024: contributor

    🐙 This pull request conflicts with the target branch and needs rebase.


BrandonOdiwuor DrahtBot jo-elimu maflcko epiccurious fanquake hernanmarino


maflcko hernanmarino epiccurious

Labels
Tests Needs rebase

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: 2024-06-29 07:13 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me