Skip to content

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

Open
JarryShaw wants to merge 1 commit into
mainfrom
docs/capture-count-prose-700
Open

JarryShaw wants to merge 1 commit into
mainfrom
docs/capture-count-prose-700

Conversation

@JarryShaw

Copy link
Copy Markdown
Owner

Fixes #700.

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.

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

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

Development

Successfully merging this pull request may close these issues.

tests: three comments state a committed-capture count that is wrong or rots on the next change

1 participant