From 118daf844fe82c816fe701cc355d46541160ac9d Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Mon, 21 Sep 2026 11:05:47 -0400 Subject: [PATCH] test(utilities,foundation): pin the quiet StreamEOFError convention and 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. --- CHANGELOG.md | 1 + docs/source/changelog/1.5.0.rst | 8 +++ pcapkit/utilities/exceptions.py | 8 +++ .../registry/test_foundation_keyword_names.py | 62 +++++++++++++++++++ tests/utilities/test_quiet_exceptions.py | 58 +++++++++++++++++ 5 files changed, 137 insertions(+) create mode 100644 tests/foundation/registry/test_foundation_keyword_names.py diff --git a/CHANGELOG.md b/CHANGELOG.md index ccc99bac9d..1debd633ba 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -34,6 +34,7 @@ The largest release since 1.0, and the first recorded here as it happened rather - **Fixed** -- constant lookups that rejected a value the registry defines. `RouterAlert(0)` is the only value [RFC 2113](https://datatracker.ietf.org/doc/html/rfc2113) defines and the one IGMP, RSVP and MLD actually send, and it was discarded because the vendor crawler skipped a header row IANA's CSV does not have; IPX `Socket(0)` is that protocol's own default, so `bytes(IPX(...))` crashed on its own defaults; and two FTP `_missing_` overrides were plain methods rather than classmethods, so every unregistered value raised `TypeError` instead of extending the enumeration (#492, #503). - **Fixed** -- `format='text'` raised `AttributeError` before writing anything, naming a `dictdumper.Text` that has never existed. It now points at `Tree`, as the `'txt'` alias beside it already did. - **Fixed** -- 45 places where a documentation page contradicted the code (#413), ambiguous cross-references and five autodoc signature failures (#416), and `Extractor`'s documented exception plus 40 phantom or stale `Args:` labels (#501). +- **Fixed** -- two more gaps the #514 keyword audit turned up, neither previously covered by a test: `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` -- so nothing pinned that silence against a future regression; and `register_extractor_engine`'s real keyword, `name`, was not itself under test, only its already-corrected docstring, so a future rename could put the two out of step again exactly as quietly as before. Preceded by `1.5.0a1` (2026-09-15), `1.5.0b1` and `1.5.0b2` (both 2026-09-18) and `1.5.0b3` (2026-09-19), all published as prereleases and so resolved only by `pip install --pre`. `1.5.0b1` half-shipped: the tag, the GitHub release and the Conda deployments landed, but PyPI rejected the wheel because `twine check` found a Sphinx-only `:mod:` role in `README.rst`, which `pyproject.toml` declares as the dynamic long description. `1.5.0b2` is what reshipped it -- the release workflow is version-driven, so an existing version cannot republish -- and `1.5.0b3` followed the CI change that stops a TestPyPI outage from costing a release its wheels (#497, #498). diff --git a/docs/source/changelog/1.5.0.rst b/docs/source/changelog/1.5.0.rst index 4e230831a2..aa536e3611 100644 --- a/docs/source/changelog/1.5.0.rst +++ b/docs/source/changelog/1.5.0.rst @@ -282,6 +282,14 @@ pull requests between #326 and #509. (#413), ambiguous cross-references and five autodoc signature failures (#416), and ``Extractor``'s documented exception plus 40 phantom or stale ``Args:`` labels (#501). +* **Fixed** -- two more gaps the #514 keyword audit turned up, neither + previously covered by a test: ``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`` -- + so nothing pinned that silence against a future regression; and + ``register_extractor_engine``'s real keyword, ``name``, was not itself + under test, only its already-corrected docstring, so a future rename could + put the two out of step again exactly as quietly as before. Preceded by ``1.5.0a1`` (2026-09-15), ``1.5.0b1`` and ``1.5.0b2`` (both 2026-09-18) and ``1.5.0b3`` (2026-09-19), all published as prereleases and so diff --git a/pcapkit/utilities/exceptions.py b/pcapkit/utilities/exceptions.py index 72d6c9a48d..0f68782eb6 100644 --- a/pcapkit/utilities/exceptions.py +++ b/pcapkit/utilities/exceptions.py @@ -431,6 +431,14 @@ class StreamEOFError(BaseError, EOFError): A *declared* zero length -- a nested schema legitimately sized to have nothing to read -- is a different situation and does not raise this. + Note: + :func:`~pcapkit.utilities.decorators.prepare` always raises this with + ``quiet=True``: reaching end of stream is the frame reader's ordinary + way of finding out there is nothing left to parse, not a fault to + log -- the same convention + :exc:`~pcapkit.utilities.exceptions.StructError` follows for the + same situation via its own ``eof=True``. + """ diff --git a/tests/foundation/registry/test_foundation_keyword_names.py b/tests/foundation/registry/test_foundation_keyword_names.py new file mode 100644 index 0000000000..8659a1ab65 --- /dev/null +++ b/tests/foundation/registry/test_foundation_keyword_names.py @@ -0,0 +1,62 @@ +"""The registry helpers' documented keyword names must be the real ones. + +:func:`~pcapkit.foundation.registry.foundation.register_extractor_engine` +once documented its first argument as ``engine`` while the signature +declared ``name``, so a caller following the docstring got a +:exc:`TypeError` rather than a registration -- noted in passing while +auditing the ``__init_subclass__`` keyword rename in #557. The docstring +has since been corrected to say ``name``, matching the signature, but +nothing pinned the pairing itself, so a future rename could put the two +back out of step just as silently as before. + +These pin the keyword spelling directly against the real signature. + +""" +from __future__ import annotations + +import importlib.util +import inspect +import unittest +from unittest import mock + +from tests._support import purge_modules + +RUNTIME_DEPS = ('tbtrim', 'aenum', 'chardet', 'dictdumper') +HAS_RUNTIME = all(importlib.util.find_spec(name) is not None for name in RUNTIME_DEPS) + + +@unittest.skipUnless(HAS_RUNTIME, 'runtime dependencies not installed') +class RegistryKeywordNameTests(unittest.TestCase): + def setUp(self) -> None: + purge_modules(['pcapkit']) + + def test_register_extractor_engine_takes_name_not_engine(self) -> None: + """``name=`` registers; ``engine=`` -- as documented -- does not.""" + from pcapkit.corekit.module import ModuleDescriptor + from pcapkit.foundation.registry import foundation as registry + + descriptor = ModuleDescriptor('pcapkit.foundation.engines', 'Engine') + + with mock.patch.object(registry.Extractor, 'register_engine') as register_engine: + registry.register_extractor_engine(name='unit-engine-by-keyword', + module=descriptor) + register_engine.assert_called_once_with('unit-engine-by-keyword', descriptor) + + with mock.patch.object(registry.Extractor, 'register_engine') as register_engine: + with self.assertRaises(TypeError) as caught: + registry.register_extractor_engine(engine='unit-engine-by-keyword', + module=descriptor) + register_engine.assert_not_called() + self.assertIn('engine', str(caught.exception)) + + def test_register_extractor_engine_signature_names_name(self) -> None: + """The documented keyword has to be the one the signature declares.""" + from pcapkit.foundation.registry import foundation as registry + + parameters = inspect.signature(registry.register_extractor_engine).parameters + self.assertIn('name', parameters) + self.assertNotIn('engine', parameters) + + +if __name__ == '__main__': + unittest.main() diff --git a/tests/utilities/test_quiet_exceptions.py b/tests/utilities/test_quiet_exceptions.py index e130b156ff..66b91aa0d8 100644 --- a/tests/utilities/test_quiet_exceptions.py +++ b/tests/utilities/test_quiet_exceptions.py @@ -17,6 +17,7 @@ """ from __future__ import annotations +import io import os import sys import traceback @@ -56,6 +57,7 @@ def setUp(self) -> None: modules = bootstrap(devmode=False) self.exceptions = modules['exceptions'] self.multidict = modules['multidict'] + self.decorators = modules['decorators'] self.logger = modules['logging'].logger def tearDown(self) -> None: @@ -133,6 +135,62 @@ def test_quiet_error_leaves_tracebacklimit_alone(self) -> None: self.assertEqual(_unrelated_failure(), before) self.assertGreater(before, 1) + def test_prepare_raises_stream_eof_error_quietly(self) -> None: + """End of stream is control flow, so ``prepare`` raises it silently. + + :func:`~pcapkit.utilities.decorators.prepare` passes ``quiet=True`` + when a *measured* -- as opposed to caller-declared -- read length comes + back zero, because that is how the frame reader learns a capture is + exhausted rather than a fault worth a log record. It is the same + convention :exc:`~pcapkit.utilities.exceptions.StructError` follows via + its own ``eof=True``. + + Dropping the ``quiet=True`` would put one ``CRITICAL`` record on + :data:`sys.stderr` for every capture parsed to completion, which is the + #362 defect this module exists for -- and would also set + :data:`sys.tracebacklimit` to ``0`` process-wide. + + """ + class DemoSchema: + @classmethod + def pre_unpack(cls, packet): + return None + + def post_process(self, packet): + return packet + + @classmethod + @self.decorators.prepare + def unpack(cls, data, length=None, packet=None): + return cls() + + if hasattr(sys, 'tracebacklimit'): + del sys.tracebacklimit + + with capture(self.logger) as recorder: + with self.assertRaises(self.exceptions.StreamEOFError) as caught: + DemoSchema.unpack(io.BytesIO(b''), None, None) + + self.assertEqual(recorder.messages, []) + self.assertEqual(str(caught.exception), 'prepare: end of stream') + self.assertFalse(hasattr(sys, 'tracebacklimit')) + + def test_loud_stream_eof_error_still_logs(self) -> None: + """The control for the test above: ``quiet`` is what silences it. + + Without this, ``recorder.messages == []`` would also pass if + :exc:`~pcapkit.utilities.exceptions.StreamEOFError` had simply stopped + logging altogether. + + """ + if hasattr(sys, 'tracebacklimit'): + del sys.tracebacklimit + + with capture(self.logger) as recorder: + self.exceptions.StreamEOFError('boom') + + self.assertEqual(recorder.messages, [('CRITICAL', 'StreamEOFError: boom')]) + def test_loud_error_still_limits_the_traceback(self) -> None: """The feature a loud error provides is unchanged.""" if hasattr(sys, 'tracebacklimit'):