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

Parameterize tests #6525

Closed
Yay295 opened this issue Aug 23, 2022 · 13 comments · Fixed by #6634
Closed

Parameterize tests #6525

Yay295 opened this issue Aug 23, 2022 · 13 comments · Fixed by #6634
Labels

Comments

@Yay295
Copy link
Contributor

Yay295 commented Aug 23, 2022

Currently many of the pytest tests start with a loop over a list of some values, with all of the test code inside this loop.

def test_image_solid(self):
for mode in ("RGBA", "RGB", "L"):
im = Image.new(mode, (200, 200), "red")
im2 = getattr(self, "gradient_" + mode)
im.paste(im2, (12, 23))
im = im.crop((12, 23, im2.width + 12, im2.height + 23))
assert_image_equal(im, im2)

Instead, we can use pytest's @pytest.mark.parametrize annotation to handle this looping automatically.

@pytest.mark.parametrize("mode", ["RGBA", "RGB", "L"])
def test_image_solid(self, mode):
im = Image.new(mode, (200, 200), "red")
im2 = getattr(self, "gradient_" + mode)
im.paste(im2, (12, 23))
im = im.crop((12, 23, im2.width + 12, im2.height + 23))
assert_image_equal(im, im2)

The benefit of using pytest to do this is that if a test fails, it tells you which parameter it failed with. Also pytest will test with each of the parameters instead of stopping on the first failure.

I've already made this change for one file (#6519), but there are a lot more tests that could benefit from this annotation.

@radarhere
Copy link
Member

I've done a pass over the tests, and created #6526. Do you think there is still more work to be done?

@Yay295
Copy link
Contributor Author

Yay295 commented Aug 23, 2022

I think there are more tests that can be parametrized. I left a comment with some of them on your pull request.

@radarhere
Copy link
Member

I've applied your suggestions.

@radarhere radarhere changed the title Parameterize pytest tests Parameterize tests Aug 25, 2022
@radarhere
Copy link
Member

radarhere commented Aug 29, 2022

#6531 and #6534 are further PRs that add parametrizations.

@radarhere
Copy link
Member

All three PRs have now been merged. Is there still more work to be done?

@Yay295
Copy link
Contributor Author

Yay295 commented Sep 14, 2022

Possibly. I haven't looked through all of the test files, but there's probably a few more that could be parametrized.

@radarhere
Copy link
Member

radarhere commented Oct 3, 2022

If there's nothing specific, then it's hard to know when this is done.

I've done another pass over the tests, and created PR #6634. I'm going to say that PR resolves this.

@hugovk
Copy link
Member

hugovk commented Oct 3, 2022

Thanks both for the issue and PRs!

Parametrising means we get more test cases, which is good because each runs in isolation.


Before this issue was opened, there were 3,177 test cases (Python 3.10/Ubuntu):

==== 2994 passed, 180 skipped, 3 xfailed, 19 warnings in 126.60s (0:02:06) =====

https://github.com/python-pillow/Pillow/actions/runs/2904499282/jobs/4622327543


After the last PR #6634 was closed, we're at 3,969 test cases:

==== 3783 passed, 183 skipped, 3 xfailed, 18 warnings in 124.52s (0:02:04) =====

https://github.com/python-pillow/Pillow/actions/runs/3176175582/jobs/5175163150


Obviously some of these new tests are from other PRs, but most of these 792 will be from parametrising. 🚀

@Yay295
Copy link
Contributor Author

Yay295 commented Oct 8, 2022

I've noticed that some of the parameters use tuples, and some use lists. I don't think it makes a difference in most cases, but is there one format we should try to use everywhere?

@radarhere
Copy link
Member

Since the parameters are an immutable series of items, tuples.

@Yay295
Copy link
Contributor Author

Yay295 commented Oct 9, 2022

I've created #6653 to use tuples more often (plus some other things I noticed).

@homm
Copy link
Member

homm commented Jul 6, 2023

@radarhere

Since the parameters are an immutable series of items, tuples.

The accepted answer is much better than the one you are referring. Immutability is a property of tuples, but not a purpose. Tuples are intended to store heterogeneous data. When you have (x, y, z) or (Name, Age, Vocation) in your program, it doesn't make sense to add another name in the end or change the order. It just different types of data. On the other hand, lists are used for homogeneous data. It worth nothing to add a person to the list of the persons or swap two persons in the list: it will remain the list of persons.

@radarhere
Copy link
Member

My understanding is that tuples are more efficient than lists, but presumably that difference is minor, so if @homm has thoughts about what makes more sense stylistically, then I'm happy to go with that. If anything, I just hope to avoid going back and forth on this with large PRs.

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 a pull request may close this issue.

4 participants