From 5ac52f56700213098a8443be2c41f47401e6b925 Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Wed, 23 Sep 2026 18:00:39 -0400 Subject: [PATCH] ci(unit-tests): parallelise pytest with xdist and drop 3.15 from the blocking matrix Contention, not job duration, was the bottleneck (#703). Changes: - Add pytest-xdist>=3.6.1 (not >=3: 3.0.2-3.5.0 lack the workerinput forwarding the fix below needs, and PyPI has yanked 3.6.0); run `-n auto --dist load` in all three pytest jobs. - Drop Python 3.15 from unit-tests.yml's blocking matrices; its compileall/import check moves to a schedule-only job in python-compatibility.yml. - Fix the tier guard's UsageError diagnostic under xdist (swallowed by a controller race, `assert not crashitem` in dsession.py) by setting session.shouldfail inside a worker before raising. - Close a second race: XdistSubprocessTests' probe module and SuiteIsCleanTests' filesystem scan can land in different xdist workers and overlap. tests/conftest.py adds a readers-writer flock mutex around both, without touching test_tier_guard.py. Verified: test_tier_guard.py and test_tier_guard_xdist.py pass serial and under xdist; the probe/scan race reproduced in 14/32 trials pre-fix, 0/139 post-fix over repeated runs. --- .github/workflows/lint.yml | 9 +- .github/workflows/python-compatibility.yml | 39 ++- .github/workflows/unit-tests.yml | 55 ++-- pyproject.toml | 6 + tests/conftest.py | 165 +++++++++++- tests/test_tier_guard_xdist.py | 293 +++++++++++++++++++++ 6 files changed, 531 insertions(+), 36 deletions(-) create mode 100644 tests/test_tier_guard_xdist.py diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml index a7db699d83..5f559d57df 100644 --- a/.github/workflows/lint.yml +++ b/.github/workflows/lint.yml @@ -74,10 +74,11 @@ jobs: steps: - uses: actions/checkout@v7 - # One interpreter, not a matrix. Unit Tests and Python Compatibility sweep - # 3.10-3.15 because they check that the package *runs* everywhere; these - # tools read the source, and running them five times over would produce - # five copies of the same findings for five times the runner cost. + # One interpreter, not a matrix. Unit Tests sweeps 3.10-3.14 and Python + # Compatibility sweeps the same plus a schedule-only 3.15 leg, because + # they check that the package *runs* everywhere; these tools read the + # source, and running them several times over would produce several + # copies of the same findings for several times the runner cost. # # 3.14 specifically, matching cron-vendor.yml, because it is the newest # non-experimental version in the test matrix and the version the recorded diff --git a/.github/workflows/python-compatibility.yml b/.github/workflows/python-compatibility.yml index d1ee883a1c..7ed0a80f6a 100644 --- a/.github/workflows/python-compatibility.yml +++ b/.github/workflows/python-compatibility.yml @@ -30,28 +30,53 @@ jobs: # and one a three-minute test run, with nothing to tell them apart. name: Compat Python ${{ matrix.python-version }} runs-on: ubuntu-latest - continue-on-error: ${{ matrix.experimental == true }} strategy: fail-fast: false matrix: python-version: + # 3.15 deliberately excluded: ruleset 23497679's required checks + # only cover 3.10-3.14, so it cannot block a merge here either. Its + # replacement is the `compatibility-nightly` job below, which runs + # only on this workflow's existing weekly schedule. - "3.10" - "3.11" - "3.12" - "3.13" - "3.14" - experimental: - - false - include: - - python-version: "3.15" - experimental: true steps: - uses: actions/checkout@v7 - uses: actions/setup-python@v7 with: python-version: ${{ matrix.python-version }} - allow-prereleases: ${{ matrix.experimental }} + + - name: Install package + run: | + python -m pip install -U pip setuptools wheel + python -m pip install -e . + + - name: Verify package + run: | + python -m compileall -q pcapkit + python -c 'import pcapkit; print(pcapkit.__version__)' + + # 3.15 is not in ruleset 23497679's required-checks list, so running it on + # every push/PR was queue contention with no gating signal (it was already + # `continue-on-error`). This job keeps early warning of a 3.15-only + # regression -- import and compileall only, ~14s -- without paying for it on + # every push: it fires solely on the schedule trigger above (line 9). + compatibility-nightly: + name: Compat Python 3.15 (scheduled) + if: ${{ github.event_name == 'schedule' }} + runs-on: ubuntu-latest + continue-on-error: true + steps: + - uses: actions/checkout@v7 + + - uses: actions/setup-python@v7 + with: + python-version: "3.15" + allow-prereleases: true - name: Install package run: | diff --git a/.github/workflows/unit-tests.yml b/.github/workflows/unit-tests.yml index 4a495ac185..5325fa99f8 100644 --- a/.github/workflows/unit-tests.yml +++ b/.github/workflows/unit-tests.yml @@ -50,22 +50,20 @@ jobs: name: Python ${{ matrix.python-version }} if: ${{ inputs.gate-only != true }} runs-on: ubuntu-latest - continue-on-error: ${{ matrix.experimental == true }} timeout-minutes: 45 strategy: fail-fast: false matrix: python-version: + # 3.15 deliberately excluded: ruleset 23497679's required checks + # only cover 3.10-3.14, so a 3.15 leg here cannot block a merge and + # was pure queue contention -- see python-compatibility.yml for its + # (non-blocking, schedule-only) replacement. - "3.10" - "3.11" - "3.12" - "3.13" - "3.14" - experimental: - - false - include: - - python-version: "3.15" - experimental: true steps: - uses: actions/checkout@v7 @@ -73,7 +71,6 @@ jobs: - uses: actions/setup-python@v7 with: python-version: ${{ matrix.python-version }} - allow-prereleases: ${{ matrix.experimental }} cache: pip - name: Install package and test dependencies @@ -81,9 +78,21 @@ jobs: python -m pip install -U pip setuptools wheel python -m pip install -e '.[test]' + # `-n auto` resolves to whatever this runner reports, and nothing else in + # the log says what that was -- print it so the number `-n auto` picked + # is a measured fact instead of an assumption about runner size. + - name: Report available parallelism + run: | + nproc + python -c "import os; print('cpu_count', os.cpu_count())" + - name: Run unit tests + # `--dist load` is pinned explicitly rather than left to default: a + # future change to xdist's own default, or someone "optimising" to + # `loadfile`/`loadscope`, cannot silently change how this suite is + # distributed. run: >- - python -m pytest -q + python -m pytest -q -n auto --dist load --ignore=tests/integration --ignore-glob='*_runtime.py' --ignore-glob='*_regression.py' @@ -92,22 +101,17 @@ jobs: name: Integration Python ${{ matrix.python-version }} if: ${{ inputs.gate-only != true }} runs-on: ubuntu-latest - continue-on-error: ${{ matrix.experimental == true }} timeout-minutes: 45 strategy: fail-fast: false matrix: python-version: + # See the `test` job above for why 3.15 is excluded here. - "3.10" - "3.11" - "3.12" - "3.13" - "3.14" - experimental: - - false - include: - - python-version: "3.15" - experimental: true steps: - uses: actions/checkout@v7 @@ -115,7 +119,6 @@ jobs: - uses: actions/setup-python@v7 with: python-version: ${{ matrix.python-version }} - allow-prereleases: ${{ matrix.experimental }} cache: pip # The Scapy extra is there for examples/generators/pcap.py and legacy.py, @@ -152,6 +155,12 @@ jobs: echo "Upstream Wireshark captures were used, matching their pinned SHA-256 digests." fi + # See the `test` job above for why this step exists. + - name: Report available parallelism + run: | + nproc + python -c "import os; print('cpu_count', os.cpu_count())" + # The ``test`` job above already ran every unit-tier test for real, on this # same commit. Re-running them here (a bare ``pytest -q`` used to) bought # no signal and cost ~21.75 minutes per Python version -- ~130 minutes per @@ -167,7 +176,8 @@ jobs: # A failure to compute the selection intentionally does not fall back to # a bare `pytest -q` -- that would silently re-introduce the whole-suite # duplication this change exists to remove, with nothing in the log to - # say so. + # say so. `--dist load` is pinned explicitly; see the `test` job above + # for why. - name: Run full test suite shell: bash run: | @@ -180,7 +190,7 @@ jobs: exit 1 fi echo "Fixture-dependent selection: $selection" - python -m pytest -q $selection + python -m pytest -q -n auto --dist load $selection # ``CHANGELOG.md`` is generated from the newest entry under # ``docs/source/changelog/`` by ``util/changelog_md.py``, so it falls out of step @@ -262,6 +272,12 @@ jobs: echo "Upstream Wireshark captures were used, matching their pinned SHA-256 digests." fi + # See the `test` job above for why this step exists. + - name: Report available parallelism + run: | + nproc + python -c "import os; print('cpu_count', os.cpu_count())" + # Deliberately still the whole suite, unlike the `integration` job above: # this is the shipping gate's own independent re-verification (see the # job comment above), not the same accidental duplication #715 measured @@ -272,6 +288,7 @@ jobs: # Whether `gate` should run at all on a commit the matrix already # covered is #715's separate item 2 ("stop the gate re-running on main # pushes"), a trigger-frequency question, not a selection one -- out of - # scope here. + # scope here. `--dist load` is pinned explicitly; see the `test` job + # above for why. - name: Run full test suite - run: python -m pytest -q + run: python -m pytest -q -n auto --dist load diff --git a/pyproject.toml b/pyproject.toml index ffc4154080..f07d6352b3 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -233,6 +233,12 @@ docs = [ ] test = [ "pytest>=8", + # >=3.6.1, not >=3: the `shouldfail` forwarding tests/conftest.py's xdist fix + # depends on (xdist/remote.py:146-147) lands in 3.6.0, which PyPI has yanked, + # so 3.6.1 is the actual floor. Below it the fix silently reverts and + # test_xdist_run_reports_the_diagnostic_and_fails goes red rather than + # skipped, since `importlib.util.find_spec('xdist')` still succeeds. + "pytest-xdist>=3.6.1", "typing-extensions", # Enough of the ``vendor`` extra for a test to import a vendor crawler. # Without these, ``tests/vendor/test_ipx_socket_unit.py`` reports "9 skipped" diff --git a/tests/conftest.py b/tests/conftest.py index cd763c0914..d0be3dfebf 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -1,9 +1,9 @@ # -*- coding: utf-8 -*- """Suite-wide :program:`pytest` configuration. -Holds two things. The first is :func:`restore_module_table`, the guard that puts -the :mod:`pcapkit` region of :data:`sys.modules` back after every test; see its -own docstring for why it is here rather than left to each test file. +Holds three things. The first is :func:`restore_module_table`, the guard that +puts the :mod:`pcapkit` region of :data:`sys.modules` back after every test; see +its own docstring for why it is here rather than left to each test file. The second is the collection-time half of the tier guard described in :mod:`tests._tiers`. Every unit-tier module about to be run is read and checked @@ -24,11 +24,22 @@ is a literal, while the runtime one sees any name however it was computed but only when the call is reached. +The third is :func:`_tier_guard_lock` and :func:`_tier_guard_reader_lock`, a +readers-writer mutex that keeps +:class:`tests.test_tier_guard.SuiteIsCleanTests`'s live filesystem scan from +observing :class:`tests.test_tier_guard_xdist.XdistSubprocessTests`'s +deliberately-violating probe module while it is momentarily on disk. See +:func:`_tier_guard_lock`'s docstring for the race and why a mutex is what closes +it. + """ from __future__ import annotations +import contextlib +import hashlib import importlib import pathlib +import tempfile import types import warnings from typing import TYPE_CHECKING @@ -36,9 +47,14 @@ import pytest from tests._support import ISOLATED_PREFIXES, restore_modules, snapshot_modules -from tests._tiers import (TierGuardWarning, audit_module, guard_unavailable_reason, +from tests._tiers import (ROOT, TierGuardWarning, audit_module, guard_unavailable_reason, is_unit_tier) +try: + import fcntl +except ImportError: # pragma: no cover -- POSIX-only + fcntl = None # type: ignore[assignment] + if TYPE_CHECKING: from typing import Iterable, Iterator, Optional @@ -264,7 +280,7 @@ def _audit(items: 'Iterable[pytest.Item]') -> 'list[str]': return findings -def pytest_collection_modifyitems(config: 'pytest.Config', +def pytest_collection_modifyitems(session: 'pytest.Session', config: 'pytest.Config', items: 'list[pytest.Item]') -> 'None': """Refuse to run a unit-tier module that depends on a generated fixture.""" reason = None # type: Optional[str] @@ -293,7 +309,144 @@ def pytest_collection_modifyitems(config: 'pytest.Config', return if findings: - raise pytest.UsageError( + message = ( f'{len(findings)} read(s) of a generated sample capture from a unit-tier test ' f'module; see tests/_tiers.py for the tier rule.\n\n' + '\n\n'.join(findings) ) + # Under pytest-xdist this hook runs inside each worker's own nested + # session (xdist/remote.py), and that session's `pytest_collection_finish` + # fires from a `finally:` regardless of the UsageError below, so the + # controller sees a normal collection and may schedule a test to a worker + # that is already unwinding. The controller then reports that test as + # crashed -- `assert not crashitem` in xdist/dsession.py -- swallowing + # this message and printing an INTERNALERROR instead. Measured against + # pytest-xdist 3.8.0/pytest 9.1.1: setting `shouldfail` first is what the + # controller already knows how to shut a worker down on cleanly, no + # crash-item assertion, and the message still comes through -- as + # ``xdist.dsession.Interrupted: `` rather than ``ERROR: + # ``, which is the one visible difference from the serial path. + # A no-op outside a worker, so serial behaviour is unchanged. + if hasattr(config, 'workerinput'): + session.shouldfail = message + raise pytest.UsageError(message) + + +#: Reserved name for the probe module +#: :class:`tests.test_tier_guard_xdist.XdistSubprocessTests` writes to disk to +#: reproduce a real tier violation under a real :program:`pytest` run. Defined +#: here rather than duplicated as a literal in that module because +#: :func:`_tier_guard_reader_lock` below has to recognise it without importing +#: that module: the nested :program:`pytest` subprocess the probe test spawns +#: loads this conftest but never collects ``test_tier_guard_xdist.py`` at all. +PROBE_MODULE_NAME = 'test_zzz_xdist_guard_probe_unit.py' + +#: Module names whose own tests manage :func:`_tier_guard_lock` themselves and +#: must not also take the shared hold :func:`_tier_guard_reader_lock` puts on +#: every other unit-tier test. +#: +#: :data:`PROBE_MODULE_NAME` is excluded because it runs *inside* the writer's +#: exclusive hold by construction (that hold is what protects it), so taking a +#: shared one too would be a self-deadlock: the same lock file, opened twice by +#: the one process tree that also has to wait on itself to finish before either +#: side would let go. ``test_tier_guard_xdist.py`` is excluded for the same +#: reason -- :class:`~tests.test_tier_guard_xdist.XdistSubprocessTests` takes +#: the exclusive hold directly in its own ``setUp``, so the generic shared hold +#: below would collide with it in exactly the same way. +#: +#: This covers every test that manages the lock *directly*. It does not +#: generalise to a unit-tier test that spawns its own nested ``pytest`` +#: subprocess while holding the outer shared hold -- +#: :mod:`tests.project.test_module_isolation` is exactly such a test, and if +#: its selection ever grew to include this module or the probe, its outer +#: shared hold (spanning the whole test, including the blocking +#: ``subprocess.run``) would wait on an inner exclusive request that cannot be +#: granted until the outer test finishes: the same deadlock shape as above, one +#: level removed. Today it never selects either, so nothing here triggers it; +#: a future change to that selection should keep it that way, or add the +#: module in question here too. +_LOCK_MANAGED_ELSEWHERE = frozenset({PROBE_MODULE_NAME, 'test_tier_guard_xdist.py'}) + + +def _tier_guard_lock_path() -> 'pathlib.Path': + """Where the probe/scan mutex lives, one file per checkout. + + Keyed by :data:`tests._tiers.ROOT` rather than a fixed name, so two + worktrees of this repository each running their own ``-n auto`` session on + the same host never share a lock file neither of them needs shared. + + """ + digest = hashlib.sha1(str(ROOT).encode('utf-8')).hexdigest()[:16] + return pathlib.Path(tempfile.gettempdir()) / f'pcapkit-tier-guard-probe-{digest}.lock' + + +@contextlib.contextmanager +def _tier_guard_lock(exclusive: 'bool') -> 'Iterator[None]': + """Hold the probe/scan mutex, shared or exclusive, for the block's duration. + + :class:`~tests.test_tier_guard.SuiteIsCleanTests` walks every unit-tier + module on disk with :meth:`pathlib.Path.rglob`, which is sound against a + committed tree but not against a concurrent write: + :class:`~tests.test_tier_guard_xdist.XdistSubprocessTests` deliberately + writes a real violation to disk, under :data:`PROBE_MODULE_NAME`, to + reproduce the tier guard's own collection-time diagnostic end to end. + Serially the two never overlap; under the ``-n auto --dist load`` this + suite's CI jobs now use, pytest-xdist may hand the two tests to different + worker processes at the same moment, and the scan can then observe the + probe mid-life and report a violation nobody committed -- a real defect in + the test suite's own concurrency, not a false positive, since the guard is + doing exactly what its docstring says: looking at everything on disk. + + The fix is an ordinary readers-writer mutex over one lock file every + process opens independently: :class:`XdistSubprocessTests` holds it + exclusively for as long as the probe module sits on disk (see its + ``setUp``), and :func:`_tier_guard_reader_lock` below holds it shared, for + the duration of the test, on behalf of every other unit-tier test -- + including :class:`SuiteIsCleanTests`, without that file having to change at + all. :func:`fcntl.flock` enforces shared-vs-exclusive across processes as + well as threads of one process, which is what makes the two truly unable to + overlap rather than merely unlikely to. + + A no-op wherever :data:`fcntl` is :data:`None` -- true only off POSIX, where + nothing in this suite's CI turns ``-n auto`` on either (every job that does + runs ``ubuntu-latest``; see :file:`.github/workflows/unit-tests.yml` and + :file:`.github/workflows/python-compatibility.yml`), so a platform this + cannot protect is exactly as exposed to the race as it always was, not more. + + Args: + exclusive: :data:`True` for the writer's hold, :data:`False` for a + reader's. + + """ + if fcntl is None: + yield + return + path = _tier_guard_lock_path() + with open(path, 'a', encoding='utf-8') as handle: + fcntl.flock(handle.fileno(), fcntl.LOCK_EX if exclusive else fcntl.LOCK_SH) + try: + yield + finally: + fcntl.flock(handle.fileno(), fcntl.LOCK_UN) + + +@pytest.fixture(autouse=True) +def _tier_guard_reader_lock(request: 'pytest.FixtureRequest') -> 'Iterator[None]': + """Hold the probe/scan mutex shared, for every unit-tier test but the two + that self-manage it. + + See :func:`_tier_guard_lock` for the race this closes. Deciding by + :func:`tests._tiers.is_unit_tier` rather than by this one test's name is + deliberate: every unit-tier test is a potential + :class:`~tests.test_tier_guard.SuiteIsCleanTests`, so the protection is + structural and covers a similarly-scanning test written later without this + fixture changing at all -- and it never has to touch that module's own + source to do it. + + """ + location = getattr(request.node, 'path', None) or getattr(request.node, 'fspath', None) + path = pathlib.Path(str(location)) if location is not None else None + if path is None or path.name in _LOCK_MANAGED_ELSEWHERE or not is_unit_tier(path): + yield + return + with _tier_guard_lock(exclusive=False): + yield diff --git a/tests/test_tier_guard_xdist.py b/tests/test_tier_guard_xdist.py new file mode 100644 index 0000000000..2b78cbce1d --- /dev/null +++ b/tests/test_tier_guard_xdist.py @@ -0,0 +1,293 @@ +# -*- coding: utf-8 -*- +"""The tier guard's diagnostic survives running under pytest-xdist. + +:func:`tests.conftest.pytest_collection_modifyitems` raises :exc:`pytest.UsageError` +with a detailed diagnostic when a unit-tier module reads a generated capture. Under +``-n auto --dist load`` that message used to be lost entirely: each xdist worker +runs this hook inside its own nested :program:`pytest` session +(:mod:`xdist.remote`), and that session's ``pytest_collection_finish`` fires from a +``finally:`` regardless of the ``UsageError``, so the controller sees what looks +like a normal collection and may schedule a test to a worker that is already +unwinding. The controller then reports that test as crashed -- +``assert not crashitem`` in :mod:`xdist.dsession` -- which prints an +``INTERNALERROR`` instead of the diagnostic and describes the violation as a +worker crash rather than as the tier-guard failure it is. + +The fix is to set ``session.shouldfail`` before raising, but only inside a worker +(``hasattr(config, 'workerinput')``): that is the signal the controller already +knows how to shut a worker down on cleanly, with no crash-item assertion, and it is +a no-op outside a worker so the serial path is untouched. + +:class:`CollectionModifyItemsXdistTests` pins the hook's own logic directly and +runs unconditionally. :class:`XdistSubprocessTests` reproduces the original bug +end-to-end with a real violation and a real ``pytest -n auto --dist load`` +invocation, and skips when :mod:`xdist` is not importable -- the repository venv +this suite normally runs under deliberately has no xdist installed. + +A second, independent race lives at the same address. :mod:`tests.test_tier_guard`'s +``SuiteIsCleanTests`` walks the whole :file:`tests/` tree looking for exactly the +kind of violation :class:`XdistSubprocessTests` deliberately writes to disk; under +``-n auto --dist load`` pytest-xdist may run the two in different worker processes +at the same moment, and the scan then reports a violation nobody committed -- +measured on a real CI run, ``Integration Python 3.13``, one leg in five, 466s in. +:func:`tests.conftest._tier_guard_lock` closes it with a readers-writer mutex: +:class:`XdistSubprocessTests` below takes the exclusive hold in ``setUp`` for as +long as the probe sits on disk, and :func:`tests.conftest._tier_guard_reader_lock` +takes the shared hold on behalf of every other unit-tier test, including +``SuiteIsCleanTests``, without that module's own source changing at all. +:class:`ProbeWriteLockTests` pins the mutex itself, independently of xdist. + +Kept separate from :mod:`tests.test_tier_guard`, which another change is already +modifying: this module touches none of its code, only :mod:`tests.conftest`. + +""" +from __future__ import annotations + +import importlib.util +import os +import subprocess +import sys +import textwrap +import threading +import unittest +import unittest.mock + +from tests import _tiers, conftest as _conftest + +#: Capture name the probe module in :class:`XdistSubprocessTests` reads. Not +#: tracked by git -- see :func:`tests._tiers.committed_captures` -- so it is a +#: tier violation regardless of what ``make samples`` has built locally. +_GENERATED_CAPTURE = 'xdist_probe_generated.pcap' + +#: Diagnostic text that must survive however the suite is run, per +#: :func:`tests.conftest.pytest_collection_modifyitems`. +_DIAGNOSTIC_MARKER = 'read(s) of a generated sample capture' + + +class _FakeSession: + """Stands in for :class:`pytest.Session`; the hook touches only this attribute.""" + + def __init__(self) -> None: + self.shouldfail = False # type: 'bool | str' + + +class _FakeConfig: + """Stands in for :class:`pytest.Config`. + + ``workerinput`` is set only by :mod:`xdist.remote` inside an actual worker + process, and its mere presence -- never its content -- is what the hook + checks; see ``xdist/remote.py``'s bootstrap, which does + ``config.workerinput = workerinput``. + + """ + + def __init__(self, is_worker: bool) -> None: + if is_worker: + self.workerinput = {'workerid': 'gw0'} # type: dict[str, str] + + +class CollectionModifyItemsXdistTests(unittest.TestCase): + """:func:`tests.conftest.pytest_collection_modifyitems` against a fake worker.""" + + def _findings(self, session: '_FakeSession', config: '_FakeConfig') -> 'str': + """Run the hook with one synthetic finding and return the raised message. + + ``_audit`` and ``guard_unavailable_reason`` are patched so the outcome + depends only on the ``if findings:`` branch under test, never on whether + this checkout's own git state happens to be clean. + + """ + with unittest.mock.patch.object(_conftest, 'guard_unavailable_reason', + return_value=None), \ + unittest.mock.patch.object(_conftest, '_audit', + return_value=['a synthetic finding']): + with self.assertRaises(Exception) as caught: + _conftest.pytest_collection_modifyitems( + session=session, config=config, items=[]) + self.assertIsInstance(caught.exception, Exception) + return str(caught.exception) + + def test_worker_context_sets_shouldfail_before_raising(self) -> None: + """Inside a worker, the diagnostic is also stashed on ``session.shouldfail``. + + Fails on the code before this change: ``session.shouldfail`` stays at its + initial :data:`False` because nothing ever assigns to it. + + """ + session = _FakeSession() + message = self._findings(session, _FakeConfig(is_worker=True)) + + self.assertEqual(session.shouldfail, message) + self.assertIn(_DIAGNOSTIC_MARKER, message) + self.assertIn('a synthetic finding', message) + + def test_serial_context_leaves_shouldfail_untouched(self) -> None: + """Outside a worker, behaviour is exactly what it was before this change.""" + session = _FakeSession() + message = self._findings(session, _FakeConfig(is_worker=False)) + + self.assertIs(session.shouldfail, False) + self.assertIn(_DIAGNOSTIC_MARKER, message) + + +def _child_environ() -> 'dict[str, str]': + """The environment a subprocess :program:`pytest` run should inherit. + + Same rationale as :func:`tests.project.test_module_isolation.child_environ`: + the child must import the tree this test runs from, and must not inherit this + process's own pytest session variables. + + """ + environ = dict(os.environ) + environ['PYTHONPATH'] = os.pathsep.join( + [str(_tiers.ROOT), environ['PYTHONPATH']] if environ.get('PYTHONPATH') + else [str(_tiers.ROOT)] + ) + environ.pop('PYTEST_ADDOPTS', None) + environ.pop('PYTEST_CURRENT_TEST', None) + return environ + + +@unittest.skipUnless(importlib.util.find_spec('xdist') is not None, + 'pytest-xdist not installed') +class XdistSubprocessTests(unittest.TestCase): + """End-to-end reproduction: a real violation, a real ``pytest`` subprocess. + + Skipped whenever the interpreter running this suite has no ``xdist`` to + import -- true of the repository's own venv by design, so this class + contributes nothing there and everything under a venv that has xdist + installed, such as the one used to verify this fix. + + """ + + #: Sorts late and is unambiguously a probe, in case a stray copy is ever left + #: behind by an interrupted run. Kept as an alias of + #: :data:`tests.conftest.PROBE_MODULE_NAME` rather than a second literal -- + #: that name is what tells :func:`tests.conftest._tier_guard_reader_lock` to + #: leave this class's own writes unlocked, and the two must never drift. + PROBE_NAME = _conftest.PROBE_MODULE_NAME + + def setUp(self) -> None: + reason = _tiers.guard_unavailable_reason() + if reason is not None: + self.skipTest(f'git cannot answer here: {reason}') + + self.probe_path = _tiers.TESTS_ROOT / 'protocols' / self.PROBE_NAME + self.assertFalse( + self.probe_path.exists(), + f'{self.probe_path} already exists -- a previous run did not clean up', + ) + + # Exclusive for as long as the probe sits on disk: see + # tests.conftest._tier_guard_lock for the SuiteIsCleanTests race this + # closes under -n auto --dist load. Entered manually, rather than with + # a `with` block around the rest of setUp, because the hold has to + # outlive this method and span the test body and cleanup too; released + # only after the probe is gone -- addCleanup runs LIFO, so registering + # the release first and the unlink second is what makes the unlink run + # while still holding the lock and the release run after it. + lock = _conftest._tier_guard_lock(exclusive=True) + lock.__enter__() + self.addCleanup(lock.__exit__, None, None, None) + + source = textwrap.dedent(f""" + import unittest + + from tests._support import sample_path + + + class ProbeTierViolationTests(unittest.TestCase): + def test_reads_a_generated_capture_with_no_handler(self) -> None: + self.assertTrue(sample_path({_GENERATED_CAPTURE!r})) + """).lstrip('\n') + self.probe_path.write_text(source, encoding='utf-8') + self.addCleanup(self.probe_path.unlink, missing_ok=True) + + def _run(self, *extra_args: str) -> 'subprocess.CompletedProcess[str]': + selection = str(self.probe_path.relative_to(_tiers.ROOT)) + return subprocess.run( + [sys.executable, '-m', 'pytest', '-p', 'no:cacheprovider', '-q', + *extra_args, selection], + cwd=str(_tiers.ROOT), env=_child_environ(), capture_output=True, + text=True, timeout=120, check=False, + ) + + def test_serial_run_reports_the_diagnostic_and_fails(self) -> None: + completed = self._run() + output = completed.stdout + completed.stderr + + self.assertNotEqual(completed.returncode, 0) + self.assertEqual(output.count(_DIAGNOSTIC_MARKER), 1) + self.assertNotIn('INTERNALERROR', output) + + def test_xdist_run_reports_the_diagnostic_and_fails(self) -> None: + """The reproduction this change exists for. + + Fails on the code before this change: the run exits non-zero from an + ``INTERNALERROR`` (``assert not crashitem`` in ``xdist/dsession.py``, + naming the probe's test id) and ``_DIAGNOSTIC_MARKER`` never appears. + + """ + completed = self._run('-n', '2', '--dist', 'load') + output = completed.stdout + completed.stderr + + self.assertNotEqual(completed.returncode, 0) + self.assertGreaterEqual(output.count(_DIAGNOSTIC_MARKER), 1) + self.assertNotIn('INTERNALERROR', output) + self.assertNotIn('crashitem', output) + + +class ProbeWriteLockTests(unittest.TestCase): + """:func:`tests.conftest._tier_guard_lock` actually excludes a concurrent reader. + + Independent of :mod:`xdist` and of the two classes above: it exercises the + mutex directly, with a background thread standing in for a concurrent + worker process. :func:`fcntl.flock` locks are per open file description, + not per process or thread, so a conflict between two file descriptors + opened by two threads of one process is the same conflict pytest-xdist's + separate worker processes would see -- this is not a weaker stand-in for + the real race, it is the identical kernel mechanism at smaller scale. + + Runs unconditionally in the venv this suite normally runs under: unlike + :class:`XdistSubprocessTests`, nothing here needs ``xdist`` importable, only + :mod:`fcntl`, so this is the case that actually exercises the mutex on the + interpreter without ``xdist`` installed. + + """ + + @unittest.skipUnless(_conftest.fcntl is not None, + 'fcntl unavailable on this platform') + def test_a_concurrent_reader_waits_out_the_writer(self) -> None: + """A shared hold cannot be granted while the exclusive one is live. + + Fails on the code before this change: :func:`tests.conftest._tier_guard_lock` + does not exist yet, so there is nothing to import and nothing to wait on. + + """ + reader_acquired = threading.Event() + reader_errors = [] # type: list[BaseException] + + def reader() -> None: + try: + with _conftest._tier_guard_lock(exclusive=False): + reader_acquired.set() + except BaseException as exc: # pylint: disable=broad-except + reader_errors.append(exc) + + with _conftest._tier_guard_lock(exclusive=True): + thread = threading.Thread(target=reader) + thread.start() + # Every chance for the reader to have raced ahead if the writer's + # hold did not actually exclude it. + got_it_early = reader_acquired.wait(timeout=0.5) + self.assertFalse( + got_it_early, + 'a concurrent reader acquired the shared lock while the writer still held it', + ) + + thread.join(timeout=5) + self.assertFalse(reader_errors, reader_errors) + self.assertTrue( + reader_acquired.wait(timeout=5), + 'the reader never acquired the lock after the writer released it', + )