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

hebasto commented at 11:02 AM on December 29, 2025: member

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.

hebasto added the label Tests on Dec 29, 2025

DrahtBot commented at 11:02 AM on December 29, 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/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 <code></code> into the comment that the bot should ignore.

Conflicts

No conflicts as of last run.

in test/functional/test_runner.py:506 in c9698f6560

 502 | @@ -503,7 +503,7 @@ def remove_tests(exclude_list):
 503 |              for exclude_item in exclude_list:
 504 |                  test_list.remove(exclude_item)
 505 |  
 506 | -        exclude_tests = [test.strip() for test in args.exclude.split(",")]
 507 | +        exclude_tests = [test.strip() for test in ",".join(args.exclude).split(",")]

rkrux commented at 2:12 PM on December 29, 2025:

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.

l0rinc commented at 11:53 AM on January 1, 2026:

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

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

maflcko commented at 4:34 PM on January 1, 2026:

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.

l0rinc commented at 4:36 PM on January 1, 2026:

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

maflcko commented at 4:41 PM on January 1, 2026:

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.

l0rinc commented at 7:56 PM on January 1, 2026:

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)

rkrux commented at 2:13 PM on December 29, 2025: contributor

Concept ACK c9698f6560b84f110365858fc707922ac1297faf

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

bensig commented at 7:11 AM on January 1, 2026: contributor

ACK c9698f6

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

DrahtBot requested review from rkrux on Jan 1, 2026

l0rinc changes_requested

maflcko commented at 4:30 PM on January 1, 2026: member

lgtm ACK c9698f6560b84f110365858fc707922ac1297faf

l0rinc commented at 8:00 PM on January 1, 2026: contributor

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 [#17765 (comment)](/bitcoin-bitcoin/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 [#17765 (comment)](/bitcoin-bitcoin/17765/#issuecomment-602068547)
+export TEST_RUNNER_EXTRA="--exclude rpc_bind --exclude feature_bind_extra"  # Excluded for now, see [#17765 (comment)](/bitcoin-bitcoin/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(",")]

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

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

c5825d4b7f

hebasto force-pushed on Jan 2, 2026

hebasto commented at 1:26 PM on January 2, 2026: member

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

hebasto renamed this:
~~qa: Allow `--exclude` to be specified multiple times~~
qa: Require `--exclude` for each excluded test
on Jan 2, 2026

in .github/workflows/ci.yml:507 in c5825d4b7f

 506 | -          EXCLUDE: '--exclude wallet_multiwallet.py,feature_unsupported_utxo_db.py'
 507 |            TEST_RUNNER_EXTRA: ${{ github.event_name != 'pull_request' && '--extended' || '' }}
 508 | -        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
 509 | +        run: |
 510 | +          py -3 test/functional/test_runner.py --jobs $NUMBER_OF_PROCESSORS --ci --quiet --tmpdirprefix="$RUNNER_TEMP" --combinedlogslen=99999999 --timeout-factor=$TEST_RUNNER_TIMEOUT_FACTOR $TEST_RUNNER_EXTRA \
 511 | +            `# feature_unsupported_utxo_db.py fails on Windows because of emojis in the test data directory.` \

l0rinc commented at 1:32 PM on January 2, 2026:

don't we need a new line here to make sure the exclusion isn't part of the comment?

hebasto commented at 1:40 PM on January 2, 2026:

I don't think so. That comment is wrapped with the shell's backticks.

l0rinc commented at 8:59 PM on January 2, 2026:

ah, since the backticked comment lines expand to empty? Cool trick, haven't seen that before.

l0rinc approved

l0rinc commented at 1:33 PM on January 2, 2026: contributor

Concept ACK

l0rinc commented at 10:26 PM on January 2, 2026: contributor

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 c5825d4b7fe9ae202ea3c74798f58cd3a920821d

DrahtBot requested review from maflcko on Jan 2, 2026

rkrux approved

rkrux commented at 2:37 PM on January 7, 2026: contributor

ACK c5825d4b7fe9ae202ea3c74798f58cd3a920821d

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

maflcko commented at 10:55 AM on January 9, 2026: member

review ACK c5825d4b7fe9ae202ea3c74798f58cd3a920821d 🛄

<details><summary>Show signature</summary>

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==

</details>

maflcko commented at 11:28 AM on January 9, 2026: member

rfm?

achow101 commented at 10:37 PM on January 13, 2026: member

ACK c5825d4b7fe9ae202ea3c74798f58cd3a920821d

achow101 merged this on Jan 13, 2026

achow101 closed this on Jan 13, 2026

hebasto deleted the branch on Jan 13, 2026