Skip to content

Conversation

@ken2812221
Copy link
Contributor

Fix broken exclusion list in functional tests. See #14007 (review)

@ken2812221 ken2812221 changed the title tests: exclude all tests with difference parameters tests: exclude all tests with difference parameters in --exclude list Sep 25, 2018
@fanquake fanquake added the Tests label Sep 25, 2018
@DrahtBot
Copy link
Contributor

DrahtBot commented Sep 25, 2018

Reviewers, this pull request conflicts with the following ones:

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.

Copy link
Contributor

@jnewbery jnewbery left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK with one request for a comment.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@jimmysong jimmysong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple of nits , but otherwise looks good.

# Remove the test cases that the user has explicitly asked to exclude.
if args.exclude:
exclude_tests = [re.sub("\.py$", "", test) + (".py" if ".py" not in test else "") for test in args.exclude.split(',')]
exclude_tests = [test.split('.py')[0] for test in args.exclude.split(',')]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't work with args?

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

test_list.remove(exclude_test)
else:
# Remove <test_name>.py and <test_name>.py --arg from the test list
exclude_list = [test for test in test_list if test.split('.py')[0] == exclude_test]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@maflcko
Copy link
Member

maflcko commented Sep 27, 2018

utACK c7b3e48

maflcko pushed a commit to maflcko/bitcoin-core that referenced this pull request Sep 27, 2018
…ers in `--exclude` list

c7b3e48 tests: exclude all tests with difference parameters (Chun Kuan Lee)

Pull request description:

  Fix broken exclusion list in functional tests. See bitcoin#14007 (review)

Tree-SHA512: b6c2b86fef13e3c00c695adaeeb3e47ee9b48877c71bc605d24201ce931b2ef3ae9f5f199071fa1ec5de2d7aadc478410094c380cc297922e683e9b2569cda03
@maflcko maflcko merged commit c7b3e48 into bitcoin:master Sep 27, 2018
@ken2812221 ken2812221 deleted the 2018-09-25-test-fix branch September 27, 2018 16:34
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants