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'))