tests: exclude all tests with difference parameters in `--exclude` list #14316

pull ken2812221 wants to merge 1 commits into bitcoin:master from ken2812221:2018-09-25-test-fix changing 2 files +7 −5

ken2812221 commented at 3:16 AM on September 25, 2018: contributor

Fix broken exclusion list in functional tests. See #14007#pullrequestreview-158309105
ken2812221 cross-referenced this on Sep 25, 2018 from issue tests: Run functional test on Windows and enable it on Appveyor by ken2812221
ken2812221 renamed this:
~~tests: exclude all tests with difference parameters~~
tests: exclude all tests with difference parameters in `--exclude` list
on Sep 25, 2018
ken2812221 force-pushed on Sep 25, 2018
fanquake added the label Tests on Sep 25, 2018
DrahtBot commented at 4:32 AM on September 25, 2018: contributor
Reviewers, this pull request conflicts with the following ones:
- #14320 ([bugfix] wallet: Fix duplicate fileid detection by ken2812221)
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 cross-referenced this on Sep 25, 2018 from issue tests: Write the notification message to different files to avoid race condition in feature_notifications.py by ken2812221

in test/functional/test_runner.py:291 in 61fb484af9 outdated

 289 | +        exclude_tests = [re.sub("\.py$", "", test) for test in args.exclude.split(',')]
 290 |          for exclude_test in exclude_tests:
 291 | -            if exclude_test in test_list:
 292 | -                test_list.remove(exclude_test)
 293 | -            else:
 294 | +            exclude_list = [test for test in test_list if test.split('.py')[0] == exclude_test]

jnewbery commented at 3:33 PM on September 25, 2018:

Could probably do with a comment here to explain why you're doing this (removing <test_name>.py and <test_name>.py --arg from the test list)

jnewbery commented at 3:33 PM on September 25, 2018: member

utACK with one request for a comment.
DrahtBot cross-referenced this on Sep 25, 2018 from issue [bugfix] wallet: Fix duplicate fileid detection by ken2812221

in test/functional/test_runner.py:288 in 61fb484af9 outdated

 284 | @@ -285,11 +285,12 @@ def main():
 285 |  
 286 |      # Remove the test cases that the user has explicitly asked to exclude.
 287 |      if args.exclude:
 288 | -        exclude_tests = [re.sub("\.py$", "", test) + (".py" if ".py" not in test else "") for test in args.exclude.split(',')]
 289 | +        exclude_tests = [re.sub("\.py$", "", test) for test in args.exclude.split(',')]

MarcoFalke commented at 5:28 PM on September 25, 2018:

nit: could remove the use of the re module, since it is no longer required? "foo.py".split('.py')[0] == "foo".split('.py')[0]

DrahtBot added the label Needs rebase on Sep 25, 2018
ken2812221 force-pushed on Sep 25, 2018
ken2812221 force-pushed on Sep 25, 2018
tests: exclude all tests with difference parameters c7b3e487f2
ken2812221 force-pushed on Sep 25, 2018
DrahtBot removed the label Needs rebase on Sep 25, 2018

in test/functional/test_runner.py:288 in c7b3e487f2

 284 | @@ -285,11 +285,13 @@ def main():
 285 |  
 286 |      # Remove the test cases that the user has explicitly asked to exclude.
 287 |      if args.exclude:
 288 | -        exclude_tests = [re.sub("\.py$", "", test) + (".py" if ".py" not in test else "") for test in args.exclude.split(',')]
 289 | +        exclude_tests = [test.split('.py')[0] for test in args.exclude.split(',')]

jimmysong commented at 12:03 AM on September 27, 2018:

nit: I prefer test.strip('.py') just in case there's a very unlikely '.py' in the middle of a file name.

MarcoFalke commented at 1:38 AM on September 27, 2018:

Wouldn't work with args?

>>> 'foo.py --arg'.strip('.py')
'foo.py --arg'

in test/functional/test_runner.py:291 in c7b3e487f2

 290 |          for exclude_test in exclude_tests:
 291 | -            if exclude_test in test_list:
 292 | -                test_list.remove(exclude_test)
 293 | -            else:
 294 | +            # Remove <test_name>.py and <test_name>.py --arg from the test list
 295 | +            exclude_list = [test for test in test_list if test.split('.py')[0] == exclude_test]

jimmysong commented at 12:04 AM on September 27, 2018:

nit: same as above. test.strip('.py') is preferable over test.split('.py')[0] and reads a little better.

jimmysong commented at 12:06 AM on September 27, 2018: contributor

Couple of nits , but otherwise looks good.
MarcoFalke commented at 1:38 AM on September 27, 2018: member

utACK c7b3e487f229142795a0b3ce6ce409ce7084d966
MarcoFalke referenced this in commit 9b8bb5f140 on Sep 27, 2018
MarcoFalke merged this on Sep 27, 2018
MarcoFalke closed this on Sep 27, 2018
ken2812221 deleted the branch on Sep 27, 2018
Bushstar cross-referenced this on Oct 11, 2018 from issue Updates from bitcoin/master by Bushstar
bitcoin locked this on Sep 8, 2021

Contributors

Labels

Linked (view graph)

#14007 tests: Run functional test on Windows and enable it on Appveyor #14306 AppVeyor: Move AppVeyor YAML to dot-file-style YAML