Skip to content

Remove test_empty_image test - #9958

Open
akx wants to merge 1 commit into
python-pillow:mainfrom
akx:test-empty-image-correction
Open

akx wants to merge 1 commit into
python-pillow:mainfrom
akx:test-empty-image-correction

Conversation

@akx

@akx akx commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Alternative to #9957.
Refs akx#21 (comment).
Refs #9933 (which fixes the timeout flake by reducing the size of the image, so the allocation is smaller, which was not a preferred approach).

@akx

akx commented Sep 4, 2026

Copy link
Copy Markdown
Contributor Author

cc @radarhere :)

@akx
akx marked this pull request as ready for review September 4, 2026 09:18
@radarhere

Copy link
Copy Markdown
Member

While I instinctively dislike reducing the severity of a test case,
and my first instinct was to try and see if Pillow could overcome it with improved performance,
I do think this test is about measuring the overall performance for the scenario, not just whether we have mitigated the problem at all.

It's just unfortunate that there's such variety when testing in CI.

I've approved #9933.

@akx

akx commented Sep 5, 2026

Copy link
Copy Markdown
Contributor Author

This is the more accurate test for 91b01f4 though: we're now measuring whether ImagingFill (which gets called for a non-None initial fill) actually bails out early when either dimension is zero. Without instrumentation at the C level, I don't think we can do much better than to measure it like this... 🤔

With this, the severity of the test case (100M x 0, or 0 x 100M) can remain as it was, with the noted caveat that the tall test will temporarily allocate 700M of memory for the row pointers.

@radarhere

Copy link
Copy Markdown
Member

I've re-examined the history of this. It turns out that #6842 was added to try and fix a security bug. The large sizes being tested aimed to replicate the security bug. The change is still valid, but it turns out that it wasn't actually the root cause of the security issue. So it doesn't work as a regression test.

Without this test, the lines still receive coverage from other tests, there's just no test that fails (which is positive - if our code still works well without this early return, great).

If I suggested removing the test altogether, what would you think of that?

@akx

akx commented Sep 16, 2026

Copy link
Copy Markdown
Contributor Author

@radarhere I'm okay with that. I'll rework the PR :)

@akx
akx force-pushed the test-empty-image-correction branch from 6d8c0c7 to e366475 Compare September 16, 2026 15:28
@akx akx changed the title test_empty_image: measure CPU time between filling and not filling Remove test_empty_image test Sep 16, 2026
Allocating a tall image with no width will still take some time
to initialize the pointer array for the rows.
@akx
akx force-pushed the test-empty-image-correction branch from e366475 to b9e7f5c Compare September 16, 2026 15:29
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.

2 participants