Only run TestFileLibTiff.test_save_many_compressed() on Windows

[akx]

Sibling of #9943, fell out of the same "hmm, what does pytest --durations say" suite.

It is the slowest test of the suite, saving the same image 10 000 times to find leaking file descriptors.

It was added in March 2023 in #6986, e953978, as a test for the commit 2299490 that fixed a file descriptor leak.

The TIFF cleanup path was subsequently cleaned up in June 2023 (#7199, e45da2a) and October 2024 (#8458, 7edf952) to avoid os.dup() at all, so the test isn't needed.

[radarhere]

The other PRs you mentioned didn't add any new tests regarding this functionality.

My personal feelings are to be pretty skeptical around 'the test isn't needed'. If we started pushing that idea further, the test suite would become very small quickly. I'm not saying there aren't ever reasons for getting rid of old tests, but I don't think test suite performance is a significant one. If the runtime of the test suite is a concern, then #9342 seems like it should be the first thing to address.

If anyone else would like to merge, go ahead, but those are my thoughts.

[akx]

The other PRs you mentioned didn't add any new tests regarding this functionality.

I mean... the broken functionality was removed in the fix PRs?

My personal feelings are to be pretty skeptical around 'the test isn't needed'.

I get that. But I looked and investigated, and I really don't think this test is needed with the current implementation. As noted, I can replace this with a test that counts fds over, say, 20 save() calls, if you do think TIFF saving is still leaking fds.

If we started pushing that idea further, the test suite would become very small quickly.

I don't think anyone is advocating pushing to make the test suite very small very quickly. But I think it's fair to say there are tests that aren't necessary, and I don't see reasons to spend CPU cycles (and in cases where tmp_path is disk-backed, SSD write cycles 😄) unnecessarily.

I'm not saying there aren't ever reasons for getting rid of old tests, but I don't think test suite performance is a significant one. If the runtime of the test suite is a concern, then #9342 seems like it should be the first thing to address.

I can take a look at that too. I locally run with pytest-xdist when suitable. #9933 touches upon the same things a bit. (To paraphrase it: stably measuring memory on PyPy will be somewhat futile, as it may decide that a function needs JIT compilation at any given moment, and secondly, the "tall" case of test_empty_image ends up allocating 700 megabytes of NULL pointers, and that sort of thing occasionally timeouts on the noisy-neighbors GitHub Actions CI VMs.

EDIT: I took a look. #9945.

I guess if someone from Pillow wants to reach out to Depot or Blacksmith for sponsored faster CI runners, that could be fun!

[radarhere]

If anyone is wondering why 10,000, it's to try and go over the limit of 8,192 - https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/setmaxstdio?view=msvc-170#remarks

[radarhere]

I don't suppose your perspective on this has changed now that #9945 is merged?

[akx]

Even with #9945 for CI, I still don't see a great reason to spend cycles saving the same file myriad times. There are other systems (I would imagine downstream distro packages, etc.) running the same tests, maybe without xdist, etc.

I'd like to know if you feel there's an actual reason for this test to remain post e45da2a and 7edf952 -- why wouldn't all other codecs need a test like this too, if the fear, as it were, was that some save routines might be leaking FDs?

[radarhere]

It's a regression test, rather than a correctness test. The functionality was broken once, maybe it will break again, the test is to ensure it doesn't.

I think it's acceptable for a test suite to check extremes, and this doesn't seem that extreme. Without pytest-xdist, the entire test suite takes 28s locally for me, and this test takes 2s out of that. I don't personally find the potential difference that compelling.

[akx]

It's a regression test, rather than a correctness test. The functionality was broken once, maybe it will break again, the test is to ensure it doesn't.

That's my reasoning in akx#15 (comment) 😄

Anyway, in lieu of removing the test it's now replaced with a fast fd-counting one. What do you think?

[radarhere]

Hmm. That is skipped on Windows, which is where the 8,192 limit was originally a problem.

Let me suggest another idea - what if we skipped the test on everything except for Windows?

diff --git a/Tests/test_file_libtiff.py b/Tests/test_file_libtiff.py
index 3d804ea4a..1279cdc21 100644
--- a/Tests/test_file_libtiff.py
+++ b/Tests/test_file_libtiff.py
@@ -1268,6 +1268,7 @@ class TestFileLibTiff(LibTiffTestCase):
         with pytest.raises(ValueError, match="cannot write empty image"):
             im.save(out, compression=compression)
 
+    @pytest.mark.skipif(sys.platform != "win32", reason="Checks a Windows limit")
     def test_save_many_compressed(self, tmp_path: Path) -> None:
         im = hopper()
         out = tmp_path / "temp.tif"