test(utilities,foundation): pin the quiet StreamEOFError convention and register_extractor_engine's keyword - #577
Conversation
…nd register_extractor_engine's keyword Two gaps the #514 keyword audit turned up while checking whether an abandoned local change was still needed. Neither had a test before, and both turned out to already be correct on main -- just unguarded. - StreamEOFError's docstring did not say that @prepare always raises it with quiet=True (the same end-of-stream convention StructError follows via its own eof=True), and nothing pinned that silence. Document it, and add a test proving @prepare's StreamEOFError logs nothing, with a loud control proving the silence comes from quiet=True and not from StreamEOFError having stopped logging altogether. - register_extractor_engine's real keyword is `name`, not `engine`; an earlier audit (noted on #557) found it documented the other way around. The docstring was already fixed, but the keyword itself was never under test, so a future rename could put docstring and signature back out of step exactly as quietly as before. Pin both directions: `name=` registers, `engine=` raises TypeError. The rest of the inherited local change -- docstring edits to Engine/Reassembly/TraceFlow's __init_subclass__ and two tests asserting that an unrecognised `name=` class keyword is silently swallowed -- is superseded by #547/#557, which made registration opt-in and rejects an unrecognised class keyword with UnsupportedCall instead of swallowing it. Confirmed by rebase conflict (the Args: text they edited no longer exists) and by running the swallowed-keyword tests against current main, where they fail because the keyword is now rejected loudly rather than ignored. Build: full unit tier (pytest -q --ignore=tests/integration --ignore-glob='*_runtime.py' --ignore-glob='*_regression.py'): 1123 passed, 5 skipped, 2704 subtests passed, exit 0. Both new tests proven to fail without their fix.
|
✅ GOOD TO MERGE — at Cross-model review (Opus 5; PR authored on Sonnet). Reviewed first at What I derived myself rather than took from the description:
Two things I did not verify, and one note worth acting on eventually, in the detailed comment. Nothing blocking. |
Detailed cross-review — #577 @
|
| run | exit | result |
|---|---|---|
pytest tests/utilities/test_quiet_exceptions.py tests/foundation/registry/test_foundation_keyword_names.py tests/utilities/test_decorators.py tests/foundation/registry/test_foundation.py |
0 |
36 passed, 14 subtests |
mypy pcapkit/utilities/exceptions.py |
0 |
Success: no issues found in 1 source file |
util/changelog_md.py --check |
0 |
CHANGELOG.md in step with docs/source/changelog/1.5.0.rst |
The 36/14 tally matches the PR body's claim exactly.
What I could not verify
Stating these rather than implying coverage I do not have:
- The full unit tier — not reproduced by me. I started
pytest -q --ignore=tests/integration --ignore-glob='*_runtime.py' --ignore-glob='*_regression.py'at118daf844and stopped it at roughly 51% when the head moved, rather than report a measurement of a superseded tree. So the body's 1123 passed, 5 skipped, 2704 subtests is unconfirmed by me. I judged the residual risk low: the only production change is an eight-line docstringNote:, and the plausible cross-file hazard — the new test file'spurge_modules(['pcapkit'])polluting later tests — is already exercised by the four-file run above, andpurge_modulesis an established pattern in this suite rather than something this PR introduces. Low is not zero, and CI covers it. - The abandoned worktree's original contents, and the rebase conflicts in
engine.py/reassembly.py/traceflow.py. The uncommitted source tree is not available to me, so the account of what was found and what conflicted is unfalsifiable from here. What I can confirm is the consequence the PR draws from it — that those three__init_subclass__docstrings no longer contain theArgs:text being edited, and that the dropped tests' behaviour no longer exists — and both hold. - CI, which I did not wait on, per standing practice (the ruleset grants an admin bypass, and waiting costs ten minutes a round).
Verdict
GOOD TO MERGE at db84b4935. Test-only in substance, the one production line is a docstring Note: that is exhaustively true, both new tests are proven sensitive to their subject by my own reverts rather than by assertion, and the salvage triage — keeping two claims and discarding the rest as superseded — is the right call and is accurately described.
One recommendation carried forward rather than blocked on: add a sentence to test_foundation_keyword_names.py recording that name (function) and engine (class keyword) differ deliberately, pointing at test_engine_base.py for the 3.10 metaclass reason.
|
✅ GOOD TO MERGE — re-pointed to The delta from CI at this head is complete and clean: 22 CheckRuns SUCCESS, 0 FAILURE, 2 SKIPPED ( Worth noting for the failure you hit before the merge: |
… own (#606) The stacklevel probes record with `simplefilter('always')`, which un-ignores the categories Python hides by default, then asserted their window held exactly one record and took the last of it. A `ResourceWarning` for a file handle an earlier test left for the collector therefore failed them, and did so unpredictably: the same failure was watched migrating between #596 and #600 nine minutes apart with neither branch touched and neither head moved, so what trips is decided by garbage-collection timing and collection order rather than by any test's code. It had reddened #577, #596 and #600 by then, none of which the test exercises. * `tests/utilities/test_stacklevel.py`: count the window's pcapkit warnings via a new `emissions()` helper and check the one found is the expected category, instead of requiring the probe's to be the only record there and picking it by position. Counting over `BaseWarning` rather than the exact category keeps what the old assertion did buy -- a second complaint from the package is still a failure -- and gives up only its sensitivity to warnings pcapkit never raised. Both probe helpers were affected, `warning_site()` and `emit()`. Adds a regression test that frames the probe with foreign warnings on both sides. * `tests/utilities/test_logging.py`: enter each `Extractor` as a context manager so `__exit__` closes the input file, removing the leaked `in.pcap` handle these three constructions were shedding into sibling modules' tests. `pytest tests/utilities/` 103 passed, and with `-W always::ResourceWarning` the unclosed-`in.pcap` warnings go from 2 to 0. The new test fails `3 != 1` without the fix. The leak's root cause is left alone deliberately: `Extractor._cleanup()` closes the handle only when the caller supplied the stream, never when pcapkit opened it, and that is production code wanting its own review. Fixes #606
JarryShaw
added
the
test
…two false packaging claims (#642) Three names appeared in string annotations that their module never imported, and they were mypy's complete set of ``name-defined`` findings for the package: * ``pcapkit/utilities/logging.py:350`` used ``Any``; added to the ``TYPE_CHECKING`` block beside ``IO``, ``Optional`` and ``Union``. * ``pcapkit/protocols/schema/internet/ipv6_route.py:136`` used ``Protocol`` and ``:271`` used ``Optional``; added, ``Protocol`` as ``ProtocolBase as Protocol``. Twenty-one sibling schema modules already spell it that way, in the same ``payload:`` stub; this makes twenty-two. mypy 2.3.1 over ``pcapkit``: 3 ``name-defined`` errors before, 0 after; 115 total errors before, 112 after, so nothing else moved. ``MANIFEST.in:14-17`` asserted that ``include README.md`` was "the only thing that puts it in an sdist" and that an sdist without it "cannot be installed at all". Both halves are false, and the file had contradicted itself since #631 wrote the correct mechanism seven lines below without correcting this. Deleting the lines and rebuilding gives a byte-identical sdist listing -- empty diff -- that installs with exit 0: ``setuptools/command/sdist.py:59-60`` ships the README unconditionally and ``setuptools/dist.py:460``'s default ``license_files`` glob ships ``LICENSE``. Of the original three ``include`` lines only ``CHANGELOG.md`` is load-bearing. The comment now says that, and the same correction is applied to the #619 entry in ``docs/source/changelog/1.5.0.rst``, which carried the identical claim. Three changelog bullets gained the citation they were missing: ``(#619)`` for the README entry, ``(#570)`` for the ``code=`` keyword and ``(#577)`` for the ``StreamEOFError`` gaps. Two of the five remain uncited because no number could be recovered for them. ``CHANGELOG.md`` regenerated with ``util/changelog_md.py``; ``--check`` exits 0. New ``tests/project/test_annotation_names.py`` resolves every string annotation in the package -- following a nested forward reference such as ``'list["Nested"]'``, while treating ``Literal`` members and ``Annotated`` metadata as the values they are -- against the names its own module binds. It reports the same three findings as mypy on the unfixed tree and none after. 128 -> 132 passed over ``tests/project`` and ``tests/utilities/test_logging.py``, both exit 0, subtests unchanged at 487.
…two false packaging claims (#642) Three names appeared in string annotations that their module never imported, and they were mypy's complete set of ``name-defined`` findings for the package: * ``pcapkit/utilities/logging.py:350`` used ``Any``; added to the ``TYPE_CHECKING`` block beside ``IO``, ``Optional`` and ``Union``. * ``pcapkit/protocols/schema/internet/ipv6_route.py:136`` used ``Protocol`` and ``:271`` used ``Optional``; added, ``Protocol`` as ``ProtocolBase as Protocol``. Twenty-one sibling schema modules already spell it that way, in the same ``payload:`` stub; this makes twenty-two. mypy 2.3.1 over ``pcapkit``: 3 ``name-defined`` errors before, 0 after; 115 total errors before, 112 after, so nothing else moved. ``MANIFEST.in:14-17`` asserted that ``include README.md`` was "the only thing that puts it in an sdist" and that an sdist without it "cannot be installed at all". Both halves are false, and the file had contradicted itself since #631 wrote the correct mechanism seven lines below without correcting this. Deleting the lines and rebuilding gives a byte-identical sdist listing -- empty diff -- that installs with exit 0: ``setuptools/command/sdist.py:59-60`` ships the README unconditionally and ``setuptools/dist.py:460``'s default ``license_files`` glob ships ``LICENSE``. Of the original three ``include`` lines only ``CHANGELOG.md`` is load-bearing. The comment now says that, and the same correction is applied to the #619 entry in ``docs/source/changelog/1.5.0.rst``, which carried the identical claim. Three changelog bullets gained the citation they were missing: ``(#619)`` for the README entry, ``(#570)`` for the ``code=`` keyword and ``(#577)`` for the ``StreamEOFError`` gaps. Two of the five remain uncited because no number could be recovered for them. ``CHANGELOG.md`` regenerated with ``util/changelog_md.py``; ``--check`` exits 0. New ``tests/project/test_annotation_names.py`` resolves every string annotation in the package -- following a nested forward reference such as ``'list["Nested"]'``, while treating ``Literal`` members and ``Annotated`` metadata as the values they are -- against the names its own module binds. It reports the same three findings as mypy on the unfixed tree and none after. 127 -> 132 passed over ``tests/project`` and ``tests/utilities/test_logging.py``, both exit 0, subtests unchanged at 487.
…a false packaging claim (#642) Three names appeared in string annotations that their module never imported, and they were mypy's complete set of ``name-defined`` findings for the package: * ``pcapkit/utilities/logging.py:350`` used ``Any``; added to the ``TYPE_CHECKING`` block beside ``IO``, ``Optional`` and ``Union``. * ``pcapkit/protocols/schema/internet/ipv6_route.py:136`` used ``Protocol`` and ``:271`` used ``Optional``; added, ``Protocol`` as ``ProtocolBase as Protocol``. Twenty-one sibling schema modules already spell it that way, in the same ``payload:`` stub; this makes twenty-two. mypy 2.3.1 over ``pcapkit``: 3 ``name-defined`` errors before, 0 after; 115 total errors before, 112 after, so nothing else moved. ``MANIFEST.in:14-17`` asserted that ``include README.md`` was "the only thing that puts it in an sdist" and that an sdist without it "cannot be installed at all". Both halves are false, and the file had contradicted itself since #631 wrote the correct mechanism seven lines below without correcting this. Deleting the lines and rebuilding gives a byte-identical sdist listing -- empty diff -- that installs with exit 0: ``setuptools/command/sdist.py:59-60`` ships the README unconditionally and ``setuptools/dist.py:460``'s default ``license_files`` glob ships ``LICENSE``. Of the original three ``include`` lines only ``CHANGELOG.md`` is load-bearing. The comment now says that. New ``tests/project/test_annotation_names.py`` resolves every string annotation in the package -- following a nested forward reference such as ``'list["Nested"]'``, while treating ``Literal`` members and ``Annotated`` metadata as the values they are -- against the names its own module binds. It reports the same three findings as mypy on the unfixed tree and none after. That module named ``ast.TypeAlias`` and ``ast.TypeVar`` directly, and both are PEP 695 nodes added in Python 3.12, so *every* test in it raised ``AttributeError`` on the 3.10 and 3.11 matrix jobs -- ``bound_names`` walks every node of every file, so the attribute is reached whatever a test does. Both are now resolved once at module scope through ``getattr(ast, ..., ())``, leaving the ``isinstance`` branches otherwise untouched: ``isinstance(x, ())`` is always False, so the branches stay live on 3.12+ and are simply unreachable below it. Chosen over a ``sys.version_info`` comparison because it writes no version number down at all -- a comparison states 3.12 next to the attribute it guards, and the two can then drift -- and over a per-node ``getattr`` because a module-level constant lifts the lookup out of a loop that runs on every node of every file. ``ast.TypeVar`` is the branch that earns its keep: it carries its name as a bare ``str`` and emits no ``ast.Name`` node, so forcing ``_TYPE_VAR`` to ``()`` on 3.14.7 turns ``T`` and ``U`` into false findings. ``ast.TypeAlias`` is defensive by comparison -- its name *is* an ``ast.Name`` in ``Store`` context, which the preceding branch already catches -- and is left as it stands rather than removed. A new ``test_a_pep695_type_parameter_is_in_scope`` pins both the guards and the behaviour, skipped below 3.12 because its fixture source cannot parse there. Measured on real interpreters rather than simulated. 3.10.21 and 3.11.15: 5 failed, exit 1 -> 5 passed, 1 skipped, exit 0. 3.14.7: all 6 pass, exit 0. 133 passed over ``tests/project`` and ``tests/utilities/test_logging.py``, exit 0, subtests unchanged at 487. No changelog entry on this branch. Per the rule that no code branch touches ``CHANGELOG.md`` or anything under ``docs/source/changelog/``, this change's entry -- and the wording correction the ``MANIFEST.in`` claim implies for the #619 entry, plus the missing ``(#570)`` and ``(#577)`` citations -- go to the shared changelog pull request #657 instead.
The changelog changes #666 was carrying on its own branch, moved here so that #666 touches only `MANIFEST.in`, `pcapkit/protocols/schema/internet/ipv6_route.py`, `pcapkit/utilities/logging.py` and `tests/project/test_annotation_names.py`. #666 was the last open branch still editing `CHANGELOG.md` itself; it no longer does. Four pieces, not one, because #666 had amended existing entries as well as needing a new one. A new **Fixed** bullet for #642: three names used in string annotations that their own module never imported -- `Any` in `pcapkit/utilities/logging.py`, and `Protocol` and `Optional` in `pcapkit/protocols/schema/internet/ipv6_route.py`. The bullet names the `typing.cast` case specifically, because that is the one no running test can catch: `cast` never evaluates its first argument. It states plainly that nothing resolves at runtime that did not before, since `TYPE_CHECKING` is `False` when the interpreter runs, so that the entry is not read as a runtime fix. mypy 2.3.1's before/after is quoted as the measurement -- three `name-defined` errors to none, 115 total to 112 -- and the new `tests/project/test_annotation_names.py` is described as what pins the invariant without a type checker installed. Three missing citations recovered: `(#570)` on the L2TPv3 worked-example line, `(#577)` on the `register_extractor_engine` keyword line, and `(#619)` on the README rename entry. And the #619 entry's packaging claim corrected in place. It asserted that `include README.md` in `MANIFEST.in` was "the only thing that puts the README in a source distribution" and that an sdist without it "cannot be installed". Both halves are false: setuptools' own `sdist` command ships the README before `MANIFEST.in` is read at all, so dropping the line leaves the listing byte-identical at 861 entries, and `setup.py` reads the file from wherever it is executing, which for a `pip` install of an sdist is the unpacked sdist. #666 corrects the same claim in the `MANIFEST.in` comment, so the two stay in step. No `:pep:` role, though the new bullet discusses PEP 695: `util/changelog_md.py` converts only double-backtick literals and the `:rfc:` role, and raises `ResidualMarkupError` on anything else, exactly as the #661 entry hit with `:obj:`. Plain prose instead. 50 lines added to the two files, 11 reflowed. `CHANGELOG.md` regenerated with `util/changelog_md.py`, not edited; `--check` exits 0 and `tests/project/` is green at 96 passed, 469 subtests, exit 0 -- the same counts the previous commit on this branch reported, so nothing else moved. Committed from a detached HEAD and pushed to the branch ref, because `docs/changelog-1.5.0` is checked out in another agent's worktree at a stale f846523 and could not be taken here. Written against b2ec64b and rebased onto d47ada0, the branch having taken #641's entry and the #652/#650 boundary correction in the meantime. Both files conflicted, both at the append point rather than in substance -- #641's bullet and this one land in the same place at the end of **Fixed** -- so the resolution keeps both, #641's first. `CHANGELOG.md` was not hand-resolved: it is generated, so it was regenerated from the resolved entry file and `--check` re-run, which is the only resolution that cannot drift. Refs #642
…a false packaging claim (#642) (#666) Three names appeared in string annotations that their module never imported, and they were mypy's complete set of ``name-defined`` findings for the package: * ``pcapkit/utilities/logging.py:350`` used ``Any``; added to the ``TYPE_CHECKING`` block beside ``IO``, ``Optional`` and ``Union``. * ``pcapkit/protocols/schema/internet/ipv6_route.py:136`` used ``Protocol`` and ``:271`` used ``Optional``; added, ``Protocol`` as ``ProtocolBase as Protocol``. Twenty-one sibling schema modules already spell it that way, in the same ``payload:`` stub; this makes twenty-two. mypy 2.3.1 over ``pcapkit``: 3 ``name-defined`` errors before, 0 after; 115 total errors before, 112 after, so nothing else moved. ``MANIFEST.in:14-17`` asserted that ``include README.md`` was "the only thing that puts it in an sdist" and that an sdist without it "cannot be installed at all". Both halves are false, and the file had contradicted itself since #631 wrote the correct mechanism seven lines below without correcting this. Deleting the lines and rebuilding gives a byte-identical sdist listing -- empty diff -- that installs with exit 0: ``setuptools/command/sdist.py:59-60`` ships the README unconditionally and ``setuptools/dist.py:460``'s default ``license_files`` glob ships ``LICENSE``. Of the original three ``include`` lines only ``CHANGELOG.md`` is load-bearing. The comment now says that. New ``tests/project/test_annotation_names.py`` resolves every string annotation in the package -- following a nested forward reference such as ``'list["Nested"]'``, while treating ``Literal`` members and ``Annotated`` metadata as the values they are -- against the names its own module binds. It reports the same three findings as mypy on the unfixed tree and none after. That module named ``ast.TypeAlias`` and ``ast.TypeVar`` directly, and both are PEP 695 nodes added in Python 3.12, so *every* test in it raised ``AttributeError`` on the 3.10 and 3.11 matrix jobs -- ``bound_names`` walks every node of every file, so the attribute is reached whatever a test does. Both are now resolved once at module scope through ``getattr(ast, ..., ())``, leaving the ``isinstance`` branches otherwise untouched: ``isinstance(x, ())`` is always False, so the branches stay live on 3.12+ and are simply unreachable below it. Chosen over a ``sys.version_info`` comparison because it writes no version number down at all -- a comparison states 3.12 next to the attribute it guards, and the two can then drift -- and over a per-node ``getattr`` because a module-level constant lifts the lookup out of a loop that runs on every node of every file. ``ast.TypeVar`` is the branch that earns its keep: it carries its name as a bare ``str`` and emits no ``ast.Name`` node, so forcing ``_TYPE_VAR`` to ``()`` on 3.14.7 turns ``T`` and ``U`` into false findings. ``ast.TypeAlias`` is defensive by comparison -- its name *is* an ``ast.Name`` in ``Store`` context, which the preceding branch already catches -- and is left as it stands rather than removed. A new ``test_a_pep695_type_parameter_is_in_scope`` pins both the guards and the behaviour, skipped below 3.12 because its fixture source cannot parse there. Measured on real interpreters rather than simulated. 3.10.21 and 3.11.15: 5 failed, exit 1 -> 5 passed, 1 skipped, exit 0. 3.14.7: all 6 pass, exit 0. 133 passed over ``tests/project`` and ``tests/utilities/test_logging.py``, exit 0, subtests unchanged at 487. No changelog entry on this branch. Per the rule that no code branch touches ``CHANGELOG.md`` or anything under ``docs/source/changelog/``, this change's entry -- and the wording correction the ``MANIFEST.in`` claim implies for the #619 entry, plus the missing ``(#570)`` and ``(#577)`` citations -- go to the shared changelog pull request #657 instead.
1 participant
Summary
Recovered and triaged an abandoned, uncommitted local change (never pushed, found only in a stale worktree) from the same #514 keyword-naming audit that produced #547, #557 and #570. Most of it turned out to already be shipped or actively contradicted by what shipped; this PR keeps only the part that is still novel.
StreamEOFError's docstring did not document that@preparealways raises it withquiet=True-- the same end-of-stream conventionStructErrorfollows via its owneof=True-- and nothing pinned that silence with a test. Added the docstring note and two regression tests: one proving@prepare'sStreamEOFErrorlogs nothing, and a loud control proving the silence comes fromquiet=Truespecifically rather thanStreamEOFErrorhaving stopped logging altogether.register_extractor_engine's real keyword isname, notengine-- an earlier audit (noted in fix(foundation): rename Engine's registry keyword toengine, which works on Python 3.10 (#514) #557's own description) found the docstring saidenginewhile the signature saidname. The docstring has since been corrected, but the keyword itself was never under test, so a future rename could put the two back out of step exactly as silently as before. Added a test pinning both directions:name=registers,engine=raisesTypeError.What was dropped, and why
The inherited diff also touched
Engine/Reassembly/TraceFlow's__init_subclass__docstrings (fixing aname:/protocol:Args:mismatch) andregister_extractor_engine's docstring (engine:->name:), plus two new tests asserting that an unrecognisedname=class keyword onReassembly/TraceFlowis silently swallowed and falls back to the class name.All of this is superseded by #547 and #557, which:
__init_subclass__docstrings from scratch for opt-in registration (confirmed by rebasing the inherited diff onto currentmain: real conflicts in all three files, because theArgs:text being edited no longer exists in that shape).register_extractor_enginedocstring verbatim (git diff origin/mainon that file, before any edit, was empty).UnsupportedCall) instead of silently falling back to the class name. Running the two "swallowed keyword" tests from the inherited diff against currentmainfails them outright, since the exact behaviour they pin no longer exists.Test plan
quiet=Trueindecorators.py, and temporarily renamedregister_extractor_engine'snameparameter toengine; each broke the corresponding new test, then reverted cleanly).tests/utilities/test_quiet_exceptions.py,tests/foundation/registry/test_foundation_keyword_names.py,tests/foundation/registry/test_foundation.py,tests/utilities/test_decorators.py: 36 passed, 14 subtests passed, exit 0.tests/project/test_public_api.py,test_documentation_claims.py,test_changelog_md.py: 47 passed, 469 subtests passed, exit 0 (no new public API surface added).util/changelog_md.py --check:CHANGELOG.mdin step withdocs/source/changelog/1.5.0.rst.examples/generators/make_samples.py):pytest -q --ignore=tests/integration --ignore-glob='*_runtime.py' --ignore-glob='*_regression.py'-- 1123 passed, 5 skipped, 2704 subtests passed, exit 0.mypy pcapkit/utilities/exceptions.py:Success: no issues found.pylint pcapkit/utilities/exceptions.py(unused-import/undefined-variable/unused-variable/no-member): 10.00/10.