-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Parametrize more tests #6531
Parametrize more tests #6531
Conversation
My comment is that these changes are not just extracting parameters from tests that start with loops - they're also splitting the parts that aren't in a loop into new tests, and moving outer code into loops. They're also parametrizing not just the inputs, but the expected results as well. I don't think that every loop in a test needs to be converted into parameters. Happy to hear thoughts from others though. |
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.
I think these changes look fine, and even found one test that can be improved.
My comment is that these changes are not just extracting parameters from tests that start with loops - they're also splitting the parts that aren't in a loop into new tests, and moving outer code into loops. [...] I don't think that every loop in a test needs to be converted into parameters.
While I'd agree that not every loop needs to be converted into a parametrized test, these changes look reasonable to me. There are a few tests where the tests now repeatedly load a test input image. Where this is an expensive operation, these can be converted into a fixture.
They're also parametrizing not just the inputs, but the expected results as well.
I think this is fine. I've already had a few PRs that add new tests with parametrized expected results merged here, e.g. test_imagefont:test_getlength.
An exception occurs before they would be checked.
for more information, see https://pre-commit.ci
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.
A few little whitespace/naming nits
Co-authored-by: Hugo van Kemenade <hugovk@users.noreply.github.com>
Co-authored-by: Hugo van Kemenade <hugovk@users.noreply.github.com>
No description provided.