Skip to content
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

Merged
merged 10 commits into from
Sep 6, 2022
Merged

Parametrize more tests #6531

merged 10 commits into from
Sep 6, 2022

Conversation

Yay295
Copy link
Contributor

@Yay295 Yay295 commented Aug 24, 2022

No description provided.

@radarhere
Copy link
Member

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.

Copy link
Contributor

@nulano nulano left a 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.

Tests/test_image_filter.py Outdated Show resolved Hide resolved
Tests/test_image_filter.py Outdated Show resolved Hide resolved
@radarhere radarhere mentioned this pull request Aug 29, 2022
Copy link
Member

@hugovk hugovk left a 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

Tests/test_image_filter.py Outdated Show resolved Hide resolved
Tests/test_image_filter.py Outdated Show resolved Hide resolved
Tests/test_image_reduce.py Outdated Show resolved Hide resolved
Tests/test_image_reduce.py Outdated Show resolved Hide resolved
Tests/test_image_reduce.py Outdated Show resolved Hide resolved
Tests/test_image_transform.py Outdated Show resolved Hide resolved
Tests/test_image_transform.py Outdated Show resolved Hide resolved
Tests/test_image_transform.py Outdated Show resolved Hide resolved
Tests/test_image_transform.py Outdated Show resolved Hide resolved
Tests/test_image_transform.py Outdated Show resolved Hide resolved
Yay295 and others added 2 commits August 29, 2022 11:35
Co-authored-by: Hugo van Kemenade <hugovk@users.noreply.github.com>
Co-authored-by: Hugo van Kemenade <hugovk@users.noreply.github.com>
@hugovk hugovk merged commit 7cff822 into python-pillow:main Sep 6, 2022
@Yay295 Yay295 deleted the parametrize branch September 6, 2022 16:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants