Skip to content

Conversation

@hugovk
Copy link
Member

@hugovk hugovk commented Dec 14, 2025

Follow on from #8972.

This runs tests by default with pytest-xdist, except disables for valgrind.

Locally, tox -e py314 goes from 44.79s for pytest to 22.78s.

And we save about an hour on the CI:

workflow before after saving faster
test 46.7m 28.9m 17.8m 1.62x
test-docker 133.7m 102.0m 31.8m 1.31x
test-windows 19.1m 13.1m 6.0m 1.46x
test-mingw 1.7m 1.0m 0.7m 1.73x
Total 201.2m 145.0m 56.2m 1.39x

This means the test order is no longer deterministic, so we don't need pytest-reverse.

Another option is to add another "extra" or "dependency group" to install pytest-xdist/pytest-sugar for certain invocations (and it'll need some config only to only set the options for these).

I put the imagegrab tests in a single group, which means they'll be allocated the same runner, to avoid some resource clashes on the CI. And I put an AVIF test into its own group because it sometimes failed locally when my machine didn't have much memory to spare.

@hugovk hugovk added the Testing label Dec 14, 2025
-e "PYTEST_ADDOPTS=-n0" \
-v $GITHUB_WORKSPACE:/Pillow \
pythonpillow/${{ matrix.docker }}:${{ matrix.dockerTag }} \
bash -c "pip install pytest-xdist && /Pillow/depends/docker-test-valgrind-memory.sh"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
bash -c "pip install pytest-xdist && /Pillow/depends/docker-test-valgrind-memory.sh"
bash -c "python3 -m pip install pytest-xdist && /Pillow/depends/docker-test-valgrind-memory.sh"

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we adding pytest-xdist when running valgrind?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a little circular, but it's because we're adding the --numprocesses=auto -dist=loadgroup options to the default pytest in pyproject.toml, and the valgrind run would then complain unrecognized arguments: --numprocesses=auto.

So we're installing it just to even though we're disabling it with PYTEST_ADDOPTS=-n0.

Perhaps we shouldn't set set those as default pytest options and instead only for the relevant builds?

@radarhere
Copy link
Member

This means that make test and make test-p actually both run the tests in parallel? Should we remove make test-p?

Pillow/Makefile

Lines 95 to 103 in 00e2198

.PHONY: test
test:
python3 -c "import pytest" > /dev/null 2>&1 || python3 -m pip install pytest
python3 -m pytest -qq
.PHONY: test-p
test-p:
python3 -c "import xdist" > /dev/null 2>&1 || python3 -m pip install pytest-xdist
python3 -m pytest -qq -n auto

@hugovk
Copy link
Member Author

hugovk commented Dec 23, 2025

Removed.

@hugovk
Copy link
Member Author

hugovk commented Dec 23, 2025

Failure on a macOS wheel build:

  =================================== FAILURES ===================================
  _________________________ TestAvifLeaks.test_leak_load _________________________
  [gw2] darwin -- Python 3.10.11 /private/var/folders/bp/kmfmhnl95kx1c8x321z7twbw0000gn/T/cibw-run-qf3svagk/cp310-macosx_arm64/venv-test-arm64/bin/python3
  
  self = <Tests.test_file_avif.TestAvifLeaks object at 0x10511e4a0>
  
      @pytest.mark.skipif(
          is_docker_qemu(), reason="Skipping on cross-architecture containers"
      )
      def test_leak_load(self) -> None:
          with open(TEST_AVIF_FILE, "rb") as f:
              im_data = f.read()
      
          def core() -> None:
              with Image.open(BytesIO(im_data)) as im:
                  im.load()
              gc.collect()
      
  >       self._test_leak(core)
  
  Tests/test_file_avif.py:793: 
  _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
  
  self = <Tests.test_file_avif.TestAvifLeaks object at 0x10511e4a0>
  core = <function TestAvifLeaks.test_leak_load.<locals>.core at 0x1063fd900>
  
      def _test_leak(self, core: Callable[[], None]) -> None:
          start_mem = self._get_mem_usage()
          for cycle in range(self.iterations):
              core()
              mem = self._get_mem_usage() - start_mem
              msg = f"memory usage limit exceeded in iteration {cycle}"
  >           assert mem < self.mem_limit, msg
  E           AssertionError: memory usage limit exceeded in iteration 0
  E           assert 14304.0 < 9216
  E            +  where 9216 = <Tests.test_file_avif.TestAvifLeaks object at 0x10511e4a0>.mem_limit
  
  Tests/helper.py:249: AssertionError

Seems 3d09b1e isn't enough for that.

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