Conversation
|
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
The four in question are Worth noting that These all live under 2. No test would catch the tier guard's live resolution being replaced by a hardcoded listRelevant 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.
Extending that one method to re-derive the tracked set and Confirmed as part of the same survey, and independently of it: neither |
|
GOOD TO MERGE Independent cross-review, different model from the PR's author (Claude Sonnet 5). Head reviewed: Per-claim verdicts1. All three passages are count-free and accurate before and after #697 — CONFIRMED.
2. No executable line changed — CONFIRMED.
3. Live resolution untouched, no hardcoded list/count in code — CONFIRMED. 4. Tier guard still passes, subtests checked — CONFIRMED. 5. Scope — CONFIRMED. 6. The "no coverage increase is possible" argument — CONFIRMED sound, and I agree no worthwhile test was skipped. at exactly those lines — The two trapsTrap one (the #697 survey) — all four observations CONFIRMED, verbatim, against
Trap two (the self-contradicting test) — CONFIRMED accurate. 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 What I could not verify
MethodRead-only throughout: |
|
❌ NEEDS CHANGES — |
Cross-review of
|
- _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.
|
Pushed a follow-up commit fixing the two defects a cross-review found on this PR: 1. It said the only Old:
New:
2. On an empty tracked set (a valid repo, zero tracked files under Proof (patched Before the fix -- reaches and passes After the fix -- fails immediately at the new guard, before the equality is ever reached: Full suite: Pushed as a fast-forward from |
fdd0434 to
e8bb4b0
Compare
- _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.
|
✅ GOOD TO MERGE — independently reproduced the empty-tracked-set failure at |
Cross-review of
|
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.
e8bb4b0 to
cbcdaf4
Compare
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: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.tests/test_tier_guard.py— correct onmainas next-after-six, "a third" after #697:No code changed
Confirmed before starting, as #700 asks:
committed_captures()resolves the set live intests/_tiers.pyby 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:
source = ["pcapkit"](pyproject.toml), so the test tree is not measured at all.coverage.parser.PythonParser, the executable statement set is identical in all three files —tests/_tiers.py160/160,tests/integration/_helpers.py51/51,tests/test_tier_guard.py164/164 — differing only by the one-line offset the shortened_tiers.pydocstring 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.pyerror anyway, since that count was simply wrong rather than formatted unusually.Runs (
coverage run -m pytest, repo venv 3.14.7,PYTHONSAFEPATH=1, worktree onPYTHONPATH, withpcapkit.__file__andtests._tiers.ROOTasserted to resolve inside the worktree before trusting any of it):tests/test_tier_guard.py— 25 passed, 19 subtests, unchanged from the pre-edit baseline. The 19 is 15 plus one subtest per extra tracked report, consistent with tests: three comments state a committed-capture count that is wrong or rots on the next change #700's count of 15 subtests against chore(examples): stop tracking the four rendered capture reports (#685) #697's two-capture branch.tests/integration/test_engine_runtime.pyandtests/integration/test_module_loading.py— the end-to-end modules that reach the edited helper needing only committed captures. 30 passed, 19 subtests together with the guard.The rest of
tests/integration/needsmake samplesto have run, which writes intoexamples/**and is out of scope for this branch, so it was not run here.Also checked
tests/integration/test_pcapng_end_to_end.pysays "The six fixtures", but that is the count of generated pcapng fixtures fromexamples/generators/pcapng.py(six:dhcp,dhcp_big_endian,dhcp_little_endian,many_interfaces,test,profile, of which onlydhcp.pcapngis committed) — not a committed-capture count, and correct. Left alone. Likewise the frame counts intests/foundation/test_extraction_no_eof.py("six-frame, 605-byte capture") count frames inside one capture, not captures.examples/legacy_smoke/README.rstbelongs to #697 and is untouched, as #700 notes.