-
Notifications
You must be signed in to change notification settings - Fork 37.7k
Require all tests to follow naming convention #12252
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
utACK 4de3d52e655902a07c85db84f0c6236302ef78a4 |
utACK 4de3d52. |
4de3d52
to
0b94082
Compare
Rebased |
Could you also fix the names in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested ACK 0b94082361d4ffe130766aa14f15dc7144f8239c with a couple of style nits.
Agree with Marco that the travis file needs to be updated.
I think this may need to be rebased to show that only the last commit is new.
test/functional/test_runner.py
Outdated
print("Consider reducing EXPECTED_VIOLATION_COUNT from %d to %d" % (EXPECTED_VIOLATION_COUNT, len(bad_script_names))) | ||
elif len(bad_script_names) > EXPECTED_VIOLATION_COUNT: | ||
print("INFO: %d tests not meeting naming conventions (expected %d):" % (len(bad_script_names), EXPECTED_VIOLATION_COUNT)) | ||
if len(bad_script_names) > 0: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
more pythonic would be if bad_script_names:
(https://www.python.org/dev/peps/pep-0008/#id51: "For sequences, (strings, lists, tuples), use the fact that empty sequences are false.")
test/functional/test_runner.py
Outdated
print(" %s" % ("\n ".join(sorted(bad_script_names)))) | ||
assert len(bad_script_names) <= EXPECTED_VIOLATION_COUNT + LEEWAY, "Too many tests not following naming convention! (%d found, expected: <= %d)" % (len(bad_script_names), EXPECTED_VIOLATION_COUNT) | ||
assert len(bad_script_names) == 0, "Some tests are not following naming convention!" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This condition has already been tested. Just raise an assertion error:
raise AssertionError("Some tests are not following naming convention!")
@MarcoFalke ah, sorry about that. #12292 should fix it. |
Are you planning on fixing up the nits? |
0b94082
to
125f4a4
Compare
Nits fixed as suggested, and rebased to make github happier |
utACK 125f4a4 |
125f4a4 [tests] Require all tests to follow naming convention (Anthony Towns) Pull request description: Based on top of bitcoin#11774 Tree-SHA512: 1eb156b5a97b30c203b7b0ad3d2055b391ef825e2e57805c7745b5259a9b1caaa115768ec225452f12f354e550f83e071f9c6fee2c36698b4679191895aab8de
125f4a4 [tests] Require all tests to follow naming convention (Anthony Towns) Pull request description: Based on top of bitcoin#11774 Tree-SHA512: 1eb156b5a97b30c203b7b0ad3d2055b391ef825e2e57805c7745b5259a9b1caaa115768ec225452f12f354e550f83e071f9c6fee2c36698b4679191895aab8de
Based on top of #11774