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, brunoerg, hebasto
Concept ACK	Prabhat1308, BrandonOdiwuor

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:

#32773 (cmake: Create subdirectories in build tree in advance by hebasto)
#32297 (bitcoin-cli: Add -ipcconnect option by ryanofsky)
#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...

janb84 commented at 8:35 am on June 15, 2025:

Weird it runs fine here, running ctest --test-dir build/, (not sure why I have 139 tests and you 141 )

EDIT: Why is the test called util_test_RUNNER in your run?

 0Internal ctest changing into directory: /Users/jan/Projects/bitcoin/build
 1Test project /Users/jan/Projects/bitcoin/build
 2[...]
 3110/139 Test [#116](/bitcoin-bitcoin/116/): util_threadnames_tests ...............   Passed    0.12 sec
 4        Start 118: validation_block_tests
 5111/139 Test [#117](/bitcoin-bitcoin/117/): util_trace_tests .....................   Passed    0.12 sec
 6        Start 119: validation_chainstate_tests
 7112/139 Test [#115](/bitcoin-bitcoin/115/): util_tests ...........................   Passed    0.15 sec
 8        Start 120: validation_chainstatemanager_tests
 9113/139 Test [#112](/bitcoin-bitcoin/112/): txvalidationcache_tests ..............   Passed    0.54 sec
10        Start 121: validation_flush_tests
11114/139 Test [#119](/bitcoin-bitcoin/119/): validation_chainstate_tests ..........   Passed    0.26 sec
12[...]

Prabhat1308 commented at 9:48 am on June 15, 2025:

https://github.com/bitcoin/bitcoin/blob/2def85847318513afa7765e042567d13f616c54c/cmake/tests.cmake#L6 This unit test is in master but was removed in this PR.

maflcko commented at 6:25 am on June 16, 2025:

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

Yes, this is the goal of this pull request to fix the mentioned issues.

Generally, and a bit unrelated to this pull, I think it could make sense to have a “meta” test runner that can run any or all tests (ctest-tests, fuzz-tests, functional-tests, …), or at least the ctest-tests and functional tests in parallel. The reason is that (at least in CI), some ctest-tests take a long time, so there will be idle/unused CPU waiting and increasing the CI feedback loop. C.f. https://github.com/bitcoin/bitcoin/issues/30852

DrahtBot requested review from brunoerg on Jun 13, 2025

brunoerg approved

brunoerg commented at 12:28 pm on June 16, 2025: contributor

code review ACK fa2f1c55b7da729eb6bb9f88bf48459235cc2664

in test/functional/tool_utils.py:30 in fa2f1c55b7 outdated

25+    def skip_test_if_missing_module(self):
26+        self.skip_if_no_bitcoin_tx()
27+        self.skip_if_no_bitcoin_util()
28+
29+    def run_test(self):
30+        self.testcase_dir = Path(self.config["environment"]["SRCDIR"]) / "test" / "functional" / "data" / "util"

hebasto commented at 7:00 pm on June 18, 2025:

This code effectively symlinks/copies the entire test/functional subdirectory into the build tree.

Why not use it the same way we do in other functional tests? For example, grep for "os.path.realpath(__file__)".

maflcko commented at 7:39 pm on June 18, 2025:

This is just moved code that preserves the prior behavior of reading from the srcdir. Seems fine to change in a follow-up, if there is a reason. (The pycache reason doesn’t apply here, IIUC)

Also, the recursive glob could hurt dev UX, at least for those that don’t rm -rf their build dir? Even if it didn’t, requiring to call cmake when editing the test data here for no reason just seems like busy-work?

hebasto approved

hebasto commented at 7:19 pm on June 18, 2025: member

ACK fa2f1c55b7da729eb6bb9f88bf48459235cc2664.

in test/functional/tool_utils.py:12 in faa18bf287 outdated

 7+
 8+from test_framework.test_framework import BitcoinTestFramework
 9+
10+import difflib
11+import json
12+import logging

achow101 commented at 10:46 pm on June 18, 2025:

In faa18bf287fc02339b7af29d54af862a289bf96d “test: Turn util/test_runner into functional test”

It shouldn’t be necessary to import logging as each previous logging. call can be converted into a self.log.

maflcko commented at 7:05 am on June 19, 2025:

thx, added new commit to keep the existing one mostly move-only

in test/functional/tool_utils.py:72 in faa18bf287 outdated

67+            except Exception:
68+                logging.error("Output file " + outputFn + " cannot be opened")
69+                raise
70+            if not outputData:
71+                logging.error("Output data missing for " + outputFn)
72+                raise Exception

achow101 commented at 10:52 pm on June 18, 2025:

In faa18bf287fc02339b7af29d54af862a289bf96d “test: Turn util/test_runner into functional test”

This could be a good time to move the error message into the Exception rather than an error log.

maflcko commented at 7:05 am on June 19, 2025:

thx, added new commit to keep the existing one mostly move-only

test: Remove useless catch-throw

This is not done anywhere else in the tests for open or subprocess.run

fa1986181f

test: Move error string into exception

This is normally expected and also less code.

fa346f7797

test: Use self.log

This is in line with all other functional tests.

fa21631595

janb84 commented at 8:03 am on June 19, 2025: contributor

re ACK fa2163159511a099ecffde2bfddf9cfe33eb9c76

Changes sinds last ACK:

Author has added 3 commits to improve the code;
- Removed try-catches (to make the code more inline with the rest of the func. tests)
- Move error message into the Exception function.
- Remove superfluous import of logging and uses the self.log now.

The author has added new commits to emphasise / differentiate between the moved code and code improvements, which I welcome. Makes the review process easier and shows what happend to the code.

code review of new additions ✅
builds & tests run ✅

DrahtBot requested review from hebasto on Jun 19, 2025

DrahtBot requested review from brunoerg on Jun 19, 2025

brunoerg approved

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

re-ACK fa2163159511a099ecffde2bfddf9cfe33eb9c76

Verified minor changes since my last review: self.logusage, catch-throw removal and error message into Exception.

maflcko requested review from achow101 on Jun 26, 2025

maflcko removed review request from BrandonOdiwuor on Jun 30, 2025

maflcko removed review request from hebasto on Jun 30, 2025

maflcko removed review request from Prabhat1308 on Jun 30, 2025

maflcko requested review from hebasto on Jun 30, 2025

hebasto approved

hebasto commented at 11:25 am on June 30, 2025: member

re-ACK fa2163159511a099ecffde2bfddf9cfe33eb9c76, additional feedback has been addressed since my previous review.

DrahtBot requested review from Prabhat1308 on Jun 30, 2025

DrahtBot requested review from BrandonOdiwuor on Jun 30, 2025

glozow merged this on Jun 30, 2025

glozow closed this on Jun 30, 2025

maflcko deleted the branch on Jul 1, 2025

test: Turn util/test_runner into functional test #32697

Code Coverage & Benchmarks

Reviews

Conflicts