test: fix exclude parsing for functional runner

m3dwards commented at 6:01 pm on September 11, 2024: contributor

This restores previous behaviour of being able to exclude a test by name without having to specify .py extension.

It was noticed in #30851 that tests were no longer being excluded.

PR #30244 introduced being able to exclude a specific tests based on args (such as --exclude "rpc_bind.py --ipv6) but it made the wrong assumption that test names intended to be excluded would include the .py extension.

The following #30244 (comment) shows that this is not how the --exclude flag was used in CI.

#30244 (comment) gave three examples of --exclude being used in CI so I compared the number of tests that the runner would run for these three examples in three situations, before #30244 was introduced, in master today and with this PR applied.

Example:

--previous-releases --coverage --extended --exclude feature_dbcrash

Test count: Before #30244 introduced: 314 Master: 315 With this PR: 314

Example:

--exclude feature_init,rpc_bind,feature_bind_extra

Test count: Before #30244 introduced: 306 Master 311 With this PR: 306

Example:

--exclude rpc_bind,feature_bind_extra

Before #30244 introduced: 307 Master 311 With this PR: 307

I’ve also tested that the functionality introduced with #30244 remains and we can still exclude specific tests by argument.

DrahtBot commented at 6:01 pm on September 11, 2024: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	maflcko, willcl-ark

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

DrahtBot added the label Tests on Sep 11, 2024

m3dwards commented at 6:03 pm on September 11, 2024: contributor

CC @maflcko

in test/functional/test_runner.py:537 in 82bb0e38f6 outdated

539-                exclude_list = [test for test in test_list if test.split('.py')[0] == exclude_test.split('.py')[0]]
540-                if not exclude_list:
541-                    print_warning_missing_test(exclude_test)
542-                for exclude_item in exclude_list:
543-                    test_list.remove(exclude_item)
544+            if '--' in exclude_test or ' ' in exclude_test:

maflcko commented at 7:02 pm on September 11, 2024:

0            if  ' ' in exclude_test:

Doesn’t -- imply that a space is before it?

m3dwards commented at 12:36 pm on September 12, 2024:

I’ll take it

maflcko approved

maflcko commented at 7:03 pm on September 11, 2024: member

lgtm

maflcko added this to the milestone 29.0 on Sep 11, 2024

test: fix exclude parsing for functional runner

This restores previous behaviour of being able to exclude a test by name without having to specify .py extension.

72b46f28bf

m3dwards force-pushed on Sep 12, 2024

maflcko commented at 12:44 pm on September 12, 2024: member

review ACK 72b46f28bf8d37a166961b5aa2a22302b932b756

willcl-ark approved

willcl-ark commented at 1:26 pm on September 12, 2024: member

ACK 72b46f28bf8d37a166961b5aa2a22302b932b756

This fixes the described issue.

It would seem reasonable to introduce something to stop this reoccurring if possible. I added a commit here which prints out excluded tests at the end of the run in the results, but we could print excluded tests at the beginning as we remove them from the test_list too:

 0TEST                                                     | STATUS    | DURATION
 1
 2example_test.py                                          | ✓ Passed  | 2 s
 3feature_abortnode.py                                     | ✓ Passed  | 2 s
 4feature_addrman.py                                       | ✓ Passed  | 3 s
 5feature_anchors.py                                       | ✓ Passed  | 3 s
 6feature_asmap.py                                         | ✓ Passed  | 4 s
 7feature_assumeutxo.py                                    | ✓ Passed  | 22 s
 8wallet_upgradewallet.py --legacy-wallet                  | ○ Skipped | 0 s
 9wallet_watchonly.py --legacy-wallet                      | ○ Skipped | 0 s
10wallet_watchonly.py --usecli --legacy-wallet             | ○ Skipped | 1 s
11feature_bind_extra.py                                    | ○ Excluded | 0 s
12feature_init.py                                          | ○ Excluded | 0 s
13rpc_bind.py --ipv4                                       | ○ Excluded | 0 s
14rpc_bind.py --ipv6                                       | ○ Excluded | 0 s
15rpc_bind.py --nonloopback                                | ○ Excluded | 0 s
16
17ALL                                                      | ✓ Passed  | 1797 s (accumulated)
18Runtime: 114 s

Feel free to take it (or of course design your own version) if you agree with the suggestion.

maflcko commented at 2:22 pm on September 12, 2024: member

(Let’s merge this minimal bugfix first, and then change the output in a follow-up. Otherwise, this will be delayed and running the s390x will not be possible out of the box)

fanquake merged this on Sep 12, 2024

fanquake closed this on Sep 12, 2024

in test/functional/test_runner.py:528 in 72b46f28bf

521@@ -522,23 +522,24 @@ def main():
522         test_list += BASE_SCRIPTS
523 
524     # Remove the test cases that the user has explicitly asked to exclude.
525+    # The user can specify a test case with or without the .py extension.
526     if args.exclude:
527         def print_warning_missing_test(test_name):
528             print("{}WARNING!{} Test '{}' not found in current test list.".format(BOLD[1], BOLD[0], test_name))

maflcko commented at 2:43 pm on September 12, 2024:

Another way this would have been caught is by making warnings in the CI fatal. There is fail_on_warn=args.ci which can be used.

maflcko commented at 1:37 pm on October 2, 2024:

Fixed in https://github.com/bitcoin/bitcoin/pull/31018

m3dwards deleted the branch on Sep 12, 2024

TheCharlatan referenced this in commit 69282950aa on Sep 16, 2024

TheCharlatan referenced this in commit dfe0cd4ec5 on Sep 16, 2024

test: fix exclude parsing for functional runner #30872

Code Coverage

Reviews