qa: Require `--exclude` for each excluded test #34168

hebasto · 2025-12-29T11:02:05Z

This PR allows a long --exclude ... argument in the test/functional/test_runner.py invocation to be split across multiple lines, with optional per-line explanatory comments. I found this useful for the CI scripts in https://github.com/hebasto/bitcoin-core-nightly.

DrahtBot · 2025-12-29T11:02:11Z

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/34168.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	l0rinc, rkrux, maflcko, achow101
Stale ACK	bensig

If your review is incorrectly listed, please copy-paste  into the comment that the bot should ignore.

Conflicts

No conflicts as of last run.

rkrux

Concept ACK c9698f6

Personally I don't have much use for it but I guess others can find this useful.

rkrux · 2025-12-29T14:12:38Z

test/functional/test_runner.py

                test_list.remove(exclude_item)

-        exclude_tests = [test.strip() for test in args.exclude.split(",")]
+        exclude_tests = [test.strip() for test in ",".join(args.exclude).split(",")]


A set can be used here instead of a list to avoid unnecessary warning in case a test name is duplicated across different occurrences of the exclude argument.

diff --git a/test/functional/test_runner.py b/test/functional/test_runner.py index 58b331ee20..6f6dc442f9 100755 --- a/test/functional/test_runner.py +++ b/test/functional/test_runner.py @@ -503,7 +503,7 @@ def main(): for exclude_item in exclude_list: test_list.remove(exclude_item) - exclude_tests = [test.strip() for test in ",".join(args.exclude).split(",")] + exclude_tests = set([test.strip() for test in ",".join(args.exclude).split(",")]) for exclude_test in exclude_tests: # A space in the name indicates it has arguments such as "rpc_bind.py --ipv4" if ' ' in exclude_test: (END)

➜ bitcoin git:(251229-ft-exclude-append) ✗ ./build/test/functional/test_runner.py --exclude=rpc_signer,rpc_setban --exclude=rpc_users,rpc_net,rpc_signer Temporary test directory at /var/folders/6v/mpspb4bx4zzf3xr2z96hgq9h0000gn/T/test_runner_₿_🏃_20251229_193516 exclude_tests: ['rpc_signer', 'rpc_setban', 'rpc_users', 'rpc_net', 'rpc_signer'] WARNING! Test 'rpc_signer' not found in current test list. Check the --exclude list.

👍, and we could even avoid the temporary string concat hack to flatten the list, a nested comprehension would be more direct:

Suggested change

exclude_tests = [test.strip() for test in ",".join(args.exclude).split(",")]

exclude_tests = {test.strip() for tests in args.exclude for test in tests.split(",")}

The current patch seems fine, but I wonder if this could be made a breaking change to drop the comma-separated feature and require --exclude for each excluded test. But just an idea and the current patch seems perfectly fine.

I haven't tried it, but wouldn't we get a failure for duplicates?

Getting a failure for duplicates seems fine and maybe even desirable, no?

The risk in removing the failure may be that someone removes an --exclude for a test, assuming it will then run in the future, but it is silently not run, because a duplicate is still present in another --exclude.

So to me it seems beneficial to sanitize the user input.

removes an --exclude for a test, assuming it will then run in the future, but it is silently not run,

Hmmm, that's a good point actually...

(edit: moved rest of the comment to main thread to be registered)

bensig · 2026-01-01T07:11:20Z

ACK c9698f6

Verified the argument parsing logic works as expected - multiple --exclude flags get joined and split correctly.

l0rinc · 2026-01-01T11:53:20Z

test/functional/test_runner.py

                test_list.remove(exclude_item)

-        exclude_tests = [test.strip() for test in args.exclude.split(",")]
+        exclude_tests = [test.strip() for test in ",".join(args.exclude).split(",")]


👍, and we could even avoid the temporary string concat hack to flatten the list, a nested comprehension would be more direct:

Suggested change

exclude_tests = [test.strip() for test in ",".join(args.exclude).split(",")]

exclude_tests = {test.strip() for tests in args.exclude for test in tests.split(",")}

maflcko · 2026-01-01T16:30:44Z

lgtm ACK c9698f6

l0rinc · 2026-01-01T20:00:16Z

I wonder if this could be made a breaking change to drop the comma-separated feature

In this context I would vote for that, let's have a single way to specify these to avoid confusion: approach NACK

It seems simpler to migrate to separate ones completely instead of having this mixed strategy, something like:

diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
index 5e5b0c48e8..e287841e6e 100644
--- a/.github/workflows/ci.yml
+++ b/.github/workflows/ci.yml
@@ -503,7 +503,7 @@ jobs:
         env:
           # TODO: Fix the excluded test and re-enable it.
           # feature_unsupported_utxo_db.py fails on windows because of emojis in the test data directory
-          EXCLUDE: '--exclude wallet_multiwallet.py,feature_unsupported_utxo_db.py'
+          EXCLUDE: '--exclude wallet_multiwallet.py --exclude feature_unsupported_utxo_db.py'
           TEST_RUNNER_EXTRA: ${{ github.event_name != 'pull_request' && '--extended' || '' }}
         run: py -3 test/functional/test_runner.py --jobs $NUMBER_OF_PROCESSORS --ci --quiet --tmpdirprefix="$RUNNER_TEMP" --combinedlogslen=99999999 --timeout-factor=$TEST_RUNNER_TIMEOUT_FACTOR $EXCLUDE $TEST_RUNNER_EXTRA
 
diff --git a/ci/test/00_setup_env_native_valgrind.sh b/ci/test/00_setup_env_native_valgrind.sh
index 884bc3955c..3e23a580da 100755
--- a/ci/test/00_setup_env_native_valgrind.sh
+++ b/ci/test/00_setup_env_native_valgrind.sh
@@ -13,7 +13,7 @@ export PIP_PACKAGES="--break-system-packages pycapnp"
 export USE_VALGRIND=1
 export NO_DEPENDS=1
 # bind tests excluded for now, see https://github.com/bitcoin/bitcoin/issues/17765#issuecomment-602068547
-export TEST_RUNNER_EXTRA="--exclude rpc_bind,feature_bind_extra"
+export TEST_RUNNER_EXTRA="--exclude rpc_bind --exclude feature_bind_extra"
 export GOAL="install"
 # TODO enable GUI
 export BITCOIN_CONFIG="\
diff --git a/ci/test/00_setup_env_s390x.sh b/ci/test/00_setup_env_s390x.sh
index b7d23fbf5e..b6f924cf5d 100755
--- a/ci/test/00_setup_env_s390x.sh
+++ b/ci/test/00_setup_env_s390x.sh
@@ -11,7 +11,7 @@ export PACKAGES="python3-zmq"
 export CONTAINER_NAME=ci_s390x
 export CI_IMAGE_NAME_TAG="mirror.gcr.io/ubuntu:24.04"
 export CI_IMAGE_PLATFORM="linux/s390x"
-export TEST_RUNNER_EXTRA="--exclude rpc_bind,feature_bind_extra"  # Excluded for now, see https://github.com/bitcoin/bitcoin/issues/17765#issuecomment-602068547
+export TEST_RUNNER_EXTRA="--exclude rpc_bind --exclude feature_bind_extra"  # Excluded for now, see https://github.com/bitcoin/bitcoin/issues/17765#issuecomment-602068547
 export RUN_FUNCTIONAL_TESTS=true
 export GOAL="install"
 export BITCOIN_CONFIG="\
diff --git a/test/functional/test_runner.py b/test/functional/test_runner.py
index 58b331ee20..dae672e7da 100755
--- a/test/functional/test_runner.py
+++ b/test/functional/test_runner.py
@@ -400,7 +400,7 @@ def main():
     parser.add_argument('--combinedlogslen', '-c', type=int, default=0, metavar='n', help='On failure, print a log (of length n lines) to the console, combined from the test framework and all test nodes.')
     parser.add_argument('--coverage', action='store_true', help='generate a basic coverage report for the RPC interface')
     parser.add_argument('--ci', action='store_true', help='Run checks and code that are usually only enabled in a continuous integration environment')
-    parser.add_argument('--exclude', '-x', action='append', help='specify a comma-separated-list of scripts to exclude.')
+    parser.add_argument('--exclude', '-x', action='append', help='specify a script to exclude (can be specified multiple times).')
     parser.add_argument('--extended', action='store_true', help='run the extended test suite in addition to the basic tests')
     parser.add_argument('--help', '-h', '-?', action='store_true', help='print help text and exit')
     parser.add_argument('--jobs', '-j', type=int, default=4, help='how many test scripts to run in parallel. Default=4.')
@@ -503,8 +503,18 @@ def main():
             for exclude_item in exclude_list:
                 test_list.remove(exclude_item)
 
-        exclude_tests = [test.strip() for test in ",".join(args.exclude).split(",")]
-        for exclude_test in exclude_tests:
+        for exclude_test in args.exclude:
+            if ',' in exclude_test:
+                print("{}WARNING!{} --exclude '{}' contains a comma. Use --exclude for each test.".format(BOLD[1], BOLD[0], exclude_test))
+                if fail_on_warn:
+                    sys.exit(1)
+
+        if len(args.exclude) != len(set(args.exclude)):
+            print("{}WARNING!{} Duplicate --exclude detected.".format(BOLD[1], BOLD[0]))
+            if fail_on_warn:
+                sys.exit(1)
+
+        for exclude_test in args.exclude:
             # A space in the name indicates it has arguments such as "rpc_bind.py --ipv4"
             if ' ' in exclude_test:
                 remove_tests([test for test in test_list if test.replace('.py', '') == exclude_test.replace('.py', '')])

Q: should we log the name of each excluded test to be safe?

diff --git a/test/functional/test_runner.py b/test/functional/test_runner.py
index 58b331ee20..fc204b858b 100755
--- a/test/functional/test_runner.py
+++ b/test/functional/test_runner.py
@@ -501,6 +501,7 @@ def main():
             if not exclude_list:
                 print_warning_missing_test(exclude_test)
             for exclude_item in exclude_list:
+                print("Excluding %s" % exclude_item)
                 test_list.remove(exclude_item)
 
         exclude_tests = [test.strip() for test in ",".join(args.exclude).split(",")]

Co-authored-by: Lőrinc <pap.lorinc@gmail.com>

hebasto · 2026-01-02T13:26:41Z

Reworked per feedback. The updated branch is mostly based on @l0rinc's suggestion.

l0rinc

Concept ACK

.github/workflows/ci.yml

l0rinc · 2026-01-02T22:26:30Z

Checked that all of my comments were applied correctly (even better than what I suggested), except for failure for duplicates, but that just behaves the same way as if we provided an invalid name for an excursion - a warning will appear:

cmake -B build -DCMAKE_BUILD_TYPE=Debug && cmake --build build -j$(nproc) && \
./build/test/functional/test_runner.py feature_includeconf.py feature_loadblock.py --exclude=feature_includeconf.py --exclude=feature_includeconf.py

Excluding feature_includeconf.py
WARNING! Test 'feature_includeconf.py' not found in current test list. Check the --exclude options.
Remaining jobs: [feature_loadblock.py]
1/1 - feature_loadblock.py passed, Duration: 1 s

May not be the best UX, but I actually prefer this over more complicated code.

tested ACK c5825d4

rkrux

ACK c5825d4

I preferred the comma separated list but the current way is fine as well if found beneficial by others.

maflcko · 2026-01-09T10:55:20Z

review ACK c5825d4 🛄

Show signature

Signature:

untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: review ACK c5825d4b7fe9ae202ea3c74798f58cd3a920821d 🛄
UQgbpGUCLuV09g7FGt+KgXbVRTQNpuB5S5ApWon8JehSgYzE0/oApwfqKUgbyosUEuHG+etgFuSVmO2Jt54nCA==

maflcko · 2026-01-09T11:28:12Z

rfm?

achow101 · 2026-01-13T22:37:08Z

ACK c5825d4

hebasto added the Tests label Dec 29, 2025

rkrux reviewed Dec 29, 2025

View reviewed changes

DrahtBot requested a review from rkrux January 1, 2026 07:11

l0rinc suggested changes Jan 1, 2026

View reviewed changes

qa: Require --exclude for each excluded test

c5825d4

Co-authored-by: Lőrinc <pap.lorinc@gmail.com>

hebasto force-pushed the 251229-ft-exclude-append branch from c9698f6 to c5825d4 Compare January 2, 2026 13:25

hebasto changed the title ~~qa: Allow --exclude to be specified multiple times~~ qa: Require --exclude for each excluded test Jan 2, 2026

l0rinc approved these changes Jan 2, 2026

View reviewed changes

.github/workflows/ci.yml Show resolved Hide resolved

DrahtBot requested a review from maflcko January 2, 2026 22:26

DrahtBot mentioned this pull request Jan 3, 2026

qa: Fix wallet_multiwallet.py #31410

Closed

rkrux approved these changes Jan 7, 2026

View reviewed changes

DrahtBot mentioned this pull request Jan 8, 2026

test: fix Windows CI failures in wallet_multiwallet and old binary tests (ancient wallets) #34233

Closed

maflcko mentioned this pull request Jan 9, 2026

ci: Run feature_unsupported_utxo_db.py on Windows #34240

Merged

achow101 merged commit c352d3c into bitcoin:master Jan 13, 2026
27 checks passed

hebasto deleted the 251229-ft-exclude-append branch January 13, 2026 22:42

This was referenced Jan 13, 2026

OpenBSD: Inline --exclude option in functional tests hebasto/bitcoin-core-nightly#154

Merged

OpenIndiana: Inline --exclude option in functional tests hebasto/bitcoin-core-nightly#156

Merged

	exclude_tests = [test.strip() for test in ",".join(args.exclude).split(",")]
	exclude_tests = {test.strip() for tests in args.exclude for test in tests.split(",")}

qa: Require --exclude for each excluded test #34168

qa: Require --exclude for each excluded test #34168

Uh oh!

Conversation

hebasto commented Dec 29, 2025

Uh oh!

DrahtBot commented Dec 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage & Benchmarks

Reviews

Conflicts

Uh oh!

rkrux left a comment

Choose a reason for hiding this comment

Uh oh!

rkrux Dec 29, 2025

Choose a reason for hiding this comment

Uh oh!

l0rinc Jan 1, 2026

Choose a reason for hiding this comment

Uh oh!

maflcko Jan 1, 2026

Choose a reason for hiding this comment

Uh oh!

l0rinc Jan 1, 2026

Choose a reason for hiding this comment

Uh oh!

maflcko Jan 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

l0rinc Jan 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bensig commented Jan 1, 2026

Uh oh!

l0rinc Jan 1, 2026

Choose a reason for hiding this comment

Uh oh!

maflcko commented Jan 1, 2026

Uh oh!

l0rinc commented Jan 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hebasto commented Jan 2, 2026

Uh oh!

l0rinc left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

l0rinc commented Jan 2, 2026

Uh oh!

rkrux left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko commented Jan 9, 2026

Uh oh!

maflcko commented Jan 9, 2026

Uh oh!

achow101 commented Jan 13, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

qa: Require `--exclude` for each excluded test #34168

qa: Require `--exclude` for each excluded test #34168

DrahtBot commented Dec 29, 2025 •

edited

Loading

maflcko Jan 1, 2026 •

edited

Loading

l0rinc Jan 1, 2026 •

edited

Loading

l0rinc commented Jan 1, 2026 •

edited

Loading