Conversation
* `examples/captures/pcapng.txt` recorded `packet -> NIL` for four blocks that had been carrying their octets since #683, and nothing had noticed, because nothing reads a rendered report back: no test asserts on one, and `docs/source/demo.rst` only quotes them as hand-written transcripts. * Regenerating them on every parser change is work with no reader, so the four reports -- `out.json`, `out.plist`, `out.txt` and `pcapng.txt` -- are no longer tracked. `.gitignore` loses their un-ignore exceptions and the existing `examples/captures/*` line covers them; they stay on disk for anyone who has them already. * `in.pcap` and `dhcp.pcapng` stay tracked. They are inputs nothing in the repository regenerates, and the unit tier reads both on a fresh clone. * `examples/legacy_smoke/` is the only thing that rebuilds the reports -- `make fixtures`, not `make samples`, since `examples/generators/make_samples.py` never produced them. That README section and the Makefile comments called them "committed"; they now say what a fresh clone has to run, and why the reports are not worth tracking. * No packaging change. `MANIFEST.in` already prunes `examples/captures` wholesale, and the build is plain setuptools with no git-aware file finder, so the sdist and wheel listings are identical either way -- measured, and neither artifact ever carried a single file from that directory. * The tier guard needs no change either: `tests._tiers.committed_captures()` asks `git ls-files` rather than holding a list of names, so it reports two tracked captures instead of six on its own. tests/project/test_capture_tracking.py is new -- 5 tests and 8 subtests -- and pins that git tracks no rendered report, that both inputs stay tracked, and that no module under tests/ reads a report through `sample_path()`. Two of its tests fail across four subtests with the reports restored to the index, which is the regression it exists for. It skips where git cannot answer, so an unpacked source tarball still runs. No pcapkit module changes, so library coverage is unmoved by construction. Fixes #685
JarryShaw
added
the
test
JarryShaw
added
the
chore
|
GOOD TO MERGE Independent cross-review, different model from the PR's author (Claude Sonnet 5 here). Reviewed head Per-claim verdicts, each derived independently: 1. Nothing reads the four reports' contents — CONFIRMED. Re-ran
2. Sphinx docs never read them off disk — CONFIRMED. 3. MANIFEST.in makes this packaging-neutral — CONFIRMED, with one number to correct. 4. The tier guard adapts without change — CONFIRMED. Read 5. The new test genuinely fails without the change — CONFIRMED, reproduced all three scenarios in the scratch clone (never mutating the real checkout):
6. The .gitignore edit is exactly right — CONFIRMED, with direct behavioral testing, not just a diff read. 7. The prose changes are true — CONFIRMED, and the correction about 8. Docs-only/test-only scope — CONFIRMED. Could not verify / not attempted: I did not run the whole test suite (out of scope per the brief), did not verify wheel-building (only sdist, as the brief asked), and did not try to reproduce the CI runner environment for Nothing else disputed. The one correction (sdist count 872, not 871, for the head tree — the base-tree count is 871) is cosmetic and doesn't require a code change. |
This branch has not been deployed
1 participant
Please follow the guide below
make testpasses, and a test case covers the changeWhat is the purpose of your pull request?
fix— corrects a defectfeat— adds a featureperf— changes performance, not behaviourrefactor— changes neither behaviour nor performancetest— tests onlydocs— documentation onlyci— workflows or build toolingchore— anything else(No
chorelabel exists in this repository, so this is labelledtest— the closest available, and honest, since the PR adds a test module. Worth creating achorelabel if that category should be distinguishable in the generated release notes.)Description
Fixes #685.
examples/captures/pcapng.txtrecordedpacket -> NILfor four blocks that hadbeen carrying their captured octets since #683. The staleness was found by
someone reading the file, not by anything failing — which is the whole problem.
The fix is not to regenerate the file. Four of the six files that were
tracked under
examples/captures/are build output, not fixtures, and a trackedbuild output with no reader goes stale silently every time the renderer changes:
out.json,out.plist,out.txtexamples/legacy_smoke/test_extractor.py,test_basic.pypcapng.txtexamples/legacy_smoke/test_pcapng.pyin.pcapdhcp.pcapngSo the four reports are untracked (
git rm --cached, files left on disk), their!examples/captures/…exceptions dropped from.gitignoreso the existingexamples/captures/*line covers them, andin.pcap/dhcp.pcapngstaytracked. A fresh clone now has exactly those two.
Verification
Nothing reads the four files. Every reference is a bare string literal or a
tmpdir path, checked individually:
tests/cli/test_main.py:83,86(an argparse assertion on the string'out.json'),tests/foundation/test_extraction.py:275,293,547(alltemp / 'out.json'),tests/protocols/internet/test_esp_unit.py:976(os.path.dirname(make_pcap(...)),a tmpdir),
tests/test_tier_guard.py:146andtests/_tiers.py:146(anassertNotInand its explanatory comment). Nosample_path()call anywherenames one — now enforced, see below.
The docs only quote them as prose.
docs/source/demo.rst:17,19,80,93,101,107,115sit inside
code-blockdirectives and console transcripts. There is noliteralinclude, noinclude::, nocsv-table :file:, noraw :file:and nodoctest extension anywhere in
docs/—conf.py'sextensionslist has nosphinx.ext.doctest, anddeploy-pages.ymlruns the plain HTML builder. Thedocs build reads none of these files from disk.
The tier guard needs no change.
tests/_tiers.committed_captures()runsgit ls-files -z -- examples/capturesrather than holding a list of names, so itreports the new state on its own; it now answers
['dhcp.pcapng', 'in.pcap'].tests/test_tier_guard.pyis 25 passed / 15 subtests passed against the change.No packaging change, measured rather than reasoned.
MANIFEST.in:10alreadyprunes
examples/captureswholesale, the backend is plainsetuptools.build_metawith no
setuptools_scmor any other git-aware file finder (so tracked-nessnever reached the file list), and
[tool.setuptools.packages.find]only takespcapkit*. An sdist built from this tree has 871 entries and zero underexamples/captures— not evenin.pcapordhcp.pcapng, which stay tracked.The wheel never contained
examples/at all. SoMANIFEST.inneeds no matchingchange, and the published artifacts are identical either way.
Regeneration was already documented, and was already slightly wrong.
examples/legacy_smoke/has amake fixturestarget for exactly these fourfiles. Note it is
make fixtures, notmake samples—examples/generators/make_samples.pynever produced them, so CI'sfixture-generation step would not have either. The README section and the
Makefile comments called them "committed"; they now say what a fresh clone has to
run, and why the reports are not worth tracking. One pleasant side effect:
README.rst:23-24already claimed "Onlyin.pcapanddhcp.pcapngarecommitted", which was false while six files were tracked and is now true.
Test
tests/project/test_capture_tracking.pyis new — 5 tests, 8 subtests — pinningthe invariant this change establishes, since the regression is one
git add -faway and looks like nothing in a diff review:
examples/captures/ends in a capture suffix (statedby suffix, so a new input fixture needs no edit here while a committed report
fails at once);
vacuously on a checkout that tracks nothing there;
tests/, in either tier, reads a report throughsample_path(). This is not covered by the tier guard's own audit, which onlyflags unit-tier reads of untracked captures — these four are worse than
untracked, because fixture generation does not rebuild them either.
Shown to fail without the fix. With the four restored to the index via
git add -f, two of the five tests fail across four subtests:It also degrades where git cannot answer, so an unpacked source distribution
still runs: 4 skipped with the reason quoted, 1 passed, verified against a
git checkout-indexexport outside any repository.Targeted runs, all green:
tests/project/+tests/test_tier_guard.py→149 passed, 494 subtests passed;
tests/test_docstring_contract.py→ 7 passed.On coverage: this change touches no
pcapkit/module, so library coveragecannot move and does not — stated plainly rather than implied.
tests/_tiers.pyalso holds at 87%, because the new tests exercise paths its own suite already
covered. What the new module adds is a regression guard over repository state,
which coverage cannot measure; it is demonstrated by the failure run above
instead.
Follow-ups, not touched here
Three comments elsewhere now understate the count, none of which affects a test.
Left alone because they are outside this change's files:
tests/_tiers.py:36-37— "there are six today, not the two the rule started with" → two again.tests/integration/_helpers.py:58-59— "four of them are committed" → two.tests/test_tier_guard.py:15— "the moment a seventh capture is committed" → a third.