test: add coverage for bitcoin wrapper argument handling #36022

cyb3ralbert commented at 1:12 PM on August 19, 2026: contributor

Current tests only check which binary is invoked via exec; the wrapper's own argument parsing is not tested. Five branches return without calling exec, and none of them are covered by tests.

src/bitcoin.cpp is absent from the coverage report when running the existing test suite. With these tests, it reaches 47.1% coverage (48 of 102 lines). exec replaces the current process before the coverage counters are flushed to disk, so those execution paths do not appear in the coverage report. Running without a command returns a non-zero exit code, but this behavior is not covered by any test.

test: add coverage for bitcoin wrapper argument handling

The bitcoin wrapper's own argument handling is not exercised by any test.
tool_bitcoin.py only checks which binary gets exec'd, so the paths that
return without exec - version, help, no command, unknown command and
unknown option - are never run.

Add checks for those five cases.

aed6402ac0

DrahtBot added the label Tests on Aug 19, 2026

DrahtBot commented at 1:12 PM on August 19, 2026: 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/36022.

Reviews

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#36190 (util: preserve Windows child exit status -1 by Bortlesboat)
#36106 (bitcoin wrapper: respect CMAKE_INSTALL_BINDIR/LIBEXECDIR by ryanofsky)

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.

test: run bitcoin wrapper under valgrind_cmd in run_wrapper

run_wrapper invoked the bitcoin binary directly, bypassing the
node.binaries.valgrind_cmd prefix that set_cmd_args already applies.
As a result test_version, test_help, test_no_command and
test_unrecognized never ran under valgrind despite --valgrind being
passed to the test runner.

Prefix the subprocess call with valgrind_cmd, matching the other
tests in this file.

Suggested-by: mercie-ux

b244610a67

in test/functional/tool_bitcoin.py:66 in aed6402ac0

  60 | @@ -60,9 +61,58 @@ def test_args(self, cmd_args, node_args, expect_exe=None, expect_error=None):
  61 |              except Exception as e:
  62 |                  raise RuntimeError(f"Unexpected output from {node.args + extra_args}: {out=!r} {err=!r} {ret=!r}") from e
  63 |  
  64 | +    def run_wrapper(self, *args):
  65 | +        """Run the bitcoin wrapper directly and return (returncode, stdout, stderr)."""
  66 | +        result = subprocess.run([self.nodes[0].binaries.paths.bitcoin_bin, *args],

mercie-ux commented at 10:48 AM on August 20, 2026:

run_wrapper calls bitcoin_bin directly, bypassing node.binaries.valgrind_cmd that set_cmd_args already uses. So test_version/test_help/test_no_command/test_unrecognized never run under valgrind, even when using the --valgrind passed to the test runner. suggested fix for this:

result = subprocess.run([*self.nodes[0].binaries.valgrind_cmd, self.nodes[0].binaries.paths.bitcoin_bin, *args],
                        capture_output=True, text=True)

I verified locally unpatched, these four tests finish in ~13ms total under --valgrind. with this change, they take ~2.5-7s each, in line with the other tests in this file that already go through valgrind_cmd.

cyb3ralbert commented at 7:45 AM on August 24, 2026:

Good catch, thanks — pushed a fix using valgrind_cmd, matching the rest of the file.

For context, native_valgrind doesn't run in PR CI (maintainers trigger it separately). When it does run, it uses binary substitution (wrap-valgrind.py) rather than the framework's valgrind_cmd, so a direct subprocess.run([bin_path, ...]) there already runs under Valgrind. This gap therefore only affected the local test_runner.py --valgrind path.

Reran locally to confirm: unpatched, the four tests take ~28ms combined; with the fix, test_version/test_help take ~1.5–1.9s each and test_no_command/test_unrecognized ~0.7–1.3s each — in line with what you reported and with the other tests in this file that already go through valgrind_cmd.