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
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.
DrahtBot added the label
Tests
on Feb 5, 2024
in
test/functional/test_framework/test_framework.py:832
in
156c1a25fcoutdated
BrandonOdiwuor
commented at 3:43 pm on February 6, 2024:
@maflckoextra_args parameter cannot be removed since it’s a named parameter, unless the TestNode.__init__(...) signature is changed
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
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
BrandonOdiwuor requested review from maflcko
on Feb 6, 2024
BrandonOdiwuor requested review from epiccurious
on Feb 6, 2024
BrandonOdiwuor marked this as ready for review
on Feb 6, 2024
in
test/functional/test_framework/test_framework.py:813
in
a4498d3addoutdated
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))
809810+ # 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(...)
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
DrahtBot added the label
CI failed
on Feb 6, 2024
BrandonOdiwuor force-pushed
on Feb 7, 2024
BrandonOdiwuor requested review from maflcko
on Feb 7, 2024
in
test/functional/test_framework/test_framework.py:811
in
1806de015eoutdated
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))
809810+ # 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
in
test/functional/test_runner.py:591
in
1806de015eoutdated
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
BrandonOdiwuor force-pushed
on Feb 10, 2024
BrandonOdiwuor force-pushed
on Feb 10, 2024
DrahtBot removed the label
CI failed
on Feb 10, 2024
in
test/functional/wallet_abortrescan.py:29
in
25042f2307outdated
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
BrandonOdiwuor force-pushed
on Feb 14, 2024
in
test/functional/wallet_transactiontime_rescan.py:147
in
e49765c891outdated
143@@ -144,6 +144,11 @@ def run_test(self):
144 restorewo_wallet.importaddress(wo2, rescan=False)
145 restorewo_wallet.importaddress(wo3, rescan=False)
146147+ # 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:
01 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
BrandonOdiwuor force-pushed
on Feb 14, 2024
BrandonOdiwuor requested review from maflcko
on Feb 14, 2024
maflcko
commented at 12:44 pm on February 14, 2024:
member
lgtm ACKfe5919d7cec62c31367be456a0dc5dcf094efbe2
DrahtBot removed review request from maflcko
on Feb 14, 2024
DrahtBot removed review request from epiccurious
on Feb 14, 2024
DrahtBot requested review from epiccurious
on Feb 14, 2024
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
18return 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.
DrahtBot removed review request from epiccurious
on Feb 14, 2024
DrahtBot requested review from epiccurious
on Feb 14, 2024
BrandonOdiwuor force-pushed
on Feb 21, 2024
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
BrandonOdiwuor requested review from maflcko
on Feb 21, 2024
maflcko
commented at 3:14 pm on February 21, 2024:
member
Can you test what happens when the wallet is completely disabled in configure?
BrandonOdiwuor
commented at 6:33 am on February 22, 2024:
contributor
@maflcko When wallet is disabled these RPS are reported as uncovered;
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?
BrandonOdiwuor
commented at 9:04 am on February 23, 2024:
contributor
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.
BrandonOdiwuor
commented at 1:34 pm on April 19, 2024:
contributor
BrandonOdiwuor requested review from hernanmarino
on Apr 22, 2024
DrahtBot added the label
Needs rebase
on Apr 26, 2024
test: add abortrescan RPC test415f2b4975
test: fix RPC coverage checkdcd28c8d26
BrandonOdiwuor force-pushed
on Apr 29, 2024
DrahtBot removed the label
Needs rebase
on Apr 29, 2024
DrahtBot added the label
Needs rebase
on May 13, 2024
DrahtBot
commented at 9:22 am on May 13, 2024:
contributor
🐙 This pull request conflicts with the target branch and needs rebase.
DrahtBot
commented at 0:27 am on August 10, 2024:
contributor
⌛ There hasn’t been much activity lately and the patch still needs rebase. What is the status here?
Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
Is it no longer relevant? ➡️ Please close.
Did the author lose interest or time to work on this? ➡️ Please close it and mark it ‘Up for grabs’ with the label, so that it can be picked up in the future.
maflcko added the label
Up for grabs
on Sep 4, 2024
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-11-24 09:12 UTC
This site is hosted by @0xB10C More mirrored repositories can be found on mirror.b10c.me
