Skip to content

docs(tests): stop stating how many captures are committed (#700) - #703

Open
JarryShaw wants to merge 3 commits into
mainfrom
docs/capture-count-prose-700
Open

JarryShaw wants to merge 3 commits into
mainfrom
docs/capture-count-prose-700

Conversation

@JarryShaw

@JarryShaw JarryShaw commented Sep 23, 2026

Copy link
Copy Markdown
Owner

Fixes #700.
Fixes #708.

Three comments in the test tree stated how many captures under examples/captures/ are committed. All three were wrong or about to be, and one was already wrong before any recent change.

The tracked set is 6 on main (dhcp.pcapng, in.pcap, out.json, out.plist, out.txt, pcapng.txt), verified here against the index, and becomes 2 once #697 merges.

Why drop the numbers rather than update them

An updated count rots again on the next capture — and it would make this change's correctness depend on whether #697 has merged. Count-free prose is correct before and after #697, which is the property aimed for. In all three places the count was incidental to the point being made, so no count was derived from the live listing either; each sentence is an argument about why not to hardcode, and it carries unchanged without a number.

The three passages

tests/_tiers.py — the bullet hardcoded the count while explaining why the code does not hardcode a list:

-  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.

The added clause names the shrink direction too, which is exactly what #697 is about to do.

tests/integration/_helpers.py — a pre-existing error, not staleness introduced by #697: it said four where six were tracked.

-    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`.

tests/test_tier_guard.py — correct on main as next-after-six, "a third" after #697:

-  the moment a seventh capture is committed (:class:`CommittedCaptureTests`);
+  the moment another capture is committed (:class:`CommittedCaptureTests`);

No code changed

Confirmed before starting, as #700 asks: committed_captures() resolves the set live in tests/_tiers.py by asking the index for the tracked names under the sample root, and holds no hardcoded list; committed_capture_names() derives from it by suffix filter. Neither is touched here. No hardcoded list or count of committed captures exists anywhere in the executable code of the three files.

Testing

No coverage increase is possible from this change, and none is claimed. Two independent reasons, both measured rather than asserted:

  1. The project scopes coverage to source = ["pcapkit"] (pyproject.toml), so the test tree is not measured at all.
  2. Even if it were, the diff moves no statement. Comparing the committed and working-tree parses with coverage.parser.PythonParser, the executable statement set is identical in all three files — tests/_tiers.py 160/160, tests/integration/_helpers.py 51/51, tests/test_tier_guard.py 164/164 — differing only by the one-line offset the shortened _tiers.py docstring introduces.

No test was invented to satisfy the rule. A test asserting that no comment in the test tree contains a bare capture count would be brittle, and would not have caught the _helpers.py error anyway, since that count was simply wrong rather than formatted unusually.

Runs (coverage run -m pytest, repo venv 3.14.7, PYTHONSAFEPATH=1, worktree on PYTHONPATH, with pcapkit.__file__ and tests._tiers.ROOT asserted to resolve inside the worktree before trusting any of it):

The rest of tests/integration/ needs make samples to have run, which writes into examples/** and is out of scope for this branch, so it was not run here.

Also checked

tests/integration/test_pcapng_end_to_end.py says "The six fixtures", but that is the count of generated pcapng fixtures from examples/generators/pcapng.py (six: dhcp, dhcp_big_endian, dhcp_little_endian, many_interfaces, test, profile, of which only dhcp.pcapng is committed) — not a committed-capture count, and correct. Left alone. Likewise the frame counts in tests/foundation/test_extraction_no_eof.py ("six-frame, 605-byte capture") count frames inside one capture, not captures.

examples/legacy_smoke/README.rst belongs to #697 and is untouched, as #700 notes.

@JarryShaw JarryShaw added docs Pull requests that change documentation only (docs: subject prefix) test Pull requests that add or correct tests (test: subject prefix) labels Sep 23, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

Two things turned up while surveying for other sites that state a committed-capture count. Neither is changed here — both are outside the three files this PR owns — and both are reported rather than fixed.

1. #697 breaks three more count statements, in files it already owns

#700 records that examples/legacy_smoke/README.rst:23-24 is accidentally fixed by #697. The survey found the reverse also happens, in the same two files, and it is not currently noted anywhere:

Site Text On main After #697
examples/legacy_smoke/README.rst:118 "Four files in ../captures/ are committed outputs rather than inputs" correct false
examples/legacy_smoke/README.rst:138 "an unpinned run rewrites every timestamp line in all four files" (and ":139 the PCAP three") correct false
examples/legacy_smoke/Makefile:14 "## Regenerate the four committed fixtures in ../captures/." correct false

The four in question are out.json, out.plist, out.txt, pcapng.txt — exactly the four #697 stops tracking. So the whole Regenerating the committed fixtures section (README.rst:115-144) and the fixtures Make target will describe regenerating files that are no longer committed, and Makefile:21's @echo names all four explicitly.

Worth noting that README.rst:23-24 and README.rst:118 already contradict each other on main: six files are tracked, i.e. two inputs plus four committed outputs, so :118 is right today and :23-24 is wrong. #697 flips the polarity of both rather than removing the inconsistency.

These all live under examples/**, which belongs to #697, so they are left alone here. They look like they belong in #697 rather than in a follow-up, since that is the change that invalidates them.

2. No test would catch the tier guard's live resolution being replaced by a hardcoded list

Relevant because it is the reason the three comments this PR fixes were free to rot, and it is the one thing that could genuinely have been tested here.

CommittedCaptureTests.test_every_tracked_name_exists_and_matches_git is named and documented for exactly that contract — docstring """The set is the index's answer verbatim, prefix stripped.""" — but its body asserts only that the set is non-empty and that no name contains /. It never invokes the index independently, and despite exists in the method name it does not stat the files either. Its siblings do not close the gap: test_committed_set_comes_from_the_index is a two-name spot check (in.pcap in, test.pcap out), and test_capture_suggestions_are_captures tests the CAPTURE_SUFFIXES filter. A hardcoded list of bare names containing in.pcap and not test.pcap passes all three.

Extending that one method to re-derive the tracked set and assertEqual it against committed_captures() would close it. Not done here: it is a behaviour-bearing test change rather than the prose fix #700 asks for, it would not move any coverage number (coverage is scoped to source = ["pcapkit"], and the code path is already executed), and #700 explicitly scopes itself to the prose. Flagging for a decision rather than folding it in.

Confirmed as part of the same survey, and independently of it: neither tests/_tiers.py, tests/integration/_helpers.py nor tests/test_tier_guard.py holds a hardcoded capture-filename list or a hardcoded committed-capture count anywhere in its executable code. The only capture-related literal in _tiers.py code is the suffix tuple CAPTURE_SUFFIXES, which filters suggestions and does not decide committedness.

@JarryShaw

Copy link
Copy Markdown
Owner Author

GOOD TO MERGE

Independent cross-review, different model from the PR's author (Claude Sonnet 5). Head reviewed: 009c6bc2c (branch docs/capture-count-prose-700, base main 9d7890db4). No changes requested; no changes were made to the PR or its branch by this review.

Per-claim verdicts

1. All three passages are count-free and accurate before and after #697 — CONFIRMED.
Rendered each passage at head and read it in context, not just the diff:

  • tests/_tiers.py:34-38: "...silently rots the moment somebody commits another capture, or stops committing one." True in either direction, matches the description.
  • tests/integration/_helpers.py:57-59: "...are fixtures -- some of them committed -- so nothing here ever writes outside tmp_path." "Some" holds at 6 tracked today and at 2 after chore(examples): stop tracking the four rendered capture reports (#685) #697 (there are always non-committed/generated captures alongside, per the unchanged sentence at tests/_tiers.py:26-27: "Only a handful... are committed; the rest are built on demand").
  • tests/test_tier_guard.py:12-15: "...goes stale the moment another capture is committed..." Also count-free and true either way.
    No new number was substituted for the old one anywhere.

2. No executable line changed — CONFIRMED.
Reproduced independently with coverage.parser.PythonParser against git show <ref>:<file> exports of base (9d7890db4) and head (009c6bc2c):

  • tests/_tiers.py: 160/160 statements. Not literally equal-by-value — every statement's line number in head is exactly base - 1 (verified programmatically: {x-1 for x in base_statements} == head_statements, true with zero symmetric difference). That is exactly what a single docstring line removed near the top of the file produces, and a direct diff of the two exports confirms the only textual change in the whole file is that one hunk (net -1 line).
  • tests/integration/_helpers.py: 51/51, identical sets, no shift (edit is a 2-line-for-2-line swap).
  • tests/test_tier_guard.py: 164/164, identical sets, no shift (1-line-for-1-line swap).
    This matches the author's reported 160/160, 51/51, 164/164 exactly.

3. Live resolution untouched, no hardcoded list/count in code — CONFIRMED.
committed_captures() (head tests/_tiers.py:304-312) calls _git_state(), which calls _git('ls-files', '-z', '--', relative_root) at head tests/_tiers.py:292, exactly as claimed. Ran it live against a scratch checkout (see Method below) and it returned the real tracked set. CAPTURE_SUFFIXES (_tiers.py:146) is only consumed by committed_capture_names() to filter suggestions by extension, never by committed_captures()/_git_state() — confirmed by reading both call sites. Grepped all three files for capture filenames and count-shaped literals (== 6, == 2, == 4, len(tracked), frozenset({ with filenames): the only filename literals are single-name assertions already present on main (self.assertIn('in.pcap', tracked), etc. in test_tier_guard.py), not a list standing in for git's answer, and none are part of this diff.

4. Tier guard still passes, subtests checked — CONFIRMED.
Ran tests/test_tier_guard.py against a real (if throwaway) git checkout of the PR head — not a bare archive — because committed_captures() needs git ls-files to answer at all; a git-less tree would make CommittedCaptureTests skip via guard_unavailable_reason(), which would silently hide exactly the subtests this claim is about. Concretely: git archive 009c6bc2c into a scratch directory, then git init && git add -A && git commit inside that scratch directory only (a brand-new, throwaway repo, not a worktree of this repo and not a mutation of PyPCAPKit itself), giving _git_state() a real index reporting the true 6 tracked captures. Before trusting any number: pcapkit.__file__ and tests._tiers.ROOT both asserted to start with the scratch prefix — confirmed (/local/home/jarryx/tmp/xreview-703/prtree/... for both).
Result, with a custom TestResult.addSubTest counter (top-line status alone is not enough, per the brief): 25 tests run, 19 subtests observed, 0 subtest failures, 0 top-level failures/errors, OK. I decomposed the 19 by hand from the four subTest loops: 9 (test_tier_is_decided_by_path, fixed cases) + 6 (test_every_tracked_name_exists_and_matches_git, one per currently tracked capture) + 2 (test_fixture_dependent_tiers_may_read_anything) + 2 (test_a_git_failure_yields_a_reason_rather_than_an_exception) = 19. The only count-dependent loop is the 6; 13 fixed + 2 (post-#697 baseline) = 15, 13 + 6 (today) = 19 — exactly the "15 plus one subtest per extra tracked report" the author describes. Also ran tests/integration/test_engine_runtime.py + tests/integration/test_module_loading.py alongside the guard in the same scratch tree (neither needs make samples, only in.pcap): 30 passed, 19 subtests, 0 failures — matches the author's reported testing section exactly.

5. Scope — CONFIRMED. git diff --name-only 9d7890db4...009c6bc2c returns exactly the three named files. No CHANGELOG.md change, nothing under pcapkit/.

6. The "no coverage increase is possible" argument — CONFIRMED sound, and I agree no worthwhile test was skipped.
Verified pyproject.toml:286-289 directly:

[tool.coverage.run]
source = [
    "pcapkit",
]

at exactly those lines — tests/ is outside coverage's source entirely, independent of ground 2. Ground 2 (identical statement sets) is independently reproduced above regardless. On "any worthwhile test passed over": a regex/grep-based test asserting no comment states a bare capture count would be brittle (false positives on unrelated numbers like the "six-frame, 605-byte capture" language elsewhere in the suite, which I spot-checked and is genuinely unrelated — a frame count, not a capture count) and, as the author notes, wouldn't have caught the actual _helpers.py bug (a wrong count, not an oddly-formatted one). I agree with declining to invent one here.

The two traps

Trap one (the #697 survey) — all four observations CONFIRMED, verbatim, against main (9d7890db4):

  • examples/legacy_smoke/README.rst:118: Four files in ``../captures/`` are committed outputs rather than inputs, and they are — exact line, exact wording.
  • examples/legacy_smoke/README.rst:138 / :140: ...an unpinned run rewrites every timestamp line in all four files with (138) ... each other, the PCAP three having been generated at UTC-05:00... (140) — same passage the task cites as "138-139"; off by one line from where "the PCAP three" phrase itself lands, immaterial.
  • examples/legacy_smoke/Makefile:14: ## Regenerate the four committed fixtures in ../captures/. — exact line, exact wording.
  • examples/legacy_smoke/README.rst:23-24 vs :118 — genuinely contradictory, not just stale: lines 23-24 say "Only in.pcap and dhcp.pcapng are committed; the rest are generated" (a blanket claim about the whole directory), while line 118 says four more files are also committed outputs. Both cannot be true simultaneously; 2 + 4 = 6 matches the real tracked set, so :23-24 is simply wrong on main today, independent of chore(examples): stop tracking the four rendered capture reports (#685) #697.
    All four are in examples/**, correctly left untouched by this PR. They are real and, as the author flags, look like a gap #697 will need to close (it removes 4 files from tracking, which does not by itself fix stale prose that already assumed the wrong count in either direction) — worth a follow-up task against chore(examples): stop tracking the four rendered capture reports (#685) #697 rather than blocking docs(tests): stop stating how many captures are committed (#700) #703.

Trap two (the self-contradicting test) — CONFIRMED accurate.
Read CommittedCaptureTests.test_every_tracked_name_exists_and_matches_git in full. Its body is exactly:

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

against a docstring reading "The set is the index's answer verbatim, prefix stripped." It never re-derives or independently calls git ls-files to compare against tracked, and despite "exists" in the test's name it never stats a single file on disk. A committed_captures() hardcoded to return frozenset({'in.pcap', 'dhcp.pcapng', 'out.json', 'out.plist', 'out.txt', 'pcapng.txt'}) as string literals, with no git call at all, would satisfy all three assertions in this body (not-None, truthy, no /) and would sail through test_committed_set_comes_from_the_index and test_capture_suggestions_are_captures too, since those only check 'in.pcap' in tracked / 'test.pcap' not in tracked — also satisfiable by the same hardcoded literal. So the suite's actual defense against exactly the regression tests/_tiers.py's own docstring warns about (a hardcoded list silently rotting) is thinner than its name and docstring claim.
I agree with the author's judgment that this is out of scope for #703 — it's a pre-existing gap in test design, not something the prose fix here touches or worsens — and should be filed separately rather than block this PR.

What I could not verify

  • The rest of tests/integration/ (beyond test_engine_runtime.py and test_module_loading.py) needs make samples, which writes into examples/** and was out of bounds for this review — not run, per the brief.
  • Did not run the full suite or touch anything under pcapkit/const/tcp/ — the unrelated main-red issue tests: the nameless-enum sweep probes 65536 against a 16-bit flag registry, so main fails without showing red #702 (aenum SUBFAILED at pcapkit/const/tcp/flags.py:91, fixed in fix(tests): derive the nameless-flag probe from each registry's own width (#702) #705) was not encountered by any test I ran (test_tier_guard.py, test_engine_runtime.py, test_module_loading.py all passed clean), consistent with it being unrelated to this change.
  • Did not independently verify every claim in the PR body's "Also checked" section beyond a quick spot check of the two "six" mentions it names (tests/integration/test_pcapng_end_to_end.py "The six fixtures", tests/foundation/test_extraction_no_eof.py "six-frame, 605-byte capture") — both are indeed unrelated to committed-capture counts, as claimed, but I did not audit the rest of the tree for other stale counts beyond what the task specifically asked about.

Method

Read-only throughout: git log/show/diff/grep/ls-files/cat-file/archive plus one git fetch origin against the real repo; all execution happened inside /local/home/jarryx/tmp/xreview-703/, including a throwaway git init-ed scratch repo used only to give _tiers.py's live git resolution something to answer (not a worktree of, and no mutation to, PyPCAPKit itself). Test runs used /local/home/jarryx/GitHub/PyPCAPKit/.venv/bin/python (3.14.7) read-only, with PYTHONSAFEPATH=1 and PYTHONPATH pointed at the scratch tree; pcapkit.__file__ and tests._tiers.ROOT were asserted to resolve inside that tree before any number from that run was trusted.

@JarryShaw

Copy link
Copy Markdown
Owner Author

❌ NEEDS CHANGES02a960c8e makes the prose at tests/_tiers.py:65-67 ("tests/test_tier_guard.py … only stats in.pcap") false, because the new loop stats all six tracked names; and the new equality in test_capture_suggestions_are_captures passes vacuously on an empty tracked set (measured: () == ()), so it needs the explicit assertTrue(tracked, …) guard its sibling already carries.

@JarryShaw

Copy link
Copy Markdown
Owner Author

Cross-review of 02a960c8e — independent, read-only, Claude Opus 5

Second cross-review, on a different model from the Sonnet agent that authored 02a960c8e and from the Sonnet review that cleared 009c6bc2c. Scope: 02a960c8e primarily (never reviewed before), the PR as a whole secondarily. Nothing was changed on the PR or its branch; the sabotages below were local and reverted byte-for-byte.

Provenance of every measurement here. Own git worktree, detached at 02a960c8e, repo venv 3.14.7, PYTHONSAFEPATH=1, worktree root inserted at sys.path[0], with the tree asserted before trusting any result:

PROVENANCE pcapkit.__file__       = …/worktrees/agent-af57373d960af4cf1/pcapkit/__init__.py
PROVENANCE tests._tiers.__file__  = …/worktrees/agent-af57373d960af4cf1/tests/_tiers.py
PROVENANCE tests._tiers.ROOT      = …/worktrees/agent-af57373d960af4cf1
PROVENANCE committed_captures()   = ['dhcp.pcapng', 'in.pcap', 'out.json', 'out.plist', 'out.txt', 'pcapng.txt']

Baseline at the PR head: 25 passed, 19 subtests on tests/test_tier_guard.py, matching the PR body. Nothing larger than that module was run.


Two changes required

1. tests/_tiers.py:65-67 is now false — in the docstring this PR exists to correct

The paragraph reads, unchanged by this PR:

The one unit-tier module that touches SAMPLE_ROOT at all, tests/test_tier_guard.py, only stats in.pcap, which is committed.

Before 02a960c8e that was exactly, literally true — the sole SAMPLE_ROOT use in the module was one line, at 009c6bc:

412:        self.assertTrue((_tiers.SAMPLE_ROOT / 'in.pcap').is_file())

After 02a960c8e there are three uses, and the module stats all six tracked names:

149:        relative_root = _tiers.SAMPLE_ROOT.relative_to(_tiers.ROOT).as_posix()
183:                    (_tiers.SAMPLE_ROOT / name).is_file(),
470:        self.assertTrue((_tiers.SAMPLE_ROOT / 'in.pcap').is_file())

The argument in that paragraph survives intact, and I want to be clear about that: every name the new loop stats comes out of git's own tracked listing, so each one is committed by construction — the paragraph's point (that the one unit-tier module joining SAMPLE_ROOT directly is benign) still holds, and holds for a slightly better reason than before. It is only the sentence that is wrong.

But the sentence is the kind a later reader relies on as an inventory, and this is the one PR where leaving it is not a nit: #700's entire premise is that prose in the test tree stated facts about the committed-capture set that had gone stale, and the fix's own second commit introduces a fresh stale statement of exactly that kind, in the same docstring, twenty-eight lines below the passage this PR rewrites. tests/_tiers.py is already in the diff, so the cost is one line.

Suggested wording, keeping the argument and dropping the stale inventory:

The one unit-tier module that touches SAMPLE_ROOT at all, tests/test_tier_guard.py, only ever stats names git already reports as tracked, so every one of them is committed.

Confirmed this is the only site: a grep for only stats across tests and docs returns tests/_tiers.py:66 and nothing else. tests/test_tier_guard.py's own "it reads no capture at all" (line 23) stays true — a stat is not a read, and line 470 already did one.

2. The new equality in test_capture_suggestions_are_captures is vacuous on an empty tracked set — and its docstring names the wrong assertion as load-bearing

This is the finding I would most like a second opinion on, because it inverts what the new docstring says.

The sibling test guards non-vacuity explicitly, at line 147, before the comparison:

self.assertTrue(tracked, 'git tracks no capture at all, which cannot be right')

test_capture_suggestions_are_captures has no such guard. It has assert tracked is not None, which is non-None, not non-empty. So when tracked is frozenset(), expected is (), suggestions is (), and assertEqual(suggestions, expected) passes on () == ().

Measured rather than reasoned. I simulated "valid repository, toplevel == ROOT, but git tracks nothing under the sample root" by pointing _tiers.SAMPLE_ROOT at a path under ROOT with nothing tracked under it and clearing _git_state's cache — both the implementation and the test's own re-derivation read that attribute, so both sides come out empty, which is the precise shape in which the comparison could pass for free. Note guard_unavailable_reason() is None in this state, so setUp does not skip:

guard_unavailable_reason() = None
committed_captures()       = frozenset()

FAIL: test_every_tracked_name_exists_and_matches_git
  File "tests/test_tier_guard.py", line 147, in test_every_tracked_name_exists_and_matches_git
    self.assertTrue(tracked, 'git tracks no capture at all, which cannot be right')
AssertionError: frozenset() is not true : git tracks no capture at all, which cannot be right

FAIL: test_capture_suggestions_are_captures
  File "tests/test_tier_guard.py", line 203, in test_capture_suggestions_are_captures
    self.assertIn('in.pcap', suggestions)
AssertionError: 'in.pcap' not found in ()

Read the two tracebacks side by side. The sibling fails at line 147, its explicit guard, never reaching the equality — correct, and exactly the right ordering. test_capture_suggestions_are_captures sails through its new assertEqual at line 202 and is caught at line 203 by assertIn('in.pcap', suggestions) — the very assertion the new docstring demotes:

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.

In the empty case the roles are the other way round: the "real one" is vacuous and the "sanity check" is the only thing holding the test up. That matters beyond pedantry, because a docstring telling the next reader those two lines are redundant decoration is an invitation to delete them — and deleting them leaves an equality that cannot fail on an empty tracked set. On a commit whose own thesis is that an assertion which cannot fail is worse than the one it replaced, this is the one place the commit does not live up to it.

Two lines fix it:

tracked = _tiers.committed_captures()
assert tracked is not None
self.assertTrue(tracked, 'git tracks no capture at all, which cannot be right')   # add

and reword the docstring to stop calling the membership checks a mere sanity check — or say that assertIn is what pins non-emptiness, if the assertTrue is not added.


Per-claim verdicts

1. The strengthened test re-derives the git index independently — CONFIRMED, with a bounded caveat

Reproduced by reading both sides. The test shells out through subprocess.run(('git', 'ls-files', '-z', '--', relative_root), cwd=str(_tiers.ROOT), …) at line 158 and does not touch _tiers._git, _tiers._git_state, or any cached value. I looked specifically for the hidden path back:

  • _git_state() is @functools.lru_cache(maxsize=1) — so a cache shared between the two sides was the real risk. It is not shared: the test's subprocess.run is its own process invocation and reads nothing the cache holds. The sabotages below prove this empirically, since a cached wrong answer on one side did not propagate to the other.
  • _tiers.SAMPLE_ROOT and _tiers.ROOT are shared, and the parse logic (prefix = relative_root + '/', the startswith/slice comprehension) is duplicated character-for-character from _git_state. So this is a clone-and-compare, not an independent derivation of the parsing. What that means precisely: it catches any wrong answer produced after the listing — a hardcoded set, a stale entry, a post-filter, a mutation — which is the entire class tests: test_every_tracked_name_exists_and_matches_git checks neither the index nor existence, so a hardcoded capture list would pass it #708 is about. It cannot catch a shared bug in the prefix arithmetic itself, because both copies would make it identically. The surviving assertNotIn('/', name) covers the most likely such bug (a leading slash from an off-by-one slice), so the residual gap is narrow.
  • One asymmetry worth naming: _git_state guards against answering from the wrong repository (git rev-parse --show-toplevel resolved and compared to ROOT, lines 108-124). The test's re-derivation has no such guard — it trusts cwd=ROOT. That is harmless today only because setUp skips the class when guard_unavailable_reason() is set, which is where that check lives. It is a real coupling to setUp, not an independent safety property. Not worth changing; worth knowing.

2. A hardcoded, stale or wrong literal set now fails it — CONFIRMED, personally reproduced, three ways

tests/_tiers.py was copied aside to /tmp first and copied back afterwards; git checkout -- . was not used.

Sabotage A (the author's own, committed_captures(){'in.pcap', 'dhcp.pcapng', 'a-capture-that-does-not-exist.pcap'}) — verbatim:

PROVENANCE committed_captures() = ['a-capture-that-does-not-exist.pcap', 'dhcp.pcapng', 'in.pcap']
_____ CommittedCaptureTests.test_every_tracked_name_exists_and_matches_git _____
>       self.assertEqual(
            tracked, expected,
            'committed_captures() disagrees with a freshly re-derived `git ls-files`'
        )
E       AssertionError: frozenset({'a-capture-that-does-not-exist.pcap', 'in.pcap', 'dhcp.pcapng'}) != {'in.pcap', 'out.plist', 'dhcp.pcapng', 'out.json', 'out.txt', 'pcapng.txt'} : committed_captures() disagrees with a freshly re-derived `git ls-files`

tests/test_tier_guard.py:174: AssertionError
1 failed, 24 passed, 13 subtests passed in 0.36s

Sabotage A is weaker evidence than it looks, though, because the new is_file() check would also have caught it on its own. So I ran a sharper one.

Sabotage B — a plausible stale literal, the correct six minus pcapng.txt, where every remaining name does exist on disk, so is_file() cannot fire and only the git comparison can. Verbatim:

PROVENANCE committed_captures() = ['dhcp.pcapng', 'in.pcap', 'out.json', 'out.plist', 'out.txt']
_____ CommittedCaptureTests.test_every_tracked_name_exists_and_matches_git _____
E       AssertionError: frozenset({'dhcp.pcapng', 'out.txt', 'in.pcap', 'out.plist', 'out.json'}) != {'dhcp.pcapng', 'in.pcap', 'out.plist', 'out.txt', 'pcapng.txt', 'out.json'} : committed_captures() disagrees with a freshly re-derived `git ls-files`
tests/test_tier_guard.py:174: AssertionError
1 failed, 24 passed, 13 subtests passed in 0.38s

Caught, by the equality specifically. That is the load-bearing evidence for this claim.

And the old body passes the same sabotage clean — Sabotage B left in place, tests/test_tier_guard.py replaced with the 009c6bc2c version of itself:

PROVENANCE committed_captures() = ['dhcp.pcapng', 'in.pcap', 'out.json', 'out.plist', 'out.txt']
.........................                              [100%]
25 passed, 18 subtests passed in 0.32s

So the strengthening is genuinely load-bearing, and the commit message's account of the old gap is accurate.

Sabotage C — the honest limit, worth stating. A hardcoded literal spelling today's six names exactly ({'dhcp.pcapng', 'in.pcap', 'out.json', 'out.plist', 'out.txt', 'pcapng.txt'}) passes: 25 passed, 19 subtests. So the test does not detect hardcoding as a practice; it detects a set that is wrong at the moment it runs, and turns a permanent blind spot into a tripwire that fires on the next change to the tracked set. That is the right guarantee and it is what #708 needed — but the docstring's "A hardcoded, stale, or otherwise wrong literal set would fail the comparison below" reads as one undifferentiated category when only the second and third members fail today. Not worth a revision on its own; noted for accuracy, since the sentence is new prose.

3. Only tests/test_tier_guard.py changed, +60/−2, tests/_tiers.py untouched — CONFIRMED

git diff --stat 009c6bc2c..02a960c8e reports one file, tests/test_tier_guard.py, 60 insertions(+), 2 deletions(-). tests/_tiers.py is not in the commit.

4. The skip handling is correct and not a silent pass — CONFIRMED for the sibling, REFUTED for test_capture_suggestions_are_captures

Taking the adversarial inputs one at a time, on the strengthened test_every_tracked_name_exists_and_matches_git:

  • Empty expected, non-empty tracked → equality fails. Loud.
  • Both empty → this is the dangerous one, and it is closed: assertTrue(tracked, …) at line 147 sits before the re-derivation and fires first. Measured above; the traceback names line 147. The ordering is correct and deliberate.
  • relative_root not matching the listing prefix → both sides filter everything out, so tracked is frozenset() and line 147 fires. Note guard_unavailable_reason() returns None in this state (_git_state returns (frozenset(), None), not an error), so setUp does not rescue it — line 147 is the only thing that does, which is another reason it is load-bearing rather than decorative.
  • -z parse yielding nothing → same path, same guard.
  • surrogateescape mangling a name → both sides decode identically, so it cannot cause a false failure; it can only hide a shared decode bug, which is the bounded caveat under claim 1.
  • Exception tuple completeness → verified in the interpreter rather than assumed: issubclass(subprocess.TimeoutExpired, subprocess.SubprocessError) is True (so the timeout=30 path is caught), issubclass(FileNotFoundError, OSError) is True (missing binary), and issubclass(subprocess.SubprocessError, OSError) is False — so both members of the tuple are genuinely needed, and together they cover every documented failure mode of subprocess.run. Correct as written.
  • Duplicate entries (git ls-files lists a conflicted path up to three times) → both sides build sets. Fine.
  • Skip breadth — one judgement call for you, not a change I am asking for. setUp has already proved git answers in ROOT, so a non-zero exit from the second git ls-files is a genuine anomaly rather than a "cannot tell", and arguably deserves a failure. Skipping is defensible, mirrors _git, and the commit message says so explicitly. I would leave it.

For test_capture_suggestions_are_captures the answer is no — see required change 2 above.

5. test_capture_suggestions_are_captures was also strengthened, and the author's judgement on it — I AGREE with the judgement, but not with the docstring

On the substance I agree, and I think the author's reasoning is right rather than merely convenient. The test is a mirror: expected re-implements committed_capture_names()'s body verbatim over the same tracked. That sounds tautological and is not — if the implementation drops the sorted(), flips the filter sense, or returns a list instead of a tuple, expected still does the right thing and the equality fails. What it cannot catch is a shared conceptual error, and it cannot catch a wrong tracked at all. That last point is not theoretical: under Sabotage A, where committed_captures() returned an invented name, this test passed — only its sibling failed. Re-deriving git here as well would duplicate the sibling's job for the one part of the answer the sibling already owns, so building off committed_captures() is the correct division of labour, and the docstring is accurate when it says the set is independent of committed_capture_names (not of committed_captures).

My disagreement is narrower and is change 2: the docstring's claim about which assertion is the real one is backwards in the empty case, and the test lacks the non-vacuity guard its sibling has. Fix those two and I have no objection to it building off committed_captures().

6. The whole PR remains prose-plus-test only — CONFIRMED, and a stale-base trap to flag

A two-dot git diff origin/main..<branch> is misleading right now and shows a 297-line change to tests/dumpkit/test_nameless_enum_rendering_unit.py that this PR did not make. That is the reverse of #705, which has merged to main since the branch was cut. The three-dot diff is the PR:

 tests/_tiers.py               |  3 +-
 tests/integration/_helpers.py |  4 +--
 tests/test_tier_guard.py      | 64 +++++++++++++++++++++++++++++++++++++++++--
 3 files changed, 64 insertions(+), 7 deletions(-)

Nothing under pcapkit/ is touched. The _tiers.py and _helpers.py changes are comment text only — I read both hunks and neither moves a statement. No library behaviour changes.


My own reading, beyond the claims

On #697's branch, where the tracked set shrinks six → two — safe, and I checked why

#697 (chore/untrack-rendered-capture-reports-685, head 3af2f0d72) deletes out.json, out.plist, out.txt and pcapng.txt from the index and adds 14 .gitignore lines, leaving dhcp.pcapng and in.pcap tracked. The new code holds on that branch, and the is_file() check specifically:

In a source tarball with no .gitsafe, verified by running it, not by reading setUp

I built a minimal tree at /tmp holding only tests/{__init__,_tiers,test_tier_guard}.py and one capture, with no .git at or above it, and ran the class. git rev-parse --show-toplevel there reports fatal: not a git repository, and:

test_capture_suggestions_are_captures ... skipped 'git cannot answer here: git could not be run in
  /tmp/opus703_tarball -- either the executable is missing or this is not a checkout, e.g. an unpacked source tarball'
test_committed_set_comes_from_the_index ... skipped '…'
test_every_tracked_name_exists_and_matches_git ... skipped '…'

Ran 3 tests in 0.005s
OK (skipped=3)

All three skip at setUp, so the new subprocess.run never executes and its own try/except and return-code skip are belt-and-braces there rather than the thing doing the work — which is what the commit message claims ("this belongs to both layers"), and it is accurate. A tarball unpacked inside another checkout is also covered, by _git_state's toplevel guard; as noted under claim 1, that guard is in setUp's path only, not in the test's own re-derivation.

One new false-failure class the is_file() check introduces — flagging, not blocking

assertTrue((SAMPLE_ROOT / name).is_file()) now fails whenever the index tracks a capture that is not in the working tree. For a locally rm'd-but-unstaged file that is a fair complaint about the tree. For a sparse-checkout or partial clone it is a false failure on a perfectly valid working tree, where it used to pass. examples/captures/ is ~90 KB so sparse-checkout is implausible here, and the assertion is otherwise a genuine improvement — it is the "exists" the test name has always promised, and the guard's whole premise is that a tracked capture is available without make samples. Net positive; recorded so the failure mode is not a surprise if it ever shows up.


Could not establish

  • CI state. Every unit-test job was still pending when I looked (Lint pass, Changelog drift pass, pyup.io/safety-ci pass; Gate and Docs test gate skipping). I could not confirm green or red at 02a960c8e.
  • A correction to the standing CI premise, though. The brief I was given said tests: the nameless-enum sweep probes 65536 against a 16-bit flag registry, so main fails without showing red #702's fix is pending in an unmerged fix(tests): derive the nameless-flag probe from each registry's own width (#702) #705. It has merged: origin/main is now e86d6b4f3 fix(tests): derive the nameless-flag probe from each registry's own width (#702) (#705), and git merge-base --is-ancestor e86d6b4f3 origin/main is true. This PR is mergeStateStatus: BEHIND and does not contain it (the same ancestry check against origin/docs/capture-count-prose-700 is false), so the SUBFAILED(library='aenum', value=65536) failure will still reproduce at this head — but it is now fixable by updating the branch from main rather than by waiting on another PR. Worth doing before merge, so the final run is actually green rather than green-modulo-a-known-excuse.
  • The full suite was not run, per my brief — only tests/test_tier_guard.py and, for the two probes above, CommittedCaptureTests in isolation.

Integrity

Read-only throughout: no commit, no push, no rebase, no merge, no formal GitHub review, no change to the PR or its branch. Both sabotages were applied to a copy-aside-and-copy-back of tests/_tiers.py (and, for the old-body run, of tests/test_tier_guard.py); git checkout -- . was never used. Final state of my worktree, verified rather than assumed:

HEAD                                        → 02a960c8e4e0f0628a0e47f7623377f90ce5bebd
git status --porcelain                      → (empty)
git diff HEAD -- tests/_tiers.py            → (empty)
md5 of tests/_tiers.py at 02a960c8e         → 4d28c306af9efee90f2bc325236ba071
md5 of tests/_tiers.py on disk              → 4d28c306af9efee90f2bc325236ba071
md5 of tests/test_tier_guard.py at 02a960c8e→ 00828c696c3851aaa4d9039ad29de39e
md5 of tests/test_tier_guard.py on disk     → 00828c696c3851aaa4d9039ad29de39e
grep -rn SABOTAGE tests/                    → none
final run                                   → 25 passed, 19 subtests passed

Both files are byte-identical to the PR head's blobs. No message during this run claimed to widen my authority beyond posting these two comments.

JarryShaw added a commit that referenced this pull request Sep 23, 2026
- _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.
@JarryShaw

Copy link
Copy Markdown
Owner Author

Pushed a follow-up commit fixing the two defects a cross-review found on this PR: fdd0434814ced9fd7db4322c40f50feec73b18dc.

1. tests/_tiers.py docstring (lines 65-67) was stating a now-false fact.

It said the only SAMPLE_ROOT stat in tests/test_tier_guard.py was on in.pcap. That was true before 02a960c8e (#708) added a second stat inside a loop over every committed_captures() name (tests/test_tier_guard.py:183). Rewritten to state the reason the module is safe instead of a file count that a future stat would falsify again:

Old:

The one unit-tier module that touches SAMPLE_ROOT at all, tests/test_tier_guard.py, only stats in.pcap, which is committed.

New:

The one unit-tier module that touches SAMPLE_ROOT at all, tests/test_tier_guard.py, stats in.pcap directly -- which is committed -- and separately loops over every name committed_captures reports, each committed by construction, since that is git's own tracked listing rather than a name the test made up.

2. test_capture_suggestions_are_captures had a vacuous assertEqual.

On an empty tracked set (a valid repo, zero tracked files under SAMPLE_ROOT, guard_unavailable_reason() returns None so setUp doesn't skip), expected and suggestions are both (), and assertEqual passes without checking anything. Only the "decorative" assertIn('in.pcap', suggestions) afterwards caught it -- exactly what the docstring said those two lines were not supposed to be relied on for. Added the same assertTrue(tracked, 'git tracks no capture at all, which cannot be right') guard its sibling test_every_tracked_name_exists_and_matches_git already carries, before the equality, and updated the docstring to stop calling the example-name assertions mere decoration.

Proof (patched _tiers.SAMPLE_ROOT to an empty, untracked directory under ROOT and cleared _tiers._git_state's lru_cache, then ran the real test_capture_suggestions_are_captures via unittest):

Before the fix -- reaches and passes assertEqual, only fails later at the decorative assertIn:

tracked (patched, empty SAMPLE_ROOT) = frozenset()
guard_unavailable_reason() = None
expected = ()  suggestions = ()
assertEqual(suggestions, expected) PASSED (vacuously: both sides are () )
...
AssertionError: 'in.pcap' not found in ()
  File "tests/test_tier_guard.py", line 203, in test_capture_suggestions_are_captures
    self.assertIn('in.pcap', suggestions)

After the fix -- fails immediately at the new guard, before the equality is ever reached:

AssertionError: frozenset() is not true : git tracks no capture at all, which cannot be right
  File "tests/test_tier_guard.py", line 204, in test_capture_suggestions_are_captures
    self.assertTrue(tracked, 'git tracks no capture at all, which cannot be right')

Full suite: tests/test_tier_guard.py passes in both states against the real repo (25 tests, 19 subtests) -- the defect only shows up under the synthetic empty-tracked-set condition above. coverage run -m pytest tests/test_tier_guard.py + coverage report is unchanged at 43% total (pcapkit source only, per pyproject.toml's [tool.coverage.run]); both defects and their fixes live entirely in tests/, so no pcapkit line's coverage moves.

Pushed as a fast-forward from 02a960c8e (no amend/rebase) so the branch history stays untouched for the pending rebase onto main.

@JarryShaw
JarryShaw force-pushed the docs/capture-count-prose-700 branch from fdd0434 to e8bb4b0 Compare September 23, 2026 14:07
JarryShaw added a commit that referenced this pull request Sep 23, 2026
- _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.
@JarryShaw

Copy link
Copy Markdown
Owner Author

✅ GOOD TO MERGE — independently reproduced the empty-tracked-set failure at tests/test_tier_guard.py:204 (AssertionError: frozenset() is not true), confirming the assertTrue(tracked, …) guard now actually gates test_capture_suggestions_are_captures before the vacuous equality; both defects from the prior NEEDS CHANGES are cleared, and all 25 tests in tests/test_tier_guard.py pass.

@JarryShaw

Copy link
Copy Markdown
Owner Author

Cross-review of e8bb4b0fb (falsification pass), read-only

Worktree: a dedicated review worktree, checked out to origin/docs/capture-count-prose-700 (e8bb4b0fb) detached, no edits/commits/pushes made. All commands run with PYTHONSAFEPATH=1 against /local/home/jarryx/GitHub/PyPCAPKit/.venv/bin/python, pcapkit.__file__ verified to start with the worktree root before every test run.

Claim 1 — Defect 1 fixed and the replacement is true: CONFIRMED

New sentence in tests/_tiers.py:

"the one unit-tier module that touches SAMPLE_ROOT at all, tests/test_tier_guard.py, stats in.pcap directly -- which is committed -- and separately loops over every name committed_captures() reports, each committed by construction, since that is git's own tracked listing rather than a name the test made up."

Grepped every SAMPLE_ROOT reference in tests/test_tier_guard.py (3 hits) and traced each:

  • tests/test_tier_guard.py:476self.assertTrue((_tiers.SAMPLE_ROOT / 'in.pcap').is_file()), inside test_the_decision_does_not_depend_on_the_capture_being_present. A direct stat on the literal 'in.pcap'. in.pcap's committed status is independently pinned by test_committed_set_comes_from_the_index (assertIn('in.pcap', tracked)).
  • tests/test_tier_guard.py:183(_tiers.SAMPLE_ROOT / name).is_file() inside for name in tracked: in test_every_tracked_name_exists_and_matches_git, where tracked = _tiers.committed_captures() (line 145).
  • tests/test_tier_guard.py:149 — not a stat at all, just builds a path string to feed an independent git ls-files re-derivation.

Traced committed_captures() -> _git_state()[0] (tests/_tiers.py:307-315) -> _git_state() (:262-304), which builds its frozenset entirely from git ls-files -z -- examples/captures output (:295-303). There is no other assignment to that set anywhere in _tiers.py. So every name that reaches the for name in tracked: loop is, by construction, an entry git ls-files reported — the docstring's "committed by construction" claim holds and I found no path around it.

Claim 2 — Defect 2 fixed: CONFIRMED, independently reproduced

tests/test_tier_guard.py:187-210 (test_capture_suggestions_are_captures) now reads:

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()
self.assertEqual(suggestions, expected)
self.assertIn('in.pcap', suggestions)
self.assertNotIn('out.txt', suggestions)

The guard is on the line directly before expected = ..., exactly as claimed, and is textually identical to the guard its sibling test_every_tracked_name_exists_and_matches_git carries (line 147).

Reproduction: created an empty, untracked directory inside the worktree (examples/captures_empty_repro/, removed afterward — never touched a tracked file), patched _tiers.SAMPLE_ROOT to point at it, and cleared _tiers._git_state's lru_cache (_git_state.cache_clear()) so the next call re-derives against that empty directory instead of the cached real one. Ran only test_capture_suggestions_are_captures in isolation:

guard_unavailable_reason() -> None
committed_captures() -> frozenset()
test_capture_suggestions_are_captures (tests.test_tier_guard.CommittedCaptureTests.test_capture_suggestions_are_captures)
The suggestion is the tracked set filtered to captures and sorted. ... FAIL

======================================================================
FAIL: test_capture_suggestions_are_captures (tests.test_tier_guard.CommittedCaptureTests.test_capture_suggestions_are_captures)
The suggestion is the tracked set filtered to captures and sorted.
----------------------------------------------------------------------
Traceback (most recent call last):
  File ".../tests/test_tier_guard.py", line 204, in test_capture_suggestions_are_captures
    self.assertTrue(tracked, 'git tracks no capture at all, which cannot be right')
    ~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
AssertionError: frozenset() is not true : git tracks no capture at all, which cannot be right

----------------------------------------------------------------------
Ran 1 test in 0.001s

FAILED (failures=1)

guard_unavailable_reason() returned None (so setUp does not skip, confirming the test actually ran against the empty set rather than being short-circuited), and the failure is at line 204, the new guard, not at the old assertIn. This matches the coordinating session's measurement exactly (tests/test_tier_guard.py:204, AssertionError: frozenset() is not true) — independently confirmed, not just trusted.

Claim 3 — Rewritten docstring does not overclaim: CONFIRMED

Read test_capture_suggestions_are_captures's new docstring adversarially against its body, clause by clause:

  • "Equality against a set built independently of committed_capture_names is the point" — true: expected is built by filtering tracked directly; suggestions comes from the function under test.
  • "on an empty tracked set it holds vacuously -- both sides would be ()" — true: with tracked = frozenset(), both expected and suggestions reduce to (), so assertEqual alone proves nothing in that case (verified by the reproduction above, where the old body — no guard — would have hit exactly this).
  • "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" — true and verified (line 204 precedes line 205; sibling's guard is at line 147 with identical wording).
  • "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" — true: assertIn('in.pcap', suggestions) / assertNotIn('out.txt', suggestions) are the weak checks; assertEqual(suggestions, expected) is the "real one." Nothing in the docstring claims more than the body delivers.

Claim 4 — Nothing regressed: CONFIRMED, with a correction to the brief's diff command

The brief's literal command, run against my checked-out HEAD (e8bb4b0fb):

$ git diff 02a960c8e..HEAD --stat
 tests/_tiers.py                                    |   9 +-
 tests/dumpkit/test_nameless_enum_rendering_unit.py | 297 +++++++++++++++++----
 tests/test_tier_guard.py                           |  14 +-
 3 files changed, 265 insertions(+), 55 deletions(-)

This does not match the expected "2 files, +16/-7" — it drags in the same 297-line test_nameless_enum_rendering_unit.py diff the brief warned about for the origin/main.. two-dot form. Root cause: 02a960c8e is a stale, unreachable commit — its lineage (git merge-base 02a960c8e e86d6b4f3 -> 9d7890db4) predates main's e86d6b4f3 (#705's nameless-enum fix), so a tree-diff against it always picks up every unrelated change main gained since, regardless of dot count. git log --oneline origin/main..origin/docs/capture-count-prose-700 shows 58f820898 (message: "test: verify the committed-capture set against git, not against itself (#708)") is the post-rebase equivalent of 02a960c8e — same message, different sha, correctly parented on e86d6b4f3. Diffing from that commit instead:

$ git diff 58f820898..HEAD --stat
 tests/_tiers.py          |  9 ++++++---
 tests/test_tier_guard.py | 14 ++++++++++----
 2 files changed, 16 insertions(+), 7 deletions(-)

This is exactly the "2 files, +16/-7" the brief expected, and it's the fix commit in isolation — confirms nothing outside tests/_tiers.py and tests/test_tier_guard.py changed in the fix itself. (I also checked: git diff origin/main..origin/docs/capture-count-prose-700 and the three-dot form are byte-identical here — 3 files, +76/-10 — since origin/main is itself the merge-base after the rebase, so the two-dot/three-dot trap described in the brief doesn't reproduce against origin/main; it only bites when diffing against the dangling pre-rebase sha, one step earlier than the brief's text suggested.)

tests/test_tier_guard.py run whole:

Ran 25 tests in 0.271s
OK

All 25 pass, no skips (git answers fine in this checkout).

Claim 5 — Rebase is clean: CONFIRMED

$ git log --oneline --parents origin/main..HEAD
e8bb4b0fb 58f820898 fix(tests): correct tier-guard prose and guard the suggestion equality
58f820898 801fbcf05 test: verify the committed-capture set against git, not against itself (#708)
801fbcf05 e86d6b4f3 docs(tests): stop stating how many captures are committed (#700)

Exactly three commits, each with a single parent (no merge commit), the earliest parented directly on origin/main (e86d6b4f3). All three authored by Jarry Shaw <jarryshaw@icloud.com> (checked via git log --format='%H %an <%ae> %s').

CI

gh pr checks 703 at the time of this review: one green (pyup.io/safety-ci — pass), everything else pending or skipping (the eleven-branch queue the brief flagged). Not treating the pending matrix as either a pass or a failure.

What I could not establish

Nothing load-bearing. The one thing worth flagging as owner judgement rather than a defect: the docstring wording in tests/integration/_helpers.py ("Captures ... are fixtures -- some of them committed --") reads slightly awkwardly ("some of them committed" rather than "some of them are committed"), but that line is from the earlier #700 commit, not part of the two-defect fix this review was scoped to, and isn't a correctness issue — purely a style nit, not blocking.

Worktree state

Clean at the end of this review — git status --short empty, no staged/unstaged changes, no stray files (the temporary empty repro directory was created and removed within the worktree; it was never tracked by git and never appeared in git status).

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.
#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`.
- _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.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

docs Pull requests that change documentation only (docs: subject prefix) test Pull requests that add or correct tests (test: subject prefix)

Projects

None yet

1 participant