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

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
Concept ACK	rkrux
Stale ACK	bensig, maflcko

If your review is incorrectly listed, please copy-paste <!–meta-tag:bot-skip–> into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#31410 (qa: Fix wallet_multiwallet.py by hebasto)

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.

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.

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

0➜  bitcoin git:(251229-ft-exclude-append) ✗ ./build/test/functional/test_runner.py --exclude=rpc_signer,rpc_setban --exclude=rpc_users,rpc_net,rpc_signer
1Temporary test directory at /var/folders/6v/mpspb4bx4zzf3xr2z96hgq9h0000gn/T/test_runner_₿_🏃_20251229_193516
2exclude_tests:  ['rpc_signer', 'rpc_setban', 'rpc_users', 'rpc_net', 'rpc_signer']
3WARNING! 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:

0        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:

 0diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
 1index 5e5b0c48e8..e287841e6e 100644
 2--- a/.github/workflows/ci.yml
 3+++ b/.github/workflows/ci.yml
 4@@ -503,7 +503,7 @@ jobs:
 5         env:
 6           # TODO: Fix the excluded test and re-enable it.
 7           # feature_unsupported_utxo_db.py fails on windows because of emojis in the test data directory
 8-          EXCLUDE: '--exclude wallet_multiwallet.py,feature_unsupported_utxo_db.py'
 9+          EXCLUDE: '--exclude wallet_multiwallet.py --exclude feature_unsupported_utxo_db.py'
10           TEST_RUNNER_EXTRA: ${{ github.event_name != 'pull_request' && '--extended' || '' }}
11         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
12 
13diff --git a/ci/test/00_setup_env_native_valgrind.sh b/ci/test/00_setup_env_native_valgrind.sh
14index 884bc3955c..3e23a580da 100755
15--- a/ci/test/00_setup_env_native_valgrind.sh
16+++ b/ci/test/00_setup_env_native_valgrind.sh
17@@ -13,7 +13,7 @@ export PIP_PACKAGES="--break-system-packages pycapnp"
18 export USE_VALGRIND=1
19 export NO_DEPENDS=1
20 # bind tests excluded for now, see [#17765 (comment)](/bitcoin-bitcoin/17765/#issuecomment-602068547)
21-export TEST_RUNNER_EXTRA="--exclude rpc_bind,feature_bind_extra"
22+export TEST_RUNNER_EXTRA="--exclude rpc_bind --exclude feature_bind_extra"
23 export GOAL="install"
24 # TODO enable GUI
25 export BITCOIN_CONFIG="\
26diff --git a/ci/test/00_setup_env_s390x.sh b/ci/test/00_setup_env_s390x.sh
27index b7d23fbf5e..b6f924cf5d 100755
28--- a/ci/test/00_setup_env_s390x.sh
29+++ b/ci/test/00_setup_env_s390x.sh
30@@ -11,7 +11,7 @@ export PACKAGES="python3-zmq"
31 export CONTAINER_NAME=ci_s390x
32 export CI_IMAGE_NAME_TAG="mirror.gcr.io/ubuntu:24.04"
33 export CI_IMAGE_PLATFORM="linux/s390x"
34-export TEST_RUNNER_EXTRA="--exclude rpc_bind,feature_bind_extra"  # Excluded for now, see [#17765 (comment)](/bitcoin-bitcoin/17765/#issuecomment-602068547)
35+export TEST_RUNNER_EXTRA="--exclude rpc_bind --exclude feature_bind_extra"  # Excluded for now, see [#17765 (comment)](/bitcoin-bitcoin/17765/#issuecomment-602068547)
36 export RUN_FUNCTIONAL_TESTS=true
37 export GOAL="install"
38 export BITCOIN_CONFIG="\
39diff --git a/test/functional/test_runner.py b/test/functional/test_runner.py
40index 58b331ee20..dae672e7da 100755
41--- a/test/functional/test_runner.py
42+++ b/test/functional/test_runner.py
43@@ -400,7 +400,7 @@ def main():
44     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.')
45     parser.add_argument('--coverage', action='store_true', help='generate a basic coverage report for the RPC interface')
46     parser.add_argument('--ci', action='store_true', help='Run checks and code that are usually only enabled in a continuous integration environment')
47-    parser.add_argument('--exclude', '-x', action='append', help='specify a comma-separated-list of scripts to exclude.')
48+    parser.add_argument('--exclude', '-x', action='append', help='specify a script to exclude (can be specified multiple times).')
49     parser.add_argument('--extended', action='store_true', help='run the extended test suite in addition to the basic tests')
50     parser.add_argument('--help', '-h', '-?', action='store_true', help='print help text and exit')
51     parser.add_argument('--jobs', '-j', type=int, default=4, help='how many test scripts to run in parallel. Default=4.')
52@@ -503,8 +503,18 @@ def main():
53             for exclude_item in exclude_list:
54                 test_list.remove(exclude_item)
55 
56-        exclude_tests = [test.strip() for test in ",".join(args.exclude).split(",")]
57-        for exclude_test in exclude_tests:
58+        for exclude_test in args.exclude:
59+            if ',' in exclude_test:
60+                print("{}WARNING!{} --exclude '{}' contains a comma. Use --exclude for each test.".format(BOLD[1], BOLD[0], exclude_test))
61+                if fail_on_warn:
62+                    sys.exit(1)
63+
64+        if len(args.exclude) != len(set(args.exclude)):
65+            print("{}WARNING!{} Duplicate --exclude detected.".format(BOLD[1], BOLD[0]))
66+            if fail_on_warn:
67+                sys.exit(1)
68+
69+        for exclude_test in args.exclude:
70             # A space in the name indicates it has arguments such as "rpc_bind.py --ipv4"
71             if ' ' in exclude_test:
72                 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?

 0diff --git a/test/functional/test_runner.py b/test/functional/test_runner.py
 1index 58b331ee20..fc204b858b 100755
 2--- a/test/functional/test_runner.py
 3+++ b/test/functional/test_runner.py
 4@@ -501,6 +501,7 @@ def main():
 5             if not exclude_list:
 6                 print_warning_missing_test(exclude_test)
 7             for exclude_item in exclude_list:
 8+                print("Excluding %s" % exclude_item)
 9                 test_list.remove(exclude_item)
10 
11         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:

0cmake -B build -DCMAKE_BUILD_TYPE=Debug && cmake --build build -j$(nproc) && \
1./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

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

Code Coverage & Benchmarks

Reviews

Conflicts

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