test: Turn util/test_runner into functional test

maflcko commented at 8:55 am on June 7, 2025: member

The test/util/test_runner.py has many issues:

The boilerplate for the test runner is duplicate or inconsistent with the other (functional) tests. For example, logging options, ConfigParser handling, Binaries handling …
The cmake/ci behavior is brittle and can silently fail, as explained in #31476
corecheck (and likely other places that manually run the tests) completely forget to run it
If the test is manually called, it runs single threaded, when it could just run in parallel with the other functional tests

Fix all issues by removing the util test_runner and moving the test logic into a new functional test file.

DrahtBot commented at 8:55 am on June 7, 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/32697.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	janb84
Concept ACK	Prabhat1308, hebasto, BrandonOdiwuor, brunoerg

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:

#32324 (test: Treat executable paths in tests consistently by hebasto)
#32297 (bitcoin-cli: Add -ipcconnect option by ryanofsky)
#31669 (cmake: Introduce WITH_PYTHON build option by hebasto)
#30437 (ipc: add bitcoin-mine test program 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.

DrahtBot renamed this:
~~test: Turn util/test_runner into functional test~~
test: Turn util/test_runner into functional test
on Jun 7, 2025

DrahtBot added the label Tests on Jun 7, 2025

maflcko force-pushed on Jun 7, 2025

DrahtBot added the label CI failed on Jun 7, 2025

DrahtBot commented at 9:03 am on June 7, 2025: contributor

🚧 At least one of the CI tasks failed. Task lint: https://github.com/bitcoin/bitcoin/runs/43661503873 LLM reason (✨ experimental): Python linting failed due to an unused import error identified by ruff.

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

maflcko force-pushed on Jun 7, 2025

maflcko commented at 9:57 am on June 7, 2025: member

force pushed to fix a python type related error after an error. Stuff like this wouldn’t happen in a type-safe language.

0                                       logging.error("OSError, Failed to execute " + execprog)
1                                                     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~
2                                   TypeError: can only concatenate str (not "list") to str

DrahtBot removed the label CI failed on Jun 7, 2025

maflcko force-pushed on Jun 7, 2025

Prabhat1308 commented at 10:07 am on June 8, 2025: contributor

fae1c12 tACK

hebasto commented at 8:05 am on June 9, 2025: member

Concept ACK.

BrandonOdiwuor commented at 6:31 pm on June 9, 2025: contributor

Concept ACK

brunoerg commented at 4:26 pm on June 11, 2025: contributor

Concept ACK

janb84 commented at 7:46 am on June 13, 2025: contributor

With the move of the test to the functional folder, shouldn’t the data that the test references also move to the functional/data folder? eg. from util/data -> functional/data/util/ ?

test: Remove duplicate ConfigParser

It is sufficient to parse once.

fac49094cd

test: Use lowercase env var as attribute name

This is a refactor.

fa91835ec6

test: Add missing tx util to Binaries fac9db6eb0

maflcko commented at 7:51 am on June 13, 2025: member

eg. from util/data -> functional/data/util/ ?

thx, done

maflcko force-pushed on Jun 13, 2025

test: Add missing skip_if_no_bitcoin_tx fa955154c7

test: Turn util/test_runner into functional test

The moved portion can be reviewed via:

--color-moved=dimmed-zebra  --color-moved-ws=ignore-all-space

faa18bf287

move-only util data to test/functional/data/util fa2f1c55b7

in test/functional/tool_utils.py:24 in fa95a22c0a outdated

19+    def set_test_params(self):
20+        self.num_nodes = 0  # No node/datadir needed
21+
22+    def setup_network(self):
23+        pass
24+

janb84 commented at 9:59 am on June 13, 2025:

0
1   def skip_test_if_missing_module(self):
2      self.skip_if_no_bitcoin_util()

NIT: Test will fail when project is build without util cmake -B build -DBUILD_UTIL=OFF add code to see if the functional test needs to be skipped.

maflcko commented at 11:07 am on June 13, 2025:

thx, done.

I wonder why there even is an option to disable those.

maflcko force-pushed on Jun 13, 2025

janb84 commented at 11:20 am on June 13, 2025: contributor

ACK fa2f1c55b7da729eb6bb9f88bf48459235cc2664

This PR moves the util tests to the functional test folder, a logical step (imo) to unify all the functional tests in one folder and with common boilerplate / common test code.

Reson to ACK :

Code review looks good ✅
Code compiles & tests succeed ✅
Tests will skip if the utils are not compiled ✅
Could not find any (historical) reason to keep the test in a separate folder, would even say that the unification was started in PR #10331 ✅

DrahtBot requested review from Prabhat1308 on Jun 13, 2025

DrahtBot requested review from BrandonOdiwuor on Jun 13, 2025

DrahtBot requested review from brunoerg on Jun 13, 2025

DrahtBot requested review from hebasto on Jun 13, 2025

in test/util/test_runner.py:8 in faa18bf287 outdated

-1@@ -1,181 +0,0 @@
0-#!/usr/bin/env python3
1-# Copyright 2014 BitPay Inc.
2-# Copyright 2016-present The Bitcoin Core developers
3-# Distributed under the MIT software license, see the accompanying
4-# file COPYING or http://www.opensource.org/licenses/mit-license.php.
5-"""Test framework for bitcoin utils.
6-
7-Runs automatically during `ctest --test-dir build/`.

brunoerg commented at 8:46 pm on June 13, 2025:

By turning it into a functional test, it won’t run anymore during ctest --test-dir build/, right?

 0Internal ctest changing into directory: /Users/brunogarcia/projects/bitcoin-core-dev/build
 1Test project /Users/brunogarcia/projects/bitcoin-core-dev/build
 2        Start   1: util_test_runner
 3  1/141 Test   [#1](/bitcoin-bitcoin/1/): util_test_runner .....................***Failed    0.13 sec
 4        Start   2: util_rpcauth_test
 5  2/141 Test   [#2](/bitcoin-bitcoin/2/): util_rpcauth_test ....................   Passed    0.05 sec
 6        Start   3: univalue_test
 7  3/141 Test   [#3](/bitcoin-bitcoin/3/): univalue_test ........................   Passed    0.01 sec
 8        Start   4: univalue_object_test
 9  4/141 Test   [#4](/bitcoin-bitcoin/4/): univalue_object_test .................   Passed    0.01 sec
10        Start   5: secp256k1_noverify_tests
11  5/141 Test   [#5](/bitcoin-bitcoin/5/): secp256k1_noverify_tests .............   Passed   20.32 sec
12        Start   6: secp256k1_tests
13...

DrahtBot requested review from brunoerg on Jun 13, 2025

test: Turn util/test_runner into functional test #32697

Code Coverage & Benchmarks

Reviews

Conflicts