From 966ffbfd5f440ff759f904da115871725d59b5d7 Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Tue, 15 Sep 2026 03:40:53 -0400 Subject: [PATCH] tests: refuse a generated sample capture from the unit tier Three PRs so far (#372, #384, and one before) shipped a unit-tier test that read a *generated* capture. Each passed locally, because the developer had already run `make samples`, and each failed in CI on a fresh checkout with a bare FileNotFoundError -- an error that blames a missing file rather than the tier rule that was broken, on a test that looks perfectly correct. The guard decides on the calling module's **tier**, never on whether the file happens to be present, so it fires on the machine that made the mistake instead of on the next fresh clone. Two layers, because each covers the other's blind spot: * collection time, in tests/conftest.py: every unit-tier module is AST-parsed and a `sample_path('literal')` naming an untracked capture aborts the session. This sees violations in tests that never execute -- one skipped for a missing optional engine would otherwise hide indefinitely. * call time, inside sample_path(): the caller's module is read out of the frame, so no test declares its own tier and none can declare it wrongly. This catches computed names, e.g. `sample_path(name)` over a list, which no static pass can. Committedness is asked of git (`git ls-files`), not hardcoded. Writing the list by hand would have been wrong immediately: examples/captures/ has **six** tracked files, not the two captures one assumes -- out.json, out.plist, out.txt and pcapng.txt are tracked too -- and it would rot the moment a third capture lands. GeneratedFixtureInUnitTierError subclasses FileNotFoundError so that anything already handling an absent capture keeps working unchanged. That is also the documented opt-out, and it is the idiom the suite already uses at tests/toolkit/test_dpkt_unit.py:438 and :693: a `sample_path()` call inside a `try` whose handler catches a missing file is tier-safe by construction and is left alone. So this needed no edits to existing tests and loses no coverage -- those two still run when the fixtures are there. Without git -- an unpacked sdist, say -- the guard warns once and no-ops rather than failing a run that is probably fine. Verified: a planted unit-tier read of a generated capture exits 4 with the fixture absent *and* with it present on disk; the computed-name form exits 1. The real unit tier is silent -- 478 passed with fixtures present, 475 passed and 3 skipped without, both exit 0, no guard output either way. Fixture-dependent tiers are untouched: tests/integration plus a *_runtime.py, 74 passed, 4 skipped. The guard's own 24 tests pass on 3.14 and on 3.10, the CI floor. Known gap, documented in the module: a test that builds the path by hand instead of calling sample_path() is invisible to both layers, as tests/protocols/misc/test_pcapng_unit.py:2924 does. It is tier-safe there (it checks isfile and skips), and the heuristic needed to catch that shape safely would risk failing correct code, so it is deliberately left out. --- tests/_support.py | 34 ++- tests/_tiers.py | 573 +++++++++++++++++++++++++++++++++++++++ tests/conftest.py | 102 +++++++ tests/test_tier_guard.py | 408 ++++++++++++++++++++++++++++ 4 files changed, 1114 insertions(+), 3 deletions(-) create mode 100644 tests/_tiers.py create mode 100644 tests/conftest.py create mode 100644 tests/test_tier_guard.py diff --git a/tests/_support.py b/tests/_support.py index 842a9c0c22..f1abf1b5aa 100644 --- a/tests/_support.py +++ b/tests/_support.py @@ -3,14 +3,14 @@ import abc import collections.abc import importlib.util +import inspect import pathlib import sys import types from typing import Iterable -ROOT = pathlib.Path(__file__).resolve().parents[1] -SAMPLE_ROOT = ROOT / 'examples' / 'captures' -REGENERATE_SAMPLES_CMD = 'python examples/generators/make_samples.py' +from tests._tiers import (ROOT, SAMPLE_ROOT, REGENERATE_SAMPLES_CMD, + GeneratedFixtureInUnitTierError, check_unit_tier_read) def sample_path(name: str) -> str: @@ -21,6 +21,12 @@ def sample_path(name: str) -> str: so that the location is recorded in exactly one place and so that the suite does not depend on the working directory :program:`pytest` was invoked from. + Going through one helper is also what makes the tier rule enforceable: this + is the single door onto :file:`examples/captures/`, so it is where a + unit-tier module reading a *generated* capture can be stopped. See + :mod:`tests._tiers` for the rule and why breaking it is otherwise invisible + until CI runs on a fresh checkout. + Args: name: Bare file name of the capture, e.g. ``'arp.pcap'`` -- not a path, and in particular not ``'sample/arp.pcap'``. @@ -34,11 +40,33 @@ def sample_path(name: str) -> str: already-open binary IO object. Raises: + GeneratedFixtureInUnitTierError: If a unit-tier module asked for a + capture git does not track, and the call site does not handle the + capture being absent. Raised whether or not the file is on disk, so + the mistake surfaces on the machine that made it rather than on the + next fresh checkout. FileNotFoundError: If the capture is not present. Most of the samples are generated rather than committed to the repository, so a fresh clone has to build them first. """ + # Where the call came from, which is what decides its tier. Read out of the + # calling frame rather than passed in, so that no test has to declare its own + # tier and none can get the declaration wrong. Both locals are dropped again + # straight away: a frame reachable from a local keeps the whole chain alive + # once a traceback references this frame, and this function raises. + frame = inspect.currentframe() + caller = frame.f_back if frame is not None else None + try: + module_path = caller.f_globals.get('__file__') if caller is not None else None + lineno = caller.f_lineno if caller is not None else None + finally: + del frame, caller + + problem = check_unit_tier_read(name, module_path, lineno) + if problem is not None: + raise GeneratedFixtureInUnitTierError(problem) + path = SAMPLE_ROOT / name if not path.is_file(): raise FileNotFoundError( diff --git a/tests/_tiers.py b/tests/_tiers.py new file mode 100644 index 0000000000..f55b253fb2 --- /dev/null +++ b/tests/_tiers.py @@ -0,0 +1,573 @@ +# -*- coding: utf-8 -*- +"""The test suite's tier rule, and the machinery that enforces it. + +The suite runs in two tiers, and the split is a property of a module's *path* +rather than of the command that happens to collect it: + +=================================== ========================================== +Tier Modules +=================================== ========================================== +unit everything under :file:`tests/` except the + fixture-dependent ones +fixture-dependent :file:`tests/integration/`, + :file:`*_runtime.py`, :file:`*_regression.py` +=================================== ========================================== + +The unit tier has to pass on a fresh clone with nothing installed but +``pip install -e '.[test]'``, which is exactly how the ``test`` job of +:file:`.github/workflows/unit-tests.yml` runs it -- see +:data:`UNIT_TIER_SELECTION` for the selection it uses. The fixture-dependent +tier runs only after :file:`examples/generators/make_samples.py` has rebuilt +:file:`examples/captures/`, so it may read any capture it likes. + +That leaves one way to break the unit tier which is invisible to the developer +who does it: read a *generated* capture from a unit-tier module. Only a handful +of the files under :file:`examples/captures/` are committed; the rest are built +on demand and gitignored. A machine that has run ``make samples`` has them all, +so the test passes locally and then fails on a fresh CI checkout with a +missing-file error that blames the fixture rather than the tier rule. It has +happened three times (GitHub pull requests #372 and #384, and once more), and +each time it cost a CI round-trip to work out. + +So this module answers, cheaply and without needing the fixtures themselves: + +* :func:`is_unit_tier` -- which tier does this module belong to? +* :func:`committed_captures` -- which captures does *git* track? Asked of git + rather than hardcoded, because a hardcoded list of names silently rots the + moment somebody commits another capture (there are six today, not the two + the rule started with). +* :func:`audit_module` -- does this module read a generated capture without + handling its absence? +* :func:`check_unit_tier_read` -- may this particular + :func:`~tests._support.sample_path` call go ahead? +* :func:`explain` -- the message that says what is wrong and what to do about + it. + +:file:`tests/conftest.py` drives the first four at collection time, and +:func:`tests._support.sample_path` consults :func:`check_unit_tier_read` on +every call. Both paths no-op when git cannot answer, so an unpacked source +tarball still runs its tests. + +The two halves cover each other. The collection-time audit is static, so it sees +a violation in a test that never runs -- one skipped for a missing optional +engine, say -- but only when the capture name is a string literal. The call-time +check sees the name however it was computed, e.g. ``sample_path(sample)`` in a +parametrised loop, but only once the call is reached. + +What neither catches, deliberately: a unit-tier module that opens a capture +without going through :func:`~tests._support.sample_path` at all. +:file:`tests/protocols/misc/test_pcapng_unit.py` does this today at the line +holding ``os.path.join('examples', 'captures', 'dhcp_big_endian.pcapng')``, and +it is tier-safe -- it checks :func:`os.path.isfile` and skips -- but it is +invisible here. Flagging that shape would mean recognising a second, +much woollier "the absence is handled" idiom on top of the ``try``/``except`` +one, and getting it wrong would fail correct code for everybody. So the rule +this module enforces is stated as it is: capture reads go through +:func:`~tests._support.sample_path`, and that is the door with the lock on it. + +Nothing here imports :mod:`tests._support` -- the dependency runs the other way, +and adding the reverse edge would make it a cycle. Nothing here imports +:mod:`pcapkit`, :mod:`pytest` or any optional engine either, so the guard is +usable from the earliest possible moment and cannot itself be the reason a +fresh clone fails. + +Nothing in this file is collected by :program:`pytest`: ``python_files`` in +:file:`pyproject.toml` is ``test_*.py``. + +""" +from __future__ import annotations + +import ast +import functools +import pathlib +import subprocess +from typing import TYPE_CHECKING, NamedTuple + +if TYPE_CHECKING: + from typing import Optional + +__all__ = [ + 'GeneratedFixtureInUnitTierError', 'TierGuardWarning', 'SampleCall', + 'ROOT', 'TESTS_ROOT', 'SAMPLE_ROOT', 'REGENERATE_SAMPLES_CMD', 'UNIT_TIER_SELECTION', + 'FIXTURE_TIER_SUFFIXES', 'FIXTURE_TIER_DIRS', + 'is_unit_tier', 'committed_captures', 'committed_capture_names', + 'guard_unavailable_reason', 'handled_lines', 'sample_path_calls', + 'audit_module', 'check_unit_tier_read', 'explain', +] + +#: Repository root, i.e. the parent of the directory holding this file. The +#: single definition of it for the whole suite: :mod:`tests._support` imports it +#: from here rather than recomputing it. +ROOT = pathlib.Path(__file__).resolve().parents[1] +#: Directory holding the test suite, which is what tier membership is relative to. +TESTS_ROOT = ROOT / 'tests' +#: Directory holding the sample captures, committed and generated alike. +SAMPLE_ROOT = ROOT / 'examples' / 'captures' +#: Command that rebuilds every generated capture. ``make samples`` runs it too. +REGENERATE_SAMPLES_CMD = 'python examples/generators/make_samples.py' +#: The unit-tier selection, verbatim from the ``test`` job of +#: :file:`.github/workflows/unit-tests.yml`. Quoted in the failure message so the +#: reader can run exactly what CI runs; :func:`is_unit_tier` below is the +#: executable statement of the same rule, and the two have to agree. +UNIT_TIER_SELECTION = ( + "pytest tests --ignore=tests/integration " + "--ignore-glob='*_runtime.py' --ignore-glob='*_regression.py'" +) +#: Module file-name suffixes that put a module in the fixture-dependent tier, +#: matching the ``--ignore-glob`` patterns above. +FIXTURE_TIER_SUFFIXES = ('_runtime.py', '_regression.py') +#: Directories under :file:`tests/` that are fixture-dependent in their entirety, +#: matching the ``--ignore`` arguments above. +FIXTURE_TIER_DIRS = frozenset({'integration'}) +#: Suffixes that make a tracked file worth suggesting as a replacement capture. +#: Committedness itself is whatever git says -- this filter only keeps the +#: suggestion in :func:`explain` from offering :file:`out.txt` as a capture. +CAPTURE_SUFFIXES = ('.pcap', '.pcapng', '.cap') +#: Module quoted in :func:`explain` as the worked example of the skip idiom. +SKIP_IDIOM_EXAMPLE = 'tests/toolkit/test_dpkt_unit.py' +#: Exception names whose handler would catch a missing capture. The list is +#: deliberately generous: a false positive aborts the suite for everybody, +#: whereas a missed detection in the rare module that wraps a capture read in +#: ``except Exception`` costs nothing beyond this guard staying quiet. The +#: sanctioned idiom is ``except FileNotFoundError``; the rest are here so that +#: code which already handles the failure some other way is not flagged. +HANDLES_MISSING_FILE = frozenset({ + 'FileNotFoundError', 'OSError', 'IOError', 'EnvironmentError', + 'Exception', 'BaseException', +}) + +#: ``try`` statement node types. :class:`ast.TryStar` is 3.11+, and Python 3.10 +#: is the floor, so it is looked up rather than named. +_TRY_NODES = (ast.Try,) + ((ast.TryStar,) if hasattr(ast, 'TryStar') else ()) # type: ignore[attr-defined] + + +class GeneratedFixtureInUnitTierError(FileNotFoundError): + """A unit-tier module asked for a capture that only ``make samples`` writes. + + Subclasses :exc:`FileNotFoundError` on purpose. This guard is not allowed to + turn a read that would have been skipped into a hard error, so anything + already handling the missing-capture case keeps working; and the condition + genuinely is "that file is not available to you", so the hierarchy stays + honest. In practice the subclass is only ever raised at a call site with no + handler, which is what makes it a mistake rather than a tolerated read. + + """ + + +class TierGuardWarning(UserWarning): + """The tier guard could not run, or ran into something unexpected. + + A warning rather than an error: not being able to *check* the tier rule is + not a reason to fail a test run that may be perfectly fine. + + """ + + +class SampleCall(NamedTuple): + """One ``sample_path(...)`` call found in a module's source.""" + + #: Line the call starts on. + lineno: 'int' + #: The capture name, when it was spelled as a string literal; :data:`None` + #: when it is computed, e.g. ``sample_path(sample)`` in a parametrised loop. + name: 'Optional[str]' + #: Whether a missing capture would be handled at this call site, i.e. + #: whether the call sits in the body of a ``try`` that catches it. + handled: 'bool' + + +def is_unit_tier(path: 'pathlib.Path | str') -> 'bool': + """Whether ``path`` names a module in the unit tier. + + The executable statement of :data:`UNIT_TIER_SELECTION`: a module is + unit-tier when it lives under :file:`tests/`, not under one of + :data:`FIXTURE_TIER_DIRS`, and its name ends with none of + :data:`FIXTURE_TIER_SUFFIXES`. + + Deciding by path rather than by which ignore flags the current invocation + passed is deliberate. It makes the answer the same for ``pytest tests + --ignore=...`` as for a bare ``pytest``, so the full-suite run enforces the + rule as well -- and it does not depend on whether the fixtures happen to be + on disk, which is the whole point of the guard. + + Args: + path: Path to a module, absolute or relative to :data:`ROOT`. + + Returns: + :data:`True` for a unit-tier module, :data:`False` for a + fixture-dependent one and for anything outside :file:`tests/`. + + """ + candidate = pathlib.Path(path) + if not candidate.is_absolute(): + candidate = ROOT / candidate + + try: + relative = candidate.resolve().relative_to(TESTS_ROOT) + except ValueError: + return False + + if relative.name.endswith(FIXTURE_TIER_SUFFIXES): + return False + return FIXTURE_TIER_DIRS.isdisjoint(relative.parts[:-1]) + + +def _git(*args: 'str') -> 'Optional[str]': + """Run :program:`git` in :data:`ROOT` and return its standard output. + + Returns: + The output as text, or :data:`None` if git could not answer -- no + executable on :envvar:`PATH`, no repository, a non-zero exit, or a + hung invocation. Every one of those is a "cannot tell", never a + "not committed": guessing the other way would fail a source tarball's + test run for a rule it cannot possibly break. + + """ + try: + completed = subprocess.run( + ('git', *args), cwd=str(ROOT), stdout=subprocess.PIPE, + stderr=subprocess.DEVNULL, timeout=30, check=False, + ) + except (OSError, subprocess.SubprocessError): + return None + if completed.returncode != 0: + return None + return completed.stdout.decode('utf-8', 'surrogateescape') + + +@functools.lru_cache(maxsize=1) +def _git_state() -> 'tuple[Optional[frozenset[str]], Optional[str]]': + """The set of git-tracked capture names, or why it could not be determined. + + Cached for the process: one :program:`git` invocation for a whole test run, + however many call sites ask. + + Returns: + ``(names, None)`` on success, where ``names`` holds each tracked path + under :data:`SAMPLE_ROOT` relative to it, or ``(None, reason)``. + + """ + toplevel = _git('rev-parse', '--show-toplevel') + if toplevel is None: + return None, ( + f'git could not be run in {ROOT} -- either the executable is missing or this is ' + f'not a checkout, e.g. an unpacked source tarball' + ) + + # Guard against answering from the wrong repository: a tarball unpacked + # inside some other checkout would otherwise get that repository's index, + # which knows nothing about these captures and would call every one of them + # generated. A git worktree reports its own root here, so worktrees are fine. + resolved = pathlib.Path(toplevel.strip()).resolve() + if resolved != ROOT: + return None, ( + f'git reports its work tree as {resolved}, not {ROOT}, so its answers are about ' + f'some other repository' + ) + + relative_root = SAMPLE_ROOT.relative_to(ROOT).as_posix() + # -z rather than the default: without it git quotes and escapes paths that + # hold unusual bytes, and the names would have to be unquoted again. + listing = _git('ls-files', '-z', '--', relative_root) + if listing is None: + return None, f'`git ls-files` failed for {relative_root}' + + prefix = relative_root + '/' + names = { + entry[len(prefix):] for entry in listing.split('\0') + if entry and entry.startswith(prefix) + } + return frozenset(names), None + + +def committed_captures() -> 'Optional[frozenset[str]]': + """Names of the captures git tracks, relative to :data:`SAMPLE_ROOT`. + + Returns: + The tracked names, or :data:`None` when git could not be asked -- see + :func:`guard_unavailable_reason`. + + """ + return _git_state()[0] + + +def guard_unavailable_reason() -> 'Optional[str]': + """Why the guard cannot run, in one sentence, or :data:`None` when it can.""" + return _git_state()[1] + + +def committed_capture_names() -> 'tuple[str, ...]': + """Tracked captures worth suggesting as a replacement, sorted. + + Filtered to :data:`CAPTURE_SUFFIXES` so the suggestion in :func:`explain` + offers captures rather than the committed reference outputs that live in the + same directory. + + """ + tracked = committed_captures() or frozenset() + return tuple(sorted(name for name in tracked if name.endswith(CAPTURE_SUFFIXES))) + + +def _normalize(name: 'str') -> 'str': + """A capture name as git spells it, i.e. relative and slash-separated.""" + return pathlib.PurePath(name).as_posix() + + +def _is_committed(name: 'str', tracked: 'frozenset[str]') -> 'bool': + return _normalize(name) in tracked + + +def _exception_name(node: 'ast.expr') -> 'Optional[str]': + """The bare name of an exception spelled in an ``except`` clause.""" + if isinstance(node, ast.Name): + return node.id + if isinstance(node, ast.Attribute): + return node.attr + return None + + +def _handles_missing_file(handler: 'ast.ExceptHandler') -> 'bool': + """Whether ``handler`` would catch a missing capture.""" + if handler.type is None: # a bare `except:` catches everything + return True + clauses = handler.type.elts if isinstance(handler.type, ast.Tuple) else [handler.type] + return any(_exception_name(clause) in HANDLES_MISSING_FILE for clause in clauses) + + +def _parse(module_path: 'str') -> 'Optional[ast.Module]': + """Parse a module, or return :data:`None` if it cannot be read or parsed. + + An unreadable or unparseable module is not this guard's problem -- pytest + will report the syntax error far better than a warning from here would. + + """ + try: + source = pathlib.Path(module_path).read_text(encoding='utf-8') + except (OSError, UnicodeDecodeError): + return None + # Cheap gate before the parse: nearly every module in the suite never + # mentions sample_path, and parsing them all would be work for nothing. + if 'sample_path' not in source: + return None + try: + return ast.parse(source, filename=module_path) + except (SyntaxError, ValueError): + return None + + +@functools.lru_cache(maxsize=None) +def handled_lines(module_path: 'str') -> 'frozenset[int]': + """Lines of ``module_path`` on which a missing capture would be handled. + + Every line in the *body* of a ``try`` whose handlers catch a missing file -- + ``else`` and ``finally`` are excluded, since a failure there is not caught. + Whole line ranges rather than the statement's first line, because the runtime + check matches a frame's ``f_lineno`` against this set and the two need not + agree on which line of a multi-line call the call "is" on. + + """ + tree = _parse(module_path) + if tree is None: + return frozenset() + + lines = set() # type: set[int] + for node in ast.walk(tree): + if not isinstance(node, _TRY_NODES): + continue + if not any(_handles_missing_file(handler) for handler in node.handlers): + continue + for statement in node.body: + end = getattr(statement, 'end_lineno', None) or statement.lineno + lines.update(range(statement.lineno, end + 1)) + return frozenset(lines) + + +def _literal_name(call: 'ast.Call') -> 'Optional[str]': + """The capture name a ``sample_path(...)`` call asks for, if it is a literal.""" + if call.args: + first = call.args[0] + if isinstance(first, ast.Constant) and isinstance(first.value, str): + return first.value + return None # computed, or *args + for keyword in call.keywords: + if keyword.arg == 'name' and isinstance(keyword.value, ast.Constant): + if isinstance(keyword.value.value, str): + return keyword.value.value + return None + + +def _is_sample_path(func: 'ast.expr') -> 'bool': + """Whether ``func`` names :func:`tests._support.sample_path`. + + Matched on the attribute name alone, so ``sample_path(...)`` and + ``_support.sample_path(...)`` both count. Nothing else in the suite is + called ``sample_path``, and a false positive here only means a capture name + is checked against git that did not need to be. + + """ + if isinstance(func, ast.Name): + return func.id == 'sample_path' + if isinstance(func, ast.Attribute): + return func.attr == 'sample_path' + return False + + +@functools.lru_cache(maxsize=None) +def sample_path_calls(module_path: 'str') -> 'tuple[SampleCall, ...]': + """Every ``sample_path(...)`` call in ``module_path``, in source order.""" + tree = _parse(module_path) + if tree is None: + return () + + handled = handled_lines(module_path) + return tuple( + SampleCall(node.lineno, _literal_name(node), node.lineno in handled) + for node in ast.walk(tree) + if isinstance(node, ast.Call) and _is_sample_path(node.func) + ) + + +def _display_path(module_path: 'pathlib.Path | str') -> 'str': + """``module_path`` relative to the repository root, when it is inside it.""" + path = pathlib.Path(module_path) + try: + return path.resolve().relative_to(ROOT).as_posix() + except ValueError: + return str(path) + + +def explain(name: 'str', module_path: 'pathlib.Path | str', + lineno: 'Optional[int]' = None) -> 'str': + """The message a caught tier violation reports. + + Long on purpose. The mistake it describes is not obvious from its symptom -- + a file that is missing on one machine and present on another -- so the + message has to say what kind of file it is, why the tier it was read from + may not have it, and what the two real ways out are. A message the reader + has to research is the failure this guard exists to replace. + + Args: + name: Capture the call asked for. + module_path: Module the call was made from. + lineno: Line of the call, when known. + + Returns: + A multi-line, self-contained explanation. + + """ + location = _display_path(module_path) + if lineno is not None: + location = f'{location}:{lineno}' + + committed = committed_capture_names() + suggestion = ', '.join(committed) if committed else 'no capture at all, currently' + + return ( + f'{location} is a unit-tier test module and reads {name!r}, which is a generated ' + f'fixture.\n' + f'\n' + f'git does not track examples/captures/{name} -- it is one of the fixtures ' + f'{REGENERATE_SAMPLES_CMD!r} (equivalently `make samples`) writes on demand, and a ' + f'fresh clone does not have it.\n' + f'\n' + f"The unit tier has to pass on a fresh clone with nothing but `pip install -e " + f"'.[test]'`, so it must not depend on a generated fixture. CI runs it as\n" + f'\n' + f' {UNIT_TIER_SELECTION}\n' + f'\n' + f'on a checkout where examples/captures/ holds only the committed files, so this read ' + f'fails there even when it passes on a machine that has run `make samples`.\n' + f'\n' + f'Two ways to fix it:\n' + f' 1. read a committed capture instead -- git tracks {suggestion}; or\n' + f' 2. move the test into a fixture-dependent tier, which is allowed to read ' + f'generated captures: rename the module to *_runtime.py (or *_regression.py), or put ' + f'it under tests/integration/. Both run only after the fixtures have been built.\n' + f'\n' + f'If the test really wants a generated capture and is happy to be skipped without ' + f'one, handle the absence at the call site, the way {SKIP_IDIOM_EXAMPLE} does:\n' + f'\n' + f' try:\n' + f' path = sample_path({name!r})\n' + f' except FileNotFoundError as exc:\n' + f' self.skipTest(str(exc))\n' + ) + + +def audit_module(module_path: 'pathlib.Path | str') -> 'list[str]': + """Tier violations in ``module_path``, one explanation each. + + A static pass, so it sees a violation in a test that never runs -- one + skipped for a missing optional engine, say -- and it does not care whether + the fixtures are on disk. That is what makes it fire on the developer's + machine rather than only on CI, which is the whole point. + + Only calls whose capture name is a string literal can be checked here; + ``sample_path(sample)`` in a parametrised loop is invisible to a static + pass, and :func:`check_unit_tier_read` catches those at call time instead. + + The caller decides tier membership -- this function audits whatever it is + handed, so a module can be audited by name in a test without having to be + in the unit tier itself. + + Args: + module_path: Module to audit. + + Returns: + One :func:`explain` message per violation, empty when there are none or + when git could not be asked. + + """ + tracked = committed_captures() + if tracked is None: + return [] + + return [ + explain(call.name, module_path, call.lineno) + for call in sample_path_calls(str(module_path)) + if call.name is not None and not call.handled and not _is_committed(call.name, tracked) + ] + + +def check_unit_tier_read(name: 'str', module_path: 'Optional[str]', + lineno: 'Optional[int]' = None) -> 'Optional[str]': + """Whether a :func:`~tests._support.sample_path` call may go ahead. + + The runtime half of the guard, and deliberately independent of whether the + capture is on disk: a unit-tier module reading a generated capture is a tier + violation on the developer's machine exactly as much as on a fresh CI + checkout, and a guard that only fires when the file is missing is the same + late failure it is meant to replace. + + Unlike :func:`audit_module` this sees the capture name however it was + computed, which is what covers ``sample_path(sample)`` in a loop. + + Args: + name: Capture the call asked for. + module_path: ``__file__`` of the calling module, or :data:`None` when it + could not be determined -- in which case the call is allowed, since + tier membership is unknowable. + lineno: Line the call was made from, used both to locate the call in the + message and to spot the ``try``/``except FileNotFoundError`` idiom + that opts a call out. + + Returns: + :data:`None` when the read is fine, otherwise the :func:`explain` + message for it. + + """ + if module_path is None or not is_unit_tier(module_path): + return None + + tracked = committed_captures() + if tracked is None or _is_committed(name, tracked): + return None + + # A call that handles the capture being absent is tier-safe by construction: + # it degrades to a skip on a fresh clone instead of failing. Leave it alone, + # including on a machine that has the fixtures, so the read keeps its + # coverage there. + if lineno is not None and lineno in handled_lines(module_path): + return None + + return explain(name, module_path, lineno) diff --git a/tests/conftest.py b/tests/conftest.py new file mode 100644 index 0000000000..d5e89f8c2a --- /dev/null +++ b/tests/conftest.py @@ -0,0 +1,102 @@ +# -*- coding: utf-8 -*- +"""Suite-wide :program:`pytest` configuration. + +Holds one thing: 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 +for a ``sample_path('...')`` call naming a capture git does not track, and the +run is stopped before any test executes if one is found. + +Stopping the run rather than failing the offending tests is the intended +behaviour. This is a repository-hygiene violation, not a bug in the code under +test -- the tests in question very likely pass on the machine that is running +them -- so the useful outcome is one loud, explanatory message at the top of the +output rather than a red test buried in a summary. Only modules the current +invocation actually collected are checked, so a narrow run is never stopped by a +file it was not going to run. + +The static pass here and the runtime pass in :func:`tests._support.sample_path` +cover each other's blind spots: this one sees a violation in a test that never +runs (skipped for a missing optional engine, say) but only when the capture name +is a literal, while the runtime one sees any name however it was computed but +only when the call is reached. + +""" +from __future__ import annotations + +import pathlib +import warnings +from typing import TYPE_CHECKING + +import pytest + +from tests._tiers import (TierGuardWarning, audit_module, guard_unavailable_reason, + is_unit_tier) + +if TYPE_CHECKING: + from typing import Iterable, Iterator, Optional + + +def _collected_modules(items: 'Iterable[pytest.Item]') -> 'Iterator[pathlib.Path]': + """Distinct module paths behind ``items``, in collection order. + + Every test of a module maps to the same file, so the paths are deduplicated + before anything reads them off disk. + + """ + seen = set() # type: set[pathlib.Path] + for item in items: + # `item.path` since pytest 7; `item.fspath` is the legacy py.path spelling + # and is kept as a fallback so the guard does not depend on which one a + # given pytest exposes. + location = getattr(item, 'path', None) or getattr(item, 'fspath', None) + if location is None: + continue + path = pathlib.Path(str(location)) + if path in seen: + continue + seen.add(path) + yield path + + +def _audit(items: 'Iterable[pytest.Item]') -> 'list[str]': + """Tier violations across the collected unit-tier modules.""" + findings = [] # type: list[str] + for path in _collected_modules(items): + if is_unit_tier(path): + findings.extend(audit_module(path)) + return findings + + +def pytest_collection_modifyitems(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] + findings = [] # type: list[str] + try: + reason = guard_unavailable_reason() + if reason is None: + findings = _audit(items) + except Exception as exc: # pragma: no cover + # The guard is not the thing under test. If it breaks, say so loudly and + # let the suite run -- an exception escaping this hook would take every + # test with it, which is a far worse outcome than an unchecked tier rule. + warnings.warn( + f'the test-tier guard (tests/_tiers.py) failed and did not run: ' + f'{type(exc).__name__}: {exc}', + TierGuardWarning, stacklevel=1, + ) + return + + if reason is not None: + warnings.warn( + f'the test-tier guard (tests/_tiers.py) did not run, because {reason}. Unit-tier ' + f'modules were not checked for reads of generated sample captures.', + TierGuardWarning, stacklevel=1, + ) + return + + if findings: + raise pytest.UsageError( + 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) + ) diff --git a/tests/test_tier_guard.py b/tests/test_tier_guard.py new file mode 100644 index 0000000000..013b64ebb8 --- /dev/null +++ b/tests/test_tier_guard.py @@ -0,0 +1,408 @@ +# -*- coding: utf-8 -*- +"""The tier guard's own tests. + +:mod:`tests._tiers` stops a unit-tier module from reading a generated sample +capture. It is the kind of code that is only exercised when somebody makes the +mistake it exists to catch, so it needs tests of its own -- a guard that has +quietly stopped working is worse than no guard, because everybody has stopped +looking. + +Four things are worth pinning, and they are the four ways this could rot: + +* the tier rule here still matches the one CI runs + (:class:`TierClassificationTests`, :class:`WorkflowAgreementTests`); +* committedness comes from git rather than from a list of names that goes stale + the moment a seventh capture is committed (:class:`CommittedCaptureTests`); +* a violation is caught and explained, and the legitimate reads next to it are + not (:class:`AuditTests`, :class:`RuntimeCheckTests`); +* the suite as it stands is clean (:class:`SuiteIsCleanTests`), and a checkout + without git degrades instead of failing (:class:`DegradationTests`). + +This module is itself unit-tier, so it reads no capture at all: the violating +modules it needs are written into a temporary directory and audited by path. + +""" +from __future__ import annotations + +import pathlib +import re +import tempfile +import textwrap +import unittest +import unittest.mock + +from tests import _tiers + +#: The unit-tier workflow job, for :class:`WorkflowAgreementTests`. Absent from a +#: source distribution, which is why that test skips rather than fails. +WORKFLOW = _tiers.ROOT / '.github' / 'workflows' / 'unit-tests.yml' + + +def write_module(directory: 'pathlib.Path', name: 'str', source: 'str') -> 'pathlib.Path': + """Write ``source`` to ``directory/name`` and return the path. + + The audit reads modules off disk by path and does not import them, so a + throwaway file in a temporary directory is enough to drive it -- and keeps a + deliberately-wrong ``sample_path`` call out of a module :program:`pytest` + collects, which would trip the very guard under test. + + The leading newline of the triple-quoted literal is stripped as well as the + indentation, so that the line numbers the assertions quote are the ones a + reader counts off the literal. + + """ + path = directory / name + path.write_text(textwrap.dedent(source).lstrip('\n'), encoding='utf-8') + return path + + +class TierClassificationTests(unittest.TestCase): + """:func:`~tests._tiers.is_unit_tier` against the CI ignore rules.""" + + def test_tier_is_decided_by_path(self) -> None: + """Each ``--ignore`` and ``--ignore-glob`` of the unit job, and a control.""" + cases = { + 'tests/corekit/test_multidict.py': True, + 'tests/test_tier_guard.py': True, + 'tests/protocols/misc/pcap/test_header_frame_unit.py': True, + 'tests/protocols/transport/test_tcp_runtime.py': False, + 'tests/protocols/test_pcapng_regression.py': False, + 'tests/integration/test_engine_parity.py': False, + 'tests/integration/nested/test_deeper.py': False, + # Outside tests/ altogether: no tier, so not the unit tier. + 'examples/generators/pcap.py': False, + 'pcapkit/interface/core.py': False, + } + for relative, expected in cases.items(): + with self.subTest(module=relative): + self.assertIs(_tiers.is_unit_tier(relative), expected) + + def test_absolute_and_relative_paths_agree(self) -> None: + """A path is classified the same however it is spelled.""" + relative = 'tests/protocols/test_pcapng_regression.py' + self.assertIs(_tiers.is_unit_tier(relative), + _tiers.is_unit_tier(_tiers.ROOT / relative)) + + +class WorkflowAgreementTests(unittest.TestCase): + """The tier rule here matches the one the workflow actually runs.""" + + def test_ignore_flags_match_the_fixture_tier_constants(self) -> None: + """Every ignored path and glob is accounted for by a constant. + + The point of failure this catches: somebody adds a fourth + fixture-dependent naming convention to the workflow and the guard goes + on classifying those modules as unit-tier, flagging their perfectly + legal capture reads. + + """ + if not WORKFLOW.is_file(): + self.skipTest(f'{WORKFLOW} is not present, e.g. in a source distribution') + + text = WORKFLOW.read_text(encoding='utf-8') + globs = set(re.findall(r"--ignore-glob='\*([^']+)'", text)) + directories = set(re.findall(r'--ignore=tests/(\S+)', text)) + + self.assertEqual(globs, set(_tiers.FIXTURE_TIER_SUFFIXES)) + self.assertEqual(directories, set(_tiers.FIXTURE_TIER_DIRS)) + + +class CommittedCaptureTests(unittest.TestCase): + """Committedness is asked of git, not remembered.""" + + def setUp(self) -> None: + reason = _tiers.guard_unavailable_reason() + if reason is not None: + self.skipTest(f'git cannot answer here: {reason}') + + def test_committed_set_comes_from_the_index(self) -> None: + """A committed capture is in the set and a generated one is not. + + ``in.pcap`` and ``test.pcap`` sit in the same directory and are + indistinguishable by name -- the only thing that separates them is that + git tracks one of them. + + """ + tracked = _tiers.committed_captures() + assert tracked is not None + self.assertIn('in.pcap', tracked) + self.assertNotIn('test.pcap', tracked) + + def test_every_tracked_name_exists_and_matches_git(self) -> None: + """The set is the index's answer verbatim, prefix stripped.""" + tracked = _tiers.committed_captures() + assert tracked is not None + self.assertTrue(tracked, 'git tracks no capture at all, which cannot be right') + for name in tracked: + with self.subTest(capture=name): + self.assertNotIn('/', name, 'names are relative to examples/captures/') + + def test_capture_suggestions_are_captures(self) -> None: + """The replacement suggestion offers captures, not reference outputs.""" + suggestions = _tiers.committed_capture_names() + self.assertIn('in.pcap', suggestions) + self.assertNotIn('out.txt', suggestions) + + +class AuditTests(unittest.TestCase): + """:func:`~tests._tiers.audit_module` on modules written for the purpose.""" + + def setUp(self) -> None: + reason = _tiers.guard_unavailable_reason() + if reason is not None: + self.skipTest(f'git cannot answer here: {reason}') + + tmpdir = tempfile.TemporaryDirectory(prefix='pcapkit-tier-guard-') + self.addCleanup(tmpdir.cleanup) + self.tmp_path = pathlib.Path(tmpdir.name) + + def test_generated_capture_is_flagged(self) -> None: + """The mistake the guard exists for, and what it says about it.""" + module = write_module(self.tmp_path, 'test_wrong_unit.py', """ + from tests._support import sample_path + + + def test_reads_a_generated_capture(): + assert sample_path('test.pcap') + """) + + findings = _tiers.audit_module(module) + self.assertEqual(len(findings), 1) + + message = findings[0] + self.assertIn('test_wrong_unit.py:5', message) + self.assertIn("'test.pcap'", message) + self.assertIn('generated fixture', message) + self.assertIn('must not depend on a generated fixture', message) + # The two real ways out have to be named, or the message only tells the + # reader that they are wrong and not what to do instead. + self.assertIn('in.pcap', message) + self.assertIn('*_runtime.py', message) + self.assertIn('tests/integration/', message) + self.assertIn(_tiers.REGENERATE_SAMPLES_CMD, message) + + def test_committed_capture_is_not_flagged(self) -> None: + """The same call with a committed capture is fine.""" + module = write_module(self.tmp_path, 'test_right_unit.py', """ + from tests._support import sample_path + + + def test_reads_a_committed_capture(): + assert sample_path('in.pcap') + """) + self.assertEqual(_tiers.audit_module(module), []) + + def test_handled_absence_is_not_flagged(self) -> None: + """The skip idiom opts a call out, because it is tier-safe already.""" + module = write_module(self.tmp_path, 'test_handled_unit.py', """ + import unittest + + from tests._support import sample_path + + + class Tests(unittest.TestCase): + def test_skips_without_the_fixture(self): + try: + path = sample_path('test.pcap') + except FileNotFoundError as exc: + self.skipTest(str(exc)) + assert path + """) + self.assertEqual(_tiers.audit_module(module), []) + + def test_handler_elsewhere_in_the_module_does_not_opt_a_call_out(self) -> None: + """Only the guarded ``try`` body counts, not the whole module.""" + module = write_module(self.tmp_path, 'test_partly_handled_unit.py', """ + import unittest + + from tests._support import sample_path + + + class Tests(unittest.TestCase): + def test_handled(self): + try: + assert sample_path('test.pcap') + except FileNotFoundError as exc: + self.skipTest(str(exc)) + + def test_unhandled(self): + assert sample_path('http6.cap') + """) + + findings = _tiers.audit_module(module) + self.assertEqual(len(findings), 1) + self.assertIn("'http6.cap'", findings[0]) + + def test_an_else_clause_is_not_a_handler(self) -> None: + """A call in ``try``/``else`` is outside the protected body.""" + module = write_module(self.tmp_path, 'test_else_unit.py', """ + import unittest + + from tests._support import sample_path + + + class Tests(unittest.TestCase): + def test_reads_in_the_else_clause(self): + try: + pass + except FileNotFoundError: + self.skipTest('no fixture') + else: + assert sample_path('test.pcap') + """) + self.assertEqual(len(_tiers.audit_module(module)), 1) + + def test_a_computed_name_is_left_to_the_runtime_check(self) -> None: + """A static pass cannot know the name, and does not guess at one.""" + module = write_module(self.tmp_path, 'test_computed_unit.py', """ + from tests._support import sample_path + + CAPTURES = ('test.pcap', 'http6.cap') + + + def test_reads_several_captures(): + for capture in CAPTURES: + assert sample_path(capture) + """) + + self.assertEqual(_tiers.audit_module(module), []) + calls = _tiers.sample_path_calls(str(module)) + self.assertEqual([call.name for call in calls], [None]) + + def test_a_qualified_call_is_recognised(self) -> None: + """``_support.sample_path(...)`` counts as much as the bare name.""" + module = write_module(self.tmp_path, 'test_qualified_unit.py', """ + from tests import _support + + + def test_reads_a_generated_capture(): + assert _support.sample_path('test.pcap') + """) + self.assertEqual(len(_tiers.audit_module(module)), 1) + + def test_a_module_that_never_mentions_the_helper_is_cheap_and_clean(self) -> None: + """The fast path: no ``sample_path``, nothing parsed, nothing found.""" + module = write_module(self.tmp_path, 'test_unrelated_unit.py', """ + def test_arithmetic(): + assert 1 + 1 == 2 + """) + self.assertEqual(_tiers.audit_module(module), []) + self.assertEqual(_tiers.sample_path_calls(str(module)), ()) + + def test_an_unparseable_module_is_not_this_guards_problem(self) -> None: + """A syntax error is reported by pytest, far better than from here.""" + module = write_module(self.tmp_path, 'test_broken_unit.py', """ + def test_broken(: + sample_path('test.pcap') + """) + self.assertEqual(_tiers.audit_module(module), []) + + +class RuntimeCheckTests(unittest.TestCase): + """:func:`~tests._tiers.check_unit_tier_read`, the call-time half.""" + + #: A unit-tier path that does not exist. Tier membership is a property of the + #: path, so nothing needs to be on disk to ask about it -- and using a real + #: module would tie these assertions to that module's line numbers. + UNIT_MODULE = str(_tiers.TESTS_ROOT / 'protocols' / 'test_imaginary_unit.py') + #: The same, in a fixture-dependent tier. + RUNTIME_MODULE = str(_tiers.TESTS_ROOT / 'protocols' / 'test_imaginary_runtime.py') + INTEGRATION_MODULE = str(_tiers.TESTS_ROOT / 'integration' / 'test_imaginary.py') + + def setUp(self) -> None: + reason = _tiers.guard_unavailable_reason() + if reason is not None: + self.skipTest(f'git cannot answer here: {reason}') + + def test_unit_tier_read_of_a_generated_capture_is_refused(self) -> None: + problem = _tiers.check_unit_tier_read('test.pcap', self.UNIT_MODULE, 12) + self.assertIsNotNone(problem) + assert problem is not None + self.assertIn('test_imaginary_unit.py:12', problem) + + def test_unit_tier_read_of_a_committed_capture_is_allowed(self) -> None: + self.assertIsNone(_tiers.check_unit_tier_read('in.pcap', self.UNIT_MODULE, 12)) + + def test_fixture_dependent_tiers_may_read_anything(self) -> None: + for module in (self.RUNTIME_MODULE, self.INTEGRATION_MODULE): + with self.subTest(module=module): + self.assertIsNone(_tiers.check_unit_tier_read('test.pcap', module, 12)) + + def test_an_unknown_caller_is_allowed(self) -> None: + """No ``__file__``, no tier, no judgement.""" + self.assertIsNone(_tiers.check_unit_tier_read('test.pcap', None, None)) + + def test_the_decision_does_not_depend_on_the_capture_being_present(self) -> None: + """The property that makes this fire locally rather than only in CI. + + Asked of a capture that is certainly on disk -- ``in.pcap``, which is + committed -- while git is made to say it tracks nothing. The read is + still refused, which is the point: the decision is "is this tracked", + never "is this here". A guard that waited for the file to be missing + would go on passing on every machine that has run ``make samples``, + which is the late failure it exists to replace. + + """ + self.assertTrue((_tiers.SAMPLE_ROOT / 'in.pcap').is_file()) + + with unittest.mock.patch.object(_tiers, 'committed_captures', + return_value=frozenset()): + problem = _tiers.check_unit_tier_read('in.pcap', self.UNIT_MODULE, 12) + + self.assertIsNotNone(problem) + assert problem is not None + self.assertIn("'in.pcap'", problem) + + +class SuiteIsCleanTests(unittest.TestCase): + """The suite as it stands satisfies the rule.""" + + def test_no_unit_tier_module_depends_on_a_generated_capture(self) -> None: + """Every unit-tier module on disk, not merely the collected ones. + + :file:`tests/conftest.py` audits what the current invocation collected, + which is the whole unit tier in CI but only a slice of it when somebody + runs one directory. This covers the rest, so a violation added to a + module that a narrow run never touches still has something looking at it. + + """ + reason = _tiers.guard_unavailable_reason() + if reason is not None: + self.skipTest(f'git cannot answer here: {reason}') + + findings = [] # type: list[str] + for path in sorted(_tiers.TESTS_ROOT.rglob('*.py')): + if _tiers.is_unit_tier(path): + findings.extend(_tiers.audit_module(path)) + + self.assertEqual(findings, [], '\n\n'.join(findings)) + + +class DegradationTests(unittest.TestCase): + """Without git the guard stands down rather than guessing.""" + + def test_audit_finds_nothing_when_git_cannot_answer(self) -> None: + """A source tarball has no index, so nothing can be called generated.""" + with tempfile.TemporaryDirectory(prefix='pcapkit-tier-guard-') as tmpdir: + module = write_module(pathlib.Path(tmpdir), 'test_wrong_unit.py', """ + from tests._support import sample_path + + + def test_reads_a_generated_capture(): + assert sample_path('test.pcap') + """) + + with unittest.mock.patch.object(_tiers, 'committed_captures', return_value=None): + self.assertEqual(_tiers.audit_module(module), []) + + def test_runtime_check_allows_everything_when_git_cannot_answer(self) -> None: + with unittest.mock.patch.object(_tiers, 'committed_captures', return_value=None): + self.assertIsNone(_tiers.check_unit_tier_read( + 'test.pcap', RuntimeCheckTests.UNIT_MODULE, 12)) + + def test_a_git_failure_yields_a_reason_rather_than_an_exception(self) -> None: + """``_git`` swallows every way the subprocess can fail.""" + for failure in (OSError('no git'), FileNotFoundError('no git')): + with self.subTest(failure=type(failure).__name__): + with unittest.mock.patch('subprocess.run', side_effect=failure): + self.assertIsNone(_tiers._git('rev-parse', '--show-toplevel'))