From 2c7738986419a3330feb910cbb623f4f1b0a48da Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Wed, 23 Sep 2026 00:21:35 -0400 Subject: [PATCH 1/3] docs(tests): stop stating how many captures are committed (#700) Three comments in the test tree hardcoded a count of the captures tracked under examples/captures/. Counts there rot on the next commit that adds or drops one, and nothing asserts them, so nothing goes red when they do. - tests/_tiers.py: the bullet explaining why committedness is asked of git rather than hardcoded stated the count itself ("there are six today, not the two the rule started with"). It now names the rot -- a capture committed, or no longer committed -- without a number. - tests/integration/_helpers.py: said "four of them are committed" when six were tracked. Already wrong before this change, not merely stale; now "some of them committed". - tests/test_tier_guard.py: "the moment a seventh capture is committed" is now "the moment another capture is committed". Prose only; no behaviour change. The committed set is still resolved live by committed_captures() via `git ls-files`, which holds no hardcoded list and is untouched here. tests/test_tier_guard.py passes 25 tests / 19 subtests, and the end-to-end modules that reach the edited helper with only committed captures present (test_engine_runtime.py, test_module_loading.py) pass too. --- tests/_tiers.py | 3 +-- tests/integration/_helpers.py | 4 ++-- tests/test_tier_guard.py | 2 +- 3 files changed, 4 insertions(+), 5 deletions(-) diff --git a/tests/_tiers.py b/tests/_tiers.py index 982c0912d..f03c9a4a9 100644 --- a/tests/_tiers.py +++ b/tests/_tiers.py @@ -34,8 +34,7 @@ * :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). + moment somebody commits another capture, or stops committing one. * :func:`audit_module` -- does this module read a generated capture without handling its absence? * :func:`check_unit_tier_read` -- may this particular diff --git a/tests/integration/_helpers.py b/tests/integration/_helpers.py index 16aab0191..2686e3bc2 100644 --- a/tests/integration/_helpers.py +++ b/tests/integration/_helpers.py @@ -55,8 +55,8 @@ class EndToEndTestCase(unittest.TestCase): Gives every test a private temporary directory in :attr:`tmp_path` and an :meth:`extract` wrapper that closes the input stream on teardown. Captures - under :file:`examples/captures/` are fixtures -- four of them are committed - -- so nothing here ever writes outside :attr:`tmp_path`. + under :file:`examples/captures/` are fixtures -- some of them committed -- + so nothing here ever writes outside :attr:`tmp_path`. """ diff --git a/tests/test_tier_guard.py b/tests/test_tier_guard.py index 3a426fb67..b03586814 100644 --- a/tests/test_tier_guard.py +++ b/tests/test_tier_guard.py @@ -12,7 +12,7 @@ * 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`); + the moment another capture is committed (:class:`CommittedCaptureTests`); * a violation is caught and explained, the legitimate reads next to it are not, and several violations in one module are listed in the order those violations appear in the file rather than in the order a tree walk happened to reach them From 507f3ec651b54d06354d5804847e61fcafde8b51 Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Wed, 23 Sep 2026 09:23:02 -0400 Subject: [PATCH 2/3] test: verify the committed-capture set against git, not against itself (#708) `CommittedCaptureTests.test_every_tracked_name_exists_and_matches_git` promised, in its own docstring, that the set is "the index's answer verbatim" -- but its body only asserted non-emptiness and the absence of a `/`. Neither check compares against git, and neither stats a file despite `exists` in the test's name, so a hardcoded literal set, a stale entry naming a deleted capture, or an outright wrong set would all have passed it. Its sibling, `test_capture_suggestions_are_captures`, had the same gap: it asserted `'in.pcap' in suggestions` and `'out.txt' not in suggestions`, which a hardcoded list containing `in.pcap` also satisfies. - `test_every_tracked_name_exists_and_matches_git` now shells out to `git ls-files -z -- examples/captures` directly via `subprocess`, strips the prefix itself, and asserts set equality against `committed_captures()`. Going through `_tiers._git` instead would only show the implementation agrees with itself; the independent call is what actually catches a wrong answer. Adds a `Path.is_file()` check per name, the "exists" the name always claimed. Failure tolerance mirrors `_git`'s own (`OSError`/`SubprocessError`/non-zero exit all skip rather than fail), matching the pattern `tests/project/test_capture_tracking.py` already uses for the same reason -- a source tarball's test run should not fail for a rule it cannot possibly break. `CommittedCaptureTests.setUp` already skips the whole class when git cannot answer, so this belongs to both layers. - `test_capture_suggestions_are_captures` now asserts full equality against the tracked set filtered to `CAPTURE_SUFFIXES` and sorted, rather than two membership checks alone; the two original assertions stay as a readable sanity check on top of it. This closes the same class of gap for `committed_capture_names()`'s filter-and-sort logic, though it still relies on `committed_captures()` for the tracked set itself -- the git fidelity of that set is what the sibling test above verifies. Verified the new assertion is load-bearing: temporarily hardcoding `committed_captures()` in `tests/_tiers.py` to return `{'in.pcap', 'dhcp.pcapng', 'a-capture-that-does-not-exist.pcap'}` fails `test_every_tracked_name_exists_and_matches_git` with an `AssertionError` naming the mismatch against a fresh `git ls-files`; the old body passed the same sabotage. Reverted before committing -- `tests/_tiers.py` is untouched here. tests/test_tier_guard.py: 25 passed / 19 subtests, against the real `_tiers.py`. --- tests/test_tier_guard.py | 62 ++++++++++++++++++++++++++++++++++++++-- 1 file changed, 60 insertions(+), 2 deletions(-) diff --git a/tests/test_tier_guard.py b/tests/test_tier_guard.py index b03586814..1a29e591b 100644 --- a/tests/test_tier_guard.py +++ b/tests/test_tier_guard.py @@ -28,6 +28,7 @@ import pathlib import re +import subprocess import tempfile import textwrap import unittest @@ -131,17 +132,74 @@ def test_committed_set_comes_from_the_index(self) -> None: 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.""" + """The set is the index's answer verbatim, prefix stripped. + + "Verbatim" is checked by re-deriving the index here, independently of + :func:`~tests._tiers.committed_captures` -- a second call to that same + function would only prove it agrees with itself, not that it agrees + with git. A hardcoded, stale, or otherwise wrong literal set would fail + the comparison below; it could only have passed the old body, which + asserted nothing but non-emptiness and the absence of a ``/``. + + """ tracked = _tiers.committed_captures() assert tracked is not None self.assertTrue(tracked, 'git tracks no capture at all, which cannot be right') + + relative_root = _tiers.SAMPLE_ROOT.relative_to(_tiers.ROOT).as_posix() + # Shell out directly rather than through `_tiers._git` -- going through + # the implementation's own helper would only show that + # `committed_captures()` agrees with itself, not with git. The failure + # tolerance mirrors `_git`'s for the same reason it exists there: no + # executable, no repository, or a non-zero exit is "cannot tell", not + # "the sets disagree", and a source tarball's test run should not fail + # for a rule it cannot possibly break. + try: + completed = subprocess.run( + ('git', 'ls-files', '-z', '--', relative_root), + cwd=str(_tiers.ROOT), stdout=subprocess.PIPE, stderr=subprocess.DEVNULL, + timeout=30, check=False, + ) + except (OSError, subprocess.SubprocessError) as exc: + self.skipTest(f'git ls-files could not be run independently: {exc}') + if completed.returncode != 0: + self.skipTest('git ls-files exited non-zero on a fresh, independent re-derivation') + + listing = completed.stdout.decode('utf-8', 'surrogateescape') + prefix = relative_root + '/' + expected = { + entry[len(prefix):] for entry in listing.split('\0') + if entry and entry.startswith(prefix) + } + self.assertEqual( + tracked, expected, + 'committed_captures() disagrees with a freshly re-derived `git ls-files`' + ) + for name in tracked: with self.subTest(capture=name): self.assertNotIn('/', name, 'names are relative to examples/captures/') + self.assertTrue( + (_tiers.SAMPLE_ROOT / name).is_file(), + f'git tracks examples/captures/{name} but it is not on disk' + ) def test_capture_suggestions_are_captures(self) -> None: - """The replacement suggestion offers captures, not reference outputs.""" + """The suggestion is the tracked set filtered to captures and sorted. + + Equality against a set built independently of + :func:`~tests._tiers.committed_capture_names` is the point: the two + example-name assertions below are satisfied by any list that happens to + contain ``in.pcap`` and omit ``out.txt``, hardcoded or not, so they stay + only as a readable sanity check on top of the real one. + + """ + tracked = _tiers.committed_captures() + assert tracked is not None + expected = tuple(sorted(name for name in tracked if name.endswith(_tiers.CAPTURE_SUFFIXES))) + suggestions = _tiers.committed_capture_names() + self.assertEqual(suggestions, expected) self.assertIn('in.pcap', suggestions) self.assertNotIn('out.txt', suggestions) From cbcdaf4863163ea61070ae2defc8736eb7e136d2 Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Wed, 23 Sep 2026 09:45:45 -0400 Subject: [PATCH 3/3] fix(tests): correct tier-guard prose and guard the suggestion equality - _tiers.py's docstring said test_tier_guard.py "only stats in.pcap", true before #708 added a loop stat over every committed_captures() name at test_tier_guard.py:183. Restate why the module is safe (every name it touches is committed by construction) instead of the now-wrong count. - test_capture_suggestions_are_captures compared two sets that are both () when git tracks nothing under SAMPLE_ROOT, so assertEqual passed vacuously and only the "decorative" assertIn caught it. Add the same assertTrue(tracked, ...) guard its sibling test_every_tracked_name_exists_and_matches_git already carries, and stop calling the example-name assertions mere decoration. Fixes two defects a cross-review found on #703 (#708). Verified: tests/test_tier_guard.py passes in full (25 tests, 19 subtests); the new guard reproduced failing on unfixed code's vacuous assertEqual and passing after the fix. --- tests/_tiers.py | 9 ++++++--- tests/test_tier_guard.py | 14 ++++++++++---- 2 files changed, 16 insertions(+), 7 deletions(-) diff --git a/tests/_tiers.py b/tests/_tiers.py index f03c9a4a9..592a45d63 100644 --- a/tests/_tiers.py +++ b/tests/_tiers.py @@ -63,9 +63,12 @@ are entitled to, e.g. :file:`tests/protocols/test_option_coverage_runtime.py`, which imports :data:`SAMPLE_ROOT` from here and joins names onto it -- legal, because that tier runs only once the fixtures exist. The one unit-tier module that -touches :data:`SAMPLE_ROOT` at all, :file:`tests/test_tier_guard.py`, only stats -:file:`in.pcap`, which is committed. But nothing stops the next unit-tier module -from opening a *generated* capture that way, and the shape stays invisible here. +touches :data:`SAMPLE_ROOT` at all, :file:`tests/test_tier_guard.py`, stats +:file:`in.pcap` directly -- which is committed -- and separately loops over every +name :func:`committed_captures` reports, each committed by construction, since that +is git's own tracked listing rather than a name the test made up. But nothing stops +the next unit-tier module from opening a *generated* capture that way, and the shape +stays invisible here. Flagging it in general was considered and rejected, with a measurement behind the decision. A rule as broad as "any string literal ending in ``.pcap``" matches 18 diff --git a/tests/test_tier_guard.py b/tests/test_tier_guard.py index 1a29e591b..2509f4b5e 100644 --- a/tests/test_tier_guard.py +++ b/tests/test_tier_guard.py @@ -188,14 +188,20 @@ def test_capture_suggestions_are_captures(self) -> None: """The suggestion is the tracked set filtered to captures and sorted. Equality against a set built independently of - :func:`~tests._tiers.committed_capture_names` is the point: the two - example-name assertions below are satisfied by any list that happens to - contain ``in.pcap`` and omit ``out.txt``, hardcoded or not, so they stay - only as a readable sanity check on top of the real one. + :func:`~tests._tiers.committed_capture_names` is the point, but on an + empty tracked set it holds vacuously -- both sides would be ``()`` -- + so the ``assertTrue`` guard below comes first, the same guard its + sibling ``test_every_tracked_name_exists_and_matches_git`` carries for + the same reason. Only past that guard do the two example-name + assertions add anything: they are satisfied by any list that happens + to contain ``in.pcap`` and omit ``out.txt``, hardcoded or not, so they + stay a readable sanity check on top of the real one rather than the + thing holding the test up. """ tracked = _tiers.committed_captures() assert tracked is not None + self.assertTrue(tracked, 'git tracks no capture at all, which cannot be right') expected = tuple(sorted(name for name in tracked if name.endswith(_tiers.CAPTURE_SUFFIXES))) suggestions = _tiers.committed_capture_names()