diff --git a/tests/cli/test_main.py b/tests/cli/test_main.py index 526beedd36..bbc7428431 100644 --- a/tests/cli/test_main.py +++ b/tests/cli/test_main.py @@ -10,25 +10,24 @@ :data:`sys.modules` is process-global, so binding a stand-in over a real module name is a write that outlives the test unless something undoes it -- and these stand-ins are *emptier* than what they replace, which is the quiet kind. Issue -#688: this file bound module names there -- the stand-ins above, the bare +#688 is this file binding module names there -- the stand-ins above, the bare ``pcapkit.foundation`` parent they hang from, and ``pcapkit.__main__`` itself --- and put none of them back. Pairing it with :mod:`tests.project.test_public_api` -gave a run of ``ImportError: cannot import name 'show_flag_values' from +-- and putting none of them back. Paired with :mod:`tests.project.test_public_api`, +that gives a run of ``ImportError: cannot import name 'show_flag_values' from 'pcapkit.utilities.compat'`` errors and one for ``SeekError``, because the real -library's next import found a ``pcapkit.utilities.compat`` carrying exactly one name. +library's next import finds a ``pcapkit.utilities.compat`` carrying exactly one name. That is the same defect as issues #660 and #674, and it is fixed the same way: :func:`tests._support.isolate_modules` in ``setUp``, which purges the region on -the way in and restores it exactly on the way out. What this file used to do -instead was roll its own purge loop, which protects this file from whatever ran -before it and promises nothing to whatever runs after -- see +the way in and restores it exactly on the way out. A purge loop of this file's +own would not do: it protects this file from whatever ran before it and promises +nothing to whatever runs after -- see :func:`tests._support.purge_modules` for why restoration is owed by the code that binds. -:func:`tests.conftest.restore_module_table` masked the symptom under -:program:`pytest` throughout, which is why it took an audit to find: it surfaces -only under ``--noconftest``, under the stdlib :mod:`unittest` runner, or on any -route that does not load that conftest. +:func:`tests.conftest.restore_module_table` masks the symptom under +:program:`pytest`: it surfaces only under ``--noconftest``, under the stdlib +:mod:`unittest` runner, or on any route that does not load that conftest. :meth:`CLIMainTests.assert_module_table_restored` is checked in a cleanup registered *before* the isolation, so it runs after the restore and is not masked by that fixture -- a regression here fails under a plain @@ -408,10 +407,10 @@ def test_layer_defaults_to_none_and_is_case_insensitive(self) -> None: """An omitted ``-L``/``-P`` forwards :data:`None`, not a sentinel string. ``Extractor.__init__`` substitutes its own ``'none'``/``'null'`` for an - omitted value, so the CLI has nothing to substitute. It used to pass the - strings ``'None'`` and ``'null'``, which only worked because - ``Extractor`` happened to lowercase the former into the sentinel it - wanted. + omitted value, so the CLI has nothing to substitute. Forwarding the + strings ``'None'`` and ``'null'`` instead works only because + ``Extractor`` happens to lowercase the former into the sentinel it + wants. """ emoji = types.SimpleNamespace(emojize=lambda text: text) diff --git a/tests/corekit/test_bitfield_unnamed_bits_1487_unit.py b/tests/corekit/test_bitfield_unnamed_bits_1487_unit.py index 9afbbcf367..72409995af 100644 --- a/tests/corekit/test_bitfield_unnamed_bits_1487_unit.py +++ b/tests/corekit/test_bitfield_unnamed_bits_1487_unit.py @@ -1,11 +1,11 @@ # -*- coding: utf-8 -*- """A BitField packs back the bits its namespace does not name. -GitHub issue #1487: :class:`~pcapkit.corekit.fields.strings.BitField` read only -its named subfields and packed from all zeros, so octets with a bit outside -every subfield set came back with it cleared. The unpacked value now carries -those bits under ``__unnamed__`` when any is set, and a value without the key -packs them as zeros, as before. +GitHub issue #1487 guards against :class:`~pcapkit.corekit.fields.strings.BitField` +reading only its named subfields and packing from all zeros, so that octets with a +bit outside every subfield set come back with it cleared. The unpacked value +carries those bits under ``__unnamed__`` when any is set, and a value without the +key packs them as zeros. :mod:`pcapkit` is imported inside each test, after :func:`~tests._support.reimport_once_per_class`, not at module load. diff --git a/tests/corekit/test_decorators.py b/tests/corekit/test_decorators.py index 1fb7552649..bc6478eb78 100644 --- a/tests/corekit/test_decorators.py +++ b/tests/corekit/test_decorators.py @@ -31,11 +31,9 @@ def _protect_global_exception_state(self) -> None: ``StructError('unexpected eof', eof=True)`` does not pass ``quiet=True``, for one -- and a loud error outside development mode installs :mod:`pcapkit.utilities.exceptions`'s own exception hook on first use - (GitHub issue #719). This file had no ``tearDown`` before that issue and - never needed one: the equivalent side effect at the time, - ``sys.tracebacklimit = 0``, was already left unrestored, corrupting - every test that ran afterwards in the same process -- the #981 shape, - just not previously visible as a failure here. Addressed the way + (GitHub issue #719). Left unrestored, that process-global corrupts every + test that runs afterwards in the same process -- the #981 shape, whether + or not it shows as a failure here. It is restored the way ``isolate_modules`` addresses its own restore: via ``addCleanup``, so it still runs if the rest of ``setUp`` raises partway through. @@ -131,9 +129,9 @@ def _demo_schema_for_call_shapes(self): Shared by the five call-shape tests below, one per row of `#444 `__'s reproduction - table: ``prepare`` used to subscript ``args[2]``/``args[3]`` - unconditionally, so only the three-positional shape worked and every - shorter or keyword call raised ``IndexError`` instead of falling back + table: without the fix, ``prepare`` subscripts ``args[2]``/``args[3]`` + unconditionally, so only the three-positional shape works and every + shorter or keyword call raises ``IndexError`` instead of falling back to the documented ``length=None, packet=None`` defaults. """ @@ -187,20 +185,20 @@ def test_prepare_accepts_keyword_length_and_packet(self) -> None: self._assert_call_shape_result(DemoSchema.unpack(b'payload', length=7, packet={})) def test_prepare_raises_typeerror_for_extra_positional_and_keyword_arguments(self) -> None: - """Extras used to vanish silently instead of being forwarded; see #454. + """Extras raise :exc:`TypeError` rather than vanishing silently; see #454. - ``prepare``'s own docstring promised the decorated function receives - ``*args, **kwargs``, but the wrapper never populated either -- - reproduced from the issue:: + Without the fix, ``prepare``'s own docstring promises the decorated + function receives ``*args, **kwargs``, but the wrapper populates neither + -- reproduced from the issue:: Probe.unpack(b'\\x07\\x08', 2, {}, 'EXTRA_POSITIONAL', extra_kw='EXTRA_KW') -> a=7 b=8 # both extras silently gone, no error - The chosen fix removes the promise (nothing in the tree ever passed - extras, and ``@prepare`` decorates exactly one function, whose real - signature never had room for them) and rejects extras instead of - forwarding them, so a misspelled or unsupported argument is a - ``TypeError`` rather than a parse that silently ignored it. + ``prepare`` makes no such promise (nothing in the tree passes extras, and + the real signature of the function ``@prepare`` decorates has no room for + them) and rejects extras instead of forwarding them, so a misspelled or + unsupported argument is a ``TypeError`` rather than a parse that silently + ignores it. """ DemoSchema = self._demo_schema_for_call_shapes() @@ -211,11 +209,11 @@ def test_prepare_raises_typeerror_for_extra_positional_and_keyword_arguments(sel def test_prepare_raises_typeerror_for_length_given_both_positionally_and_by_keyword(self) -> None: """The narrower case #454 calls out. - Passing ``length`` both positionally and by keyword used to silently - keep the positional value and drop the keyword one -- the keyword - never reached ``kwargs.pop``, since that branch only runs when the - positional slot was *not* supplied. It is now a caller error like any - other unconsumed argument. + Passing ``length`` both positionally and by keyword is a caller error + like any other unconsumed argument. Without the fix, the positional + value is silently kept and the keyword one dropped -- the keyword never + reaches ``kwargs.pop``, since that branch only runs when the positional + slot is *not* supplied. """ DemoSchema = self._demo_schema_for_call_shapes() @@ -241,13 +239,13 @@ def unpack(cls, data, length=None, packet=None): DemoSchema.unpack(b'', None, None) def test_prepare_raises_stream_eof_error_not_a_bare_eof_error(self) -> None: - """The genuinely-exhausted case now raises an in-library exception. + """The genuinely-exhausted case raises an in-library exception. - A caller could not previously tell a truncated capture apart from any - other ``EOFError``. ``StreamEOFError`` still *is* an ``EOFError`` -- - so ``pcapkit.foundation.extraction.Extractor``'s existing ``except - (EOFError, StopIteration)`` keeps working unchanged -- but it can now - be caught specifically via + That lets a caller tell a truncated capture apart from any other + ``EOFError``. ``StreamEOFError`` *is* an ``EOFError`` -- so + ``pcapkit.foundation.extraction.Extractor``'s ``except + (EOFError, StopIteration)`` catches it -- but it can also be caught + specifically via :class:`pcapkit.utilities.exceptions.StreamEOFError`. """ @@ -299,7 +297,7 @@ def test_prepare_accepts_a_declared_zero_length_schema(self) -> None: class Empty(Schema): pass Empty.unpack(b'', 0, {}) - -> EOFError: EOFError() # before the fix + -> EOFError: EOFError() # without the fix Distinguishing this from the truncated-stream case above is what ``length is not None`` (a *declared* length, even zero) checks for. @@ -333,12 +331,12 @@ def test_prepare_leaves_the_schema_clean_after_post_process(self) -> None: ``Schema.unpack`` clears ``__updated__`` before returning, but ``post_process`` runs after it and every field it assigns sets the flag - again. The schema was therefore left dirty, and the next - ``bytes(schema)`` re-packed it -- which calls ``post_process`` a second + again. Without the fix the schema is therefore left dirty, and the next + ``bytes(schema)`` re-packs it -- which calls ``post_process`` a second time with a packet context rebuilt from the schema's own fields, so any - value ``post_process`` had derived from the *enclosing* layer's context - was overwritten with the fallback. ``Schema.pack`` clears the flag after - ``post_process``, and this asserts the unpacking path now matches it. + value ``post_process`` derived from the *enclosing* layer's context is + overwritten with the fallback. ``Schema.pack`` clears the flag after + ``post_process``, and this asserts the unpacking path matches it. """ class DemoSchema: @@ -496,8 +494,8 @@ def test_beholder_uses_the_protocol_payload_accessor_not_the_schema(self) -> Non so neither header schema has a ``payload`` field: reaching for the schema raises ``ProtocolUnbound('unknown field: payload')`` *from the recovery path*, turning a next-layer failure that should have degraded to - :class:`Raw` into a crash. It was unreachable until something was - registered on an SCTP payload protocol identifier, which NGAP now is. + :class:`Raw` into a crash. It is reachable once something is registered + on an SCTP payload protocol identifier, as NGAP is. """ exceptions = self.exceptions diff --git a/tests/corekit/test_decorators_path_1735_unit.py b/tests/corekit/test_decorators_path_1735_unit.py index f7390fa750..06fba69126 100644 --- a/tests/corekit/test_decorators_path_1735_unit.py +++ b/tests/corekit/test_decorators_path_1735_unit.py @@ -3,10 +3,11 @@ GitHub issue #1735, applying #1712's placement rule. ``seekset``, ``beholder`` and ``prepare`` act on a protocol's file and the schema it unpacks, and -``beholder`` falls back to protocol classes, so they moved from -``pcapkit.utilities.decorators`` to :mod:`pcapkit.corekit.decorators`. Per the -ruling on #1715 nothing is left at the old path, and :mod:`pcapkit.utilities` -no longer exports the three names. This pins both halves of that: the old path +``beholder`` falls back to protocol classes, so they live in +:mod:`pcapkit.corekit.decorators` rather than ``pcapkit.utilities.decorators``. +Per the ruling on #1715 nothing is left at the old path, and +:mod:`pcapkit.utilities` does not export those names. This pins both halves of +that: the old path and the old exports are gone, and every importer uses the new module. """ @@ -23,7 +24,7 @@ #: Where the decorators live. NEW_PATH = 'pcapkit.corekit.decorators' -#: Where they lived before #1735; removed, not re-exported. +#: The path they moved from (#1735); nothing is left there, not even a re-export. OLD_PATH = 'pcapkit.utilities.decorators' #: The three decorators. diff --git a/tests/corekit/test_deferred_packet_1516_unit.py b/tests/corekit/test_deferred_packet_1516_unit.py index a97b805cbc..eb7fa2a83d 100644 --- a/tests/corekit/test_deferred_packet_1516_unit.py +++ b/tests/corekit/test_deferred_packet_1516_unit.py @@ -2,11 +2,9 @@ """GitHub issue #1516: one :class:`~pcapkit.corekit.packet.DeferredPacket` for reassembly and flow tracing. -There used to be two copies of the mixin, in -``pcapkit/foundation/reassembly/data/data.py`` and -``pcapkit/foundation/traceflow/data/data.py``. Their method bodies were -AST-identical, and the one thing that differed was which module-level -``Deferred`` the ``isinstance`` test named. The merged class takes that +The mixin serves the models of both ``pcapkit/foundation/reassembly/data/data.py`` +and ``pcapkit/foundation/traceflow/data/data.py``, which differ only in which +module-level ``Deferred`` the ``isinstance`` test names. The class takes that difference as the ``__deferred__`` hook each subclass sets, so a model resolves its own placeholder class and nothing else. diff --git a/tests/corekit/test_enum_get_exception_provenance_923_unit.py b/tests/corekit/test_enum_get_exception_provenance_923_unit.py index dec049ba77..2503bb0f72 100644 --- a/tests/corekit/test_enum_get_exception_provenance_923_unit.py +++ b/tests/corekit/test_enum_get_exception_provenance_923_unit.py @@ -9,47 +9,43 @@ Measured on Python 3.14.7, that fixes the shape rather than leaving it open: ``E['nosuch']`` raises :exc:`KeyError` and ``E(999)`` raises :exc:`ValueError`. -The base already matched it -- a census of the 127 concrete -:class:`~pcapkit.corekit.enum.EnumLookup` subclasses found 119 answering a -string name miss with :exc:`KeyError`, 5 with :exc:`ValueError` and 3 minting -- -so this issue changes **provenance only**: the bare builtin :exc:`KeyError` from -``cls._member_map_[key]`` becomes -:exc:`~pcapkit.utilities.exceptions.EnumKeyError`, and :mod:`aenum`'s own bare -:exc:`ValueError` from ``cls(key)`` becomes -:exc:`~pcapkit.utilities.exceptions.EnumValueError`. +The base's shape matches it, so what #923 settles is **provenance**: a name miss +on ``cls._member_map_[key]`` raises +:exc:`~pcapkit.utilities.exceptions.EnumKeyError` rather than the bare builtin +:exc:`KeyError`, and a value miss on ``cls(key)`` raises +:exc:`~pcapkit.utilities.exceptions.EnumValueError` rather than :mod:`aenum`'s +own bare :exc:`ValueError`. Deriving :exc:`~pcapkit.utilities.exceptions.EnumKeyError` from :exc:`KeyError` -is what keeps the blast radius at nothing: six in-library sites catch the base's -name miss -- :meth:`~pcapkit.const.http.method.Method.get`, -:mod:`pcapkit.toolkit.scapy`, and two each in -:mod:`pcapkit.protocols.internet.hopopt` and +is what keeps the blast radius at nothing for the in-library sites that catch the +base's name miss -- in :meth:`~pcapkit.const.http.method.Method.get`, +:mod:`pcapkit.toolkit.scapy`, :mod:`pcapkit.protocols.internet.hopopt` and :mod:`pcapkit.protocols.internet.ipv6_opts` -- and ``Method.get`` catches it in order to **mint**, so for that one a name miss is part of a *successful* call. :class:`MethodStillMintsTests` pins that one directly. -Three call sites also changed, all of them cases of the minority shape the -ruling rejects: +Call sites that would otherwise carry the shape the ruling rejects are pinned too: * ``TransportProtocol.get`` (and its codegen twin in - :mod:`pcapkit.vendor.reg.apptype.apptype`) dropped its ``KeyError`` -> - ``ValueError`` conversion, keeping only the ``key.lower()`` call -- + :mod:`pcapkit.vendor.reg.apptype.apptype`) has no ``KeyError`` -> + ``ValueError`` conversion, only the ``key.lower()`` call -- :class:`TransportProtocolCaseFoldingTests`. -* ``Criticality.get`` was deleted outright, its body having become a pure +* ``Criticality.get`` does not exist, since its body would be a pure pass-through -- :class:`CriticalityGetDeletionTests`. * :class:`~pcapkit.protocols.internet.mh.FastBindingAcknowledgmentStatus` and - :class:`~pcapkit.protocols.internet.mh.IPv6AddressPrefixCode` already raised - from :mod:`pcapkit.utilities.exceptions` on a *name* miss, but as - ``EnumValueError`` -- the right provenance in the wrong shape -- + :class:`~pcapkit.protocols.internet.mh.IPv6AddressPrefixCode` raise from + :mod:`pcapkit.utilities.exceptions` on a *name* miss as ``EnumKeyError``, not + as ``EnumValueError`` -- the right provenance in the wrong shape -- :class:`MobilityHeaderNameMissTests`. -On the tree before this change every test below fails at import, since -:exc:`~pcapkit.utilities.exceptions.EnumKeyError` does not exist there; with the -exception added but the base left alone, each fails on its own assertion instead. -Two are regression guards rather than defect repros and cannot fail on the -pre-#923 tree -- :meth:`TransportProtocolCaseFoldingTests. -test_upper_case_still_resolves` and the resolving half of -:class:`CriticalityGetDeletionTests` -- and they are here because they are what -the deletion and the reduction could plausibly have broken. +Without the fix every test below fails at import, since +:exc:`~pcapkit.utilities.exceptions.EnumKeyError` does not exist; with the +exception but not the base's use of it, each fails on its own assertion instead. +:meth:`TransportProtocolCaseFoldingTests.test_upper_case_still_resolves` and the +resolving half of :class:`CriticalityGetDeletionTests` are regression guards +rather than defect repros and cannot fail without the fix; they pin what +reducing ``TransportProtocol.get`` and doing without ``Criticality.get`` could +plausibly break. """ from __future__ import annotations @@ -251,8 +247,7 @@ class BaseGetQuietnessTests(unittest.TestCase): :meth:`~pcapkit.const.http.method.Method.get` catches it in order to build an unregistered member, so a loud error there would put one - :data:`logging.CRITICAL` record on the logger -- and, until #983, set - :data:`sys.tracebacklimit` to ``0`` process-wide -- for every *successful* + :data:`logging.CRITICAL` record on the logger for every *successful* ``Method.get`` call that misses. That is the GitHub issue #362 defect :class:`~pcapkit.utilities.exceptions.BaseError` documents ``quiet`` for, and :mod:`pcapkit.protocols.protocol` states the same rule @@ -299,9 +294,8 @@ def test_the_minting_override_stays_silent_end_to_end(self) -> 'None': self.assertEqual(minted.value, 'QUIET-923-PROBE') def test_the_value_miss_is_quiet_too(self) -> 'None': - """GitHub issue #1845: the owner ruled the value miss quiet as well, so - this was the loud control until then. It is still raised -- only not - reported.""" + """GitHub issue #1845: the owner ruled the value miss quiet as well. It + is raised -- only not reported.""" with capture(logger) as recorder: with self.assertRaises(EnumValueError): _Closed.get(99) @@ -311,7 +305,7 @@ def test_a_loud_error_still_logs_which_is_what_quiet_is_measured_against(self) - """The control. Without it ``recorder.messages == []`` above would also pass if :class:`~pcapkit.utilities.exceptions.BaseError` had simply stopped logging, rather than these raises being quiet. Constructed - directly, now that no ``get`` miss is loud.""" + directly, since no ``get`` miss is loud.""" saved_sys, saved_threading = sys.excepthook, threading.excepthook def _restore() -> 'None': @@ -328,10 +322,10 @@ def _restore() -> 'None': class MethodStillMintsTests(unittest.TestCase): """The sharpest call site in the blast radius. - :meth:`~pcapkit.const.http.method.Method.get` is wired directly to the - old contract: it catches the base's name miss *in order to* hand back an + :meth:`~pcapkit.const.http.method.Method.get` relies on the base's name + miss being a :exc:`KeyError`: it catches it *in order to* hand back an unregistered member. If :exc:`~pcapkit.utilities.exceptions.EnumKeyError` - had not derived from :exc:`KeyError`, this would raise instead of minting. + did not derive from :exc:`KeyError`, this would raise instead of minting. """ def test_an_unregistered_method_token_still_mints(self) -> 'None': @@ -345,7 +339,7 @@ def test_an_unregistered_method_token_still_mints(self) -> 'None': self.assertEqual(minted.name, 'FROBNICATE-923') self.assertEqual(minted.value, 'FROBNICATE-923') # ``_unregistered_member`` hands back a throwaway, so neither lookup - # table grew -- the GitHub issue #860 contract, unchanged here. + # table grew -- the GitHub issue #860 contract. self.assertEqual(len(Method._member_map_), before_names) self.assertEqual(len(Method._value2member_map_), before_values) @@ -357,18 +351,18 @@ def test_a_registered_method_still_resolves_by_name_and_by_value(self) -> 'None' class TransportProtocolCaseFoldingTests(unittest.TestCase): - """``TransportProtocol.get`` survives, reduced to its ``key.lower()``. + """``TransportProtocol.get`` is reduced to its ``key.lower()``. Case-insensitivity is not on the base and ``get('TCP')`` resolving is live public behaviour -- :meth:`~pcapkit.const.reg.apptype.apptype.AppType. _dispatch` is the one live call site and it passes ``proto.lower()``, but the - public surface takes either casing. The exception conversion is what went. + public surface takes either casing. It carries no exception conversion. """ def test_upper_case_still_resolves(self) -> 'None': - """A regression guard, not a defect repro: it passes on the pre-#923 - tree too, and is here because reducing the override is exactly what - could have dropped the fold along with the conversion.""" + """A regression guard, not a defect repro: it passes without the #923 + fix too, and is here because reducing the override is exactly what + could drop the fold along with the conversion.""" from pcapkit.const.reg.apptype.apptype import TransportProtocol self.assertIs(TransportProtocol.get('TCP'), TransportProtocol.tcp) @@ -429,13 +423,13 @@ def test_the_override_carries_no_handler_or_raise_left(self) -> 'None': class CriticalityGetDeletionTests(unittest.TestCase): - """``Criticality.get`` is gone, and the inherited base answers identically. + """``Criticality`` has no ``get`` of its own, and the inherited base answers identically. - Its body had become a pure pass-through once the ``KeyError`` -> - ``ValueError`` conversion went, so every assertion here is about the - *inherited* :meth:`~pcapkit.corekit.enum.EnumLookup.get`. ``default`` - handling is the one thing that could have made the deletion unsafe, so it is - pinned in both directions. + Without the ``KeyError`` -> ``ValueError`` conversion its body would be a pure + pass-through, so every assertion here is about the *inherited* + :meth:`~pcapkit.corekit.enum.EnumLookup.get`. ``default`` handling is the one + thing that could make relying on the base unsafe, so it is pinned in both + directions. """ def test_the_override_is_gone(self) -> 'None': @@ -444,8 +438,8 @@ def test_the_override_is_gone(self) -> 'None': self.assertNotIn('get', Criticality.__dict__) def test_names_and_values_resolve_as_before(self) -> 'None': - """Invariants rather than repros -- they pass on the pre-#923 tree too, - and are the ones the deletion had to preserve.""" + """Invariants rather than repros -- they pass without the #923 fix too, + and are what relying on the inherited ``get`` has to preserve.""" from pcapkit.protocols.application.ngap import Criticality self.assertIs(Criticality.get('reject'), Criticality.reject) @@ -497,12 +491,12 @@ def test_a_value_miss_still_goes_through_missing(self) -> 'None': class MobilityHeaderNameMissTests(unittest.TestCase): """The two :rfc:`5568` inline enumerations found by #923's own census. - Both already raised from :mod:`pcapkit.utilities.exceptions` on a name miss - -- the right provenance -- but chose ``EnumValueError`` deliberately, *"so - the two ways of getting this wrong do not report differently"*. That is the - conversion this ruling rejects, so the name half moves to + Both raise from :mod:`pcapkit.utilities.exceptions` on a name miss -- the + right provenance. Choosing ``EnumValueError`` for it, *"so the two ways of + getting this wrong do not report differently"*, is the conversion this + ruling rejects, so the name half is :exc:`~pcapkit.utilities.exceptions.EnumKeyError` and only ``_missing_``'s - value half stays :exc:`ValueError`-shaped. + value half is :exc:`ValueError`-shaped. """ def test_a_name_miss_is_key_shaped(self) -> 'None': @@ -560,7 +554,7 @@ def test_neither_class_grows(self) -> 'None': @unittest.skipUnless(importlib.util.find_spec('requests') is not None, 'pcapkit.vendor needs requests') class VendorTemplateParityTests(unittest.TestCase): - """A regeneration must not undo the change. + """A regeneration must not undo the fix. ``pcapkit/const/`` is generated from ``pcapkit/vendor/``, so the committed module passing everything above is not evidence that the template agrees @@ -595,7 +589,7 @@ def test_the_template_renders_the_committed_get_region(self) -> 'None': # Not vacuous: both halves have to carry the reduced body, so a template # that still converts cannot match a committed module that does not, and - # a pair that agree on the *old* body fails here too. + # a pair that agree on a *converting* body fails here too. self.assertIn('super().get(key.lower(), default)', rendered_region.group(0)) # type: ignore[union-attr] self.assertNotIn("raise ValueError(f'{key!r} is not a valid", diff --git a/tests/corekit/test_enum_get_quiet_fallback_1845_unit.py b/tests/corekit/test_enum_get_quiet_fallback_1845_unit.py index be684a6baf..8c657e2957 100644 --- a/tests/corekit/test_enum_get_quiet_fallback_1845_unit.py +++ b/tests/corekit/test_enum_get_quiet_fallback_1845_unit.py @@ -241,7 +241,7 @@ def test_an_uncaught_miss_prints_the_interpreters_own_traceback(self) -> 'None': class LoudPathTests(_HookGuard): - """What stays loud. These pass before the fix too: they guard against a fix + """What stays loud. These pass without the fix too: they guard against a fix that quiets more than ``get``'s own lookup miss.""" def test_register_is_loud(self) -> 'None': diff --git a/tests/corekit/test_enum_lookup_base_unit.py b/tests/corekit/test_enum_lookup_base_unit.py index f9693f428c..f4b8fc06c5 100644 --- a/tests/corekit/test_enum_lookup_base_unit.py +++ b/tests/corekit/test_enum_lookup_base_unit.py @@ -16,8 +16,8 @@ What this module pins, and why each part is worth pinning: * the split is where it was ruled to be -- the lookup methods on the base, the - mutating ones still on :class:`~pcapkit.corekit.enum.EnumRegistry`, asserted as - an **exact** partition rather than as mere presence, so that a later change + mutating ones on :class:`~pcapkit.corekit.enum.EnumRegistry`, asserted as + an **exact** partition rather than as mere presence, so that a change cannot quietly migrate one across (:class:`TwoTierSplitTests`); * the base works as a base, for an :class:`int`-valued and a :class:`str`-valued enumeration alike, without either being a registry (:class:`BareLookupTests`); @@ -29,24 +29,22 @@ ``str``-key path -- that last one is a documented omission, so it is asserted rather than left to be rediscovered (:class:`ValidateValueTests`); * the declared-but-unassigned asymmetry :meth:`get`'s own docstring describes - survives on the new base (:class:`UnassignedAsymmetryTests`); -* members still pickle, on every protocol, for all three shapes - (:class:`PickleTests`) -- flagged unverified while the change was being - designed, since inserting a class into the MRO is exactly the kind of thing - that can disturb :meth:`~object.__reduce_ex__`. + holds on the base (:class:`UnassignedAsymmetryTests`); +* members pickle, on every protocol, for all three shapes + (:class:`PickleTests`) -- since inserting a class into the MRO is exactly the + kind of thing that can disturb :meth:`~object.__reduce_ex__`. -On the tree before this change every test below fails at **import**:: +Without the base every test below fails at **import**:: ImportError: cannot import name 'EnumLookup' from 'pcapkit.corekit.enum' That is not an artefact of how the module is written: the classes these tests -exercise inherit from the base being introduced, so there is nothing to exercise -until it exists. Two of the tests fail pre-change for a second, independent -reason as well -- :meth:`TwoTierSplitTests.test_the_split_is_an_exact_partition` -and :meth:`TwoTierSplitTests.test_enum_registry_subclasses_the_lookup_base` -both contradict the single-tier shape even when rewritten against -:class:`~pcapkit.corekit.enum.EnumRegistry` alone, because ``get`` and -``get_all`` live in ``EnumRegistry.__dict__`` there. +exercise inherit from the base, so there is nothing to exercise without it. +:meth:`TwoTierSplitTests.test_the_split_is_an_exact_partition` and +:meth:`TwoTierSplitTests.test_enum_registry_subclasses_the_lookup_base` also fail +for a second, independent reason -- both contradict a single-tier shape even when +rewritten against :class:`~pcapkit.corekit.enum.EnumRegistry` alone, because +``get`` and ``get_all`` then live in ``EnumRegistry.__dict__``. """ from __future__ import annotations @@ -175,7 +173,7 @@ def test_the_split_is_an_exact_partition(self) -> 'None': Asserted against ``__dict__`` rather than with :func:`hasattr`, which cannot tell "defined here" from "inherited": the mutating five are reachable from :class:`EnumLookup`'s subclasses either way, so presence - alone would pass on the single-tier tree this change replaces. + alone would pass on a single-tier hierarchy. """ for name in LOOKUP: @@ -201,7 +199,7 @@ def test_registries_still_reach_every_mutating_method(self) -> 'None': self.assertTrue(callable(getattr(_PickleInt, name))) def test_lookup_methods_resolve_to_the_base_on_a_registry(self) -> 'None': - """A registry's ``get`` is now the base's, not a copy of it.""" + """A registry's ``get`` is the base's, not a copy of it.""" for name in ('get', 'get_all'): with self.subTest(method=name): self.assertIs(getattr(_PickleInt, name).__func__, @@ -512,10 +510,9 @@ class PickleTests(unittest.TestCase): """Members survive the round trip on every protocol, for all three shapes. Inserting a class into the MRO is exactly the sort of change that can disturb - :meth:`~object.__reduce_ex__`, and this was flagged unverified while the - change was being designed. Each case also asserts that the class really does - reach the new base, so the test cannot pass vacuously on the single-tier tree - this change replaces. + :meth:`~object.__reduce_ex__`. Each case also asserts that the class really + does reach the base, so the test cannot pass vacuously on a single-tier + hierarchy. """ diff --git a/tests/corekit/test_enum_lookup_reparent_877_unit.py b/tests/corekit/test_enum_lookup_reparent_877_unit.py index 8af9693fef..0967713f0f 100644 --- a/tests/corekit/test_enum_lookup_reparent_877_unit.py +++ b/tests/corekit/test_enum_lookup_reparent_877_unit.py @@ -7,8 +7,8 @@ overrode the recommendation the thread had reached before it. Phase 1 (#906) split :class:`~pcapkit.corekit.enum.EnumLookup` out of :class:`~pcapkit.corekit.enum.EnumRegistry` for exactly this; this module -pins that the split half of the tree that is not blocked by #913 or #904 -actually took the base -- :class:`TransportProtocol +pins that the half of the tree that is not blocked by #913 or #904 derives +from the base -- :class:`TransportProtocol `, :class:`FinalisedState `, :class:`Completion `, @@ -23,34 +23,28 @@ ` plus its six concrete per-frame subclasses. -Two of those eleven, :class:`TransportProtocol` and :class:`Criticality`, -already defined their own ``get`` -- both as a :class:`staticmethod`, while -:meth:`~pcapkit.corekit.enum.EnumLookup.get` is a :class:`classmethod`, the -exact trap GitHub issue #908 hit and #915 fixed for -:meth:`~pcapkit.const.http.method.Method.get`. Both became classmethods that -delegate, each keeping only the behaviour the base does not reproduce on its -own -- :class:`TransportProtocolGetTests` and :class:`CriticalityGetTests` pin -that each of those kept behaviours is unchanged, not merely that the -delegation compiles. - -GitHub issue #923 then reduced that pair to one. Both overrides converted the -base's name-miss :exc:`KeyError` into a :exc:`ValueError`, and #923's ruling -retired the conversion -- so ``Criticality.get``, whose *only* remaining job -that was, is gone entirely and :class:`CriticalityGetTests` now exercises the -inherited base; ``TransportProtocol.get`` survives, reduced to its -``key.lower()`` call, because case folding has no equivalent on the base. - -The other nine are pure re-parenting -- no ``get`` or ``_missing_`` of their -own to reconcile -- so :class:`PureReparentGetTests` pins the one thing that -actually changes for them: ``get``/``get_all`` now exist and resolve, where -before this change the attribute did not exist at all. - -On the tree before this change every test below that calls ``.get()`` on one -of these nine fails with ``AttributeError: type object '' has no -attribute 'get'`` -- the base did not carry those methods to them yet -- and -every base-tuple assertion in :class:`ReparentedBasesTests` and -:class:`HttpV2FlagsHierarchyTests` fails since ``EnumLookup`` is not yet in -any of these seventeen classes' ``__bases__`` or MRO. +:class:`TransportProtocol` defines its own ``get``, as a :class:`classmethod` +that delegates: :meth:`~pcapkit.corekit.enum.EnumLookup.get` is a +:class:`classmethod`, so a :class:`staticmethod` override is the exact trap +GitHub issue #908 hit and #915 fixed for +:meth:`~pcapkit.const.http.method.Method.get`. It keeps only its +``key.lower()`` call, because case folding has no equivalent on the base; +:class:`TransportProtocolGetTests` pins that behaviour, not merely that the +delegation compiles. :class:`Criticality` has no ``get`` of its own: one would +only convert the base's name-miss :exc:`KeyError` into a :exc:`ValueError`, a +conversion GitHub issue #923's ruling retired, so :class:`CriticalityGetTests` +exercises the inherited base. + +The rest are pure re-parenting -- no ``get`` or ``_missing_`` of their own to +reconcile -- so :class:`PureReparentGetTests` pins that ``get``/``get_all`` +exist on them and resolve. + +Without the re-parenting, every test below that calls ``.get()`` on one of +these fails with ``AttributeError: type object '' has no attribute +'get'`` -- the base is what carries those methods to them -- and every +base-tuple assertion in :class:`ReparentedBasesTests` and +:class:`HttpV2FlagsHierarchyTests` fails since ``EnumLookup`` is in none of +these classes' ``__bases__`` or MRO. """ from __future__ import annotations @@ -73,9 +67,9 @@ class ReparentedBasesTests(unittest.TestCase): - """Every one of the 11 unblocked classes gained :class:`EnumLookup` as a - base, mixed in *ahead of* its enum base so ``_member_type_`` still - resolves to :class:`int` or :class:`str`.""" + """Every unblocked class has :class:`EnumLookup` as a base, mixed in + *ahead of* its enum base so ``_member_type_`` resolves to :class:`int` or + :class:`str`.""" def test_transport_protocol(self) -> None: from aenum import IntEnum @@ -169,10 +163,9 @@ class HttpV2FlagsHierarchyTests(unittest.TestCase): Each concrete subclass declares ``class Flags(FrameType.Flags):`` with no base list of its own (see :mod:`pcapkit.protocols.schema.application. - httpv2`), so re-parenting the root should carry all six rather than - needing each done individually. Verified here at runtime rather than - assumed from Python's MRO rules, per the task's own instruction to check - rather than assume. + httpv2`), so the root's base carries to every subclass without each being + re-parented individually. Verified here at runtime rather than assumed + from Python's MRO rules. """ def test_frame_type_flags_itself(self) -> None: @@ -202,7 +195,7 @@ def test_all_six_concrete_subclasses_carry_it_transitively(self) -> None: for name, cls in subclasses.items(): with self.subTest(cls=name): # No base list of its own -- inherits solely from - # ``FrameType.Flags``, which is where ``EnumLookup`` was added. + # ``FrameType.Flags``, which is where ``EnumLookup`` sits. self.assertEqual(cls.__bases__, (FrameType.Flags,)) self.assertIn(EnumLookup, cls.__mro__) # And the contract actually works, not merely appears in the MRO. @@ -211,8 +204,8 @@ def test_all_six_concrete_subclasses_carry_it_transitively(self) -> None: class PureReparentGetTests(unittest.TestCase): - """The nine classes with no ``get``/``_missing_`` of their own: before - this change, ``.get()`` did not exist on any of them at all.""" + """The classes with no ``get``/``_missing_`` of their own: ``.get()`` + reaches them only through the base.""" def test_finalised_state_get(self) -> None: from pcapkit.corekit.infoclass import FinalisedState @@ -272,9 +265,9 @@ def test_a_pure_reparent_still_refuses_an_unknown_name(self) -> None: class TransportProtocolGetTests(unittest.TestCase): - """:class:`~pcapkit.const.reg.apptype.apptype.TransportProtocol` kept its - own ``get``, now delegating. Every behaviour the hand-rolled version had - is pinned here, not merely that the delegated version runs.""" + """:class:`~pcapkit.const.reg.apptype.apptype.TransportProtocol` keeps its + own ``get``, delegating. Every behaviour the override carries is pinned + here, not merely that the delegated call runs.""" def test_get_is_now_a_classmethod(self) -> None: from pcapkit.const.reg.apptype.apptype import TransportProtocol @@ -283,8 +276,8 @@ def test_get_is_now_a_classmethod(self) -> None: def test_case_folding_is_preserved(self) -> None: """The base's own ``str`` branch is case-sensitive; this override - still lower-cases before delegating, so an upper-cased spelling - must still resolve -- unlike a class that relies on the base alone + lower-cases before delegating, so an upper-cased spelling + must resolve -- unlike a class that relies on the base alone (see :class:`~tests.corekit.test_enum_lookup_reparent_877_unit. PureReparentGetTests`, all of whose members are matched exactly).""" from pcapkit.const.reg.apptype.apptype import TransportProtocol @@ -299,18 +292,14 @@ def test_int_path_unchanged(self) -> None: self.assertIs(TransportProtocol.get(1), TransportProtocol.tcp) def test_unrecognised_name_still_raises_naming_the_key(self) -> None: - """Maintainer ruling on PR #836: refuse, never mint. Still refused, and - still nothing minted -- but GitHub issue #923 retired the - ``KeyError`` -> ``ValueError`` conversion this override used to do, so - the refusal now reaches the caller as the base's own - :exc:`~pcapkit.utilities.exceptions.EnumKeyError`. Updated from the - pre-#923 tree, which asserted ``assertNotIsInstance(..., KeyError)``: - that assertion described the minority shape -- #923's census found - most concrete subclasses answering a name miss with a - :exc:`KeyError` -- and #923's ruling is that a name miss follows stdlib - ``E['nosuch']`` and stays :exc:`KeyError`-derived. The message is - unchanged, so the two ``assertIn``\\ s below are the same ones that - passed before.""" + """Maintainer ruling on PR #836: refuse, never mint. Nothing is minted, + and the refusal reaches the caller as the base's own + :exc:`~pcapkit.utilities.exceptions.EnumKeyError`: GitHub issue #923 + retired the ``KeyError`` -> ``ValueError`` conversion, ruling that a name + miss follows stdlib ``E['nosuch']`` and stays :exc:`KeyError`-derived. + ``assertNotIsInstance(..., KeyError)`` would describe the minority shape + -- #923's census found most concrete subclasses answering a name miss + with a :exc:`KeyError`.""" from pcapkit.utilities.exceptions import EnumKeyError from pcapkit.const.reg.apptype.apptype import TransportProtocol @@ -325,9 +314,9 @@ def test_unrecognised_name_still_raises_naming_the_key(self) -> None: self.assertEqual(len(TransportProtocol.__members__), before) def test_default_is_a_new_capability_not_exercised_before(self) -> None: - """``default`` did not exist on this override before this change -- - added purely because dropping an optional parameter the base - declares is a genuine ``mypy`` ``[override]`` violation. Forwarded + """``default`` exists on this override purely because dropping an + optional parameter the base declares is a genuine ``mypy`` + ``[override]`` violation. Forwarded verbatim, so it behaves exactly as :meth:`~pcapkit.corekit.enum.EnumLookup.get`'s own ``default``: a fallback to an *already-registered* value, never a new mint.""" @@ -335,26 +324,24 @@ def test_default_is_a_new_capability_not_exercised_before(self) -> None: self.assertIs(TransportProtocol.get('bogus', default=TransportProtocol.udp), TransportProtocol.udp) - # Omitted, as every call site in this tree omits it: still raises, as - # a KeyError since GitHub issue #923 rather than a ValueError. + # Omitted, as every call site in this tree omits it: raises a KeyError + # (GitHub issue #923), not a ValueError. with self.assertRaises(KeyError): TransportProtocol.get('bogus') class CriticalityGetTests(unittest.TestCase): - """:class:`~pcapkit.protocols.application.ngap.Criticality` kept its own - ``get`` for this issue, then lost it to GitHub issue #923: the only thing - the override still did that the base does not was convert the base's - name-miss :exc:`KeyError` into a :exc:`ValueError`, and #923's ruling - retired that conversion, leaving a pure pass-through. Every behaviour - below is therefore now the *inherited* base's, which is exactly what - makes the deletion worth pinning -- case-**sensitive**, unlike + """:class:`~pcapkit.protocols.application.ngap.Criticality` has no ``get`` + of its own (GitHub issue #923): an override would only convert the base's + name-miss :exc:`KeyError` into a :exc:`ValueError`, a conversion #923's + ruling retired. Every behaviour below is therefore the *inherited* + base's -- case-**sensitive**, unlike ``TransportProtocol``, which keeps its override for the case folding the base has no equivalent of.""" def test_get_is_inherited_rather_than_overridden(self) -> None: - """GitHub issue #923 deleted the override. ``get`` is still a - :class:`classmethod` reached through the class -- it is now + """There is no override (GitHub issue #923). ``get`` is a + :class:`classmethod` reached through the class -- :meth:`~pcapkit.corekit.enum.EnumLookup.get` itself, which is what the rest of this class exercises.""" from pcapkit.corekit.enum import EnumLookup @@ -378,10 +365,10 @@ def test_value_lookup(self) -> None: self.assertIs(Criticality.get(2), Criticality.notify) def test_a_criticality_instance_resolves_to_itself(self) -> None: - """The removed ``isinstance(key, Criticality): return key`` branch is - absorbed by the base's own ``cls(key)`` call -- a ``Criticality`` is - also an :class:`int` (it is an :class:`~aenum.IntEnum`), so the - non-``str`` path already hands back the identical canonical member.""" + """An ``isinstance(key, Criticality): return key`` branch is not + needed: the base's own ``cls(key)`` call covers it -- a ``Criticality`` + is also an :class:`int` (it is an :class:`~aenum.IntEnum`), so the + non-``str`` path hands back the identical canonical member.""" from pcapkit.protocols.application.ngap import Criticality self.assertIs(Criticality.get(Criticality.reject), Criticality.reject) @@ -389,7 +376,7 @@ def test_a_criticality_instance_resolves_to_itself(self) -> None: def test_case_sensitivity_is_unlike_transport_protocol(self) -> None: """Deliberately the opposite of ``TransportProtocol.get`` -- nothing here lower-cases, so an upper-cased spelling must be refused rather - than folded. Now :exc:`KeyError`-shaped, per GitHub issue #923.""" + than folded. :exc:`KeyError`-shaped, per GitHub issue #923.""" from pcapkit.protocols.application.ngap import Criticality with self.assertRaises(KeyError) as caught: @@ -397,15 +384,15 @@ def test_case_sensitivity_is_unlike_transport_protocol(self) -> None: self.assertIn('REJECT', str(caught.exception)) def test_unresolved_name_raises_key_error_not_value_error(self) -> None: - """The inverse of what this test asserted before GitHub issue #923. + """A name miss is a :exc:`KeyError`, not a :exc:`ValueError` (GitHub issue #923). - The deleted override converted the base's name-miss :exc:`KeyError` - into a :exc:`ValueError` so that an unknown name and an unknown value - reported identically. #923's ruling is that they must *not*: a stdlib + Converting the base's name-miss :exc:`KeyError` into a + :exc:`ValueError` would make an unknown name and an unknown value + report identically. #923's ruling is that they must *not*: a stdlib ``E['nosuch']`` raises :exc:`KeyError` and ``E(999)`` raises :exc:`ValueError`, so the two ways of getting this wrong are - deliberately distinguishable, and only the provenance of each moved - into :mod:`pcapkit.utilities.exceptions`.""" + deliberately distinguishable, and each is raised from + :mod:`pcapkit.utilities.exceptions`.""" from pcapkit.utilities.exceptions import EnumKeyError from pcapkit.protocols.application.ngap import Criticality @@ -417,7 +404,7 @@ def test_unresolved_name_raises_key_error_not_value_error(self) -> None: self.assertIn('NoSuchMember', str(caught.exception)) def test_unresolved_value_still_raises_via_missing(self) -> None: - """Still a :exc:`ValueError`, and now this class's own + """A :exc:`ValueError`, specifically :exc:`~pcapkit.utilities.exceptions.EnumValueError`: ``_missing_`` raises a bare :exc:`ValueError` and the base converts it, per GitHub issue #923's "no builtin exceptions" half.""" diff --git a/tests/corekit/test_enum_lookup_reparent_930_unit.py b/tests/corekit/test_enum_lookup_reparent_930_unit.py index adf25ed910..f02b478264 100644 --- a/tests/corekit/test_enum_lookup_reparent_930_unit.py +++ b/tests/corekit/test_enum_lookup_reparent_930_unit.py @@ -2,90 +2,50 @@ """GitHub issue #930, the blocked half of #877's phase 2: re-parenting the last seven non-registry enumerations onto :class:`~pcapkit.corekit.enum.EnumLookup`. -GitHub pull request #921 re-parented every other non-registry enumeration and deliberately -left these seven alone, because the files holding them were still open under other pull -requests at the time: :mod:`pcapkit.const.ftp.command` (``CommandType``, -``ConformanceRequirement``) under #913, and :mod:`pcapkit.protocols.internet.esp` -(``ESPStatus``) plus :mod:`pcapkit.protocols.internet.mh` -(``FastBindingAcknowledgmentStatus``, ``IPv6AddressPrefixCode``, ``LMAAddressCode``, -``LocalizedRoutingStatus``) under #904. Both blockers have since merged, and this -module pins that the remainder actually took the base -- finishing phase 2 with every -non-registry enumeration inside the hierarchy and none still outside it. - -Two of the seven, :class:`FastBindingAcknowledgmentStatus` and -:class:`IPv6AddressPrefixCode`, defined their own ``get`` at #930's own revision -- both as -a :class:`staticmethod`, the exact shape GitHub issue #908 found dangerous once a base -``get`` becomes a :class:`classmethod`: calling ``super().get(...)`` from a -``staticmethod`` raises :exc:`RuntimeError: super(): no arguments`. Neither override -called ``super()`` at all, so the trap never fired, and per this issue's own brief they -were left **untouched** there -- their ``except KeyError: raise EnumKeyError(...)`` -conversions were put there by GitHub issue #923 and already answered a name miss in -exactly the shape the base now uses. Keeping the plain ``@staticmethod`` cost something at -that revision, though: re-parenting made both **advertise** the base's two-argument -``get(key, default)`` through inheritance while still only accepting one, so a ``default`` -argument raised :exc:`TypeError` instead of resolving through the base's own fallback, and -``mypy``'s ``[override]`` check plus ``pylint``'s ``arguments-differ`` both flagged the -resulting shape mismatch, silenced with a suppression. GitHub issue #935 first answered -that on the owner's lean toward its first option -- widen both signatures to accept -``default`` and delete the suppression. On GitHub pull request #940, which was -implementing that widening, the owner then asked why the two overrides had to exist at -all rather than fall back to the base class's, and once a comparison against the base -showed them redundant, ruled for the second of the two options laid out there: deleting -both overrides outright rather than widening them. - -Measured before that final ruling: neither override ever minted an alias -- ``__members__`` -and ``list(cls)`` agree at 6 and 4 -- so what each docstring called "Backport support for -original codes" was the int-or-name dual resolution -:meth:`~pcapkit.corekit.enum.EnumLookup.get` already provides for every other -:class:`int`-valued registry in this tree. Checked before acting on it: none of the call -sites either override had (all in tests, none in :mod:`pcapkit`) passed a key the base -would have resolved differently. There was nothing left to backport, so ``get``/``get_all`` -on both now come from the base alone, the same as the five classes below that were pure -re-parents from the start. :class:`ReparentedBasesTests` used to pin, alongside each -class's own base-tuple change, that the ``@staticmethod`` decorator survived re-parenting -and then the signature widening; now that the method is deleted rather than converted, -there is nothing left to decorate, and :class:`AllSevenInheritTheBareClassmethodTests` -covers these two the same way it always covered the other five. - -Deleting the overrides is a real behaviour change, deliberately so: each branched on -``isinstance(key, int)`` and routed every other type -- ``None``, a :class:`float`, ... -- -through its own *name* path, so ``get(None)`` and ``get(1.5)`` used to answer with a quiet -:exc:`~pcapkit.utilities.exceptions.EnumKeyError` on these two while the base -- branching -on ``isinstance(key, str)`` instead -- answers every other -:class:`~pcapkit.corekit.enum.EnumLookup` subclass with a loud +GitHub pull request #921 re-parented every other non-registry enumeration and left these +seven alone, because the files holding them were open under other pull requests: +:mod:`pcapkit.const.ftp.command` (``CommandType``, ``ConformanceRequirement``) under #913, +and :mod:`pcapkit.protocols.internet.esp` (``ESPStatus``) plus +:mod:`pcapkit.protocols.internet.mh` (``FastBindingAcknowledgmentStatus``, +``IPv6AddressPrefixCode``, ``LMAAddressCode``, ``LocalizedRoutingStatus``) under #904. This +module pins that they derive from the base, so that every non-registry enumeration is +inside the hierarchy and none is outside it. + +None of the seven declares a ``get`` of its own: ``get`` and ``get_all`` come from the base +alone. For :class:`FastBindingAcknowledgmentStatus` and :class:`IPv6AddressPrefixCode` that +is GitHub issue #935's ruling, given on GitHub pull request #940: a hand-rolled override +there resolves by :class:`int` or by name exactly as +:meth:`~pcapkit.corekit.enum.EnumLookup.get` does -- neither class mints an alias, and +``__members__`` and ``list(cls)`` agree -- so the owner preferred no override to one widened +to accept ``default``. A one-argument :class:`staticmethod` override is a poor fit for the +base in two ways: it advertises the base's ``get(key, default)`` through inheritance while +raising :exc:`TypeError` on a ``default``, and calling ``super().get(...)`` from it raises +:exc:`RuntimeError: super(): no arguments` -- the shape GitHub issue #908 found dangerous +once a base ``get`` is a :class:`classmethod`. + +Inheriting the base's ``get`` also settles how a key of a third type is answered. An +override branching on ``isinstance(key, int)`` routes every other type -- ``None``, a +:class:`float`, ... -- through its own *name* path and answers with an +:exc:`~pcapkit.utilities.exceptions.EnumKeyError`, while the base, branching on +``isinstance(key, str)`` instead, answers with an :exc:`~pcapkit.utilities.exceptions.EnumValueError`. -:class:`NonCanonicalKeyConvergenceTests` pins the convergence this deletion produces: all -seven now answer such a key alike, for the first time. - -Re-parenting separately made both former overrides reachable through a door that did not -exist before: :meth:`~pcapkit.corekit.enum.EnumLookup.get_all`, inherited from the base for -the first time, calls ``get`` internally. Before this issue, that mattered because ``get`` -itself raised **loud**: both overrides used to log once at :data:`logging.CRITICAL` and set -:data:`sys.tracebacklimit` to ``0`` process-wide on a name miss, unlike the base's own quiet -raise. GitHub issue #930 converged both onto the base's quiet shape instead -- a real -behaviour change, not merely a re-parent -- settled on GitHub issue #933, where the owner -reversed an earlier answer: the two overrides should follow the library's house convention -for a name miss, which is the base's quiet raise, rather than stay loud as a special case. -:class:`InheritedQuietnessTests` (renamed from ``KeptOverrideQuietnessTests`` once GitHub -issue #935 deleted the overrides that name described) pins that the quiet shape survived -the deletion too -- purely inherited now, rather than reconciled by hand on each class. - -The other five -- :class:`CommandType`, :class:`ConformanceRequirement`, -:class:`ESPStatus`, :class:`LocalizedRoutingStatus` and :class:`LMAAddressCode` -- were -pure re-parents from the start: none defines a ``get`` of its own to reconcile with the -base, so each gained ``get``/``get_all`` for the first time in #930. :class:`CommandType`, -:class:`LocalizedRoutingStatus` and :class:`LMAAddressCode` do carry their own -``_missing_`` range guards, which are untouched -- :class:`EnumLookup` does not -override that hook, so a re-parent cannot change what it does. - -On the tree before this change, every ``.get(...)`` call below fails with -``AttributeError: type object '' has no attribute 'get'`` for the five pure -re-parents, and every base-tuple assertion in :class:`ReparentedBasesTests` fails since -:class:`~pcapkit.corekit.enum.EnumLookup` is not yet in any of these seven classes' -``__bases__`` or MRO. Quoted verbatim from an actual run of -:meth:`ReparentedBasesTests.test_command_type` with the three source files reverted to -``af2324522`` (origin/main immediately before this change; structurally identical to -``4f3d43df7`` for these seven classes) while this module itself stayed as written:: +:class:`NonCanonicalKeyConvergenceTests` pins that all seven answer such a key alike. + +:meth:`~pcapkit.corekit.enum.EnumLookup.get_all`, inherited from the base, calls ``get`` +internally, so a name miss reached through it is as quiet as one reached through ``get``. +GitHub issue #933 settled that these classes follow the library's house convention for a +name miss -- the base's quiet raise -- rather than logging at :data:`logging.CRITICAL` as a +special case; :class:`InheritedQuietnessTests` pins it. + +:class:`CommandType`, :class:`LocalizedRoutingStatus` and :class:`LMAAddressCode` carry +their own ``_missing_`` range guards, which :class:`EnumLookup` does not override, so the +re-parent cannot change what they do. + +Without the re-parenting, every ``.get(...)`` call below fails with +``AttributeError: type object '' has no attribute 'get'`` on the classes that declare +no ``get``, and every base-tuple assertion in :class:`ReparentedBasesTests` fails since +:class:`~pcapkit.corekit.enum.EnumLookup` is in none of these classes' ``__bases__`` or +MRO -- :meth:`ReparentedBasesTests.test_command_type`, for one:: AssertionError: Tuples differ: (,) != (, ) @@ -100,39 +60,8 @@ - (,) + (, ) -Measured against that reverted tree (plain :mod:`unittest`, 27 test methods, -each ``subTest`` resolved back to its parent method via -``getattr(test, 'test_case', test)`` rather than counted on its own -- -without that resolution a ``subTest``-only failure is recorded against -``unittest.case._SubTest`` and its parent method misreads as passing): -**22 of the 27 fail or error, and five hold** -- - -* ``ZeroRemainOutsideEnumLookupTests.test_the_walk_found_something_to_count`` -- - a guard that keeps the census assertion from passing vacuously on an empty - discovery, not itself a claim about this change, so it is expected to hold - either way. -* ``FailedLookupTests.test_fast_binding_acknowledgment_status``, - ``FailedLookupTests.test_ipv6_address_prefix_code``, - ``GetByNameAndValueTests.test_fast_binding_acknowledgment_status`` and - ``GetByNameAndValueTests.test_ipv6_address_prefix_code`` hold for the most - interesting reason in this file: :class:`FastBindingAcknowledgmentStatus` - and :class:`IPv6AddressPrefixCode` already had a working ``get`` before this - change -- their own kept :class:`staticmethod`, described above -- so their - lookup and failed-lookup behaviour is unaffected by the re-parent and these - four assertions hold on both trees. What *does* fail for both classes on the - reverted tree is their own :class:`ReparentedBasesTests` method, since the - base-tuple/MRO change is real for all seven regardless of whether ``get`` - itself was already working. - -Neither of the first two methods of :class:`InheritedQuietnessTests` (called -``KeptOverrideQuietnessTests`` at the time this measurement was taken, before GitHub issue -#935 deleted the overrides that name described) holds against the reverted tree, and -deliberately so -- both pin the one thing this issue actually changes about the two -classes' ``get``. ``test_get_is_now_quiet_on_both_classes`` fails against the reverted tree -because ``get`` really was loud there (see that class's own docstring): this is not a -scaffolding failure, it is the behaviour change itself, caught in the act. -``test_get_all_is_new_and_quiet_too`` fails with ``AttributeError`` instead, since -``get_all`` does not exist on the reverted tree at all. +:meth:`ZeroRemainOutsideEnumLookupTests.test_the_walk_found_something_to_count` holds either +way: it keeps the census assertion from passing vacuously on an empty discovery. """ from __future__ import annotations @@ -210,11 +139,9 @@ def test_esp_status(self) -> None: self.assertEqual(len(list(ESPStatus)), 6) def test_fast_binding_acknowledgment_status(self) -> None: - """GitHub pull request #940 later deleted its kept ``get`` override - outright (the owner preferred deleting it to widening it to accept - ``default``), so the decorator this once pinned no longer exists to - pin -- :class:`AllSevenInheritTheBareClassmethodTests` now covers this - class alongside the other six.""" + """No ``get`` override to pin here (GitHub pull request #940) -- + :class:`AllSevenInheritTheBareClassmethodTests` covers this class + alongside the other six.""" from aenum import IntEnum from pcapkit.corekit.enum import EnumLookup @@ -226,7 +153,7 @@ class alongside the other six.""" self.assertEqual(len(list(FastBindingAcknowledgmentStatus)), 6) def test_ipv6_address_prefix_code(self) -> None: - """Same history as :class:`FastBindingAcknowledgmentStatus`.""" + """Same as for :class:`FastBindingAcknowledgmentStatus`.""" from aenum import IntEnum from pcapkit.corekit.enum import EnumLookup @@ -305,11 +232,9 @@ def test_a_battery_of_lookups_does_not_grow_any_of_the_seven(self) -> None: class GetByNameAndValueTests(unittest.TestCase): - """``get`` resolves by name and by value on each of the seven, now - uniformly through the base's own inherited implementation -- GitHub - issue #935 deleted the two hand-rolled overrides that used to answer - this for :class:`FastBindingAcknowledgmentStatus` and - :class:`IPv6AddressPrefixCode`.""" + """``get`` resolves by name and by value on each of the seven, uniformly + through the base's own inherited implementation -- none of them carries + an override (GitHub issue #935).""" def test_command_type(self) -> None: from pcapkit.const.ftp.command import CommandType @@ -364,10 +289,8 @@ class FailedLookupTests(unittest.TestCase): """What a miss raises on each of the seven -- a name miss is :exc:`KeyError`-derived and a value miss :exc:`ValueError`-derived, matching stdlib :class:`~enum.Enum`'s own shape (GitHub issue #923) on - every one of the seven, now uniformly through the base's own ``get`` - since GitHub issue #935 deleted the two hand-rolled overrides that used - to answer this for :class:`FastBindingAcknowledgmentStatus` and - :class:`IPv6AddressPrefixCode`. + every one of the seven, uniformly through the base's own ``get`` (GitHub + issue #935). """ def test_command_type(self) -> None: @@ -407,9 +330,9 @@ def test_esp_status(self) -> None: self.assertIsInstance(value_miss.exception, EnumValueError) def test_fast_binding_acknowledgment_status(self) -> None: - """Since GitHub issue #935 deleted its kept override, this now - resolves through the base's own ``get`` -- the same mechanism - :meth:`test_localized_routing_status` below exercises.""" + """Resolves through the base's own ``get`` (GitHub issue #935) -- the + same mechanism :meth:`test_localized_routing_status` below + exercises.""" from pcapkit.protocols.internet.mh import FastBindingAcknowledgmentStatus from pcapkit.utilities.exceptions import EnumKeyError, EnumValueError @@ -464,50 +387,22 @@ def test_lma_address_code(self) -> None: class InheritedQuietnessTests(unittest.TestCase): """The quiet raise :class:`FastBindingAcknowledgmentStatus` and - :class:`IPv6AddressPrefixCode` answer a name miss with, and the door - this re-parenting opened to reach it -- both now purely inherited from - the base rather than reconciled by a hand-rolled override. - - Named for what survives rather than for what used to sit here: this - class was ``KeptOverrideQuietnessTests`` while both classes still - carried their own ``get``, first through #930's re-parenting and - briefly again through GitHub issue #935's first attempt, which widened - that override to accept ``default`` rather than delete it. The owner's - final ruling, given on GitHub pull request #940, went the other way: an - earlier lean on GitHub issue #935 had favoured widening, but on - reviewing that attempt the owner preferred deleting both overrides - outright. What this class pins did not change with that deletion -- the - quiet raise -- only *how* it is produced through - :meth:`~pcapkit.corekit.enum.EnumLookup.get` - (:mod:`pcapkit.corekit.enum`) directly now, rather than through an - override that reconciled itself onto the base's shape. - - Before either override existed, ``get`` on both classes raised - **loud** -- logging once at :data:`logging.CRITICAL` and setting - :data:`sys.tracebacklimit` to ``0`` process-wide on a name miss, unlike - the base's own quiet raise. GitHub issue #930 converged both onto that - quiet shape instead, settled on GitHub issue #933's follow-up ruling, - in which the owner reversed an earlier answer: the two should follow - house convention and not be loud. (The owner's first answer on the - same issue was a bare refusal, which selected the issue's own numbered - option for keeping them loud, and an earlier revision of this file - pinned loud as the settled answer on that basis; the follow-up five - minutes later reversed it, and what follows is the corrected version.) - - The first two methods are pinned quiet, and for two different reasons - against the tree reverted to before #930. - :meth:`test_get_is_now_quiet_on_both_classes` covers ``get`` itself, - which existed and was already loud on the tree before that issue -- so - this test fails against that reverted tree not because ``get`` is - structurally different there, but because its behaviour genuinely - changes: reverted, it is loud; here, it is quiet. - :meth:`test_get_all_is_new_and_quiet_too` covers - :meth:`~pcapkit.corekit.enum.EnumLookup.get_all`, which did not exist - on either class before #930 at all and is now inherited from the base, - calling this same ``get`` internally -- so a miss reached through - ``get_all`` is quiet for the same reason ``get`` itself is, and this - test fails against the reverted tree with ``AttributeError`` before it - ever reaches the quietness assertion. + :class:`IPv6AddressPrefixCode` answer a name miss with, and + :meth:`~pcapkit.corekit.enum.EnumLookup.get_all`, the path the + re-parenting opens to reach it -- both inherited from the base + (:mod:`pcapkit.corekit.enum`) rather than reconciled by a hand-rolled + override. + + Without the fix, ``get`` on both classes raises **loud** -- logging once + at :data:`logging.CRITICAL` and setting :data:`sys.tracebacklimit` to + ``0`` process-wide on a name miss, unlike the base's own quiet raise. + GitHub issue #933 settled that the two follow house convention and are + not loud. + + :meth:`test_get_is_now_quiet_on_both_classes` covers ``get`` itself, and + :meth:`test_get_all_is_new_and_quiet_too` covers ``get_all``, which calls + this same ``get`` internally -- so a miss reached through ``get_all`` is + quiet for the same reason ``get`` itself is. """ def setUp(self) -> None: @@ -521,9 +416,9 @@ def tearDown(self) -> None: sys.tracebacklimit = self._saved_tracebacklimit def test_get_is_now_quiet_on_both_classes(self) -> None: - """The behaviour GitHub issue #930 actually changes: ``get`` was - loud on the tree before this issue (see the class docstring) and - is quiet now, matching the base.""" + """The behaviour GitHub issue #930 pins: ``get`` is quiet, matching + the base, where without the fix it is loud (see the class + docstring).""" from pcapkit.protocols.internet.mh import (FastBindingAcknowledgmentStatus, IPv6AddressPrefixCode) from pcapkit.utilities.exceptions import EnumKeyError @@ -539,17 +434,16 @@ def test_get_is_now_quiet_on_both_classes(self) -> None: self.assertEqual(recorder.messages, []) def test_get_all_is_new_and_quiet_too(self) -> None: - """The half this issue adds outright: ``get_all`` did not exist on - either class before, and the miss it now reaches through this same - ``get`` is quiet for the same reason ``get`` itself is.""" + """``get_all`` exists on both classes, and the miss it reaches through + this same ``get`` is quiet for the same reason ``get`` itself is.""" from pcapkit.protocols.internet.mh import (FastBindingAcknowledgmentStatus, IPv6AddressPrefixCode) from pcapkit.utilities.exceptions import EnumKeyError for cls in (FastBindingAcknowledgmentStatus, IPv6AddressPrefixCode): with self.subTest(cls=cls.__name__): - # Not merely present: it resolves through this class's own - # kept ``get`` rather than some other path -- ``get_all`` + # Not merely present: it resolves through ``get`` rather than + # some other path -- ``get_all`` # calls ``cls.get(key)`` and wraps the canonical member in a # one-item tuple (see ``EnumLookup.get_all``). member = next(iter(cls)) @@ -564,15 +458,13 @@ def test_get_all_is_new_and_quiet_too(self) -> None: self.assertEqual(recorder.messages, []) def test_get_with_unusable_default_is_quiet_too(self) -> None: - """The base's own ``get`` (:mod:`pcapkit.corekit.enum`) has always - accepted ``default``; a name miss whose ``default`` does not itself - resolve falls through to the same quiet - :exc:`~pcapkit.utilities.exceptions.EnumKeyError` a bare miss would - have raised -- not a new, louder path. Before GitHub issue #935 - deleted the two hand-rolled overrides, this same call raised - ``TypeError`` on a second positional argument instead; between - #935's first attempt and its final ruling, the overrides answered - it themselves rather than through this inherited path. + """The base's own ``get`` (:mod:`pcapkit.corekit.enum`) accepts + ``default``; a name miss whose ``default`` does not itself resolve + falls through to the same quiet + :exc:`~pcapkit.utilities.exceptions.EnumKeyError` a bare miss + raises -- not a new, louder path. A one-argument override raises + ``TypeError`` on the second positional argument instead (GitHub + issue #935). """ from pcapkit.protocols.internet.mh import (FastBindingAcknowledgmentStatus, IPv6AddressPrefixCode) @@ -595,24 +487,16 @@ def test_get_with_unusable_default_is_quiet_too(self) -> None: class AllSevenInheritTheBareClassmethodTests(unittest.TestCase): - """All seven now inherit the base's ``classmethod`` outright, none + """All seven inherit the base's ``classmethod`` outright, none declaring a ``get`` of its own. - Five were always this way -- pure re-parents, having defined no - ``get`` of their own to begin with. The other two, - :class:`FastBindingAcknowledgmentStatus` and - :class:`IPv6AddressPrefixCode`, joined them only at GitHub pull request #940's - final revision: their own kept ``get`` stayed a :class:`staticmethod` - through #930's re-parenting and briefly again through #935's first attempt - (which widened it to accept ``default`` rather than delete it), and only - the owner's final ruling on #940 -- outright deletion over widening the - signature -- removed it, collapsing the seven-way split this - class used to test as five-plus-two into one uniform case. This class - was named for the five alone before that ruling, and - :class:`ReparentedBasesTests` pinned the other two's surviving - ``@staticmethod`` separately, since a test that only checked the - decorator would have passed identically whether ``get`` were kept or - converted, and pinned nothing about which. + For :class:`FastBindingAcknowledgmentStatus` and + :class:`IPv6AddressPrefixCode` that is the owner's ruling on GitHub pull + request #940 -- no override rather than one widened to accept + ``default`` -- so the seven are one uniform case. A test that only + checked the decorator would pass identically whether an override were + kept or converted, so this also asserts that ``get`` is absent from each + class's own ``__dict__``. """ def test_all_seven_inherit_the_bare_classmethod(self) -> None: @@ -628,39 +512,34 @@ def test_all_seven_inherit_the_bare_classmethod(self) -> None: with self.subTest(cls=cls.__name__): self.assertIsInstance(inspect.getattr_static(cls, 'get'), classmethod) # Inherited, not redeclared: the class's own ``__dict__`` carries - # no ``get`` of its own -- the fact that used to distinguish a - # pure re-parent from a kept-and-converted override, and now - # holds for all seven alike. + # no ``get`` of its own -- which holds for all seven alike. self.assertNotIn('get', vars(cls)) class NonCanonicalKeyConvergenceTests(unittest.TestCase): """The point of GitHub issue #935's final ruling, made literally true: - ``get(None)`` and ``get(1.5)`` now answer alike on all seven. + ``get(None)`` and ``get(1.5)`` answer alike on all seven. - Before the two overrides were deleted, each branched on - ``isinstance(key, int)`` and routed every other type through its own - *name* path -- so a key that is neither an :class:`int` nor a - :class:`str` fell through to a subscript lookup that always misses, - answering with a quiet + An override branching on ``isinstance(key, int)`` routes every other + type through its own *name* path -- so a key that is neither an + :class:`int` nor a :class:`str` falls through to a subscript lookup that + always misses, answering with a quiet :exc:`~pcapkit.utilities.exceptions.EnumKeyError`. The base (:mod:`pcapkit.corekit.enum`) branches on ``isinstance(key, str)`` instead, so the same key falls through to :meth:`~pcapkit.corekit.enum.EnumLookup._validate_value` and the constructor, reaching :meth:`_missing_` and answering with an - :exc:`~pcapkit.utilities.exceptions.EnumValueError`. Deleting the - overrides removes the branch that disagreed, so all seven now answer - both keys the same way -- measured here rather than assumed, since - "all seven behave alike" is exactly the claim GitHub issue #935 set - out to make true, and the two overrides are exactly what kept it from - being true before this. - - Scope of that change, stated as narrowly as the evidence supports: - what moved is the exception a key of some third type raises. Whether - any caller ever passed one was not established -- a key reaching - ``get`` through a variable cannot be read off a search of the tree -- - so this test pins the converged behaviour directly rather than resting - on the divergence having been unreachable. + :exc:`~pcapkit.utilities.exceptions.EnumValueError`. With no override, + all seven answer both keys the same way -- measured here rather than + assumed, since "all seven behave alike" is exactly the claim GitHub + issue #935 set out to make true. + + Scope, stated as narrowly as the evidence supports: what this settles is + the exception a key of some third type raises. Whether any caller passes + one is not established -- a key reaching ``get`` through a variable + cannot be read off a search of the tree -- so this test pins the + converged behaviour directly rather than resting on the divergence being + unreachable. """ def test_none_and_float_keys_all_raise_enumvalueerror(self) -> None: @@ -685,7 +564,7 @@ def test_none_and_float_keys_all_raise_enumvalueerror(self) -> None: class ZeroRemainOutsideEnumLookupTests(unittest.TestCase): """The census this issue exists to finish: a runtime walk over every importable :mod:`pcapkit.*` module (vendor templates excluded), filtering - on ``issubclass(cls, EnumLookup)``, must now find **zero** enumerations + on ``issubclass(cls, EnumLookup)``, must find **zero** enumerations outside the hierarchy -- the assertion that stops this recurring yet again. Nested classes are walked too, since GitHub issue #877's own census of non-registry enumerations includes the ``httpv2`` ``Flags`` diff --git a/tests/corekit/test_fields_bytearray_pack_2004_unit.py b/tests/corekit/test_fields_bytearray_pack_2004_unit.py index 2658fb45e0..4dda430a83 100644 --- a/tests/corekit/test_fields_bytearray_pack_2004_unit.py +++ b/tests/corekit/test_fields_bytearray_pack_2004_unit.py @@ -3,9 +3,10 @@ GitHub issue #2004. CPython's :func:`struct.pack` takes a :obj:`bytearray` for the ``s`` and ``p`` formats, but PyPy's takes :obj:`bytes` alone and raises -``TypeError: expected bytes, got bytearray object``, so a :obj:`bytearray` value -packed by :meth:`Field.pack ` failed on -PyPy. The field hands :func:`struct.pack` a :obj:`bytes` copy. +``TypeError: expected bytes, got bytearray object``, so without the fix a +:obj:`bytearray` value packed by :meth:`Field.pack +` fails on PyPy. The field hands +:func:`struct.pack` a :obj:`bytes` copy. The PyPy behaviour is reproduced on any interpreter with a :mod:`struct` stand-in that refuses a :obj:`bytearray`, as PyPy's does. The real diff --git a/tests/corekit/test_fields_byteorder_1519_unit.py b/tests/corekit/test_fields_byteorder_1519_unit.py index c163ecf4e5..6af8fc48a1 100644 --- a/tests/corekit/test_fields_byteorder_1519_unit.py +++ b/tests/corekit/test_fields_byteorder_1519_unit.py @@ -1,20 +1,20 @@ # -*- coding: utf-8 -*- """The byte-order callback the PCAP and PCAP-NG schemas share. -GitHub issue #1519. The PCAP frame header and the PCAP-NG blocks each had a -``byteorder_callback``, and the PCAP-NG one additionally fell back to an -enclosing ``__packet__``. One helper now serves both, with the PCAP-NG body: a -superset that gives the frame header the same answer on every packet the +GitHub issue #1519. One helper serves both the PCAP frame header and the +PCAP-NG blocks, with the PCAP-NG body and its fallback to an enclosing +``__packet__``: a superset that gives the frame header the same answer on every +packet the library hands it, since :meth:`Frame.pack ` and :meth:`Frame.unpack ` always seed ``byteorder``. This pins the helper, its ``__packet__`` fallback, and that both schemas use it. -GitHub issue #1711 then moved the helper from -``pcapkit.protocols.schema.misc.byteorder`` to -:mod:`pcapkit.corekit.fields.byteorder`, with no re-export left behind. This -also pins that the old path no longer imports, and that the new module reaches -no higher than :mod:`pcapkit.corekit`. +Per GitHub issue #1711 the helper lives in +:mod:`pcapkit.corekit.fields.byteorder`, with no re-export at the old +``pcapkit.protocols.schema.misc.byteorder`` path. This also pins that the old +path does not import, and that the module reaches no higher than +:mod:`pcapkit.corekit`. """ @@ -33,7 +33,7 @@ SHARED = 'pcapkit.corekit.fields.byteorder' -#: Where the helper lived before #1711; removed, not re-exported. +#: The path the helper moved from (#1711); nothing is left there, not even a re-export. OLD_PATH = 'pcapkit.protocols.schema.misc.byteorder' #: Source of :data:`SHARED`, read with :mod:`ast` rather than imported. diff --git a/tests/corekit/test_fields_copy_dispatch_runtime.py b/tests/corekit/test_fields_copy_dispatch_runtime.py index 068c425acc..e91eb70a18 100644 --- a/tests/corekit/test_fields_copy_dispatch_runtime.py +++ b/tests/corekit/test_fields_copy_dispatch_runtime.py @@ -2,10 +2,10 @@ """Byte-identity proof for GitHub issue #730, across the classic-PCAP sample captures. :mod:`tests.corekit.test_fields_copy_dispatch_unit` proves the *mechanism* the -issue asked about: every field ``__call__`` now reaches +issue asked about: every field ``__call__`` reaches :meth:`~pcapkit.corekit.fields.field.FieldBase.__copy__` directly instead of going through :func:`copy.copy`. This module proves the thing that actually -matters -- that swapping the dispatch did not move a single byte of what +matters -- that the direct dispatch does not move a single byte of what :func:`~pcapkit.interface.core.extract` produces. Every field of every protocol is copied through one of the call sites @@ -24,12 +24,11 @@ PCAP-NG is deliberately out of scope here: its block framing is not a fixed 16-octet record header, so the same offset walk does not apply, and its own round-trip fidelity is already covered by -:mod:`tests.protocols.test_pcapng_regression`. The field ``__call__`` change +:mod:`tests.protocols.test_pcapng_regression`. The field ``__call__`` dispatch this module guards is format-agnostic -- every field, in every schema, of -every protocol, in every container format, goes through one of the same eight +every protocol, in every container format, goes through one of the same call sites -- so classic PCAP alone already exercises the mechanism; a -passing run of the existing PCAP-NG suite (unaffected by this change) is the -rest of the proof. +passing run of the PCAP-NG suite is the rest of the proof. """ from __future__ import annotations diff --git a/tests/corekit/test_fields_copy_dispatch_unit.py b/tests/corekit/test_fields_copy_dispatch_unit.py index 75ee23a10c..8d7c11fa70 100644 --- a/tests/corekit/test_fields_copy_dispatch_unit.py +++ b/tests/corekit/test_fields_copy_dispatch_unit.py @@ -19,8 +19,8 @@ module proves that: every one of those call sites reaches :meth:`FieldBase.__copy__` without ever calling :func:`copy.copy`. -On the tree before this fix, every test below fails: each ``__call__`` -listed in the issue called :func:`copy.copy` exactly once (twice for +Without the fix, every test below fails: each ``__call__`` +listed in the issue calls :func:`copy.copy` exactly once (twice for :class:`~pcapkit.corekit.fields.misc.ConditionalField`, :class:`~pcapkit.corekit.fields.misc.SwitchField` and :class:`~pcapkit.corekit.fields.misc.ForwardMatchField`, whose ``__call__`` @@ -71,8 +71,8 @@ def assertCallReachesCopyDunderOnly(self, field: 'FieldBase[Any]') -> None: field: A field instance built with whatever a real call site would give it -- a real callback default, a real selector, etc. -- rather than a bare :class:`~unittest.mock.Mock`, so this - exercises the exact ``__call__`` bodies :func:`copy.copy(self) - ` used to run through. + exercises the exact ``__call__`` bodies that would otherwise + run through :func:`copy.copy(self) `. """ with mock.patch('copy.copy', side_effect=copy.copy) as copy_copy, \ diff --git a/tests/corekit/test_fields_field.py b/tests/corekit/test_fields_field.py index 4b7db1c861..34ec2444b3 100644 --- a/tests/corekit/test_fields_field.py +++ b/tests/corekit/test_fields_field.py @@ -18,10 +18,10 @@ class FieldBaseUnpackBoundsTests(unittest.TestCase): ``length`` there is frequently wire-derived -- resolved by a ``_length_callback`` against the very packet under parse, or built by a schema selector from a value it just read off the wire -- and is thus - attacker- or corruption-controlled. Before #554, - ``buffer[:length].rjust(length, b'\\x00')`` zero-padded up to ``length`` - octets regardless of how little data ``buffer`` actually held, so a - short, otherwise unremarkable capture could declare a multi-gigabyte + attacker- or corruption-controlled. Without the #554 guard, + ``buffer[:length].rjust(length, b'\\x00')`` zero-pads up to ``length`` + octets regardless of how little data ``buffer`` actually holds, so a + short, otherwise unremarkable capture can declare a multi-gigabyte field and force that allocation. The fix is deliberately *not* "reject any length the buffer falls short @@ -78,12 +78,12 @@ def test_small_field_short_by_one_octet_still_zero_pads(self) -> None: by its snapshot length would start failing to parse instead of reporting the tail as padding. This field is nowhere near :data:`~pcapkit.utilities.config.MAX_ZERO_PAD_LENGTH`, so the - shortfall must still be padded exactly as before #554. + shortfall must be padded exactly as it is without #554's guard. The zeros land on the *tail*, which is where the unread octets were: a short read has lost the end of the buffer, not the start of it. - Before #604 they were placed at the front instead, which corrupted - the value rather than merely padding it -- see + Without the #604 fix they are placed at the front instead, which + corrupts the value rather than merely padding it -- see :class:`FieldBaseShortReadPaddingSideTests` below. """ field = self.BytesField(length=4) @@ -113,7 +113,7 @@ def test_field_at_the_ceiling_still_zero_pads(self) -> None: # ljust() left-justifies: the one real octet stays where it was read, # at the front, and the padding fills the tail that was never read. - # Before #604 this was the other way round. + # Without the #604 fix this is the other way round. self.assertEqual(len(result), self.ceiling) self.assertEqual(result[:1], b'A') self.assertEqual(result[1:], b'\x00' * (self.ceiling - 1)) @@ -168,8 +168,8 @@ def test_wire_declared_length_far_larger_than_the_buffer_is_rejected_without_all ``length`` whatever it likes. A ~40-octet PCAP-NG Decryption Secrets Block with a bogus inner - length was enough to force a multi-gigabyte ``rjust()`` under the - unfixed code. ``length`` here is set to 16 GiB -- large enough that, + length is enough to force a multi-gigabyte ``rjust()`` without the + fix. ``length`` here is set to 16 GiB -- large enough that, if the guard were absent, comparing against the wrong thing, or otherwise not actually reached, this test would try to allocate that much and either hang or fail with :exc:`MemoryError` rather than @@ -200,13 +200,13 @@ class FieldBaseCumulativePaddingBudgetTests(unittest.TestCase): holds many fields, so a declared length sitting just *under* :data:`~pcapkit.utilities.config.MAX_ZERO_PAD_LENGTH` is honoured however many times it is declared, and the sum has no bound at all. Measured - on the tree that carried only the per-field ceiling, 200 minimal PCAP-NG + with only the per-field ceiling in place, 200 minimal PCAP-NG Decryption Secrets Blocks -- 4,800 wire octets, each declaring ``secrets_length`` of 262,142 against two supplied octets -- retained **50.0 MiB**, an amplification of 10,922x per block, with the ceiling never firing once because every single field was within it. - :meth:`FieldBase.unpack ` now + :meth:`FieldBase.unpack ` keeps a running ledger of octets supplied against octets synthesised, and refuses a shortfall *past* :data:`~pcapkit.utilities.config.MAX_ZERO_PAD_SHORTFALL` once the total @@ -258,7 +258,7 @@ def setUp(self) -> None: # NOTE: deliberately *not* read off the module. The behavioural tests # below have to fail on their assertions when the budget is absent, not # on an :exc:`AttributeError` raised here before any of them runs -- a - # setup that reaches for a name the unfixed code does not have turns every + # setup that reaches for a name absent without the fix turns every # case in the class into the same uninformative error and proves nothing # about behaviour. Only the two cases that exist to pin the constants # themselves name them. @@ -295,9 +295,9 @@ def test_many_fields_each_under_the_ceiling_are_refused_in_total(self) -> None: Each declares two octets short of :data:`~pcapkit.utilities.config.MAX_ZERO_PAD_LENGTH` against two supplied octets, so #554's per-field guard is *correct* to let each one - through -- and before #573 all 200 went through, retaining 50.0 MiB from - 4,800 octets of input. At least one has to be refused now, or the sum is - still unbounded. + through -- and without the #573 fix all 200 go through, retaining 50.0 MiB + from 4,800 octets of input. At least one has to be refused, or the sum is + unbounded. """ _, accepted, refused = self.pad_until_refused(self.ceiling - 2, b'\xaa\xbb') @@ -387,8 +387,8 @@ def test_reading_real_octets_earns_allowance_for_padding_them(self) -> None: with self.assertRaises(self.FieldValueError): self.BytesField(length=4 * 1024 * 1024).unpack(b'A', {}) - # but several near-ceiling fields in a row are now affordable, where from - # a cold start only the first was. + # but after that bulk several near-ceiling fields in a row are affordable, + # where from a cold start only the first is. padded, accepted, refused = self.pad_until_refused(self.ceiling - 2, b'\xaa\xbb', limit=4) self.assertEqual((accepted, refused), (4, 0)) @@ -604,7 +604,7 @@ class FieldBaseShortReadPaddingSideTests(unittest.TestCase): exhausting memory, and it does so silently -- no exception, no warning, just a plausible integer that is wrong. - Before #604 the padding was applied with ``rjust()``, which places the zeros + Without the fix the padding is applied with ``rjust()``, which places the zeros at the *front* of the buffer. That asserts that the octets which were never read were the *leading* ones. A short read asserts the opposite: the buffer ran out, so what is missing is whatever came *after* what was read. The zeros @@ -751,9 +751,9 @@ def test_a_full_read_is_untouched_at_every_width_and_order(self) -> None: """The regression guard: nothing about a complete field may move. The padding side is only ever consulted when the buffer falls short, so a - field that got all its octets must answer exactly as it did before #604 -- - for a big-endian field especially, since that is the one whose short-read - behaviour this change alters and whose full-read behaviour must not. + field that got all its octets must answer exactly as it would without the + #604 fix -- for a big-endian field especially, since that is the one whose + short-read behaviour #604 alters and whose full-read behaviour it must not. """ cases = [ (2, self.UInt16Field, 0x0102), @@ -857,7 +857,7 @@ def test_a_byte_string_field_keeps_its_octets_at_the_front(self) -> None: data[:kept] + b'\x00' * (width - kept)) def test_an_entirely_empty_buffer_still_reads_as_zero(self) -> None: - """The #431 invariant, which this change must not disturb. + """The #431 invariant, which the #604 fix must not disturb. ``OptionField.unpack`` and ``ListField.unpack`` read a fixed-width, few-octet field past the end of a truncated option area and need it to @@ -865,9 +865,9 @@ def test_an_entirely_empty_buffer_still_reads_as_zero(self) -> None: short by its snapshot length reads as end-of-option-list or ``Pad1`` rather than raising. With nothing at all in the buffer the whole field is padding, so both ``rjust()`` and ``ljust()`` produce all zeros and the - answer is the same before and after #604. Asserted explicitly, because it - is the property a fix to the padding side could most easily have broken - and the one that declined #571. + answer is the same with or without the #604 fix. Asserted explicitly, + because it is the property a fix to the padding side could most easily + break and the one that declined #571. """ for byteorder in ('big', 'little'): for cls in (self.UInt16Field, self.UInt32Field, self.UInt64Field): diff --git a/tests/corekit/test_fields_ipaddress.py b/tests/corekit/test_fields_ipaddress.py index 03e6209523..61a3eefc4f 100644 --- a/tests/corekit/test_fields_ipaddress.py +++ b/tests/corekit/test_fields_ipaddress.py @@ -10,10 +10,10 @@ HAS_RUNTIME = all(importlib.util.find_spec(name) is not None for name in RUNTIME_DEPS) #: Prefix lengths whose octet happens to be an ASCII digit, i.e. ``0x30``--``0x39``. -#: :meth:`IPv6InterfaceField.post_process` used to read the octet as an ASCII -#: decimal string, so these were the only prefix lengths that parsed at all -- -#: and every one of them decoded to ``prefixlen - 48``. Every other prefix length, -#: including all the common ones, raised :exc:`ValueError`. +#: Without the fix, :meth:`IPv6InterfaceField.post_process` reads the octet as an +#: ASCII decimal string, so these are the only prefix lengths that parse at all -- +#: and every one of them decodes to ``prefixlen - 48``. Every other prefix length, +#: including all the common ones, raises :exc:`ValueError`. ASCII_DIGIT_PREFIX_LENGTHS = tuple(range(48, 58)) #: Section 4.2 of the PCAP-NG specification: ``if_IPv6addr`` is 17 octets, of @@ -84,14 +84,14 @@ def test_ipv6_interface_does_not_read_prefix_length_octet_as_an_ascii_digit(self interface = ipaddress.ip_interface(f'2001:db8::1/{prefixlen}') raw = field.pack(interface, {}) - # the octet really is an ASCII digit, which is why these ten used - # to parse while every other prefix length raised + # the octet really is an ASCII digit, which is why, without the + # fix, these parse while every other prefix length raises self.assertIn(raw[16:], [str(digit).encode() for digit in range(10)]) parsed = field.unpack(raw, {}) self.assertEqual(parsed, interface) self.assertEqual(parsed.network.prefixlen, prefixlen) - # the old ASCII reading decoded /48../57 as /0../9 + # without the fix, the ASCII reading decodes /48../57 as /0../9 self.assertNotEqual(parsed.network.prefixlen, prefixlen - 48) def test_ipv6_interface_rejects_out_of_range_prefix_length(self) -> None: @@ -152,13 +152,13 @@ def test_ipv4_interface_rejects_the_other_version(self) -> None: IPv4InterfaceField().pre_process('2001:db8::1/64', {}) def test_pre_process_malformed_value_raises_in_library_error(self) -> None: - """A malformed address/interface string used to let a bare - :exc:`ValueError` from :mod:`ipaddress` escape -- unlike the very next - statement in the same method, which already raised the library's own + """A malformed address/interface string must not let a bare + :exc:`ValueError` from :mod:`ipaddress` escape -- the very next + statement in the same method raises the library's own :exc:`FieldValueError` for a value that is merely the wrong IP - version. So ``except BaseError`` could not reliably catch a bad field - value: whether the exception was in-library depended on *how* the - value was wrong. Every public field class must raise + version. Otherwise ``except BaseError`` cannot reliably catch a bad + field value: whether the exception is in-library would depend on + *how* the value is wrong. Every public field class must raise :exc:`FieldValueError` here too, with the original :mod:`ipaddress` message preserved. """ @@ -186,7 +186,7 @@ def test_pre_process_malformed_value_raises_in_library_error(self) -> None: def test_wrong_version_message_is_not_relabelled_as_a_malformed_value(self) -> None: """Wrapping the conversion must not broaden into swallowing the - pre-existing wrong-version ``FieldValueError`` -- its message stays + wrong-version ``FieldValueError`` the method raises of its own -- its message stays the version-mismatch message, not the "invalid IP ..." message used for a genuinely malformed value. """ @@ -208,12 +208,12 @@ def test_address_and_interface_fields_reject_a_bool_value_for_every_version(self :obj:`bool` is an :class:`int` subclass, and :func:`ipaddress.ip_address`/ :func:`ipaddress.ip_interface` both treat any :class:`int` below - ``2**32`` as IPv4 -- so before this fix, ``True``/``False`` were + ``2**32`` as IPv4 -- so without the fix, ``True``/``False`` are silently converted to ``0.0.0.1``/``0.0.0.0`` (or the equivalent interface) on an IPv4-typed field, with no exception and no warning. - On an IPv6-typed field the same conversion happened to raise instead, + On an IPv6-typed field the same conversion happens to raise instead, because the resulting :class:`~ipaddress.IPv4Address`'s version - mismatched -- and that asymmetry is exactly what let this survive + mismatches -- and that asymmetry is exactly what let this survive #481, which fixed the identical mechanism at only one call site (``MH._make_opt_mn_id``). See #491. @@ -254,9 +254,9 @@ def test_address_and_interface_fields_reject_a_bool_value_for_every_version(self def test_ipv4_make_rejects_a_bool_address_through_the_public_api(self) -> None: """The corruption reported in #491, reproduced through the public API. - Before this fix, ``IPv4.make(src=True, dst=False)`` packed and - decoded without any exception or warning, silently corrupting the - addresses to ``0.0.0.1``/``0.0.0.0``. It must now raise instead. + Without the fix, ``IPv4.make(src=True, dst=False)`` packs and + decodes without any exception or warning, silently corrupting the + addresses to ``0.0.0.1``/``0.0.0.0``. It must raise instead. """ from pcapkit.protocols.internet.ipv4 import IPv4 from pcapkit.utilities.exceptions import BaseError @@ -330,8 +330,8 @@ def test_parse_ip_address_version_check_survives_the_passthrough_branch(self) -> def test_parse_ip_address_malformed_value_raises_in_library_error(self) -> None: """A malformed value must not leak :mod:`ipaddress`'s bare :exc:`ValueError`. - The construction-path callers used to let it out verbatim -- e.g. - ``MH._make_opt_bid(address='nonsense')`` raised a plain + Without the fix, the construction-path callers let it out verbatim -- + e.g. ``MH._make_opt_bid(address='nonsense')`` raises a plain :exc:`ValueError`, which ``except BaseError`` cannot catch. """ from pcapkit.corekit.fields.ipaddress import parse_ip_address @@ -457,11 +457,11 @@ def test_both_bool_guards_stay_catchable_as_value_error_and_as_base_error(self) so neither documented handler can tell them apart. This pins that, since it is the whole basis for leaving the two alone. - It also pins the compatibility half of #508's two deliberate behaviour - changes: ``_make_opt_bid(address='nonsense')`` and the HIP locator's - wrong-family case used to raise a **bare** :exc:`ValueError`, so they were - catchable by ``except ValueError`` and not by ``except BaseError``. They are - now catchable by both -- a widening, not a break. + It also pins the compatibility half of #508 for + ``_make_opt_bid(address='nonsense')`` and the HIP locator's wrong-family + case: without the fix they raise a **bare** :exc:`ValueError`, catchable + by ``except ValueError`` and not by ``except BaseError``. They are + catchable by both -- a widening, not a break. """ from pcapkit.const.hip.parameter import Parameter from pcapkit.const.mh.mn_id_subtype import MNIDSubtype @@ -517,8 +517,8 @@ def test_switch_backed_address_makers_reject_a_bool(self) -> None: ``HIP``'s locator and ``TCP``'s Multipath ``ADD_ADDR`` address are both backed by a :class:`~pcapkit.corekit.fields.misc.SwitchField`, and both makers derive the wire form from the address family before the schema - exists. Measured before the fix: ``ip=True`` packed a locator of ``::1`` - with no error at all, and ``addr=True`` gave an IPv4 ``MPTCPAddAddress`` + exists. Without the fix, ``ip=True`` packs a locator of ``::1`` + with no error at all, and ``addr=True`` gives an IPv4 ``MPTCPAddAddress`` with ``address=IPv4Address('0.0.0.1')``. The ``mh`` sites are covered next to the rest of that module, in @@ -548,46 +548,36 @@ def test_switch_backed_address_makers_reject_a_bool(self) -> None: self.assertIsInstance(context.exception, BaseError) self.assertIn('must not be a bool', str(context.exception)) - # the same two sites still take every legitimate value they took before. + # the same two sites take every legitimate value. # - # Both literals moved with #679, and in a way worth stating precisely - # because it is not the direction a reader of #651 would guess: the - # records are still **32 octets**, and only the two ``Length`` octets - # changed, from ``0004`` to ``0018``. Measured with ``.pack()`` on both - # trees rather than reasoned about -- ``f0999858e`` emits - # ``00c10004...00000000`` and the fix emits ``00c10018...00000000``, 32 - # octets each. Note the argument order: the first argument is the - # produced octets and the second is the literal. + # The LOCATOR_SET record is **32 octets** with ``Length`` octets ``0018`` + # (#679): :rfc:`7401` Section 5.2.1's ``Length`` is "length of the + # Contents, in bytes", so one IPv6 locator's 24-octet contents declare 24, + # and the padding is ``rfc_total(24) - 4 - 24`` = 4. Note the argument + # order: the first argument is the produced octets and the second is the + # literal. # - # ``LOCATOR_SET`` was the one parameter #651 and #664 left on the old - # padding expression, because two defects in it cancelled. Its ``len`` - # was ``sum(Locator.len)`` -- 4-octet units, where :rfc:`7401` - # Section 5.2.1's ``Length`` is "length of the Contents, in bytes" -- so - # it declared ``4n`` for a ``24n``-octet contents; and its padding - # callback read the nested ``Locator.len`` instead of the parameter's, - # because ``ListField`` packs each ``Locator`` into the shared packet - # context and ``padding`` is evaluated after the list. That shadowed - # value is 4 for any IPv6 locator, so the old expression appended exactly - # four octets at every count: ``4 + 24n + 4 = 24n + 8``, which is also - # the RFC total for a byte-count ``Length`` of ``24n``. Hence 32 octets - # here before and after, by two wrongs then and by the arithmetic now. - # - # #679 fixed the pair together. ``Length`` is now the byte count 24, the - # padding is ``rfc_total(24) - 4 - 24`` = 4, and the total is the same 32 - # -- while the shapes the cancellation never covered stopped being - # wrong: an empty set went from 4 octets to the 8 the RFC requires, one - # SPI-bearing locator from 35 to 32, two from 63 to 64, and a mixed pair - # from 59 or 60 to 56. Those live in + # Without the fix, two defects cancel at this shape. ``len`` is + # ``sum(Locator.len)`` -- 4-octet units, declaring ``4n`` for a + # ``24n``-octet contents -- and the padding callback reads the nested + # ``Locator.len`` instead of the parameter's, because ``ListField`` packs + # each ``Locator`` into the shared packet context and ``padding`` is + # evaluated after the list. That shadowed value is 4 for any IPv6 locator, + # so without the fix the expression appends exactly four octets at every count: + # ``4 + 24n + 4 = 24n + 8``, which is also the RFC total for a byte-count + # ``Length`` of ``24n``. So this record is 32 octets either way, and only + # the ``Length`` octets (``0004`` without the fix) tell the two apart. The shapes + # the cancellation does not cover -- an empty set, SPI-bearing locators, a + # mixed pair -- live in # :mod:`tests.protocols.internet.test_hip_locator_set_length_unit`, which # sweeps shapes rather than counts for exactly that reason. # # They pin exact octets rather than a length or a prefix, deliberately. # Exact octets are the whole subject of #651, and pcapkit round-trips its - # own output whatever the padding rule says -- writer and reader shared the - # error -- so a comparison that tolerated trailing bytes would have gone on - # passing through the defect and through the fix alike. For the same reason - # these stay 32 octets: a shorter pin here would silently bless a - # four-octet shortfall. + # own output whatever the padding rule says -- writer and reader share the + # error -- so a comparison that tolerated trailing bytes would pass through + # the defect and through the fix alike. For the same reason these stay 32 + # octets: a shorter pin here would silently bless a four-octet shortfall. self.assertEqual( hip._make_param_locator_set( # type: ignore[arg-type] Parameter.LOCATOR_SET, version=2, diff --git a/tests/corekit/test_fields_misc.py b/tests/corekit/test_fields_misc.py index 2cbd94fa86..c2af744a52 100644 --- a/tests/corekit/test_fields_misc.py +++ b/tests/corekit/test_fields_misc.py @@ -221,10 +221,9 @@ def test_address_typed_switch_branches_already_reject_a_bool(self) -> None: Every such branch, across the protocol modules -- not just :mod:`~pcapkit.protocols.schema.internet.mh`'s -- because the claim being pinned is about ``SwitchField`` in general, so a fix scoped to one - module would not settle it. Measured on ``main`` at ``27bb315d5``, i.e. - *before* #508's fix: every probe (each branch, with ``True`` and with - ``False``) raised, and none was laundered. That is what makes a - guard in ``SwitchField`` dead code. + module would not settle it. Without #508's fix, every probe (each + branch, with ``True`` and with ``False``) raises, and none is + laundered. That is what makes a guard in ``SwitchField`` dead code. """ from pcapkit.corekit.fields.misc import SwitchField from pcapkit.utilities.exceptions import BaseError, FieldValueError diff --git a/tests/corekit/test_fields_misc_packet_context.py b/tests/corekit/test_fields_misc_packet_context.py index 1a5d87b70f..568f94c3ad 100644 --- a/tests/corekit/test_fields_misc_packet_context.py +++ b/tests/corekit/test_fields_misc_packet_context.py @@ -14,44 +14,40 @@ class NestedPacketContextSemanticsTests(unittest.TestCase): """The packet context a nested schema's field callbacks see -- issue #445. - ``SchemaField.pack``/``unpack`` used to hand a nested schema a nested - context whose *only* content was ``{'__packet__': packet}``: a name the - nested schema declares itself resolves normally, but a name it does not + Without the fix, ``SchemaField.pack``/``unpack`` hand a nested schema a + nested context whose *only* content is ``{'__packet__': packet}``: a name + the nested schema declares itself resolves normally, but a name it does not declare -- which every top-level schema writes as ``pkt['length']`` and - which a nested one inherited unmodified -- raised :exc:`KeyError` instead + which a nested one inherits unmodified -- raises :exc:`KeyError` instead of reaching the enclosing schema. ``pcapkit.corekit.fields.misc. - nested_packet_context`` replaces that literal with a plain :class:`dict` - holding a shallow copy of the enclosing schema's own names, so a name the - nested schema does not declare resolves to the enclosing schema's value, + nested_packet_context`` builds a plain :class:`dict` holding a shallow + copy of the enclosing schema's own names, so a name the nested schema + does not declare resolves to the enclosing schema's value, while ``__packet__`` keeps naming the enclosing mapping explicitly. :meth:`test_nested_schema_reads_enclosing_field_by_name_and_does_not_leak_writes` - is the load-bearing case: it fails with the recorded ``KeyError`` before - the fix and passes after, and in the same pass checks that the mapping + is the load-bearing case: it fails with the recorded ``KeyError`` without + the fix and passes with it, and in the same pass checks that the mapping does not confuse a name the nested schema shadows with the enclosing schema's own, and that nothing the nested schema writes through the mapping is ever written back to the enclosing schema's own data. - Intermediate versions of this fix returned a :class:`collections.ChainMap` - and then a hand-written :class:`dict` subclass, the latter adopted when the - ``ChainMap`` was suspected of corrupting a shared :class:`~abc.ABCMeta` - cache on CPython <= 3.10 (issue #439) -- a suspicion that is probably wrong - but is no longer decidable, since #439's direct fix removed the mechanism; - see :func:`pcapkit.corekit.fields.misc.nested_packet_context` for why it is - recorded there as two measurements that do not fully reconcile rather than - as a settled reversal. - - Both are retired. What the plain :class:`dict` settles that neither - predecessor did: the subclass's own ``.setdefault()`` bypassed the fallback + A :class:`collections.ChainMap` or a hand-written :class:`dict` subclass + would not do. The subclass's own ``.setdefault()`` bypasses the fallback exactly as :class:`dict`'s built-in one does, silently inserting a name - locally instead of honouring what the enclosing schema already had for it, - and its ``__eq__`` compared only its own storage; the ``ChainMap`` got both - right by delegation but needed a :func:`~typing.cast` at the call site, + locally instead of honouring what the enclosing schema already has for it, + and its ``__eq__`` compares only its own storage; the ``ChainMap`` gets both + right by delegation but needs a :func:`~typing.cast` at the call site, since it does not satisfy ``Schema.pack``'s ``dict[str, Any]`` annotation. - A copy needs neither a cast nor an override -- every operation is - :class:`dict`'s own. + (A ``ChainMap`` was also suspected of corrupting a shared + :class:`~abc.ABCMeta` cache on CPython <= 3.10, issue #439 -- a suspicion + that is probably wrong but not decidable, since #439's direct fix removed + the mechanism; see :func:`pcapkit.corekit.fields.misc.nested_packet_context` + for why it is recorded there as two measurements that do not fully + reconcile rather than as a settled reversal.) A copy needs neither a cast + nor an override -- every operation is :class:`dict`'s own. :meth:`test_nested_schema_reads_enclosing_field_by_name_and_does_not_leak_writes` - pins the behaviour that was the gap (``setdefault`` on a name absent + pins the behaviour that is the gap (``setdefault`` on a name absent locally but present in the parent returns the parent's value rather than inserting a new one), which is issue #474. @@ -179,11 +175,10 @@ def test_nested_schema_reads_enclosing_field_by_name_and_does_not_leak_writes(se def test_pcapng_byteorder_consumer_still_works_with_both_shapes(self) -> None: """The one existing, hand-rolled ``__packet__`` fallback is unaffected. - ``packet_byteorder`` (:mod:`pcapkit.corekit.fields.byteorder`) - is called both with a plain dict built by hand -- as several tests and + :func:`~pcapkit.corekit.fields.byteorder.packet_byteorder` is called + both with a plain dict built by hand -- as several tests and :func:`~pcapkit.foundation.engines.pcapng` construct -- and with what - ``SchemaField`` actually builds. Both must keep working, and - neither is expected to change: the site already implements the + ``SchemaField`` actually builds. Both must work: the site implements the fallback itself and does not route through :func:`~pcapkit.corekit.fields.misc.nested_packet_context`. @@ -195,7 +190,7 @@ def test_pcapng_byteorder_consumer_still_works_with_both_shapes(self) -> None: # Hand-built, the shape every existing caller uses. self.assertEqual(packet_byteorder({'__packet__': outer}), 'little') - # What SchemaField hands a real nested schema now. + # What SchemaField hands a real nested schema. self.assertEqual(packet_byteorder(nested_packet_context(outer)), 'little') # No enclosing packet at all -- the top-level case. self.assertEqual(packet_byteorder({}), sys.byteorder) diff --git a/tests/corekit/test_fields_misc_schema_length_1148_unit.py b/tests/corekit/test_fields_misc_schema_length_1148_unit.py index e6192898c6..7f919b0407 100644 --- a/tests/corekit/test_fields_misc_schema_length_1148_unit.py +++ b/tests/corekit/test_fields_misc_schema_length_1148_unit.py @@ -2,14 +2,14 @@ """A length-less ``SchemaField`` sizes its nested schema by what is left to read. GitHub issue #1148. ``SchemaField``'s default length is ``-1``, meaning "not -declared", and :meth:`SchemaField.unpack -` passed that ``-1`` straight on +declared", and without the fix :meth:`SchemaField.unpack +` passes that ``-1`` straight on as the nested schema's length. The nested schema's ``__length__`` therefore -started negative, and every sub-field it read warned ``packet length < 0`` -- +starts negative, and every sub-field it reads warns ``packet length < 0`` -- six warnings per HIP ``Locator`` and per MH ``CGAParameter``, on input that parses correctly. -The nested schema is now told the true remainder of the buffer. A remainder of +The nested schema is told the true remainder of the buffer. A remainder of zero is passed as a declared length, so a truncated item is still reported as malformed data rather than as the quiet end-of-stream signal that :func:`~pcapkit.corekit.decorators.prepare` raises for a derived zero. diff --git a/tests/corekit/test_fields_numbers_bit_length_1488_unit.py b/tests/corekit/test_fields_numbers_bit_length_1488_unit.py index b64d1d1a6f..ef6ac4ae81 100644 --- a/tests/corekit/test_fields_numbers_bit_length_1488_unit.py +++ b/tests/corekit/test_fields_numbers_bit_length_1488_unit.py @@ -1,12 +1,12 @@ # -*- coding: utf-8 -*- """A number field keeps the bits outside its ``bit_length`` as captured. -GitHub issue #1488: :meth:`NumberField.post_process -` masked the decoded +GitHub issue #1488 guards against :meth:`NumberField.post_process +` masking the decoded value to ``bit_length`` bits and :meth:`~pcapkit.corekit.fields.numbers.NumberField.pre_process` -masked it again, so ``UInt16Field(bit_length=12)`` read ``ffff`` and packed -``0fff``. The parsed value now records the bits it does not interpret, and a -value a caller supplies is still truncated to ``bit_length`` bits. +masking it again, so that ``UInt16Field(bit_length=12)`` reads ``ffff`` and packs +``0fff``. The parsed value records the bits it does not interpret, and a value a +caller supplies is truncated to ``bit_length`` bits. Every case builds its own octets in memory and reads no capture. Classes are imported inside each test, after :func:`~tests._support.reimport_once_per_class`. diff --git a/tests/corekit/test_fields_numbers_callable_length.py b/tests/corekit/test_fields_numbers_callable_length.py index 11358a16d4..3d75358d46 100644 --- a/tests/corekit/test_fields_numbers_callable_length.py +++ b/tests/corekit/test_fields_numbers_callable_length.py @@ -1,12 +1,12 @@ """A :class:`~pcapkit.corekit.fields.numbers.NumberField` whose ``length`` is a callable. -GitHub issue #591. ``_need_process`` used to be a *latch*: ``build_template`` -raised it :data:`True` in its fall-through branch and nothing ever put it back. -A callable ``length`` is a placeholder of ``-1`` until +GitHub issue #591. Without the fix ``_need_process`` is a *latch*: +``build_template`` raises it :data:`True` in its fall-through branch and nothing +ever puts it back. A callable ``length`` is a placeholder of ``-1`` until :meth:`~pcapkit.corekit.fields.numbers.NumberField.__call__` resolves it, ``-1`` -takes that fall-through branch, and the flag then outlived the very rebuild that -installed the real width. ``pre_process`` consequently handed :obj:`bytes` to a -template that had become ``>Q``, and :func:`struct.pack` refused it with +takes that fall-through branch, and the flag then outlives the very rebuild that +installs the real width. ``pre_process`` consequently hands :obj:`bytes` to a +template that has become ``>Q``, and :func:`struct.pack` refuses it with ``required argument is not an integer``. Every test here is written as a **comparison against the same width supplied @@ -80,8 +80,8 @@ def test_a_callable_length_packs_exactly_as_the_same_static_length_does(self) -> Identical resolved length, identical template; the *only* difference is whether ``length`` arrived as an :obj:`int` or as a callable returning - that same :obj:`int`. Before the fix the static column packed and the - callable column raised, for all four native widths. + that same :obj:`int`. Without the fix the static column packs and the + callable column raises, for every native width. """ from pcapkit.corekit.fields.numbers import NumberField @@ -128,7 +128,7 @@ def test_a_callable_resolving_to_a_byte_packed_width_still_needs_processing(self ``build_template`` takes the same fall-through branch for any width outside ``{1, 2, 4, 8}`` as it does for the ``-1`` placeholder. So clearing the flag whenever a callable is resolved -- rather than - recomputing it from the width now in force -- would have broken every + recomputing it from the width now in force -- would break every one of these, which genuinely do need :meth:`int.to_bytes`. """ @@ -172,8 +172,8 @@ def test_the_placeholder_itself_still_needs_processing(self) -> None: While ``length`` is still the ``-1`` placeholder there is no width to pack to, so :data:`True` is the correct answer for it -- and it is the - answer the unresolved field keeps. What changed is that the answer is - no longer *sticky*. + answer the unresolved field keeps. What matters is that the answer is + not *sticky*. """ from pcapkit.corekit.fields.numbers import NumberField diff --git a/tests/corekit/test_fields_numbers_negative_length.py b/tests/corekit/test_fields_numbers_negative_length.py index e4f0774733..69654b151c 100644 --- a/tests/corekit/test_fields_numbers_negative_length.py +++ b/tests/corekit/test_fields_numbers_negative_length.py @@ -66,10 +66,10 @@ def test_a_literal_negative_length_raises_protocolerror(self) -> None: def test_protocolerror_is_a_baseerror_unlike_the_stock_valueerror(self) -> None: """The defect in one assertion: catchable as a pcapkit error. - On stock ``21e9588af`` this raises a bare :class:`builtins.ValueError` + Without the fix this raises a bare :class:`builtins.ValueError` (``negative shift count``), which is *not* an instance of :class:`~pcapkit.utilities.exceptions.BaseError` and so cannot be - caught as one -- measured directly below, since that is the whole + caught as one -- asserted directly below, since that is the whole point of the regression. """ @@ -179,23 +179,22 @@ class NegativeBitLengthTests(unittest.TestCase): ``NumberField.__init__`` caches ``self._bit_mask`` from ``bit_length`` whenever one is supplied, and shifts by it immediately: - ``(1 << bit_length) - 1``. #829 guarded the *resolved-length* shift in - :meth:`~pcapkit.corekit.fields.numbers.NumberField.__call__` but left this - one, in ``NumberField.__init__`` on ``5576708d4``, open -- - a negative ``bit_length`` still raised a bare, uncatchable - :exc:`ValueError` (``negative shift count``) rather than a + ``(1 << bit_length) - 1``. #829 guards the *resolved-length* shift in + :meth:`~pcapkit.corekit.fields.numbers.NumberField.__call__`; without the + #831 fix this one is open -- a negative ``bit_length`` raises a bare, + uncatchable :exc:`ValueError` (``negative shift count``) rather than a :class:`~pcapkit.utilities.exceptions.ProtocolError`. - The last two cases below are the second half of #831: with ``bit_length`` - supplied, #829's guard in :meth:`__call__` sits entirely inside the - ``bit_length``-not-supplied branch and so never runs, and a field combining - a fixed ``bit_length`` with a callable ``length`` that resolves negative - used to fall through to ``template='...-1s'`` and a *different* + The last two cases below are the second half of #831: without the fix, with + ``bit_length`` supplied, #829's guard in :meth:`__call__` sits entirely + inside the ``bit_length``-not-supplied branch and so never runs, and a field + combining a fixed ``bit_length`` with a callable ``length`` that resolves + negative falls through to ``template='...-1s'`` and a *different* :exc:`ProtocolError` message from :attr:`~pcapkit.corekit.fields.field.FieldBase.length` -- the same wire condition, two different messages, depending on an unrelated constructor argument. The fix validates the resolved ``length`` unconditionally in - ``__call__``, so both shapes now raise the identical message. + ``__call__``, so both shapes raise the identical message. """ def setUp(self) -> None: @@ -222,9 +221,9 @@ def test_a_negative_bit_length_raises_protocolerror(self) -> None: def test_negative_bit_length_protocolerror_is_a_baseerror(self) -> None: """The defect in one assertion: catchable as a pcapkit error. - On stock ``5576708d4`` this raises a bare :class:`builtins.ValueError` + Without the fix this raises a bare :class:`builtins.ValueError` (``negative shift count``), which is *not* an instance of - :class:`~pcapkit.utilities.exceptions.BaseError` -- measured directly + :class:`~pcapkit.utilities.exceptions.BaseError` -- asserted directly below, mirroring :meth:`NegativeResolvedLengthTests. test_protocolerror_is_a_baseerror_unlike_the_stock_valueerror` @@ -279,9 +278,9 @@ def test_a_positive_bit_length_is_unaffected(self) -> None: def test_bit_length_zero_still_works_exactly_as_it_does_on_main(self) -> None: """The boundary the fix must not break: ``bit_length=0`` is not negative. - Checked against stock behaviour rather than assumed: on ``5576708d4``, - prior to any change here, ``bit_length=0`` already constructs and - calls cleanly with a mask of ``0``, and that has to keep being true. + ``bit_length=0`` constructs and calls cleanly with a mask of ``0``, + with or without the fix, and the guard against a negative + ``bit_length`` must not catch it. """ from pcapkit.corekit.fields.numbers import NumberField @@ -300,7 +299,7 @@ def test_bit_length_supplied_plus_negative_callable_length_matches_the_unsupplie No in-tree call site combines a fixed ``bit_length`` with a callable ``length`` -- ``vlan.py``'s three ``bit_length`` fields all take positive literal ``length`` -- so this is a constructed reproduction, - not a real wire shape. Before the fix, #829's guard in ``__call__`` + not a real wire shape. Without the fix, #829's guard in ``__call__`` sits inside ``if new_self._bit_length < 0:`` and this combination never enters that branch, so no ``ProtocolError`` fires there at all; the field instead degrades to ``template='>-1s'`` and the error is diff --git a/tests/corekit/test_fields_numbers_port_enum_field_peek_1774_unit.py b/tests/corekit/test_fields_numbers_port_enum_field_peek_1774_unit.py index 1d70ff5bdf..c68adf175c 100644 --- a/tests/corekit/test_fields_numbers_port_enum_field_peek_1774_unit.py +++ b/tests/corekit/test_fields_numbers_port_enum_field_peek_1774_unit.py @@ -1,12 +1,12 @@ # -*- coding: utf-8 -*- -"""Why :meth:`~pcapkit.corekit.fields.numbers.PortEnumField.post_process` still peeks. +"""Why :meth:`~pcapkit.corekit.fields.numbers.PortEnumField.post_process` peeks. GitHub issue #1774. The field looks a port up in the registry rows and in ``AppType._missing_``'s spans before calling ``AppType.get``, so that a genuine miss builds its member through ``EnumField._unregistered_member``. That peek was -written to stop ``get`` minting a member per ephemeral port. Since #860 ``get`` -no longer mints, so the growth it guarded against is gone; but the member ``get`` -builds on a miss is still not the one a parse needs. These tests pin both +written to stop ``get`` minting a member per ephemeral port. ``get`` does not +mint (#860), so the growth it guarded against cannot happen; but the member +``get`` builds on a miss is not the one a parse needs. These tests pin both halves, so that the peek's documented reason fails here the day it stops being true, and pin that a composite ``proto`` resolves a miss as ``get`` does. @@ -53,7 +53,7 @@ def _tables(owner: 'Any') -> 'tuple[Any, ...]': return (dict(owner.__members__), by_value, rows) def test_neither_the_field_nor_get_grows_the_registry(self) -> None: - """The peek's original reason is gone: ``get`` itself no longer mints.""" + """The peek's original reason does not apply: ``get`` itself does not mint.""" from pcapkit.const.reg.apptype import AppType for proto in self._protos(): @@ -108,9 +108,9 @@ def test_a_composite_proto_resolves_a_miss_as_get_does(self) -> None: """``tcp | udp`` is a bare :class:`int` (``3``, which is SCTP's value). ``get`` dispatches it to SCTP for a hit and a miss alike. The field's - own miss branch used to read ``.name`` off it, raising - :exc:`AttributeError` for a port ``get`` resolves; it now takes the - transport from the registry, as ``get`` does. + own miss branch takes the transport from the registry, as ``get`` does; + reading ``.name`` off the bare :class:`int` instead raises + :exc:`AttributeError` for a port ``get`` resolves. """ from pcapkit.const.reg.apptype import AppType, TransportProtocol diff --git a/tests/corekit/test_fields_numbers_port_option_no_mint_unit.py b/tests/corekit/test_fields_numbers_port_option_no_mint_unit.py index 1ed8cf164f..cc974c2458 100644 --- a/tests/corekit/test_fields_numbers_port_option_no_mint_unit.py +++ b/tests/corekit/test_fields_numbers_port_option_no_mint_unit.py @@ -1,21 +1,21 @@ # -*- coding: utf-8 -*- -"""A registry-backed ``EnumField`` subclass minted a member per unassigned value. +"""A registry-backed ``EnumField`` subclass must not mint a member per unassigned value. -GitHub issue #575. :meth:`PortEnumField.post_process +GitHub issue #575. Without the fix, :meth:`PortEnumField.post_process ` (TCP, UDP, SCTP) and :meth:`OptionEnumField.post_process ` (PCAP-NG) -each resolved a wire value by calling their registry's ``.get()`` directly:: +each resolve a wire value by calling their registry's ``.get()`` directly:: return self._namespace.get(value, proto=...) -which minted a fresh member -- via :func:`aenum.extend_enum` -- for any value +which mints a fresh member -- via :func:`aenum.extend_enum` -- for any value neither an existing row nor one of the registry's own documented spans accounts for. On ``examples/captures/http.pcap`` that is 111 calls, all from ephemeral TCP ports with no IANA assignment, and measured at 13-17% of extraction self time. -Each of them now falls through to +Each of them falls through to :meth:`EnumField._unregistered_member ` instead of minting: an actual instance of the registry the field names -- ``isinstance`` holds, and it renders and dispatches exactly like a declared @@ -109,11 +109,11 @@ def test_an_ephemeral_port_resolves_without_growing_the_registry(self) -> None: 54321 carries no IANA assignment on any of the three transports and sits in none of ``AppType._missing_``'s documented spans, so this is - exactly the value that used to mint ``PORT_54321_``. The + exactly the value that, without the fix, mints ``PORT_54321_``. The resolved member still carries ``.port``, ``.svc`` and ``.proto`` -- read unconditionally by e.g. ``Transport._decode_next_layer``'s ``srcport.port`` -- because losing them would trade one crash for - another. Per the owner's ruling, it is now a real member of the + another. Per the owner's ruling, it is a real member of the registry -- ``isinstance`` holds against both the per-transport registry and ``AppType`` itself -- rather than some foreign stand-in, and two such members for the same value are distinct objects, which @@ -179,8 +179,8 @@ def test_a_value_lookup_on_the_resolved_port_still_raises(self) -> None: A direct call to ``.get()`` is different on purpose and stays that way: the owner's ruling and the docstring above are both explicit that ``get()`` is the public API and answering a caller who asked for - this exact value directly is defensible -- since #874 with an - unregistered ``unknown`` member rather than a mint -- which is also + this exact value directly is defensible -- with an unregistered + ``unknown`` member rather than a mint (#874) -- which is also what ``tests/const/test_const_apptype_split_unit.py`` pins for ports 59001 and 51. So this only asserts the value-lookup side, never the ``.get()`` side. @@ -220,7 +220,7 @@ def test_a_documented_span_still_mints_its_own_bounded_member(self) -> None: ``_missing_`` answers regardless of transport, so it has always resolved to a real, named member -- the mechanism the issue explicitly leaves alone, as distinct from ``get()``'s unbounded - fall-through this fix removes. + fall-through the fix removes. """ from pcapkit.const.reg.apptype import AppType diff --git a/tests/corekit/test_fields_numbers_unassigned_enum.py b/tests/corekit/test_fields_numbers_unassigned_enum.py index d83b09d033..221591b403 100644 --- a/tests/corekit/test_fields_numbers_unassigned_enum.py +++ b/tests/corekit/test_fields_numbers_unassigned_enum.py @@ -127,11 +127,11 @@ def setUp(self) -> None: def test_an_unassigned_registry_value_resolves_instead_of_raising(self) -> None: """The reported lookup, as a field unpack. - Measured on the unfixed tree, this raised ``ValueError: 28 is not a valid - BlockType`` straight out of :mod:`aenum`. It now resolves, and the value + Without the fix, this raises ``ValueError: 28 is not a valid + BlockType`` straight out of :mod:`aenum`. It resolves, and the value it resolves to carries the octets it came from -- which is the whole point, since a pseudo-member that lost the code would be no more useful - to the dispatch than the exception was. + to the dispatch than the exception. """ from pcapkit.const.pcapng.block_type import BlockType @@ -186,14 +186,14 @@ def test_a_missing_rule_still_takes_precedence_over_the_fallback(self) -> None: ` covers. Per the mint/unmint ruling recorded for #775 (``docs/source/contributing/conventions/mint-criterion.rst``), - ``Reserved`` names a procedure rather than a party, so this range no - longer *mints* a registered ``Reserved_0bad0bad`` member -- it now + ``Reserved`` names a procedure rather than a party, so this range does + not *mint* a registered member such as ``Reserved_0bad0bad`` -- it returns an unregistered member via ``_unregistered_member``, bearing the - bare label ``Reserved`` and the original value. The fallback must still - not shadow ``_missing_``: if it did, a reserved code would come back + bare label ``Reserved`` and the original value. The fallback must not + shadow ``_missing_``: if it did, a reserved code would come back nameless and lose the only thing the registry knows about it. Because - the name alone (``Reserved``) no longer pins the value the way - ``Reserved_0bad0bad`` used to, the value is asserted explicitly too. + the name alone (``Reserved``) does not pin the value, the value is + asserted explicitly too. """ from pcapkit.const.pcapng.block_type import BlockType @@ -284,7 +284,7 @@ def test_a_field_with_no_registry_at_all_is_unchanged(self) -> None: A namespace-less :class:`~pcapkit.corekit.fields.numbers.EnumField` has always produced this nameless member; the fix reuses the same mechanism rather than inventing a second shape, so an unassigned code and a - registry-less field now answer alike. + registry-less field answer alike. """ from pcapkit.corekit.fields.numbers import EnumField @@ -331,7 +331,7 @@ def test_no_value_of_any_width_escapes_the_field_layer(self) -> None: a one- and two-octet field can carry, across four registries of different shapes, and asserts that nothing leaves the field layer except a member. - Measured on the unfixed tree this raised for the overwhelming majority of + Without the fix this raises for the overwhelming majority of values in each registry -- the registries declare a few hundred codes between them and the two-octet ones have 65,536 to answer for. @@ -398,9 +398,9 @@ def _protochains(self, path: str) -> 'list[str]': def test_an_unassigned_block_type_no_longer_costs_the_extraction(self) -> None: """The issue's repro, with a packet block behind the unknown one. - Measured on the unfixed tree, ``Extractor`` raised ``ValueError: 28 is - not a valid BlockType`` and produced nothing at all -- so the Enhanced - Packet Block behind the unknown block was lost along with it, which is + Without the fix, ``Extractor`` raises ``ValueError: 28 is + not a valid BlockType`` and produces nothing at all -- so the Enhanced + Packet Block behind the unknown block is lost along with it, which is what "one unknown block costs the whole extraction" means. The control is the same capture without the unknown block, and the diff --git a/tests/corekit/test_fields_numbers_width_repair.py b/tests/corekit/test_fields_numbers_width_repair.py index 0982717132..3a2cdd956f 100644 --- a/tests/corekit/test_fields_numbers_width_repair.py +++ b/tests/corekit/test_fields_numbers_width_repair.py @@ -1,13 +1,13 @@ -"""``pre_process``'s width repair sized values with floor division. +"""``pre_process``'s width repair must not size values with floor division. GitHub issue #599. When a :class:`~pcapkit.corekit.fields.numbers.NumberField` is packed while ``length`` is still the ``-1`` placeholder, ``pre_process`` -derives a width from the value itself. It derived it with:: +derives a width from the value itself. Without the fix it derives it with:: self._length = math.ceil(value.bit_length() // 8) :func:`math.ceil` on an :obj:`int` is a no-op -- ``//`` has already floored it -- -so the expression was plain floor division and the width came out one octet short +so the expression is plain floor division and the width comes out one octet short for every value whose bit length is *not* an exact multiple of eight. The intended expression is ``math.ceil(value.bit_length() / 8)``. @@ -16,20 +16,20 @@ **The boundaries are tested one method each, not one parametrised sweep.** The defect is a pattern rather than an instance -- it fails at *every* octet boundary -- so a single case would pass against a fix that special-cased the reported -width. Standalone methods also keep each failure individually visible: the -pytest in use has no ``pytest-subtests``, so a failing :meth:`~unittest.TestCase -.subTest` still leaves its parent reported as passed. +width. Standalone methods also keep each failure attached to its own test: +pytest reports and counts a failing :meth:`~unittest.TestCase.subTest` as a +``SUBFAILED`` entry, but counts the parent method itself as passed. -**Each boundary is asserted as a pair.** The value *below* the boundary already -packed correctly before the fix and must go on doing so; only the value *above* -it was broken. Asserting the pair is what distinguishes a fix from a width that +**Each boundary is asserted as a pair.** The value *below* the boundary packs +correctly with or without the fix and must go on doing so; only the value *above* +it breaks. Asserting the pair is what distinguishes a fix from a width that has merely been shifted by one in the other direction. -The failure was not a single exception type, and this suite deliberately does not -assert one. Since #591's fix, ``_need_process`` is recomputed from the width in -force, so a mis-sized width that lands on 1, 2 or 4 octets now comes back from +The failure is not a single exception type, and this suite deliberately does not +assert one. ``_need_process`` is recomputed from the width in force (#591), so +without the fix a mis-sized width that lands on 1, 2 or 4 octets comes back from :func:`struct.pack` as ``struct.error: 'B' format requires 0 <= number <= 255`` -while a mis-sized 3 octets still comes back from :meth:`int.to_bytes` as +while a mis-sized 3 octets comes back from :meth:`int.to_bytes` as ``OverflowError: int too big to convert``. What these tests pin instead is the width and the octets -- the thing that is actually wrong. """ @@ -50,8 +50,8 @@ HAS_RUNTIME = all(importlib.util.find_spec(name) is not None for name in RUNTIME_DEPS) #: The reported table, as ``(value, octets the value needs)``. The first of each -#: pair sits on an octet boundary and packed correctly before the fix; the second -#: is one past it and did not. +#: pair sits on an octet boundary and packs correctly with or without the fix; the +#: second is one past it and packs correctly only with it. BOUNDARY_PAIRS = ( (255, 1, 256, 2), (65535, 2, 65536, 3), @@ -161,11 +161,11 @@ def test_the_smallest_mis_sized_value_is_one_not_two_hundred_and_fifty_six(self) """The issue understates the reach, and this is where it shows. #599 frames the defect as hitting values "just past an octet boundary", - which reads as though small values were safe. They were not: floor + which reads as though small values were safe. They are not: floor division is wrong for *every* bit length that is not a multiple of eight, - so ``1`` -- bit length 1, floored to **zero** octets -- was mis-sized - too, and every value from 1 to 127 with it. Measured on the unfixed tree, - ``pack(1, {})`` raised ``OverflowError: int too big to convert`` against a + so ``1`` -- bit length 1, floored to **zero** octets -- is mis-sized + too, and every value from 1 to 127 with it. Without the fix, + ``pack(1, {})`` raises ``OverflowError: int too big to convert`` against a zero-octet width. """ @@ -234,7 +234,7 @@ def test_a_repaired_field_can_read_back_what_it_wrote(self) -> None: def test_math_ceil_of_a_floor_division_is_the_floor_division(self) -> None: """The root cause, pinned directly rather than only described. - The old expression *looked* like a ceiling and was not one. Stating that + Without the fix, the expression *looks* like a ceiling and is not one. Stating that as an assertion keeps the next reader of ``pre_process`` from reintroducing it: the two expressions differ for seven of every eight bit lengths, and coincide only on the eighth. @@ -252,7 +252,7 @@ def test_math_ceil_of_a_floor_division_is_the_floor_division(self) -> None: @unittest.skipUnless(HAS_RUNTIME, 'runtime dependencies not installed') class WidthRepairReachabilityTests(unittest.TestCase): - """How a caller gets to the repair at all, which bounds what this fix touches.""" + """How a caller gets to the repair at all, which bounds what the #599 fix touches.""" def setUp(self) -> None: reimport_once_per_class(self) diff --git a/tests/corekit/test_graalpy_semantics_1963_unit.py b/tests/corekit/test_graalpy_semantics_1963_unit.py index 37765ce62b..6026d37d57 100644 --- a/tests/corekit/test_graalpy_semantics_1963_unit.py +++ b/tests/corekit/test_graalpy_semantics_1963_unit.py @@ -6,12 +6,13 @@ CPython's: * :class:`io.BytesIO` ``read``, ``read1`` and ``readline`` from the start of an - exported buffer can hand back :obj:`bytes` sharing its memory, so - :class:`~pcapkit.corekit.io.SeekableReader` returned octets that its next write - to the buffer then changed. + exported buffer can hand back :obj:`bytes` sharing its memory, so without the + fix :class:`~pcapkit.corekit.io.SeekableReader` returns octets that its next + write to the buffer then changes. * ``dict.__init__`` and ``dict.update`` given ``(key, value)`` pairs store them - through a subclass's ``__setitem__``, so ``MultiDict(MultiDict)``, and a - ``MultiDict`` restored from pairs of its state, wrapped every value list in another. + through a subclass's ``__setitem__``, so without the fix ``MultiDict(MultiDict)``, + and a ``MultiDict`` restored from pairs of its state, wrap every value list in + another. On GraalPy itself, ``test_end_of_stream_reads_ahead_only_as_far_as_asked`` in ``test_io_seekable_reader_short_reads_1586_unit.py`` and the ``MultiDict`` copies diff --git a/tests/corekit/test_info_child_finalisation_1490_unit.py b/tests/corekit/test_info_child_finalisation_1490_unit.py index 6ae1574d1b..8bc9572a83 100644 --- a/tests/corekit/test_info_child_finalisation_1490_unit.py +++ b/tests/corekit/test_info_child_finalisation_1490_unit.py @@ -2,11 +2,11 @@ """GitHub issue #1490: each non-final :class:`~pcapkit.corekit.infoclass.Info` subclass is finalised on its own state, whatever was built before it. -:meth:`Info.__new__ ` used to read -``__finalised__`` through inheritance, so once a non-final parent had been built -its non-final children read the parent's ``BASE`` and were never finalised. They -kept the ``__excluded__`` copied at declaration, and ``to_dict()``, ``len()`` and -iteration then included ``__map__``, ``__map_reverse__`` and ``__multi__``. +Without the fix, :meth:`Info.__new__ ` +reads ``__finalised__`` through inheritance, so once a non-final parent has been +built its non-final children read the parent's ``BASE`` and are never finalised. +They keep the ``__excluded__`` copied at declaration, and ``to_dict()``, ``len()`` +and iteration then include ``__map__``, ``__map_reverse__`` and ``__multi__``. The defect depends on process-wide state -- which class was built first -- so every order runs in a fresh interpreter, started from the repository root so it diff --git a/tests/corekit/test_io.py b/tests/corekit/test_io.py index 01af4a150b..063fd4d2b7 100644 --- a/tests/corekit/test_io.py +++ b/tests/corekit/test_io.py @@ -164,18 +164,18 @@ def readinto(self, buffer) -> int: os.unlink(path) def test_writable_is_the_method_the_io_protocol_consults(self) -> None: - """Issue #645: the override was spelled ``writeable``, so it overrode nothing. + """Issue #645: an override spelled ``writeable`` overrides nothing. The :mod:`io` API spells it ``writable``. With the misspelling in place, - ``'writable' in SeekableReader.__dict__`` was :data:`False` and - ``SeekableReader.writable is io.IOBase.writable`` was :data:`True` -- so :mod:`io`, - :mod:`shutil` and any third-party caller read the inherited value and never saw the one - defined in this file. Both returned :data:`False`, which is the coincidence that hid it: - there was no symptom to notice, and editing the misspelled method would silently have - had no effect. + ``'writable' in SeekableReader.__dict__`` is :data:`False` and + ``SeekableReader.writable is io.IOBase.writable`` is :data:`True` -- so :mod:`io`, + :mod:`shutil` and any third-party caller read the inherited value and never see the one + defined in this file. Both return :data:`False`, which is the coincidence that hides it: + there is no symptom to notice, and editing the misspelled method would silently have + no effect. The identity assertion is the load-bearing one. ``writable()`` returning :data:`False` - passed before the fix too, by inheritance, so a value-only test cannot tell the two + passes without the fix too, by inheritance, so a value-only test cannot tell the two trees apart. """ @@ -262,8 +262,8 @@ def test_seek_variants_warnings_and_truncate_sizes(self) -> None: reader.read(2) self.assertEqual(reader.seek(-1, io.SEEK_CUR), 1) # NOTE: the end is a buffer's worth past the position, 1 + 4, so one before it is - # 4. It was the buffer's far edge, 0 + 4, until #1586 -- which a full buffer's - # edge reaches no further than the stream has been read. + # 4. Without the #1586 fix it is the buffer's far edge, 0 + 4 -- which a full + # buffer's edge reaches no further than the stream has been read. self.assertEqual(reader.seek(-1, io.SEEK_END), 4) with mock.patch.object(self.io_module, 'warn') as warn: self.assertEqual(reader.seek(20), 6) @@ -389,7 +389,7 @@ def test_truncate_buffer_keeps_the_content_and_not_the_padding(self) -> None: self.assertEqual(reader.seek(0), 0) # NOTE: the stream holds five octets, so five is the whole of what an eight octet - # request can be answered with; it returned b'\x00\x00\x00e' before the fix -- four + # request can be answered with; without the fix it returns b'\x00\x00\x00e' -- four # octets, the content displaced by the padding, and one octet of it lost outright. self.assertEqual(reader.read(8), b'abcde') self._close_reader(reader) @@ -497,8 +497,8 @@ def test_truncate_buffer_without_a_size_resizes_to_the_current_position(self) -> def test_truncate_buffer_below_the_content_leaves_the_reader_usable(self) -> None: """A truncation has to bring ``_buffer_cur`` down with the buffer it indexes. - Left above the new size it addressed octets the buffer no longer has, and the next - read raised ``ValueError: memoryview assignment: lvalue and rvalue have different + Left above the new size it addresses octets the buffer no longer has, and the next + read raises ``ValueError: memoryview assignment: lvalue and rvalue have different structures`` from :meth:`_write_buffer` rather than returning anything. """ @@ -541,19 +541,19 @@ def test_buffered_read_returns_every_buffered_octet(self) -> None: The existing round trip at :meth:`test_buffered_readline_read_read1_and_peek_paths` cannot see this: it seeks to 1, where ``_buffer_cur - 1`` happens to equal the octets actually available, so the two candidate caps agree. Seeking to 0 separates - them -- four octets are available and the old cap allowed three, making up the - fourth from the stream *past* the octet it had skipped. + them -- four octets are available and, without the fix, the cap allows three, + making up the fourth from the stream *past* the octet it skipped. """ reader = self.SeekableReader(io.BytesIO(b'abcde'), buffer_size=8) self.assertEqual(reader.read(4), b'abcd') self.assertEqual(reader.seek(0), 0) - self.assertEqual(reader.read(5), b'abcde') # was b'abce': four octets, d dropped + self.assertEqual(reader.read(5), b'abcde') # without the fix: b'abce', d dropped self.assertEqual(reader.tell(), 5) self._close_reader(reader) # NOTE: an unbounded read must not hand back the padding behind the content - # either; this returned b'abcd\x00\x00\x00\x00e' before the fix. + # either; without the fix this returns b'abcd\x00\x00\x00\x00e'. reader = self.SeekableReader(io.BytesIO(b'abcde'), buffer_size=8) self.assertEqual(reader.read(4), b'abcd') self.assertEqual(reader.seek(0), 0) @@ -679,15 +679,15 @@ def test_seek_refuses_a_negative_absolute_position_under_every_whence(self) -> N self._close_reader(reader) def test_seek_refuses_a_negative_position_with_a_saved_buffer_too(self) -> None: - """With ``buffer_save=True`` the refusal was skipped and the seek reported success. + """With ``buffer_save=True`` the refusal must not be skipped, nor the seek report success. The window refusal is conditional on there being no buffer file, since a saved buffer really can supply octets from before the window. A negative absolute position - is not that case, and with a file the old code fell straight through and *returned* - ``-5`` as though the seek had worked. The failure then surfaced on an unrelated call - as a bare ``OSError: [Errno 22] Invalid argument`` out of the middle of - :meth:`read`, from a line only reachable because :meth:`seek` had accepted something - it should not have. + is not that case, and with a file, without the fix, the code falls straight + through and *returns* ``-5`` as though the seek had worked. The failure then surfaces on an + unrelated call as a bare ``OSError: [Errno 22] Invalid argument`` out of the middle + of :meth:`read`, from a line only reachable because :meth:`seek` has accepted + something it should not have. """ with tempfile.NamedTemporaryFile(delete=False) as temp: @@ -771,7 +771,7 @@ def test_buffered_reads_return_every_octet_available_from_the_position(self) -> ``_buffer_cur`` counts from the window's base, not from the position being read from, so it offers octets that lie *behind* the position; and the ``- 1`` is short of even that. What is genuinely available is - ``_buffer_set + _buffer_cur - _tell``, which is what each path now asks for. + ``_buffer_set + _buffer_cur - _tell``, which is what each path asks for. The two halves fail differently, so each gets its own case: @@ -784,8 +784,8 @@ def test_buffered_reads_return_every_octet_available_from_the_position(self) -> ``size_rem`` zero and skips the top-up entirely -- so the real octets are never fetched at all. - ``read`` already carried the corrected count before this change; ``readline``, - ``read1`` and ``peek`` are the three sites that did not. All four are asserted, the + ``read`` carries the corrected count even without the fix; ``readline``, + ``read1`` and ``peek`` are the sites that do not. All four are asserted, the first as a regression guard. """ @@ -824,12 +824,12 @@ def test_buffered_reads_return_every_octet_available_from_the_position(self) -> def test_an_unbounded_buffered_readline_still_finishes_the_line(self) -> None: """Capping ``readline`` at the buffer must not truncate a line that runs past it. - With no size the old cap was ``min(-1, _buffer_cur - 1) == -1``, i.e. uncapped, and - :meth:`io.BytesIO.readline` then ran off the content into the NUL padding behind it. - Capping at the content is right, but on its own it would leave an unbounded - ``readline`` stopping at the window's edge rather than at the newline, so the - continuation from the stream now runs for a negative size as well -- the same shape - :meth:`read` already had. + With no size, without the fix, the cap is ``min(-1, _buffer_cur - 1) == -1``, i.e. + uncapped, and :meth:`io.BytesIO.readline` then runs off the content into the NUL + padding behind it. Capping at the content is right, but on its own it would leave an + unbounded ``readline`` stopping at the window's edge rather than at the newline, so + the continuation from the stream runs for a negative size as well -- the same shape + :meth:`read` has. """ reader = self.SeekableReader(io.BytesIO(b'abcdefghij\nklm'), buffer_size=8) diff --git a/tests/corekit/test_io_peekable_stream_copy_1524_unit.py b/tests/corekit/test_io_peekable_stream_copy_1524_unit.py index 5a77d86d1b..cde4a9c2f6 100644 --- a/tests/corekit/test_io_peekable_stream_copy_1524_unit.py +++ b/tests/corekit/test_io_peekable_stream_copy_1524_unit.py @@ -2,10 +2,10 @@ """Copying or pickling a :class:`~pcapkit.corekit.io.PeekableStream` follows its stream (#1524). :mod:`copy` and :mod:`pickle` build the proxy without calling ``__init__``, then -ask it for ``__setstate__``. ``__getattr__`` forwarded that to ``self._stream``, -which was not set yet, so the lookup of ``_stream`` came back through -``__getattr__`` until :exc:`RecursionError`. It now refuses ``_stream`` and every -dunder, and the default protocols do what they do for any wrapper: a shallow +ask it for ``__setstate__``. Without the fix, ``__getattr__`` forwards that to +``self._stream``, which is not set yet, so the lookup of ``_stream`` comes back +through ``__getattr__`` until :exc:`RecursionError`. ``__getattr__`` refuses +``_stream`` and every dunder, and the default protocols do what they do for any wrapper: a shallow copy shares the stream, while a deep copy or a pickle copies it, and so works exactly when the stream's own does. diff --git a/tests/corekit/test_io_seekable_reader_short_reads_1586_unit.py b/tests/corekit/test_io_seekable_reader_short_reads_1586_unit.py index 4b87bc6e29..eb9db6c023 100644 --- a/tests/corekit/test_io_seekable_reader_short_reads_1586_unit.py +++ b/tests/corekit/test_io_seekable_reader_short_reads_1586_unit.py @@ -175,11 +175,11 @@ def test_reading_ahead_a_bogus_length_stops_at_the_real_end(self) -> None: self.assertEqual(reader.read(), DATA[:50]) def test_close_after_the_stream_was_closed(self) -> None: - """A stream its owner closed first made :meth:`close` raise, from a finaliser too. + """A stream its owner closed first must not make :meth:`close` raise, finaliser or not. - Measured on a dpkt ``NeedData``: the extractor left with the reader, which was - collected after the caller closed its stream, and printed ``Exception ignored - ... ValueError: I/O operation on closed file``. + Without the fix, on a dpkt ``NeedData`` the extractor leaves with the reader, + which is collected after the caller closes its stream, and prints ``Exception + ignored ... ValueError: I/O operation on closed file``. """ from pcapkit.corekit.io import SeekableReader # pylint: disable=import-outside-toplevel diff --git a/tests/corekit/test_listfield_untyped_pack_1489_unit.py b/tests/corekit/test_listfield_untyped_pack_1489_unit.py index ebc1a96926..8691aa1001 100644 --- a/tests/corekit/test_listfield_untyped_pack_1489_unit.py +++ b/tests/corekit/test_listfield_untyped_pack_1489_unit.py @@ -3,10 +3,10 @@ GitHub issue #1489. Without an ``item_type``, :meth:`ListField.unpack ` returns the field's -octets as one :obj:`bytes`, but :meth:`ListField.pack -` iterated that value item -by item and rejected each :obj:`int` with ``FieldValueError``. ``pack`` now -writes such a value as-is; a typed ``ListField`` is unchanged. +octets as one :obj:`bytes`, but without the fix :meth:`ListField.pack +` iterates that value item +by item and rejects each :obj:`int` with ``FieldValueError``. ``pack`` writes +such a value as-is; a typed ``ListField`` is not affected. """ import unittest diff --git a/tests/corekit/test_module.py b/tests/corekit/test_module.py index 57cafd499a..525fe620a7 100644 --- a/tests/corekit/test_module.py +++ b/tests/corekit/test_module.py @@ -178,13 +178,13 @@ def test_klass_raises_protocolerror_when_class_name_was_never_given(self) -> Non def test_klass_treats_an_explicit_null_string_as_a_real_class_name(self) -> None: """``class_='(null)'`` is a class name, not the sentinel. - Before GitHub issue #833, :data:`NULL` was the plain :class:`str` - ``'(null)'``, so a descriptor built with that exact string was - indistinguishable from one built from the sentinel default -- an - equal-but-distinct string took whichever branch the identity check - happened to land on. The sentinel is no longer a :class:`str` at all, - so ``'(null)'`` now always resolves as an ordinary (missing) class - name, and the failure names it like any other bad class name would. + Without the GitHub issue #833 fix, :data:`NULL` is the plain + :class:`str` ``'(null)'``, so a descriptor built with that exact string + is indistinguishable from one built from the sentinel default -- an + equal-but-distinct string takes whichever branch the identity check + happens to land on. The sentinel is not a :class:`str` at all, so + ``'(null)'`` always resolves as an ordinary (missing) class name, and + the failure names it like any other bad class name would. """ target_module = types.ModuleType('demo.explicit_null') @@ -198,10 +198,10 @@ def test_klass_treats_an_explicit_null_string_as_a_real_class_name(self) -> None def test_null_sentinel_is_not_a_string(self) -> None: """:data:`NULL` means "absent", and nothing else compares equal to it. - The pre-#833 sentinel was the plain string ``'(null)'``, compared by - identity -- so an equal-but-distinct ``'(null)'`` from a caller took a - different branch than the sentinel itself, purely as a function of - string interning. :data:`NULL` is now a dedicated + Without the #833 fix, the sentinel is the plain string ``'(null)'``, + compared by identity -- so an equal-but-distinct ``'(null)'`` from a + caller takes a different branch than the sentinel itself, purely as a + function of string interning. :data:`NULL` is a dedicated :class:`~pcapkit.corekit.sentinels.NullType` singleton, so no string a caller passes can compare equal to it by ``==`` or by ``is``. @@ -223,16 +223,15 @@ def test_null_sentinel_is_not_a_string(self) -> None: def test_null_sentinel_identity_survives_copy_and_pickle(self) -> None: """:data:`NULL` stays the same object through :mod:`copy` and :mod:`pickle`. - Before this fix, :class:`~pcapkit.corekit.sentinels.NullType` had no + Without the fix, :class:`~pcapkit.corekit.sentinels.NullType` has no :meth:`~pcapkit.corekit.sentinels.NullType.__copy__`, :meth:`~pcapkit.corekit.sentinels.NullType.__deepcopy__` or :meth:`~pcapkit.corekit.sentinels.NullType.__reduce__` of its own, so :func:`copy.copy`, :func:`copy.deepcopy` and every :mod:`pickle` - protocol fell back to the default behaviour for a plain object and - each produced a *second*, non-identical :class:`NullType` instance. - Measured on GitHub pull request #835's head, ``375b50c85``: this - test's ``copy.deepcopy`` and ``pickle`` assertions below all fail - with an :class:`AssertionError` there, since the round-tripped object + protocol fall back to the default behaviour for a plain object and + each produces a *second*, non-identical :class:`NullType` instance, + so this test's ``copy.deepcopy`` and ``pickle`` assertions below all + fail with an :class:`AssertionError`, since the round-tripped object ``is not`` :data:`NULL`. Every pickle protocol the running interpreter supports -- ``0`` @@ -279,20 +278,20 @@ def test_klass_raises_protocolerror_not_typeerror_after_deepcopying_the_descript ``self.name is NULL`` to tell "the caller never named a class" apart from "the caller named a class that does not exist" -- see :meth:`test_klass_raises_protocolerror_when_class_name_was_never_given`. - Before this fix, :func:`copy.deepcopy` on the descriptor recursed into - that check's operand and minted a second, non-identical + Without the fix, :func:`copy.deepcopy` on the descriptor recurses into + that check's operand and mints a second, non-identical :class:`~pcapkit.corekit.sentinels.NullType`, so the identity check - failed silently and ``self.name`` -- still that stray sentinel, now - masquerading as an ordinary class name -- reached :func:`getattr` - directly. :func:`getattr` on a non-:class:`str` name raises a bare + fails silently and ``self.name`` -- that stray sentinel, masquerading + as an ordinary class name -- reaches :func:`getattr` directly. + :func:`getattr` on a non-:class:`str` name raises a bare :exc:`TypeError` that :meth:`klass` does not catch, downgrading what should be a clean :exc:`~pcapkit.utilities.exceptions.ProtocolError` into ``TypeError: attribute name must be string, not 'NullType'``. - Measured on GitHub pull request #835's head, ``375b50c85``: the - ``assertRaises(self.module.ProtocolError)`` below does not catch that - :exc:`TypeError` there, since :exc:`ProtocolError` derives from - :exc:`BaseError` and :exc:`ValueError`, never :exc:`TypeError`, so the - test fails with the :exc:`TypeError` propagating out uncaught. + The ``assertRaises(self.module.ProtocolError)`` below does not catch + that :exc:`TypeError`, since :exc:`ProtocolError` derives from + :exc:`BaseError` and :exc:`ValueError`, never :exc:`TypeError`, so + without the fix the test fails with the :exc:`TypeError` propagating + out uncaught. The target module is registered directly into :data:`sys.modules`, the same way :meth:`_register` does for every other test in this diff --git a/tests/corekit/test_sentinel_exports_unit.py b/tests/corekit/test_sentinel_exports_unit.py index f68a6ba014..6f233658ef 100644 --- a/tests/corekit/test_sentinel_exports_unit.py +++ b/tests/corekit/test_sentinel_exports_unit.py @@ -7,61 +7,45 @@ bare rule would not give: the types are not part of the public surface, so exposing only the final objects is what keeps the published API to the things a caller actually uses. GitHub issue #911 is the issue the -implementing work for that ruling belongs to. That is one rule with two -directions, and the tree on ``origin/main`` broke it in both: - -* ``pcapkit.corekit.module.__all__`` was ``['NULL', 'NullType', 'ModuleDescriptor']`` - -- the type is exported; -* ``pcapkit.corekit.enum.__all__`` was - ``['NO_DEFAULT', 'NoDefaultType', 'EnumLookup', 'EnumRegistry']`` -- likewise; -* ``pcapkit.corekit.fields.field.__all__`` was ``['Field']`` -- the *object* is - missing, so this one needs a name added rather than removed. - -So two names come out and one goes in, which is why the fix is not "remove the -types". :class:`SentinelExportTests` asserts the shape in both directions and per -module, rather than as one aggregate that a half-applied change would still satisfy. - -What only ``import *`` narrows: the type stays importable by its dotted path, so -``from pcapkit.corekit.module import NullType`` keeps working and an annotation -naming it keeps resolving. That is the whole extent of the breaking change the issue -is labelled for, and -:meth:`SentinelExportTests.test_every_sentinel_type_is_still_importable_by_name` +implementing work for that ruling belongs to. + +Every sentinel is *defined* in :data:`CANONICAL_MODULE`: the owner chose one +shared module over one module per sentinel, on #911, so the ``Type`` +naming convention and the dunder methods the sentinels share or differ on sit +together rather than scattered over a near-empty file per sentinel. Under the +owner's ruling on #1715 that a moved name keeps no re-export at its old path +(GitHub issue #1744), the export rule is asserted against +:data:`CANONICAL_MODULE`, the only import path, and :mod:`pcapkit.corekit.module`, +:mod:`pcapkit.corekit.fields.field` and :mod:`pcapkit.corekit.enum`, which use +the sentinels, export neither the object nor the type. :class:`SentinelExportTests` +asserts the shape in both directions and per module, rather than as one aggregate +that a half-applied change would still satisfy. + +What only ``import *`` narrows: the type stays importable by its dotted path under +:data:`CANONICAL_MODULE`, so an annotation naming it keeps resolving. +:meth:`SentinelExportTests.test_every_sentinel_type_is_importable_from_the_canonical_module` pins it so a later reading of the ruling cannot escalate into deleting the types. -The population is **four**, not the three -:file:`docs/source/contributing/conventions/sentinel-convention.rst` documented -- -``ABSENT`` / -``AbsentType`` in :mod:`pcapkit.protocols.protocol` is the fourth, missed because a -sweep filtered on capitalised names did not see it when it was still spelled -``_Absent``/``_AbsentType``, with a leading underscore. :class:`SentinelPopulationTests` -pins the count and the doc together, so the next sentinel cannot be added to one -without the other. ``ABSENT`` is private and stays out of :attr:`__all__` in both -directions -- not under the export ruling, which argues the opposite for an -object like this one, but under the privacy ruling stated below, which treats a -sentinel documented as private as having no place in :attr:`__all__` at all. - -A follow-up to this same issue moved every sentinel's *definition* into -:mod:`pcapkit.corekit.sentinels`: the owner chose one shared module over one module -per sentinel, on #911. The shared module is where the ``Type`` naming -convention and the dunder methods the sentinels share or differ on can sit -together, which a near-empty file per sentinel would scatter. -Every assertion above still holds unchanged, since it is about -each original module's ``__all__``, which the re-export shims left untouched; what -changed is only :attr:`type.__module__` for the four types, which -:meth:`SentinelExportTests.test_every_sentinel_type_is_still_importable_by_name` now -checks against :data:`CANONICAL_MODULE` rather than against a different module per -sentinel. - -GitHub issue #937 later renamed two of the *objects* to SCREAMING_SNAKE -- -``NoValue`` to ``NO_VALUE`` and ``_Absent`` to ``ABSENT``, the latter also dropping -its leading underscore -- so every instance name agrees on one casing. Privacy for -what is now ``ABSENT`` stopped being signalled by the name at all and became -documentation-only. GitHub issue #719 is where the owner ruled that the underscore -can go, because documenting the object and its type as private, and not for public -use, is enough on its own to carry the privacy. -:meth:`SentinelExportTests.test_the_private_sentinel_is_exported_neither_way` is what -now pins that privacy under the new name, since the mechanical underscore signal it -used to double-check is gone. +The population includes ``ABSENT`` / ``AbsentType``, private to +:mod:`pcapkit.protocols.protocol` -- the kind of sentinel a sweep filtered on +capitalised names misses while it is spelled with a leading underscore. +:class:`SentinelPopulationTests` pins the population and the conventions doc +together, so the next sentinel cannot be added to one without the other. +``ABSENT`` stays out of :attr:`__all__` in both directions -- not under the export +ruling, which argues the opposite for an object like this one, but under the +privacy ruling stated below, which treats a sentinel documented as private as +having no place in :attr:`__all__` at all. + +Every object name is SCREAMING_SNAKE (GitHub issue #937), so every instance name +agrees on one casing, and the privacy of ``ABSENT`` is signalled by documentation +rather than by the name: GitHub issue #719 is where the owner ruled that the +underscore can go, because documenting the object and its type as private, and +not for public use, is enough on its own to carry the privacy. +:meth:`SentinelExportTests.test_the_private_sentinel_is_exported_neither_way` pins +that privacy. :meth:`SentinelPopulationTests.test_every_sentinel_object_is_screaming_snake` +pins the object's own casing directly, which +``test_every_sentinel_follows_the_naming_convention`` cannot: it derives the type +name from whatever the object is called, via :func:`_expected_type_name`. Ported code is deliberately **not** held to any of this. The case that settled it, ``_NOT_FOUND = object()`` in :file:`pcapkit/utilities/compat.py`'s ``cached_property`` @@ -69,58 +53,6 @@ :meth:`SentinelPopulationTests.test_the_vendored_bare_object_sentinel_went_with_its_backport` pins that, and the exemption stays in the conventions doc as a rule. -On the tree before this change, four of these fail: - -* ``test_module_exports_the_object_and_not_the_type`` -- ``'NullType'`` is in - :attr:`__all__`; -* ``test_enum_exports_the_object_and_not_the_type`` -- ``'NoDefaultType'`` is; -* ``test_field_exports_the_object_and_not_the_type`` -- ``'NoValue'`` is not; -* ``test_star_import_binds_the_objects_and_not_the_types`` -- all three of the above, - through the one operation that actually reads :attr:`__all__`. - -and two more fail on the documentation half: - -* ``test_conventions_doc_lists_every_sentinel_in_the_tree`` -- the table has three - rows and the prose says "three"; -* ``test_conventions_doc_carves_out_the_vendored_bare_object`` -- the carve-out is - not there at all. - -The rest pass either way and are regression guards: the sibling exports that must -survive the edit (``ModuleDescriptor``, ``Field``, ``EnumLookup``, -``EnumRegistry``), the types staying importable, and the naming convention itself. - -GitHub issue #937 also adds one genuinely new assertion rather than only renaming -existing ones: :meth:`SentinelPopulationTests.test_every_sentinel_object_is_screaming_snake` -pins the object's own casing directly, which nothing before it did -- -``test_every_sentinel_follows_the_naming_convention`` only ever pinned that -``Type`` derives from whatever the object is called, by design, via -:func:`_expected_type_name`, which is precisely why it passed on ``main`` at -``382375811`` even though ``NoValue`` and ``_Absent`` were not SCREAMING_SNAKE -there -- its two special cases (a ``bare.isupper()`` guard and a leading-underscore -carry-through) existed *because* the objects disagreed, so the check that derives -the type from the object could never by itself catch the object's casing being -wrong. The new test fails twice on that commit, measured directly:: - - $ git show 382375811:pcapkit/corekit/sentinels.py | grep -n 'NoValue = \\|_Absent = ' - 227:NoValue = NoValueType() - 469:_Absent = _AbsentType() - $ python3 -c "import re; p = re.compile(r'^[A-Z][A-Z0-9_]*$'); \\ - print([n for n in ('NULL', 'NoValue', 'NO_DEFAULT', '_Absent') if not p.match(n)])" - ['NoValue', '_Absent'] - -Now that every object is SCREAMING_SNAKE, :func:`_expected_type_name` collapses to -the one mechanical rule those two special cases used to carve exceptions around; -see its own docstring below for what dropped and why the assertion strength survives -the drop. - -GitHub issue #1744 then dropped the per-module re-exports, under the owner's -ruling on #1715 that a moved name keeps no re-export at its old path. So the -export rule is now asserted against :data:`CANONICAL_MODULE`, the only import -path, and the three per-module tests flip: :mod:`pcapkit.corekit.module`, -:mod:`pcapkit.corekit.fields.field` and :mod:`pcapkit.corekit.enum` export -neither the object nor the type. The type is importable by its dotted path -under :data:`CANONICAL_MODULE` only. - """ from __future__ import annotations @@ -146,26 +78,23 @@ ROOT = pathlib.Path(__file__).resolve().parents[2] #: Where every sentinel is *defined*: the single shared module GitHub -#: issue #911 chose over one module per sentinel. Each entry in :data:`SENTINELS` -#: below used to name a different module here (``pcapkit.corekit.module``, -#: ``pcapkit.corekit.fields.field``, ``pcapkit.corekit.enum`` and -#: ``pcapkit.protocols.protocol`` respectively); each reports this one. +#: issue #911 chose over one module per sentinel. Every entry in :data:`SENTINELS` +#: below reports this one. CANONICAL_MODULE = 'pcapkit.corekit.sentinels' #: Every sentinel in the tree that follows the house ``Type`` convention, -#: as ``(instance name, instance, type)``. That is more sentinels than -#: :file:`docs/source/contributing/conventions/sentinel-convention.rst` used to -#: document -- see the module docstring. -#: Every one shares :data:`CANONICAL_MODULE` as its defining module, which is -#: why a per-entry module column is no longer part of this tuple -- see -#: :data:`PUBLIC_SENTINELS` below for the (still distinct) *shim* locations. +#: as ``(instance name, instance, type)``. +#: Every one shares :data:`CANONICAL_MODULE` as its defining module, so the tuple +#: carries no per-entry module column -- see :data:`PUBLIC_SENTINELS` below for +#: the distinct *consumer* modules. #: Built by :func:`_sentinels` from the live import, not at module load. #: The public three of :data:`SENTINELS`, as ``(consumer module, object name, type -#: name)``. The consumer is the module that used to re-export the sentinel before -#: GitHub issue #1744. ``ABSENT`` is absent from it deliberately: it is private, so -#: it is exported neither way and the export rule does not reach it -- GitHub issue -#: #937 dropped its leading underscore, but not its privacy. +#: name)``. The consumer is the module that uses the sentinel and, per GitHub issue +#: #1744, does not re-export it. ``ABSENT`` is absent from it deliberately: it is +#: private, so it is exported neither way and the export rule does not reach it -- +#: its name carries no leading underscore (GitHub issue #937), but it is private +#: all the same. PUBLIC_SENTINELS = ( ('pcapkit.corekit.module', 'NULL', 'NullType'), ('pcapkit.corekit.fields.field', 'NO_VALUE', 'NoValueType'), @@ -194,15 +123,12 @@ def setUp(self) -> 'None': def _expected_type_name(instance_name: 'str') -> 'str': """The type name the house convention derives from an instance name. - One mechanical rule since GitHub issue #937 made every instance name - SCREAMING_SNAKE: split on ``_``, title-case each word, and append ``Type``. + One mechanical rule, since every instance name is SCREAMING_SNAKE (GitHub + issue #937): split on ``_``, title-case each word, and append ``Type``. ``NULL`` gives ``NullType``, ``NO_VALUE`` gives ``NoValueType``, ``NO_DEFAULT`` - gives ``NoDefaultType`` and ``ABSENT`` gives ``AbsentType``. Before #937 this - needed two special cases that are gone now: ``NoValue`` was already CamelCase, - so a ``bare.isupper()`` guard skipped re-title-casing it, and ``_Absent`` - carried a leading underscore that had to be stripped and re-added around that - guard. Neither input shape exists any more, so neither does the code that - handled it. + gives ``NoDefaultType`` and ``ABSENT`` gives ``AbsentType``. No instance name + is CamelCase or carries a leading underscore, so no special case handles + either shape. """ return ''.join(word.capitalize() for word in instance_name.split('_')) + 'Type' @@ -221,18 +147,13 @@ def _star_import(module: 'str') -> 'dict[str, object]': def _sentinel_section() -> 'str': - """The "Naming a Sentinel" section, now its own page. - - Read whole rather than sliced between two anchors: GitHub issue #918 split - the single-page :file:`docs/source/contributing/conventions.rst` into one - file per ``.. _label:`` anchor, and the "Naming a Sentinel" section *is* - :file:`docs/source/contributing/conventions/sentinel-convention.rst` now, so - there is no following anchor left in the same file to slice against. That - retires the ``text.index('.. _sentinel-convention:')`` / - ``text.index('.. _registry-protocol:', start)`` pairing this used before the - split -- the two anchors moved into separate files, which is exactly what - GitHub issue #930 flagged as the split's concrete blocker before #918 - resolved it. + """The "Naming a Sentinel" section, which is its own page. + + Read whole rather than sliced between two anchors: the conventions are one + file per ``.. _label:`` anchor (GitHub issue #918), and the "Naming a + Sentinel" section *is* + :file:`docs/source/contributing/conventions/sentinel-convention.rst`, so + there is no following anchor in the same file to slice against. """ path = ROOT / 'docs/source/contributing/conventions/sentinel-convention.rst' @@ -352,12 +273,10 @@ def test_the_private_sentinel_is_exported_neither_way(self) -> 'None': Pinned rather than assumed: the ruling says *export the objects*, and a literal reading of that would add ``ABSENT`` to :attr:`pcapkit.protocols.protocol.__all__`, which would publish a sentinel - whose own docstring says it never leaves the module. This kept pinning the - same fact under ``_Absent``/``_AbsentType`` before GitHub issue #937 dropped - the leading underscore that used to be the mechanical signal of "private" -- - the rename makes this test the *only* thing enforcing that any more, which - is why it still checks both the object and the type, and both directions of - ``import *``. + whose own docstring says it never leaves the module. With no leading + underscore to signal "private" (GitHub issue #937), this test is the *only* + thing enforcing it, which is why it checks both the object and the type, + and both directions of ``import *``. """ import pcapkit.protocols.protocol as protocol @@ -388,22 +307,13 @@ def test_every_sentinel_object_is_screaming_snake(self) -> 'None': object's own casing *is* -- a tree where every object were, say, CamelCase would satisfy it just as well, so long as the type followed suit. This test is what actually pins SCREAMING_SNAKE, independent of the type-derivation - rule, which is the rule this issue's ruling adds and the gap that let - ``NULL``, ``NoValue``, ``NO_DEFAULT`` and ``_Absent`` disagree on ``main`` - before it landed. - - Fails on the tree before this change, at ``382375811``: ``NoValue`` and - ``_Absent`` are the two objects that were not SCREAMING_SNAKE, measured - directly against that commit rather than assumed:: - - $ git show 382375811:pcapkit/corekit/sentinels.py | grep -n 'NoValue = \\|_Absent = ' - 227:NoValue = NoValueType() - 469:_Absent = _AbsentType() + rule -- the rule GitHub issue #937's ruling adds, and the gap that lets + object names disagree on casing. - Neither ``NoValue`` (``N``, ``o``, ``V``, ... mixed case) nor ``_Absent`` - (leading underscore, mixed case) matches ``^[A-Z][A-Z0-9_]*$``, so this test - fails twice over on that tree -- once for each -- and passes on this one for - every sentinel. + Without the fix the objects include ``NoValue`` and ``_Absent`` beside + ``NULL`` and ``NO_DEFAULT``. Neither ``NoValue`` (``N``, ``o``, ``V``, ... + mixed case) nor ``_Absent`` (leading underscore, mixed case) matches + ``^[A-Z][A-Z0-9_]*$``, so this test then fails once for each. """ pattern = re.compile(r'^[A-Z][A-Z0-9_]*$') @@ -443,25 +353,22 @@ def test_conventions_doc_lists_every_sentinel_in_the_tree(self) -> 'None': self.assertNotIn('The three sentinels deliberately differ', section) def test_conventions_doc_records_that_only_the_object_is_exported(self) -> 'None': - """The ruling this change implements belongs in the doc that states the rule. + """The #911 ruling belongs in the doc that states the rule. - Pinned as the *claim* rather than as wording. The first version asserted the - literal ``'ONLY'`` -- the maintainer's own capitalisation, lifted from a block - quote of his #911 reply -- so paraphrasing that quote under GitHub issue #949 - reddened this test without anything about the rule having changed. Asserting - someone's emphatic casing is exactly the defect #949 exists to remove, and a - test outside :mod:`tests.project` pinning a convention page's prose is how it - went unnoticed there. + Pinned as the *claim* rather than as wording. Asserting a literal such as + the maintainer's own emphatic capitalisation reddens the test whenever the + page paraphrases the quote (GitHub issue #949) without anything about the + rule having changed -- exactly the defect #949 exists to remove. So the substance is checked against the tree, which is where the ruling actually takes effect, and the page is only required to name the export boundary (``__all__``, an identifier rather than prose) and the issue that settled it. - The citation is accepted in either markup form. The page now writes it with the + The citation is accepted in either markup form. The page writes it with the ``:issue:`` role configured in ``docs/source/conf.py``, because a bare ``#NNN`` resolves to nothing in the built documentation -- so pinning the bare spelling - would again redden this test over markup while the ruling it checks is + would redden this test over markup while the ruling it checks is untouched, which is the same defect the paragraph above records. """ @@ -551,10 +458,9 @@ def test_no_default_is_deliberately_truthy(self) -> 'None': def test_the_private_sentinel_reprs_as_its_own_name(self) -> 'None': """``ABSENT``, not ````. - The reason the house rule prefers a class at all. ``AbsentType`` used to - keep a ``__repr__`` of its own, printing ````; the shared one now - prints the object's own name, as ``repr(None)`` is ``'None'``, so it no - longer needs one. + The reason the house rule prefers a class at all. The shared ``__repr__`` + prints the object's own name, as ``repr(None)`` is ``'None'``, so + ``AbsentType`` needs no ``__repr__`` of its own. """ live = _sentinels() diff --git a/tests/corekit/test_sentinel_py_abc_1960_unit.py b/tests/corekit/test_sentinel_py_abc_1960_unit.py index 992bb3e584..18756cff4b 100644 --- a/tests/corekit/test_sentinel_py_abc_1960_unit.py +++ b/tests/corekit/test_sentinel_py_abc_1960_unit.py @@ -4,10 +4,11 @@ Where the C ``_abc`` module is missing, as on GraalPy, :mod:`abc` falls back to ``_py_abc``, whose ``__subclasscheck__`` rebinds ``_abc_negative_cache`` and ``_abc_negative_cache_version`` on the class once any ABC has registered a -virtual subclass since the class was built. :class:`~pcapkit.corekit.sentinels.SentinelMeta` -refused every write to a built sentinel type, so ``isinstance(1, NullType)`` -raised :exc:`TypeError` there. It lets those two names through, and no -other. +virtual subclass since the class was built. Without the fix, +:class:`~pcapkit.corekit.sentinels.SentinelMeta` refuses every write to a built +sentinel type, so ``isinstance(1, NullType)`` raises :exc:`TypeError` there. +:class:`~pcapkit.corekit.sentinels.SentinelMeta` lets those two names through, +and no other. :class:`PyABCFallbackTests` runs the checks in a child interpreter that really takes the fallback. It starts with ``-S``, so that no module but :mod:`io` has diff --git a/tests/corekit/test_sentinel_singleton_1842_unit.py b/tests/corekit/test_sentinel_singleton_1842_unit.py index 0d44c2af93..2d66720e63 100644 --- a/tests/corekit/test_sentinel_singleton_1842_unit.py +++ b/tests/corekit/test_sentinel_singleton_1842_unit.py @@ -7,10 +7,10 @@ calling the type again, :func:`copy.copy`, :func:`copy.deepcopy` and every :mod:`pickle` protocol hand back the one object. -Before the change only ``NULL`` passed all of that. ``NO_DEFAULT`` failed -pickle protocols 0 and 1, and ``NO_VALUE`` and ``ABSENT`` failed everything. +Without the fix only ``NULL`` passes all of that. ``NO_DEFAULT`` fails +pickle protocols 0 and 1, and ``NO_VALUE`` and ``ABSENT`` fail everything. -In review of that change the owner asked for the base to be public, named +In review of the fix the owner asked for the base to be public, named ``Sentinel`` after the library's other abstract bases, for every sentinel and sentinel type to be immutable, and for their repr and related behaviour to agree with Python's own singletons. :class:`BuiltinParityTests` runs the @@ -18,9 +18,9 @@ and against each sentinel, and the classes after it pin each behaviour on its own. -:data:`OLD_PICKLES` are pickles written by the module as it stood at -``75a9f237b``, the commit this change is based on, so they pin that a pickle -made before the change still loads. They were produced by executing +:data:`OLD_PICKLES` are pickles written by :mod:`pcapkit.corekit.sentinels` as +it stood at ``75a9f237b``, without the #1842 fix, so they pin that a pickle +written without the fix still loads. They were produced by executing ``git show 75a9f237b:pcapkit/corekit/sentinels.py`` as a module named ``pcapkit.corekit.sentinels``, registering it in :data:`sys.modules`, and calling :func:`pickle.dumps` on each sentinel at each protocol. diff --git a/tests/corekit/test_sentinels_housing_unit.py b/tests/corekit/test_sentinels_housing_unit.py index 9e605b0ada..5336da6d44 100644 --- a/tests/corekit/test_sentinels_housing_unit.py +++ b/tests/corekit/test_sentinels_housing_unit.py @@ -12,16 +12,16 @@ :class:`~pcapkit.corekit.sentinels.NoValueType`, :class:`~pcapkit.corekit.sentinels.NoDefaultType` and :class:`~pcapkit.corekit.sentinels.AbsentType`, and their instances. The -original modules (:mod:`pcapkit.corekit.module`, +modules that use them (:mod:`pcapkit.corekit.module`, :mod:`pcapkit.corekit.fields.field`, :mod:`pcapkit.corekit.enum` and -:mod:`pcapkit.protocols.protocol`) kept a re-export of each until GitHub issue -#1744 dropped them, under the ruling on #1715 that a moved name keeps no -re-export at its old path; ``tests/project/test_no_moved_name_re_exports_1744_unit.py`` -pins that. Each still imports the *object* it uses from here. - -None of this module exists before that move, which is exactly what makes it a -regression test rather than a description: at ``d31c0aaf6``, a ``main`` -commit from before #922, ``import pcapkit.corekit.sentinels`` itself +:mod:`pcapkit.protocols.protocol`) import the *object* they use from here and +keep no re-export of it (GitHub issue #1744), under the ruling on #1715 that a +moved name keeps no re-export at its old path; +``tests/project/test_no_moved_name_re_exports_1744_unit.py`` pins that. + +Without #922 the sentinels module does not exist, which is exactly what makes +this a regression test rather than a description: at ``d31c0aaf6``, a ``main`` +commit without #922, ``import pcapkit.corekit.sentinels`` itself fails -- .. code-block:: text @@ -48,7 +48,7 @@ asserts that with ``is``, never ``==``. :class:`NoImportCycleTests` pins the other thing the issue asked to be checked -"on purpose" rather than left to the pre-existing ``cyclic-import`` findings +"on purpose" rather than left to the ``cyclic-import`` findings :command:`pylint` reports for this package, which run to hundreds and would swallow one more without comment: that :mod:`pcapkit.corekit.sentinels` itself imports nothing from any of the modules that depend on it, so the @@ -132,10 +132,10 @@ def test_sentinels_module_imports_none_of_its_four_consumers(self) -> 'None': :mod:`pcapkit.corekit.module`, :mod:`pcapkit.corekit.fields.field`, :mod:`pcapkit.corekit.enum` or :mod:`pcapkit.protocols.protocol` -- each of which imports *it*. Walked with :mod:`ast` rather than grepped for the - dotted name as plain text, because the module's own docstring above - names those modules *in prose*, explaining where each sentinel used to live -- - a text search would flag that explanation as if it were the cycle it is - warning readers there is no longer any risk of. + dotted name as plain text, because the sentinels module's own docstring + names those modules *in prose*, explaining that they import from it -- + a text search would flag that explanation as if it were the cycle this + test rules out. """ import ast diff --git a/tests/corekit/test_stringfield_lossless_unit.py b/tests/corekit/test_stringfield_lossless_unit.py index 46705b5ca9..b07cef2043 100644 --- a/tests/corekit/test_stringfield_lossless_unit.py +++ b/tests/corekit/test_stringfield_lossless_unit.py @@ -1,14 +1,14 @@ # -*- coding: utf-8 -*- """StringField packs back exactly the octets it unpacked. -GitHub issue #1293: :class:`~pcapkit.corekit.fields.strings.StringField` was -lossy three ways. Without an ``encoding`` it decoded with the charset -:func:`~pcapkit.utilities.chardet.detect` guessed and packed the text back as -UTF-8; an octet the charset could not decode became U+FFFD and was packed as -U+FFFD's own three octets; and a length of ``-1`` was taken from the *character* -count, so multi-octet text lost its tail. - -The octets are now remembered by a :class:`DecodedString +GitHub issue #1293: without the fix, :class:`~pcapkit.corekit.fields.strings.StringField` +is lossy three ways. Without an ``encoding`` it decodes with the charset +:func:`~pcapkit.utilities.chardet.detect` guesses and packs the text back as +UTF-8; an octet the charset cannot decode becomes U+FFFD and is packed as +U+FFFD's own three octets; and a length of ``-1`` is taken from the *character* +count, so multi-octet text loses its tail. + +The octets are remembered by a :class:`DecodedString ` and packed verbatim, while the decoded text is left exactly as it was -- U+FFFD and all. That the text is unchanged is load-bearing rather than incidental, so diff --git a/tests/dumpkit/test_common_unit.py b/tests/dumpkit/test_common_unit.py index 9879c4b616..63537dbe37 100644 --- a/tests/dumpkit/test_common_unit.py +++ b/tests/dumpkit/test_common_unit.py @@ -108,12 +108,12 @@ def setUp(self) -> None: def test_dumper_subclass_registration_is_opt_in(self) -> None: """Registration happens if and only if ``fmt`` is given. - This is the #514 opt-in contract, extended to the fifth pair. Before it, - an absent ``fmt`` was *inferred* from the subclass' + This is the #514 opt-in contract, extended to the fifth pair. It guards + against *inferring* an absent ``fmt`` from the subclass' :attr:`~dictdumper.dumper.Dumper.kind` property -- which is an instance - property, so the old code constructed an instance against a - :func:`tempfile.NamedTemporaryFile` while the ``class`` statement was - still executing. A class definition no longer touches the filesystem. + property, so inferring it constructs an instance against a + :func:`tempfile.NamedTemporaryFile` while the ``class`` statement is + still executing. A class definition does not touch the filesystem. """ from pcapkit.dumpkit.common import Dumper @@ -146,7 +146,7 @@ def kind(self): extractor.assert_not_called() traceflow.assert_not_called() - # the old inference path is gone, not merely unused + # no instance is constructed to infer ``fmt`` named_temp.assert_not_called() def test_dumper_subclass_rejects_ext_without_fmt(self) -> None: @@ -325,7 +325,7 @@ def test_make_dumper_hands_other_output_the_mapping_it_was_given(self) -> None: one that keeps this branch free of any risk of double-escaping -- a plain :class:`dict` is not even rebuilt: the writer is handed the caller's own mapping, with the caller's own key objects in it, exactly - as it was before #772. A :class:`bytes` key stays :class:`bytes` there, + as given. A :class:`bytes` key stays :class:`bytes` there, which is what ``dictdumper``'s ``json`` writer then breaks on, for reasons of its own. @@ -440,7 +440,7 @@ def test_null_dumper_noops_and_pcap_dumper_writes_file(self) -> None: def test_pcap_dumper_writes_the_record_header_in_the_global_header_byte_order(self) -> None: # The dumper writes the 16-octet record header itself instead of building a - # Frame to pack it, so the byte order of those four uint32 fields is now this + # Frame to pack it, so the byte order of those four uint32 fields is this # module's responsibility rather than the schema's. It has to match the magic # number in front of them, and 'big' has to keep working on a little-endian # host -- which is exactly what a struct format hardcoded to one endianness @@ -478,13 +478,13 @@ def test_pcap_dumper_writes_the_record_header_in_the_global_header_byte_order(se ) def test_pcap_dumper_truncates_an_out_of_range_record_field(self) -> None: - # UInt32Field, which used to pack these four fields, masks to the field width - # in NumberField.pre_process instead of rejecting an out-of-range value, so - # the dumper silently wrote a wrapped one. struct.pack raises on the same - # input, so writing the header directly would have turned a written -- if - # wrong -- record into a struct.error. Pinned so the mask is not mistaken for - # dead defensiveness and dropped: this is behaviour being preserved, not - # behaviour being chosen. + # UInt32Field, the field class that would otherwise pack these four fields, + # masks to the field width in NumberField.pre_process instead of rejecting an + # out-of-range value, so a wrapped one is silently written. struct.pack raises + # on the same input, so writing the header directly without the mask would + # turn a written -- if wrong -- record into a struct.error. Pinned so the + # mask is not mistaken for dead defensiveness and dropped: this is behaviour + # being preserved, not behaviour being chosen. from pcapkit.const.reg.linktype import LinkType from pcapkit.dumpkit.pcap import PCAPIO from pcapkit.protocols.data.misc.pcap.frame import Frame, FrameInfo @@ -508,12 +508,12 @@ def test_pcap_dumper_truncates_an_out_of_range_record_field(self) -> None: def test_pcap_dumper_appends_each_frame_without_reparsing_it(self) -> None: # Successive frames have to accumulate, and each record has to be the caller's - # own octets verbatim -- the dumper no longer round-trips them through a Frame - # construction, which packed the record and then dissected it again through + # own octets verbatim -- the dumper does not round-trip them through a Frame + # construction, which would pack the record and then dissect it again through # the whole protocol stack to reach bytes it had just been handed. # - # The payload here is deliberately not a valid Ethernet frame: under the old - # rebuild it was dissected anyway, so a dumper that reparses is not merely + # The payload here is deliberately not a valid Ethernet frame: such a rebuild + # dissects it anyway, so a dumper that reparses is not merely # slower but is doing work that can warn or raise on data it only had to copy. from pcapkit.const.reg.linktype import LinkType from pcapkit.dumpkit.pcap import PCAPIO diff --git a/tests/dumpkit/test_dumper_roundtrip_unit.py b/tests/dumpkit/test_dumper_roundtrip_unit.py index 31ffd63c60..d7b814fd91 100644 --- a/tests/dumpkit/test_dumper_roundtrip_unit.py +++ b/tests/dumpkit/test_dumper_roundtrip_unit.py @@ -132,7 +132,7 @@ def test_plist_keeps_none_values_and_items(self) -> None: self.assertEqual(got, {'error': none, 'items': [1, none, 2]}) def test_unhandled_value_types_get_an_explicit_rendering(self) -> None: - """#1258: networks, sentinels and timezones no longer dump as ``{}``.""" + """#1258: networks, sentinels and timezones do not dump as ``{}``.""" from pcapkit.corekit.sentinels import NO_VALUE, NULL value = { @@ -148,7 +148,7 @@ def test_unhandled_value_types_get_an_explicit_rendering(self) -> None: 'nl': '', 'tz': 28800.0}) def test_fallback_reads_the_slots_of_every_base_class(self) -> None: - """#1258: a leaf ``__slots__ = ()`` no longer hides its bases' slots.""" + """#1258: a leaf ``__slots__ = ()`` does not hide its bases' slots.""" class Base: __slots__ = ('first', 'unset') @@ -174,7 +174,7 @@ def test_json_writes_astral_characters_as_surrogate_pairs(self) -> None: self.assertEqual(json.loads(data)['x'], value) def test_plist_writes_strings_xml_cannot_spell_as_data(self) -> None: - """#1262: a control character no longer makes the document unparseable.""" + """#1262: a control character does not make the document unparseable.""" value = {'c': 'a\x01b\x1bc', 'ok': 'tab\there & '} got = plistlib.loads(self.dump('PLIST', value))['x'] self.assertEqual(got, {'c': 'a\x01b\x1bc'.encode(), 'ok': 'tab\there & '}) diff --git a/tests/dumpkit/test_fallback_cycle_unit.py b/tests/dumpkit/test_fallback_cycle_unit.py index eb966d647c..822231abc9 100644 --- a/tests/dumpkit/test_fallback_cycle_unit.py +++ b/tests/dumpkit/test_fallback_cycle_unit.py @@ -2,12 +2,12 @@ """GitHub issue #1372: the fallback writer expands an object without bound. ``_append_fallback`` dumps an object no other branch recognises through its -attributes. A class among them was expanded too, through ``vars()``, which -reaches the whole type graph, again along every path to each shared node. A -frame's info object meeting the fallback -- as it does when its +attributes. Without the fix, a class among them is expanded too, through +``vars()``, which reaches the whole type graph, again along every path to each +shared node. A frame's info object meeting the fallback -- as it does when its :class:`~pcapkit.corekit.infoclass.Info` class comes from another import of :mod:`pcapkit` -- carries its next layer's protocol class, and its JSON output -grew until the disk filled. A class is now written as its name, a cycle as a +grows until the disk fills. A class is written as its name, a cycle as a marker, and an expansion past ``FALLBACK_LIMIT`` objects raises. :mod:`pcapkit` is imported inside each test, after diff --git a/tests/dumpkit/test_make_dumper_subclass_2144_unit.py b/tests/dumpkit/test_make_dumper_subclass_2144_unit.py index a509447fe7..a249880ae4 100644 --- a/tests/dumpkit/test_make_dumper_subclass_2144_unit.py +++ b/tests/dumpkit/test_make_dumper_subclass_2144_unit.py @@ -1,11 +1,11 @@ # -*- coding: utf-8 -*- -"""GitHub issue #2144: ``make_dumper``'s ``object_hook`` recursed in a subclass. +"""GitHub issue #2144: ``make_dumper``'s ``object_hook`` must not recurse in a subclass. -The fallback branch of the ``object_hook`` that -:func:`~pcapkit.dumpkit.common.make_dumper` builds called +Without the fix, the fallback branch of the ``object_hook`` that +:func:`~pcapkit.dumpkit.common.make_dumper` builds calls ``super(type(self), self).object_hook(o)``. In a subclass of the class it -returns, ``type(self)`` is the subclass, so the call resolved back to that same -``object_hook`` and recursed until :exc:`RecursionError`. A value no branch +returns, ``type(self)`` is the subclass, so the call resolves back to that same +``object_hook`` and recurses until :exc:`RecursionError`. A value no branch recognises falls through to the output class's own ``object_hook``. :mod:`pcapkit` is imported inside each test, after diff --git a/tests/dumpkit/test_nameless_enum_rendering_unit.py b/tests/dumpkit/test_nameless_enum_rendering_unit.py index 09ec2eae78..3ce79dd7e7 100644 --- a/tests/dumpkit/test_nameless_enum_rendering_unit.py +++ b/tests/dumpkit/test_nameless_enum_rendering_unit.py @@ -99,17 +99,17 @@ class NamelessEnumRenderingTests(unittest.TestCase): was about zero. The values probed are derived from each enumeration's own declared bits - rather than written down, which is #702. This file used to sweep the literal - ``(0, 1, 8, 9, 65536)``, and ``65536`` is ``0x10000`` -- one bit past the - sixteen-bit field :class:`~pcapkit.const.tcp.flags.Flags` bounds its - :meth:`~enum.Enum._missing_` to. That guard is correct, so the probe was - what had to move: a value the registry is right to refuse cannot also be a - value the dumper renders. Every one of the library's flag registries - carries such a guard, at differing widths, so exempting the guarded - ones instead would have left this half of the sweep with nothing in it at - all. (GitHub issue #808 retyped another, ``TransportProtocol``, out of - this sweep entirely -- see ``test_no_flag_registry_renders_the_literal_none``'s - docstring below.) + rather than written down, which is #702. A literal such as ``65536`` is + ``0x10000`` -- one bit past the sixteen-bit field + :class:`~pcapkit.const.tcp.flags.Flags` bounds its + :meth:`~enum.Enum._missing_` to. That guard is correct, so the probe has to + respect it: a value the registry is right to refuse cannot also be a value + the dumper renders. Every one of the library's flag registries carries such + a guard, at differing widths, so exempting the guarded ones instead would + leave this half of the sweep with nothing in it at all. (``TransportProtocol`` + is a plain :class:`~aenum.IntEnum`, GitHub issue #808, and so outside this + sweep -- see ``test_no_flag_registry_renders_the_literal_none``'s docstring + below.) """ @@ -148,7 +148,7 @@ def test_scalar_return_renders_a_nameless_member_as_its_value(self) -> None: self.assertIsNone(member.name) rendered = dumper.object_hook(member) self.assertEqual(rendered, f'{registry.__name__}::{value} [{value}]') - # The defect, stated as what must no longer appear. Asserted + # The defect, stated as what must not appear. Asserted # separately from the equality above so a future change to # the rendering cannot quietly reintroduce the literal. self.assertNotIn('None', rendered) @@ -249,20 +249,15 @@ def test_no_flag_registry_renders_the_literal_none(self) -> None: Mobility Header flag registries. Naming them here would rot the moment another is added, so they are discovered. - GitHub issue #808 dropped one registry from this sweep: - :class:`~pcapkit.const.reg.apptype.TransportProtocol` was a bounded - :class:`~aenum.IntFlag` before that issue and is a plain - :class:`~aenum.IntEnum` after, so it no longer matches the + :class:`~pcapkit.const.reg.apptype.TransportProtocol` is a plain + :class:`~aenum.IntEnum` (GitHub issue #808), so it does not match the ``issubclass(attribute, (enum.Flag, aenum.Flag))`` test - :func:`_flag_registries` sweeps on. It contributed nothing to - ``nameless`` either before or after -- it declared ``0`` as - ``undefined`` and had no undeclared bits left in its field, the same - shape as ``CommandType`` -- so only the registry *count* below moved. + :func:`_flag_registries` sweeps on. - Since #702 the sweep is over every nameless value each registry admits - rather than over ``registry(0)`` alone. Before that, the four Mobility - Header registries were only ever exercised at the one value they share, - so the sweep was broad across registries and one value deep in each. + The sweep is over every nameless value each registry admits rather than + over ``registry(0)`` alone (#702). At ``registry(0)`` alone the Mobility + Header registries are exercised only at the one value they share, so the + sweep would be broad across registries and one value deep in each. """ from pcapkit.dumpkit.common import make_dumper @@ -302,14 +297,11 @@ def test_no_flag_registry_renders_the_literal_none(self) -> None: f'{registry.__name__}::{member.name} [{value}]') # Not an incidental detail: if this ever drops to zero the test above - # stops exercising the fix at all and would pass on unfixed code. - # Unchanged at 5 by GitHub issue #808 dropping the sweep from seven - # registries to six -- see the class docstring above: the registry it - # removed, TransportProtocol, was never one of the nameless five. + # stops exercising the fix at all and would pass without the fix. self.assertGreaterEqual(len(nameless), 5, nameless) def test_a_value_past_the_field_is_refused_rather_than_rendered(self) -> None: - """#702 -- the probe that was wrong, asserted the right way round. + """#702 -- a value past the field, asserted as the rejection it is. Every flag registry bounds its :meth:`~enum.Enum._missing_` to the width of its own field, and the widths differ: three bits for @@ -324,19 +316,9 @@ def test_a_value_past_the_field_is_refused_rather_than_rendered(self) -> None: holds only if the mask :func:`_field_mask` computes is exactly the bound each registry wrote down for itself. - Until GitHub issue #808 another width sat here too, - :class:`~pcapkit.const.reg.apptype.TransportProtocol`'s four bits -- - derived as ``max(cls.__members__.values()) * 2 - 1`` since it extended - itself at runtime and could not hard-code a bound. #808 retyped it to a - plain :class:`~aenum.IntEnum`, dropping it out of :func:`_flag_registries`' - sweep entirely (see the class docstring above), which is why its width - no longer appears. - - ``65536`` survives here, as the bound of the sixteen-bit registries - -- asserted as the rejection it always was, rather than as a value the - dumper was expected to render. Which also keeps the ``raise`` in those - guards covered: all but one were never reached by any test, and that - one was reached only by this file failing on it. + ``65536`` appears here as the bound of the sixteen-bit registries -- + asserted as a rejection, not as a value the dumper renders. That also + keeps the ``raise`` in those guards covered by a test. """ registries = _flag_registries() @@ -355,16 +337,14 @@ def test_a_value_past_the_field_is_refused_rather_than_rendered(self) -> None: with self.assertRaises(ValueError) as caught: registry(mask + 1) # ``enum`` requires ``ValueError`` from a refused lookup, and - # #677 brought the last six divergent registries onto the bare - # built-in the rest of :mod:`pcapkit.const` already raised, so - # this asserts the message rather than a type of its own. + # every registry in :mod:`pcapkit.const` raises the bare built-in + # (#677), so this asserts the message rather than a type of its own. self.assertIn(str(mask + 1), str(caught.exception)) self.assertIn(registry.__name__, str(caught.exception)) # The literal could not have been right for all of them, and this is the # measurement that says so: several distinct widths, and ``65536`` is - # outside every single one of them. 3 rather than 4 since GitHub issue - # #808 -- see the docstring above. + # outside every single one of them. self.assertGreaterEqual(len(widths), 3, widths) @@ -446,14 +426,9 @@ def _field_mask(registry: 'FlagRegistry') -> int: Mobility Header flags, ``0x07`` for the three-bit :class:`~pcapkit.const.ftp.command.CommandType`. - A seventh registry used to belong here on the same terms: - :class:`~pcapkit.const.reg.apptype.TransportProtocol` spelled its own bound - as ``max(cls.__members__.values()) * 2 - 1`` because it extends itself at - runtime and could not hard-code one. GitHub issue #808 retyped it from - :class:`~aenum.IntFlag` to a plain :class:`~aenum.IntEnum` once nothing - built a composite, dropping it out of :func:`_flag_registries`'s sweep - entirely rather than merely changing its bound -- it no longer matches - ``issubclass(attribute, (enum.Flag, aenum.Flag))``, so this function never + :class:`~pcapkit.const.reg.apptype.TransportProtocol` is a plain + :class:`~aenum.IntEnum` (GitHub issue #808), so it does not match + ``issubclass(attribute, (enum.Flag, aenum.Flag))`` and this function never sees it at all. Derived rather than read off the source, so it cannot drift from the guard @@ -481,8 +456,8 @@ def _nameless_values(registry: 'FlagRegistry') -> 'tuple[int, ...]': Three kinds of value come back and no more -- no bits at all, each single undeclared bit on its own, and every undeclared bit at once. The last is the multi-bit case, and it is the largest value the field admits with no declared - bit in it, which is what the old literal's ``9`` and its out-of-range - ``65536`` were each reaching for. Returning every subset would be exhaustive + bit in it, which is what a hand-written probe such as ``9``, or the + out-of-range ``65536``, reaches for. Returning every subset would be exhaustive and is not worth 8192 subtests for :class:`StdFlags`' thirteen undeclared bits. @@ -494,9 +469,6 @@ def _nameless_values(registry: 'FlagRegistry') -> 'tuple[int, ...]': and yields an empty tuple. :class:`~pcapkit.const.ftp.command.CommandType` is of that shape, declaring ``0`` as ``undefined``, which is why not every registry :func:`_flag_registries` discovers is nameless at zero. - :class:`~pcapkit.const.reg.apptype.TransportProtocol` used to be a second - example of the same shape before GitHub issue #808 retyped it out of the - sweep entirely -- see :func:`_field_mask`'s docstring. Args: registry: The flag enumeration to inspect. diff --git a/tests/dumpkit/test_pcap_trace_truncated_record_1541_unit.py b/tests/dumpkit/test_pcap_trace_truncated_record_1541_unit.py index b8027f6f8e..c01ad2e51c 100644 --- a/tests/dumpkit/test_pcap_trace_truncated_record_1541_unit.py +++ b/tests/dumpkit/test_pcap_trace_truncated_record_1541_unit.py @@ -4,9 +4,9 @@ A record whose ``incl_len`` runs past the end of its file, or a PCAP-NG block whose captured length runs past the block (#1405), is kept as captured by its reader: the declared length stays in ``frame_info`` and the packet is the octets -present. :class:`~pcapkit.dumpkit.pcap.PCAPIO` copied the declared length into the -trace record header, so the trace claimed octets it did not hold -- measured on a -record claiming ``0x7fffffff`` over 585 octets. +present. Without the fix, :class:`~pcapkit.dumpkit.pcap.PCAPIO` copies the declared +length into the trace record header, so the trace claims octets it does not hold -- +measured on a record claiming ``0x7fffffff`` over 585 octets. Each case traces a TCP flow whose last record is cut that way, then reads the trace back with :mod:`struct` alone: every record must hold exactly ``incl_len`` @@ -145,7 +145,7 @@ def test_the_dumper_writes_the_packet_length(self) -> None: self.assertEqual(_records(file.read()), [(3, 86, b'abc'), (3, 86, b'abc'), (0, 86, b'')]) def test_a_whole_record_is_traced_unchanged(self) -> None: - """A record the file holds whole keeps its octets, as before #1541.""" + """A record the file holds whole keeps its octets, with or without the #1541 fix.""" data = pcap([(SECOND + number, 0, frame) for number, frame in enumerate(_frames())]) self.assertEqual(self.trace(data)[24:].hex(), data[24:].hex()) diff --git a/tests/dumpkit/test_plist_escaping_regression.py b/tests/dumpkit/test_plist_escaping_regression.py index b8562c8d43..adeb583f6e 100644 --- a/tests/dumpkit/test_plist_escaping_regression.py +++ b/tests/dumpkit/test_plist_escaping_regression.py @@ -4,16 +4,16 @@ :file:`tests/dumpkit/test_common_unit.py` pins what :meth:`~pcapkit.dumpkit.common.make_dumper.DictDumper.object_hook` returns. This module pins the only thing that actually matters about it -- that the file on -disk parses -- against the fixture that used to prove it did not. +disk parses -- against a fixture that, without the fix, proves it does not. :file:`examples/captures/test.pcapng` carries a decryption secrets block whose TLS key log entries are keyed by a raw :class:`bytes` client random (:meth:`~pcapkit.protocols.schema.misc.pcapng.TLSKeyLog.post_process`), so the report's key is a ``bytes`` repr holding ``&``, ``<`` and ``>``. GitHub issue #772 and JarryShaw/DictDumper#125 are the two halves of that: ``dictdumper`` -interpolates a mapping key into its markup without escaping it, and until the -in-repo half landed the ``plist`` and ``xml`` reports of this fixture stopped -parsing at line 1517 of 1958 with ``not well-formed (invalid token)``. +interpolates a mapping key into its markup without escaping it, and without the +in-repo half the ``plist`` and ``xml`` reports of this fixture stop parsing at +line 1517 of 1958 with ``not well-formed (invalid token)``. This module is fixture-tier by its ``_regression.py`` name -- ``test.pcapng`` is generated rather than committed, so a unit-tier module may not read it (see @@ -136,8 +136,8 @@ def test_the_plist_report_escapes_a_pseudo_member_key_exactly_once(self) -> None This fixture carries three undeclared PCAP-NG option types, each of which reaches the writer as a mapping key rendered - ``OptionType:: [opt_unknown [N]]`` -- #771's case, and now the - same escaping as every other key rather than a special case beside it. + ``OptionType:: [opt_unknown [N]]`` -- #771's case, escaped the + same way as every other key rather than as a special case beside it. ``&lt;`` anywhere in the report is what a second pass over an already-escaped key would leave behind, so it is what this looks for. diff --git a/tests/dumpkit/test_repeated_field_dump_1484_unit.py b/tests/dumpkit/test_repeated_field_dump_1484_unit.py index f3abbf2fdc..ac0e0eb7e5 100644 --- a/tests/dumpkit/test_repeated_field_dump_1484_unit.py +++ b/tests/dumpkit/test_repeated_field_dump_1484_unit.py @@ -2,8 +2,8 @@ """Every value of a repeated field reaches the ``json``, ``plist`` and ``tree`` dumps. GitHub issue #1484. An IPv6 packet carrying two Destination Options headers -holds both under the one ``opts`` key, and every dump held only the first. -Each dump now writes the field as the list of both headers, in wire order, +holds both under the one ``opts`` key, and without the fix every dump holds only +the first. Each dump writes the field as the list of both headers, in wire order, while a field held once is written as its value. Every case builds its input in a temporary directory, and reads no capture. diff --git a/tests/dumpkit/test_tree_lossy_unit.py b/tests/dumpkit/test_tree_lossy_unit.py index e9d5a98b4c..0aeecb79ac 100644 --- a/tests/dumpkit/test_tree_lossy_unit.py +++ b/tests/dumpkit/test_tree_lossy_unit.py @@ -4,7 +4,7 @@ GitHub issue #1266: :meth:`dictdumper.tree.Tree._append_number` hands every number to :func:`math.isnan`, which raises ``TypeError: must be real number, not complex`` for the :class:`complex` values :class:`~dictdumper.tree.Tree` routes -there. :func:`~pcapkit.dumpkit.common.make_dumper` now writes those itself, in +there. :func:`~pcapkit.dumpkit.common.make_dumper` writes those itself, in the writer's own ``-> `` style. ``json`` and ``plist`` are untouched: they already write a :class:`complex` as the string ``(1+2j)``. diff --git a/tests/interface/test_core.py b/tests/interface/test_core.py index 2c01891541..89eac13929 100644 --- a/tests/interface/test_core.py +++ b/tests/interface/test_core.py @@ -308,9 +308,8 @@ def test_scapy_constant_selects_the_scapy_engine(self) -> None: # tests above, but running it needs the :program:`tshark` *binary* as well as the # :mod:`pyshark` package, and the engine drives it as a subprocess -- so a # stand-in would be a stand-in for the whole extraction rather than for a module, - # and would stop testing the routing this class is about. Its constant predates - # this change and its value is asserted against the registry above, which is what - # a wrong constant would break. + # and would stop testing the routing this class is about. Its constant's value is + # asserted against the registry above, which is what a wrong constant would break. def _fake_pcap_module(self, *, is_pcap_ct: bool): """A stand-in :mod:`pcap` module, as one distribution or the other. diff --git a/tests/interface/test_misc.py b/tests/interface/test_misc.py index 3f38e2e6a7..48bc9574a3 100644 --- a/tests/interface/test_misc.py +++ b/tests/interface/test_misc.py @@ -26,18 +26,18 @@ #: is expected to agree on it, either by dissecting the capture itself or by being #: redirected to the default engine. #: -#: **Two, not three.** Flow tracing was keyed on (source, destination), so the two -#: halves of one connection came back as two flows; it is now keyed on the pair of -#: endpoints, so a conversation is one flow. ``in.pcap``'s three TCP frames are -#: frame 3 (``123.129.210.135:80`` -> ``192.168.1.100:55232``), frame 4 (the same -#: connection the other way) and frame 5 (``192.168.1.100:55216`` -> -#: ``123.129.210.135:80``, a second connection). Frames 3 and 4 share an address -#: pair and a port pair and differ only in direction, so they are one TCP -#: connection and now one flow -- leaving two conversations, not three. +#: **Two, not three.** Flow tracing is keyed on the pair of endpoints rather than on +#: (source, destination), so both halves of one connection are one flow. +#: ``in.pcap``'s three TCP frames are frame 3 (``123.129.210.135:80`` -> +#: ``192.168.1.100:55232``), frame 4 (the same connection the other way) and +#: frame 5 (``192.168.1.100:55216`` -> ``123.129.210.135:80``, a second +#: connection). Frames 3 and 4 share an address pair and a port pair and differ +#: only in direction, so they are one TCP connection and one flow -- leaving two +#: conversations, not three. IN_PCAP_TCP_STREAMS = 2 -#: The same capture with ``trace_bidirectional=False``, i.e. the per-direction -#: behaviour flow tracing had before conversations became one flow. +#: The same capture with ``trace_bidirectional=False``, i.e. per-direction flow +#: tracing, in which each direction of a conversation is a flow of its own. IN_PCAP_TCP_FLOWS_PER_DIRECTION = 3 @@ -47,11 +47,11 @@ class FollowTCPStreamTests(unittest.TestCase): ``examples/captures/in.pcap`` (committed, so unit-tier safe) holds three TCP frames making up **two** conversations -- see - :data:`IN_PCAP_TCP_STREAMS` for which frames pair up and why. #399: the - reassembly adapter was chosen by comparing the engine *instance* to a string -- - a comparison that is never true -- so every engine silently used the pcapkit - adapter regardless of which engine ran. That crashed on DPKT frames - (``AttributeError: 'dict' object has no attribute 'packet'``) and returned an + :data:`IN_PCAP_TCP_STREAMS` for which frames pair up and why. #399 guards + against choosing the reassembly adapter by comparing the engine *instance* to a + string -- a comparison that is never true -- so that every engine silently uses + the pcapkit adapter regardless of which engine ran. That crashes on DPKT frames + (``AttributeError: 'dict' object has no attribute 'packet'``) and returns an empty, misleading result on Scapy frames. Both third-party engines that dissect this capture must therefore find every @@ -59,13 +59,12 @@ class FollowTCPStreamTests(unittest.TestCase): no dissection at all (PyPCAP) or have no reassembly adapter (PyShark) are redirected to the default engine instead, and are asserted separately below. - #406: this class previously expected Scapy to find *zero* streams, on the - stated rationale that it could not read the capture's link layer -- "a - documented capability gap". That rationale was wrong. ``in.pcap`` is plain - Ethernet (link type 1), which Scapy dissects perfectly well; the zero came - from :class:`~pcapkit.foundation.engines.scapy.Scapy` importing only + #406: Scapy finding *zero* streams here is not a capability gap in reading + the capture's link layer. ``in.pcap`` is plain Ethernet (link type 1), which + Scapy dissects perfectly well; a zero is the symptom of + :class:`~pcapkit.foundation.engines.scapy.Scapy` importing only :mod:`scapy.sendrecv`, which populates none of Scapy's layer registries, so - every frame came back as an undissected ``Raw``. Zero was the polluted + every frame comes back as an undissected ``Raw``. Zero is the polluted answer and the count here is the truthful one. """ @@ -117,8 +116,8 @@ def test_per_direction_tracing_still_splits_the_conversation(self) -> None: def test_dpkt_engine_follows_every_stream(self) -> None: # DPKT dissects in.pcap's Ethernet/IP/TCP in full, so it finds both streams, # of two frames and one, with no payload to reassemble. This is the - # regression guard for #399: before the fix the DPKT frame reached the - # pcapkit adapter and raised AttributeError, and the counted-vs-uncounted + # regression guard for #399: without the fix the DPKT frame reaches the + # pcapkit adapter and raises AttributeError, and the counted-vs-uncounted # call convention is what makes the reassembled numbering line up. with warnings.catch_warnings(): warnings.simplefilter('ignore') @@ -203,9 +202,9 @@ def test_pyshark_and_pypcap_fall_back_to_the_default_engine(self) -> None: self.assertEqual(len(streams), IN_PCAP_TCP_STREAMS) def test_dpkt_and_scapy_keep_the_pcap_trace_format(self) -> None: - # Both engines' flow tracing adapters now hand the tracer a PCAP frame + # Both engines' flow tracing adapters hand the tracer a PCAP frame # record built from the captured octets (#1507), so neither an explicit - # 'pcap' nor an unset format is replaced any more: every stream is written + # 'pcap' nor an unset format is replaced: every stream is written # as PCAP, and nothing warns about a substitution. from pcapkit.utilities.warnings import FormatWarning diff --git a/tests/utilities/test_config_unit.py b/tests/utilities/test_config_unit.py index ab01446e9c..9d6b5bc86c 100644 --- a/tests/utilities/test_config_unit.py +++ b/tests/utilities/test_config_unit.py @@ -1,10 +1,10 @@ # -*- coding: utf-8 -*- """Every tunable limit has one home, :mod:`pcapkit.utilities.config`. -GitHub issue #1726: the limits were defined across five packages, and the -readers in ``ipv6.py`` and ``toolkit/pypcapfile.py`` copied two of them with -``from ... import``, so patching the defining module left those readers on the -old value. Now each is a ``PCAPKIT_*`` environment variable read once by +GitHub issue #1726 guards against the limits being spread across packages, with +the readers in ``ipv6.py`` and ``toolkit/pypcapfile.py`` copying two of them with +``from ... import``, so that patching the defining module leaves those readers on +the old value. Each is a ``PCAPKIT_*`` environment variable read once by :mod:`pcapkit.utilities.config`, every reader looks it up there at call time, and the old import paths are gone. A bad value warns and falls back to the default; ``-1`` lifts a limit, and that or any value above the default warns of diff --git a/tests/utilities/test_exceptions_excepthook.py b/tests/utilities/test_exceptions_excepthook.py index f58659cb7d..e3f153373c 100644 --- a/tests/utilities/test_exceptions_excepthook.py +++ b/tests/utilities/test_exceptions_excepthook.py @@ -1,11 +1,11 @@ """Regression tests for the ``sys.tracebacklimit`` leak in a loud ``BaseError``. -GitHub issue #719: :class:`~pcapkit.utilities.exceptions.BaseError` set -:data:`sys.tracebacklimit` to ``0`` for every loud error outside development -mode, and nothing ever restored it. The attribute is process-global, so after -one pcapkit error every subsequent traceback in the *process* was truncated -- +GitHub issue #719: without the fix, :class:`~pcapkit.utilities.exceptions.BaseError` +sets :data:`sys.tracebacklimit` to ``0`` for every loud error outside development +mode, and nothing ever restores it. The attribute is process-global, so after +one pcapkit error every subsequent traceback in the *process* is truncated -- including exceptions that have nothing to do with :mod:`pcapkit`. Measured -against the tree before this fix:: +without the fix:: DEVMODE: False before hasattr(sys, 'tracebacklimit') = False @@ -13,14 +13,14 @@ then, unrelated: [][5] -> IndexError traceback.extract_tb(...) -> 0 frames -The fix removes the assignment outright and produces the same terse, one-line +``BaseError`` makes no such assignment, and produces the same terse, one-line output for a loud error through an exception hook instead -- :func:`pcapkit.utilities.exceptions._excepthook`, installed lazily by :func:`pcapkit.utilities.exceptions._install_excepthook` the first time a loud error actually needs it. A hook only shortens the printing of the exception it recognises as its own; it does not reach into how :mod:`traceback`, or any other -code, formats a *different* exception, which is what let the old mechanism leak -in the first place. +code, formats a *different* exception, which is what lets a +:data:`sys.tracebacklimit` assignment leak. A hook is awkward to probe from inside the very process that installs it -- :data:`sys.excepthook` only fires for an exception that reaches the real @@ -456,16 +456,16 @@ def exploding_delegate(etype, value, tb): class CrossInstanceReinstallTests(unittest.TestCase): """A fresh module instance must install *over* a stale instance's hook. - GitHub pull request #983: CI caught this one. ``_install_excepthook``'s guard used - to skip installing whenever :data:`sys.excepthook` already carried the + GitHub pull request #983. Without the fix, ``_install_excepthook``'s guard + skips installing whenever :data:`sys.excepthook` already carries the ``installed_by_pcapkit`` marker -- a plain ``True``, identical on *every* reloaded copy of :func:`~pcapkit.utilities.exceptions._excepthook`, so it - could not tell "my own instance already installed" from "some *other*, - possibly long-gone instance did". A worker that had already run some other - test touching a loud :class:`~pcapkit.utilities.exceptions.BaseError` left + cannot tell "my own instance already installed" from "some *other*, + possibly long-gone instance did". A worker that has already run some other + test touching a loud :class:`~pcapkit.utilities.exceptions.BaseError` leaves exactly that: a marked hook with no live module behind it any more. The next instance's own :func:`~pcapkit.utilities.exceptions._install_excepthook` - then saw the marker, concluded there was nothing to do, and left + then sees the marker, concludes there is nothing to do, and leaves ``_previous_excepthook`` at :data:`None` -- never actually installing *its own* hook at all. @@ -538,10 +538,10 @@ class ThreadingExcepthookTests(unittest.TestCase): :data:`threading.excepthook` needed its own hook for a reason distinct from "symmetry with :data:`sys.excepthook`": the interpreter's *default* :data:`threading.excepthook` itself consults :data:`sys.tracebacklimit` - when printing, so removing the global without replacing it on this side too - would have *regressed* a loud :class:`~pcapkit.utilities.exceptions.BaseError` - on a worker thread from one line to a full default traceback -- the - opposite of this change's purpose. Measured: 2 lines with + when printing, so without the global and without a hook on this side too, a + loud :class:`~pcapkit.utilities.exceptions.BaseError` on a worker thread + prints a full default traceback rather than one line -- the opposite of + what the hook is for. Measured: 2 lines with ``sys.tracebacklimit = 0`` in place, 12 without it and without this hook. """ diff --git a/tests/utilities/test_exceptions_warnings.py b/tests/utilities/test_exceptions_warnings.py index c87da7ba73..400992ba12 100644 --- a/tests/utilities/test_exceptions_warnings.py +++ b/tests/utilities/test_exceptions_warnings.py @@ -32,11 +32,11 @@ def test_struct_error_records_eof_flag(self) -> None: def test_base_error_installs_an_excepthook_in_non_dev_mode(self) -> None: # Only a *loud* error does this: the mechanism is process-global, and a # quiet error is internal control flow that must not disturb it. See - # tests/utilities/test_quiet_exceptions.py. It used to be + # tests/utilities/test_quiet_exceptions.py. The mechanism is an + # exception hook, which only shortens the printing of a BaseError, not # `sys.tracebacklimit = 0`, which is the #719 defect -- a loud error's - # own terseness truncated every *other* exception in the process too. - # It now installs an exception hook instead, which only shortens the - # printing of a BaseError; see + # own terseness truncating every *other* exception in the process too; + # see # tests/utilities/test_exceptions_excepthook.py for that hook's actual # printing behaviour, end to end. original_limit = getattr(sys, 'tracebacklimit', None) @@ -67,9 +67,9 @@ def test_base_error_devmode_logs_with_verbose_metadata(self) -> None: self.assertTrue(critical.call_args.kwargs['stack_info']) def test_warn_delivers_base_warning_categories_to_the_caller(self) -> None: - # pcapkit no longer installs an `ignore` filter of its own, so a caller - # who asks to see the warning sees it. Whether it is displayed, raised or - # dropped is now entirely the application's decision -- see + # pcapkit installs no `ignore` filter of its own, so a caller who asks + # to see the warning sees it. Whether it is displayed, raised or dropped + # is entirely the application's decision -- see # tests/utilities/test_warning_filters.py for that contract in full. with mock.patch.object(self.warnings.logger, 'warning'): with pywarnings.catch_warnings(record=True) as records: diff --git a/tests/utilities/test_logging.py b/tests/utilities/test_logging.py index 9b2005fedd..9159475f4a 100644 --- a/tests/utilities/test_logging.py +++ b/tests/utilities/test_logging.py @@ -63,7 +63,7 @@ def test_boolean_environment_flags_are_parsed(self) -> None: self.assertTrue(logging_module.SPHINX_TYPE_CHECKING) def test_invalid_boolean_environment_values_fall_back_to_false_and_warn(self) -> None: - # GitHub issue #1726: the value used to be taken as off in silence. + # GitHub issue #1726 guards against the value being taken as off in silence. # logging loads before warnings, so it records the value and warnings # reports it once it has loaded, naming the variable and the value. os.environ['PCAPKIT_DEVMODE'] = 'maybe' @@ -469,10 +469,10 @@ class ExtractorLoggingTests(unittest.TestCase): so a handle one of them leaks is reported -- as a :exc:`ResourceWarning`, at whatever unrelated point the collector reaches it -- inside a sibling module's test. :mod:`tests.utilities.test_stacklevel` records warnings to inspect their - attribution, and used to assert that its window held exactly one; a stray - handle from here was therefore enough to fail it (:issue:`606`). + attribution, and a window asserted to hold exactly one record is failed by a + stray handle from here (:issue:`606`). - The constructions are otherwise unchanged: with ``auto=True`` the extraction + Beyond that, the constructions are plain: with ``auto=True`` the extraction runs in ``__init__``, so the bodies are empty and everything asserted is read afterwards. diff --git a/tests/utilities/test_quiet_exceptions.py b/tests/utilities/test_quiet_exceptions.py index f655aeec26..7dbf9c580a 100644 --- a/tests/utilities/test_quiet_exceptions.py +++ b/tests/utilities/test_quiet_exceptions.py @@ -1,18 +1,18 @@ """Regression tests for #362 -- ``quiet=True`` must mean *emit nothing*. -``BaseError.__init__`` used to treat ``quiet`` as "log at ``ERROR`` instead of -``CRITICAL``", so every absent-key lookup through +Without the fix, ``BaseError.__init__`` treats ``quiet`` as "log at ``ERROR`` +instead of ``CRITICAL``", so every absent-key lookup through :meth:`MultiDict.get ` -- which raises :exc:`~pcapkit.utilities.exceptions.MissingKeyError` with ``quiet=True`` and -catches it immediately -- put an ``ERROR`` record on :data:`sys.stderr`. Parsing a -capture with unfragmented IPv6 and reassembly enabled produced one such record +catches it immediately -- puts an ``ERROR`` record on :data:`sys.stderr`. Parsing +a capture with unfragmented IPv6 and reassembly enabled produces one such record per frame, for what the calling code itself documents as the ordinary case. -The same constructor also set :data:`sys.tracebacklimit` to ``0`` on both -branches, so an absent-key ``.get()`` truncated the tracebacks of entirely -unrelated exceptions for the rest of the process. ``quiet=True`` now means no log -record on any channel and no process-global side effect; a loud error is -unchanged. +Without the fix, the constructor also sets :data:`sys.tracebacklimit` to ``0`` on both +branches, so an absent-key ``.get()`` truncates the tracebacks of entirely +unrelated exceptions for the rest of the process. ``quiet=True`` means no log +record on any channel and no process-global side effect; a loud error logs at +``CRITICAL``. """ from __future__ import annotations @@ -71,7 +71,7 @@ def tearDown(self) -> None: del sys.tracebacklimit else: sys.tracebacklimit = self._saved_tracebacklimit - # A loud error constructed by any test below now installs an exception + # A loud error constructed by any test below installs an exception # hook (see tests/utilities/test_exceptions_excepthook.py) rather than # setting sys.tracebacklimit -- so this has to be restored too. sys.excepthook = self._saved_excepthook @@ -208,12 +208,12 @@ def test_loud_stream_eof_error_still_logs(self) -> None: self.assertEqual(recorder.messages, [('CRITICAL', 'StreamEOFError: boom')]) def test_loud_error_leaves_tracebacklimit_unset_and_installs_a_hook(self) -> None: - """The feature a loud error provides is unchanged; the mechanism is not. + """A loud error shortens its own printing without touching the global. - A loud error used to shorten its own printing by setting - ``sys.tracebacklimit = 0`` -- process-wide, for every exception, for the - rest of the process, which is the #719 defect. It now installs an - exception hook instead, which only shortens the printing of a + Shortening it by setting ``sys.tracebacklimit = 0`` -- process-wide, for + every exception, for the rest of the process -- is the #719 defect. A + loud error installs an exception hook instead, which only shortens the + printing of a :class:`~pcapkit.utilities.exceptions.BaseError` itself; see ``tests/utilities/test_exceptions_excepthook.py`` for what that hook actually prints, end to end. diff --git a/tests/utilities/test_stacklevel.py b/tests/utilities/test_stacklevel.py index 8ded5a628b..ac477f168e 100644 --- a/tests/utilities/test_stacklevel.py +++ b/tests/utilities/test_stacklevel.py @@ -4,21 +4,21 @@ :func:`warnings.warn` and the :mod:`logging` module both count ``stacklevel`` outwards from the frame that made the call: ``1`` is that frame, ``2`` its caller, -and so on. ``stacklevel()`` used to return an absolute index into -:func:`traceback.extract_stack`'s result instead, so the value grew with the depth -of the caller's *own* stack and the attributed frame drifted one frame further out -for every extra outer frame. Under :program:`pytest`, or any deep call chain, the -function inverted its own purpose and blamed a frame with nothing to do with the -complaint. - -It also returned ``-1``, and not only in theory: :func:`traceback.extract_stack` -honours :data:`sys.tracebacklimit`, which :class:`BaseError -` *used to* set to ``0`` for every loud -error outside development mode (GitHub issue #719 removed that -- it prints its -terse line through an exception hook instead, and no longer touches the global -at all). After the first such error the extracted stack was *empty*, the -``for``/``else`` branch ran, and every subsequent warning in the process got -``-1``. +and so on. Without the fix, ``stacklevel()`` returns an absolute index into +:func:`traceback.extract_stack`'s result instead, so the value grows with the +depth of the caller's *own* stack and the attributed frame drifts one frame +further out for every extra outer frame. Under :program:`pytest`, or any deep +call chain, the function inverts its own purpose and blames a frame with nothing +to do with the complaint. + +Without the fix, the function also returns ``-1``, and not only in theory: +:func:`traceback.extract_stack` honours :data:`sys.tracebacklimit`, and setting +it to ``0`` for every loud error outside development mode is the GitHub issue +#719 defect in :class:`BaseError `, which +prints its terse line through an exception hook and does not touch the global +at all. Under that defect, after the first such error the extracted stack is +*empty*, the ``for``/``else`` branch runs, and every subsequent warning in the +process gets ``-1``. Three groups of tests here, because they fail for different reasons: @@ -27,7 +27,7 @@ can produce; * :class:`StacklevelAttributionTests` -- the end-to-end attribution, driving real :mod:`pcapkit` APIs from a range of stack depths, which is what a user actually - sees and where the drift showed up; + sees and where the drift shows; * :class:`WarnWrapperLevelTests` -- :func:`pcapkit.utilities.warnings.warn`'s own ``stacklevel`` argument, which every call site in the package forwards a ``stacklevel()`` through. @@ -72,8 +72,8 @@ INTERNAL = os.path.join(os.path.sep, 'site-packages', 'pcapkit', 'internal.py') #: Outer stack depths the end-to-end tests run every probe at. The point of the -#: list is its spread: the old absolute level was right by coincidence at exactly -#: one depth, so a single-depth test passed against the defect. +#: list is its spread: without the fix, the absolute level is right by coincidence at +#: exactly one depth, so a single-depth test passes against the defect. DEPTHS = (0, 1, 2, 5, 12, 30) #: A checkout inside a directory literally named ``pcapkit`` would make this very @@ -291,7 +291,7 @@ def test_level_never_walks_past_the_outermost_frame(self) -> None: self.assertEqual(self.level_for(outer=0, inner=inner), inner - 1) def test_no_pcapkit_frame_at_all_yields_the_immediate_caller(self) -> None: - """The ``for``/``else`` branch, which used to return ``len(tb) - 1``.""" + """The ``for``/``else`` branch, which without the fix returns ``len(tb) - 1``.""" for outer in (1, 4, 25): with self.subTest(outer=outer): self.assertEqual(self.level_for(outer=outer, inner=0), 1) @@ -327,7 +327,7 @@ def test_level_is_one_when_the_caller_is_outside_pcapkit(self) -> None: """Called from non-:mod:`pcapkit` code, the answer is ``1`` at any depth. The boundary is ``stacklevel()``'s own frame, so the only external frame - to name is its immediate caller. The old absolute index returned + to name is its immediate caller. Without the fix, the absolute index returns ``len(tb) - 2`` here, which under :program:`pytest` is a couple of dozen and climbing. @@ -369,15 +369,15 @@ def test_warning_is_not_attributed_to_pcapkit_itself(self) -> None: def test_a_truncated_traceback_limit_does_not_blind_the_walk(self) -> None: """``sys.tracebacklimit = 0`` must not change the answer. - :class:`~pcapkit.utilities.exceptions.BaseError` *used to* set it for - every loud error outside development mode (GitHub issue #719 removed - that), and :func:`traceback.extract_stack` honours it -- returning an - *empty* list, on which the old implementation fell through to its - ``for``/``else`` and returned ``-1``. So in ordinary use the first error - silently broke the attribution of every warning after it, for the life - of the process. It is also why this only ever reproduced in a - full-suite run: alone, this module never raised a loud error first. - Nothing in :mod:`pcapkit` sets the global any more, but the frame walk + Setting it for every loud error outside development mode is the GitHub + issue #719 defect in :class:`~pcapkit.utilities.exceptions.BaseError`, + and :func:`traceback.extract_stack` honours it -- returning an *empty* + list, on which the implementation without the fix falls through to its + ``for``/``else`` and returns ``-1``. So under that defect the first loud + error silently breaks the attribution of every warning after it, for the + life of the process, and it reproduces only in a full-suite run: alone, + this module never raises a loud error first. Nothing in :mod:`pcapkit` + sets the global, but the frame walk this guards is correct regardless of who does, which is exactly the point of setting it by hand below rather than provoking it indirectly. @@ -405,12 +405,12 @@ def test_a_foreign_warning_in_the_window_does_not_fail_the_probe(self) -> None: The probes record with ``simplefilter('always')``, which un-ignores the categories Python hides by default, so a file handle an earlier test left for the collector arrives in the window as a :exc:`ResourceWarning`. Two - assumptions used to break on that: the count, which required the window to - hold exactly one record, and the *position*, since the probe returned the - last record rather than its own (:issue:`606`). + assumptions break on that (:issue:`606`): a count requiring the window to + hold exactly one record, and picking by *position*, which returns the last + record rather than the probe's own. Warnings are raised on both sides of the probe here deliberately -- one - before and one after -- because the trailing one is what made picking by + before and one after -- because the trailing one is what makes picking by position wrong rather than merely lucky. Emitting them directly keeps the test deterministic: the real thing depends on garbage-collection timing, which is why it moved between unrelated pull requests instead of failing @@ -460,7 +460,7 @@ def error_site(self) -> 'logging.LogRecord': return recorder.records[-1] def test_error_log_blames_the_same_line_from_every_depth(self) -> None: - """``BaseError`` used to negate the level, which blamed :mod:`logging`.""" + """Guards against ``BaseError`` negating the level, which blames :mod:`logging`.""" seen = {} # type: dict[int, tuple[str, int]] for depth in DEPTHS: record = through(depth, self.error_site) diff --git a/tests/utilities/test_warning_emission.py b/tests/utilities/test_warning_emission.py index e6b7969c0d..357f4efee5 100644 --- a/tests/utilities/test_warning_emission.py +++ b/tests/utilities/test_warning_emission.py @@ -11,8 +11,8 @@ the log record carries, never how many records there are; * constructing a warning category is **not** an emission, and reports nothing. -Before the fix the counts were: 1 outside development mode (the -:mod:`warnings` emission was swallowed by the filter the constructor installed -- +Without the fix the counts are: 1 outside development mode (the +:mod:`warnings` emission is swallowed by the filter the constructor installs -- see #364) and 3 in development mode (two log records, from ``warn()`` and again from ``BaseWarning.__init__``, plus the :mod:`warnings` emission). @@ -91,9 +91,9 @@ def test_counts_do_not_depend_on_devmode_or_verbose(self) -> None: def test_a_non_pcapkit_category_is_reported_the_same_way(self) -> None: """The counts must not depend on the category being a ``BaseWarning``. - A ``BaseWarning`` used to be reported a different number of times from a - plain :exc:`UserWarning`, because only the former ran the constructor that - logged and filtered. + Guards against a ``BaseWarning`` being reported a different number of + times from a plain :exc:`UserWarning`, as happens when only the former runs + a constructor that logs and filters. """ for devmode in (False, True): diff --git a/tests/utilities/test_warning_filters.py b/tests/utilities/test_warning_filters.py index 049d30d69a..afe1eaf6af 100644 --- a/tests/utilities/test_warning_filters.py +++ b/tests/utilities/test_warning_filters.py @@ -1,22 +1,22 @@ """Regression tests for #364 -- constructing a pcapkit warning must not touch the process-global :data:`warnings.filters`. -``BaseWarning.__init__`` used to call ``warnings.simplefilter('ignore', -type(self))`` outside development mode. That inserted an entry at index 0 of the +Without the fix, ``BaseWarning.__init__`` calls ``warnings.simplefilter('ignore', +type(self))`` outside development mode. That inserts an entry at index 0 of the filter list -- *ahead* of everything the interpreter, :option:`-W` and the host -application had put there -- and never removed it. Two consequences reached code +application have put there -- and never removes it. Two consequences reach code that has nothing to do with :mod:`pcapkit`: -1. the application's own configuration for the category was overridden, so - ``-W error`` and a programmatic :func:`warnings.simplefilter` were both +1. the application's own configuration for the category is overridden, so + ``-W error`` and a programmatic :func:`warnings.simplefilter` are both defeated; 2. :func:`warnings.simplefilter` calls ``warnings._filters_mutated()``, which invalidates every module's ``__warningregistry__``, so an unrelated - once-per-location warning was reported a second time. + once-per-location warning is reported a second time. -Every test here forces ``PCAPKIT_DEVMODE=0``, because the defect only reproduced -outside development mode -- in development mode the constructor logged instead of -filtering, and these assertions would pass against the unfixed code. +Every test here forces ``PCAPKIT_DEVMODE=0``, because the defect only reproduces +outside development mode -- in development mode the constructor without the fix +logs instead of filtering, and these assertions would pass without the fix. """ from __future__ import annotations @@ -80,8 +80,8 @@ def test_constructing_a_warning_leaves_the_global_filters_identical(self) -> Non category('just constructing the object') self.assertEqual(pywarnings.filters, snapshot) - # Called out separately: index 0 is the slot `simplefilter` used to - # take over, and the one that decides which action wins. + # Called out separately: index 0 is the slot the #364 defect's + # `simplefilter` takes over, and the one that decides which action wins. self.assertEqual(pywarnings.filters[0], snapshot[0]) self.assertEqual(len(pywarnings.filters), len(snapshot)) @@ -101,7 +101,7 @@ def test_application_filter_configuration_is_honoured(self) -> None: """``simplefilter('error')`` must turn a pcapkit warning into an error. This is the programmatic equivalent of ``python -W error``, which the - index-0 override used to defeat. + #364 defect's index-0 override defeats. """ with capture(self.logger): @@ -113,7 +113,7 @@ def test_application_filter_configuration_is_honoured(self) -> None: self.warnings.warn('a complaint', self.warnings.SchemaWarning, stacklevel=1) def test_application_can_still_silence_pcapkit_warnings(self) -> None: - """The suppression pcapkit used to impose must remain available to the caller.""" + """Suppressing pcapkit warnings is available to the application itself.""" with capture(self.logger): with pywarnings.catch_warnings(record=True) as records: pywarnings.resetwarnings()