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

Consolidate getdata and putdata tests #8017

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

Yay295
Copy link
Contributor

@Yay295 Yay295 commented Apr 25, 2024

Moved all of the getdata and putdata tests to the same file (because test_getdata_putdata doesn't fit in either individual file), and parametrized two tests.

@radarhere
Copy link
Member

test_image_getdata_putdata.py seems like a mouthful. How about just test_image_data.py?

@Yay295
Copy link
Contributor Author

Yay295 commented Apr 28, 2024

sure

@Yay295 Yay295 force-pushed the test_consolidation branch from 55f68cc to 2182da0 Compare April 28, 2024 03:47
@radarhere
Copy link
Member

Moved all of the getdata and putdata tests to the same file (because test_getdata_putdata doesn't fit in either individual file)

Sorry, I've just realised what this meant. In main, we currently have test_image_getdata.py and test_image_putdata.py, and here you've merged them together into a single file because you'd like to find a new home for a test from test_image.py / #7209, and you don't think a roundtrip test belongs in either file.

I think test_image_putdata.py already has roundtrip tests, using both getdata() and putdata().

@pytest.mark.parametrize("mode", ("I", "I;16", "I;16L", "I;16B"))
def test_mode_i(mode: str) -> None:
src = hopper("L")
data = list(src.getdata())
im = Image.new(mode, src.size, 0)
im.putdata(data, 2, 256)
target = [2 * elt + 256 for elt in data]
assert list(im.getdata()) == target

@Yay295
Copy link
Contributor Author

Yay295 commented Apr 28, 2024

I'd say those should be in the combined file for the same reason. Though I suppose I shouldn't have added putdata to those function names.

@Yay295
Copy link
Contributor Author

Yay295 commented Apr 28, 2024

Cleaned up the tests a bit more.

@Yay295 Yay295 force-pushed the test_consolidation branch 4 times, most recently from 9b10658 to ba83ec4 Compare May 15, 2024 16:30
@Yay295
Copy link
Contributor Author

Yay295 commented May 15, 2024

rebased, multiple times because I didn't check it locally and kept missing something when resolving the conflict

@Yay295 Yay295 force-pushed the test_consolidation branch from b00fded to 32c46eb Compare May 23, 2024 13:32
@Yay295 Yay295 force-pushed the test_consolidation branch from 32c46eb to 7236203 Compare June 3, 2024 04:01
@Yay295 Yay295 force-pushed the test_consolidation branch from 7236203 to 97d8ffe Compare June 15, 2024 15:12
@Yay295 Yay295 force-pushed the test_consolidation branch 2 times, most recently from e9c0ae5 to db1764a Compare June 25, 2024 16:01
@Yay295 Yay295 force-pushed the test_consolidation branch 2 times, most recently from 2ce6977 to 81f1cd0 Compare June 28, 2024 20:32
@Yay295 Yay295 force-pushed the test_consolidation branch from 81f1cd0 to 8096db8 Compare July 8, 2024 23:08
@Yay295 Yay295 force-pushed the test_consolidation branch 2 times, most recently from d3b0a6a to 8d9ab96 Compare July 19, 2024 13:19
@Yay295 Yay295 force-pushed the test_consolidation branch 2 times, most recently from 66bf305 to d248a8b Compare August 1, 2024 15:00
@Yay295 Yay295 force-pushed the test_consolidation branch from d248a8b to bd5d479 Compare August 14, 2024 13:27
@Yay295 Yay295 force-pushed the test_consolidation branch from bd5d479 to b85492a Compare August 28, 2024 13:10
@Yay295 Yay295 force-pushed the test_consolidation branch from 0eabe82 to ebe4749 Compare October 7, 2024 13:54
Yay295 and others added 9 commits October 12, 2024 18:33
This test had some duplicate checks because originally in Python 2 the numbers in those lines had a "L" suffix, which is no longer used in Python 3.
The original bug this was referring to seems to have been lost, but it looks to have been something to do with Python 2/3 compatibility and big numbers.
See commit c8ce29c and python-pillow#7991.
Co-authored-by: Ondrej Baranovič <ondreko.tiba@gmail.com>
@Yay295 Yay295 force-pushed the test_consolidation branch from ebe4749 to a96e38c Compare October 12, 2024 23:33
@Yay295
Copy link
Contributor Author

Yay295 commented Oct 17, 2024

What is currently blocking this from being merged?

@radarhere
Copy link
Member

As I've said, my opinion is that this change isn't needed. If others think it is though, feel free to outvote me.

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.

3 participants