From 85e6f513e72390412ae8ca090a61bff8eb8984c8 Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Sun, 25 Oct 2026 18:31:13 -0400 Subject: [PATCH] docs(tests): sixth whole-repo pass over tests-a (#1103) Prose-only pass over tests/protocols, tests/vendor, tests/const and tests/integration: change-relative narrative restated as present-tense behaviour ("guards against", "without the fix"), codebase counts dropped, timed context removed, and stale claims corrected (the #646 dump size is 88 octets, the PCAP-NG frame-length NOTE, the BitField probe docstring, EXPECTED_FAILURES references to entries that no longer exist). AST guard clean on every changed file; nearest tests and the docs/ conventions/issue-ref/tier/docstring-contract checks pass. --- tests/const/test_const_apptype_split_unit.py | 14 +- tests/const/test_const_enum_builtin_parity.py | 57 +-- tests/const/test_const_enum_lookup.py | 52 +-- tests/const/test_const_enum_no_mint.py | 243 +++++----- tests/const/test_const_ethertype_862_unit.py | 16 +- ...onst_ftp_command_get_key_type_1824_unit.py | 15 +- .../test_const_ftp_featcode_case_903_unit.py | 10 +- tests/const/test_const_linktype_209_unit.py | 89 ++-- ...st_const_method_case_sensitive_896_unit.py | 2 +- ...test_const_method_value_lookup_908_unit.py | 13 +- .../const/test_const_mh_lla_code_1848_unit.py | 10 +- ...ster_validates_every_registry_1816_unit.py | 9 +- tests/const/test_const_registry_protocol.py | 123 +++--- .../const/test_const_str_payload_870_unit.py | 6 +- tests/integration/test_cli_subprocess.py | 4 +- tests/integration/test_engine_runtime.py | 6 +- tests/integration/test_files_output_naming.py | 18 +- tests/integration/test_frame_iteration.py | 24 +- .../integration/test_traceflow_end_to_end.py | 6 +- tests/protocols/application/test_ftp_unit.py | 6 +- tests/protocols/application/test_http_unit.py | 19 +- .../test_httpv2_payload_length_unit.py | 28 +- tests/protocols/application/test_ngap_unit.py | 2 +- ..._ospf_cryptographic_authentication_unit.py | 1 - .../test_ospf_dead_pattern_1844_unit.py | 13 +- tests/protocols/application/test_ospf_unit.py | 6 +- tests/protocols/application/test_rarp_unit.py | 6 +- .../internet/test_hip_fixed_bits_unit.py | 6 +- .../internet/test_hip_locator_flags_unit.py | 9 +- .../test_hip_locator_set_length_unit.py | 25 +- .../test_hip_r1_counter_width_unit.py | 46 +- .../internet/test_hip_reserved_bits_unit.py | 9 +- tests/protocols/internet/test_hip_unit.py | 72 +-- ...test_ipv4_tso_total_length_1547_runtime.py | 12 +- tests/protocols/internet/test_ipv4_unit.py | 97 ++-- .../internet/test_ipv6_calipso_pad_unit.py | 2 +- .../test_ipv6_ext_after_fragment_unit.py | 22 +- .../protocols/internet/test_ipv6_ext_unit.py | 32 +- .../internet/test_ipv6_extension_unit.py | 51 +-- tests/protocols/internet/test_mh_unit.py | 90 ++-- .../link/test_l2tp_data_export_1797_unit.py | 17 +- .../protocols/link/test_l2tp_version_unit.py | 31 +- .../misc/pcap/test_frame_length_runtime.py | 10 +- ...t_fromtimestamp_windows_range_2099_unit.py | 6 +- .../misc/test_pcap_magic_lookup_1739_unit.py | 11 +- tests/protocols/misc/test_pcapng_unit.py | 123 +++--- .../schema/test_bitfield_coverage_unit.py | 4 +- .../schema/test_enum_schema_registry_unit.py | 8 +- .../test_schema_metaclass_abc_cache_unit.py | 25 +- tests/protocols/schema/test_schema_unit.py | 8 +- .../test_construction_keyword_check_unit.py | 41 +- .../protocols/test_dispatch_bindings_unit.py | 14 +- .../test_dispatch_default_resolution_unit.py | 20 +- .../test_dispatch_reachability_unit.py | 24 +- .../protocols/test_dispatch_registry_unit.py | 31 +- tests/protocols/test_option_roundtrip_unit.py | 415 ++++-------------- .../test_padding_kept_as_captured_unit.py | 14 +- .../test_parser_registrar_identity_unit.py | 2 +- tests/protocols/test_pcapng_regression.py | 2 +- .../test_protocol_code_registration_unit.py | 6 +- tests/protocols/test_protocol_make_unit.py | 4 +- .../test_trial_parse_warnings_1580_unit.py | 2 +- tests/protocols/transport/test_sctp_unit.py | 35 +- .../transport/test_tcp_http_dispatch_unit.py | 6 +- .../transport/test_tcp_mptcp_addaddr_unit.py | 10 +- .../test_tcp_mptcp_error_message_unit.py | 2 +- .../test_tcp_mptcp_join_flag_ordering_unit.py | 4 +- .../test_tcp_mptcp_length_arithmetic_unit.py | 82 ++-- .../transport/test_tcp_mptcp_subtype_unit.py | 2 +- tests/protocols/transport/test_tcp_runtime.py | 8 +- .../protocols/transport/test_tcp_udp_unit.py | 20 +- tests/vendor/test_ftp_return_code_unit.py | 24 +- .../test_re_sub_positional_flag_unit.py | 18 +- tests/vendor/test_vendor_dest_path_unit.py | 4 +- tests/vendor/test_vendor_missing_body_unit.py | 2 +- tests/vendor/test_vendor_ngap_unit.py | 2 +- ...test_vendor_reference_formats_1664_unit.py | 2 +- .../test_vendor_reg_apptype_generator_unit.py | 17 +- .../test_vendor_snapshot_restore_unit.py | 12 +- 79 files changed, 1010 insertions(+), 1329 deletions(-) diff --git a/tests/const/test_const_apptype_split_unit.py b/tests/const/test_const_apptype_split_unit.py index ac8f35bc4f..5c7f35d243 100644 --- a/tests/const/test_const_apptype_split_unit.py +++ b/tests/const/test_const_apptype_split_unit.py @@ -275,11 +275,11 @@ class is created -- see the sweep over every member that pins this for each of them, aliased or not, and ``test_the_new_dunders_are_byte_identical_to_the_percent_form`` for the - sample #1062 formats against the pre-#798 form. + sample #1062 formats against the ``%``-formatted form. """ from pcapkit.const.reg.apptype import TCP - # No aliases: byte-identical to the pre-#807 form. + # No aliases: no suffix at all. self.assertEqual(repr(TCP.finger), '') self.assertEqual(str(TCP.finger), 'finger [79 - tcp]') @@ -335,7 +335,7 @@ def test_register_alias_is_scoped_to_one_per_transport_registry(self) -> None: self.assertEqual(SCTP.get(80).aliases, ()) def test_register_alias_is_reachable_by_its_sanitised_identifier(self) -> None: - """The finding this issue's second round fixed, pinned directly. + """An alias is reachable by subscription on its sanitised name. An alias whose own name does not resolve through subscription is not much of an alias -- ``TCP['http-alt-test']`` is a ``KeyError`` because @@ -385,9 +385,9 @@ def test_register_alias_refuses_an_unsanitisable_name(self) -> None: """A name with nothing left after sanitising, or a Python cannot start with. Refused explicitly rather than falling back to a synthetic identifier - such as the port-and-counter scheme this method used before this - round -- a fallback here would just recreate the original defect of - an alias unreachable by anything resembling its own name. + such as a port-and-counter scheme -- a fallback here would just + recreate the defect of an alias unreachable by anything resembling its + own name. """ from pcapkit.const.reg.apptype import TCP @@ -525,7 +525,7 @@ def test_a_port_lookup_on_the_base_dispatches_by_transport(self) -> None: AppType.get(80) def test_a_service_name_is_not_a_port_lookup(self) -> None: - """GitHub issue #734's junk-minting path is gone rather than fixed. + """A service name is refused rather than resolved (GitHub issue #734). The string branch #734 reported tested ``key in __members_proto__``, which is keyed by transport protocol, so a name never matched and diff --git a/tests/const/test_const_enum_builtin_parity.py b/tests/const/test_const_enum_builtin_parity.py index b677f56965..4f46c26ff5 100644 --- a/tests/const/test_const_enum_builtin_parity.py +++ b/tests/const/test_const_enum_builtin_parity.py @@ -136,8 +136,8 @@ } #: The pair GitHub issue #804 finished, as ``(vendor module, class name, const -#: module)``. Their templates are the two of :data:`BESPOKE_TEMPLATES`' four -#: that #798 left on ``%``-formatted ``__repr__`` methods. +#: module)``. Their templates are those of :data:`BESPOKE_TEMPLATES` that #798 +#: left on ``%``-formatted ``__repr__`` methods. ISSUE_804_PAIR = ( ('pcapkit.vendor.ftp.command', 'Command', 'pcapkit.const.ftp.command'), ('pcapkit.vendor.http.method', 'Method', 'pcapkit.const.http.method'), @@ -153,9 +153,8 @@ #: ``pcapkit.const.http.{error_code,frame,setting}``, whose ``%`` lives in #: their ``_missing_`` guard (inherited from the *shared* #: :mod:`pcapkit.vendor.default` template) and which the issue's own last -#: comment split back out, plus ``pcapkit.const.reg.apptype.apptype`` (already -#: an f-string since #792, and off the table anyway while PR #815 -#: owns that tree). What is left with an actual ``%``-formatted ``__repr__`` +#: comment split back out, plus ``pcapkit.const.reg.apptype.apptype`` (an +#: f-string, #792). What is left with an actual ``%``-formatted ``__repr__`` #: is exactly these three -- each its own bespoke vendor template, like #: :data:`ISSUE_804_PAIR`'s pair, so each gets a const half and a vendor half. ISSUE_818_TRIPLE = ( @@ -365,30 +364,23 @@ def test_the_sweep_size_is_pinned(self) -> None: if issubclass(obj, int) and not issubclass(obj, aenum.Flag)] # The decomposition is asserted, not just the total, so this sweep stays - # in step with the three narrower ones it overlaps: 112 non-flag IntEnum - # and 6 IntFlag in tests.const.test_const_enum_lookup, and 118 -- their - # sum, unchanged -- in tests.const.test_const_enum_get. GitHub issue #808 - # moved ``TransportProtocol`` from the flag count to the int count by - # dropping its ``IntFlag`` base, one for one, so the sum each of those - # counts on stays the same even though the two addends moved. + # in step with the three narrower ones it overlaps: the non-flag IntEnum + # and the IntFlag sweeps of tests.const.test_const_enum_lookup, and their + # sum in tests.const.test_const_enum_get. ``TransportProtocol`` has no + # ``IntFlag`` base (GitHub issue #808), so it is in the int count. # - # Five of the ten string registries are the application layer one, which - # GitHub issue #732 split into a package: the memberless - # pcapkit.const.reg.apptype.apptype.AppType base plus one registry per - # transport protocol. It is discovered exactly like a member-bearing + # The application layer registry, which GitHub issue #732 split into a + # package -- the memberless pcapkit.const.reg.apptype.apptype.AppType base + # plus one registry per transport protocol -- accounts for several of the + # string registries. It is discovered exactly like a member-bearing # registry, since this sweep is structural and never looks at members. - # The tenth is GitHub issue #886's - # pcapkit.const.pcapng.tls_key_label.TLSKeyLabel, moved here from a - # hand-written class under pcapkit.protocols.misc.pcapng -- it defines - # no ``_missing_`` of its own, so it is not among the three ``value. - # upper()`` outliers #647 fixed above, and it never reached + # Another is GitHub issue #886's + # pcapkit.const.pcapng.tls_key_label.TLSKeyLabel -- it defines no + # ``_missing_`` of its own, so it is not among the ``value.upper()`` + # outliers #647 fixed above, and it is outside # tests.const.test_const_enum_get's narrower (IntEnum, IntFlag)-only - # sweep, so that module's own pinned count does not move. - # 112 became 114 with GitHub issue #880's ngap.ProcedureCode and - # ngap.ProtocolIE, moved onto EnumRegistry from hand-rolled IntEnum - # classes in pcapkit.protocols.application.ngap -- two more IntEnum - # registries, two more modules, no change to the flag or str counts -- - # and the sum in tests.const.test_const_enum_get became 120 with them. + # sweep. The int count includes GitHub issue #880's ngap.ProcedureCode + # and ngap.ProtocolIE, which are on EnumRegistry. self.assertEqual(len(ints), 114) self.assertEqual(len(flags), 6) self.assertEqual(len(strs), 10) @@ -897,9 +889,9 @@ class ConstEnumGuardTemplateTests(unittest.TestCase): :mod:`pcapkit.const` is generated from :mod:`pcapkit.vendor`, so the committed tree passing the tests above is not evidence that the templates - agree with it -- the next crawl would simply revert them. The four templates + agree with it -- the next crawl would simply revert them. The templates below each carry their own copy of the guard rather than inheriting the one - in :mod:`pcapkit.vendor.default`, which is why all four are checked -- + in :mod:`pcapkit.vendor.default`, which is why each is checked -- each against its own guard text in :data:`BESPOKE_TEMPLATES`, none of which uses ``%`` formatting (GitHub issues #792 and #798). """ @@ -945,7 +937,7 @@ def test_no_bespoke_const_module_still_uses_percent_formatting_for_its_guard(sel def test_the_disable_drops_only_where_nothing_else_needs_percent_formatting(self) -> None: """GitHub issue #798's third part, finished by #804: the disable is earned. - All four bespoke const modules carry no ``%``-formatted code, so none + No bespoke const module carries ``%``-formatted code, so none carries a module-level ``consider-using-f-string`` disable. ``pcapkit.const.tcp.flags`` and ``pcapkit.const.reg.apptype.apptype`` are covered by PR #803, which converted their guards -- and, for the @@ -1202,12 +1194,11 @@ def test_the_tcp_flags_template_renders_the_committed_module(self) -> None: NOTE: the class statement names ``EnumRegistry`` and the first method after the enumeration block is a ``classmethod``, because ``tcp/flags`` - is on :class:`~pcapkit.corekit.enum.EnumRegistry` alongside the four - ``mh/*_flag`` templates (GitHub issue #775's tier 2 found its own - exclusion rationale did not hold -- see + is on :class:`~pcapkit.corekit.enum.EnumRegistry` alongside the + ``mh/*_flag`` templates (GitHub issue #775's tier 2 -- see ``tests.const.test_const_registry_protocol``). No generated ``@staticmethod get`` closes the block, leaving ``_missing_`` next, the - same shape ``test_const_enum_lookup.py`` expects for those four. + same shape ``test_const_enum_lookup.py`` expects for those templates. """ import pathlib diff --git a/tests/const/test_const_enum_lookup.py b/tests/const/test_const_enum_lookup.py index eb8a0ba387..a117cee092 100644 --- a/tests/const/test_const_enum_lookup.py +++ b/tests/const/test_const_enum_lookup.py @@ -19,24 +19,23 @@ fail this suite rather than silently reintroducing GitHub issue #492. That sweep deliberately skips :class:`~aenum.IntFlag`, on the grounds that it is -"a different value-lookup contract" -- which is exactly why it did not catch +"a different value-lookup contract" -- which is exactly why it cannot catch GitHub issue #623, a third lookup defect of the same family living in the class -of enum it excluded. The four Mobility Header flag enumerations overrode -``_missing_`` with a body ending in ``return cls(value)``, the same constructor -that had just failed to find the value, so every in-range value that is not -already a member re-entered ``_missing_`` unbounded and raised +of enum it excludes. It guards against the Mobility Header flag enumerations +overriding ``_missing_`` with a body ending in ``return cls(value)``, the same +constructor that has just failed to find the value, under which every in-range +value that is not already a member re-enters ``_missing_`` unbounded and raises :exc:`RecursionError`. Because their members are single bits, the reachable hole -was ``0`` -- no flags set -- and every composite of two defined bits. Overriding -``_missing_`` at all is what caused it: the override shadowed the ``aenum`` -:class:`~aenum.Flag` machinery that resolves precisely those values, which is why -:class:`~pcapkit.const.tcp.flags.Flags`, which at the time defined no -``_missing_``, never had the defect -- and, as GitHub issue #647 then found, had -no range guard either, so it resolved *any* integer and ``Flags(-1)`` read back -as every TCP header flag set at once. It carries the same guard ending in -``return super()._missing_(value)``, which is what keeps issue #623 fixed while -issue #647 is too; :mod:`tests.const.test_const_enum_builtin_parity` is where -that lands. :class:`~pcapkit.const.mh.binding_ack_flag.BindingACKFlag` and its -three siblings end in that same tail, the one +is ``0`` -- no flags set -- and every composite of two defined bits. Such an +override shadows the ``aenum`` :class:`~aenum.Flag` machinery that resolves +precisely those values. Defining no ``_missing_`` at all avoids that, but -- as +GitHub issue #647 finds for :class:`~pcapkit.const.tcp.flags.Flags` -- leaves no +range guard either, so *any* integer resolves and ``Flags(-1)`` reads back as +every TCP header flag set at once. ``Flags`` carries the same guard ending in +``return super()._missing_(value)``, which keeps both issues fixed; +:mod:`tests.const.test_const_enum_builtin_parity` is where that lands. +:class:`~pcapkit.const.mh.binding_ack_flag.BindingACKFlag` and its siblings end +in that same tail, the one :mod:`pcapkit.vendor.default` emits for every other generated enumeration. So this module carries a companion sweep over the :class:`~aenum.IntFlag` classes, covering the contract the :class:`~aenum.IntEnum` sweep declines to. @@ -196,12 +195,10 @@ def test_every_registry_enum_was_discovered(self) -> None: # Pins the size of the sweep itself: if this drifts, a const enum was # added, removed, or renamed, and EXPECTED_TO_REJECT_ZERO (and the # analysis in GitHub issue #492) needs a fresh look rather than a - # silent pass. 112 rather than 111 since GitHub issue #808: dropping - # ``TransportProtocol``'s ``IntFlag`` base moved it from - # ``_iter_const_int_flags`` into this sweep. 114 rather than 112 since - # GitHub issue #880's ngap.ProcedureCode and ngap.ProtocolIE joined - # this sweep, moved onto EnumRegistry from hand-rolled IntEnum classes - # in pcapkit.protocols.application.ngap. + # silent pass. ``TransportProtocol`` has no ``IntFlag`` base (GitHub + # issue #808), so it is in this sweep rather than + # ``_iter_const_int_flags``'s, and so are GitHub issue #880's + # ngap.ProcedureCode and ngap.ProtocolIE, which are on EnumRegistry. names = {f'{obj.__module__}.{obj.__qualname__}' for obj in self.enums} self.assertEqual(len(self.enums), 114) self.assertTrue(EXPECTED_TO_REJECT_ZERO.issubset(names), @@ -245,9 +242,9 @@ class ConstMissingClassmethodTests(unittest.TestCase): aenum invokes lookup failures as ``cls._missing_(value)``. Without ``@classmethod``, the sole positional argument binds to ``cls`` and ``value`` goes unfilled, so *every* call raises :class:`TypeError` instead - of running the intended fallback body -- see GitHub issue #492 (LOW - severity there because, at the time, nothing reachable at runtime ever - triggered the broken path). + of running the intended fallback body -- see GitHub issue #492, which + rates it LOW severity because nothing reachable at runtime triggers the + broken path. """ @@ -314,9 +311,8 @@ def setUpClass(cls) -> None: def test_the_sweep_size_is_pinned(self) -> None: """A flag enum added or removed needs a fresh look, not a silent pass. - 6 rather than 7 since GitHub issue #808: dropping - ``TransportProtocol``'s ``IntFlag`` base moved it out of this sweep and - into ``_iter_const_int_enums`` instead. + ``TransportProtocol`` has no ``IntFlag`` base (GitHub issue #808), so + it is in ``_iter_const_int_enums``'s sweep rather than this one. """ names = {f'{obj.__module__}.{obj.__qualname__}' for obj in self.flags} self.assertEqual(len(self.flags), 6) diff --git a/tests/const/test_const_enum_no_mint.py b/tests/const/test_const_enum_no_mint.py index 36a251da28..7b59f118fc 100644 --- a/tests/const/test_const_enum_no_mint.py +++ b/tests/const/test_const_enum_no_mint.py @@ -31,7 +31,7 @@ registry to every ``_missing_`` calling :func:`~aenum.extend_enum` -- measured by an AST walk over ``pcapkit/const/*.py`` rather than estimated -- the ruling converts them (:data:`RULING_CONVERTED_REGISTRIES` below). -:class:`~pcapkit.const.mh.cga_type.CGAType` keeps minting, for a different +:class:`~pcapkit.const.mh.cga_type.CGAType` mints, for a different reason: its ``Tag_`` mint is not an IANA-style range at all (see its own module for why), so the ruling does not cover it. @@ -125,19 +125,17 @@ own value through :meth:`AppType._dispatch`. None of the tests below pin that, per the same ruling. -GitHub issue #775's final round closes the two registries tier 2 left -*mixed* (keeping some branches minting and converting others): -every one of :class:`~pcapkit.const.reg.ethertype.EtherType`'s -range branches, and :class:`~pcapkit.const.ipx.socket.Socket`'s -``Registered by Xerox`` one, convert to -:meth:`~pcapkit.corekit.enum.EnumRegistry._unregistered_member` too -- PR #878 -scoped it this way: keeping the existing hex-suffixed name, since the +GitHub issue #775 also converts the attributed branches: every one of +:class:`~pcapkit.const.reg.ethertype.EtherType`'s range branches, and +:class:`~pcapkit.const.ipx.socket.Socket`'s ``Registered by Xerox`` one, +return :meth:`~pcapkit.corekit.enum.EnumRegistry._unregistered_member` too -- +PR #878 scoped it this way: keeping the existing hex-suffixed name, since the conversion is about not registering rather than renaming. So each keeps the hex-suffixed name (``Xyplex_0x0888``, not a bare ``Xyplex``) even though it does not register -- both are wholly converted like those in :data:`RULING_CONVERTED_REGISTRIES`, and :class:`EtherTypeMixedMintTests`/:class:`IPXSocketMixedMintTests` below -prove that the probe each once kept minting does not mint. The one +prove that their attributed-vendor probes do not mint. The one consequence worth naming: this reintroduces the exact "manufactured, value-suffixed name" shape :func:`is_manufactured` exists to flag, on calls that are safe because :meth:`_unregistered_member` never registers @@ -345,10 +343,9 @@ def _first_unassigned_value(cls: 'type') -> 'int': #: mint is a bare status word (``Unassigned``, ``Reserved [...]``, ``Reserved #: for Private/Experimental Use``, ``Unspecified in the IANA registry``, and #: similar) rather than a name anyone assigned. Derived from an AST walk over -#: the modules that mint without the fix, including the registries -#: tier 2 left *mixed* until #775's final round (:class:`~pcapkit. -#: const.reg.ethertype.EtherType` and :class:`~pcapkit.const.ipx.socket. -#: Socket` -- see :data:`ETHERTYPE_UNASSIGNED_PROBES` and +#: the modules that mint without the fix, including the attributed branches +#: of :class:`~pcapkit.const.reg.ethertype.EtherType` and +#: :class:`~pcapkit.const.ipx.socket.Socket` (see :data:`ETHERTYPE_UNASSIGNED_PROBES` and #: :data:`IPX_SOCKET_UNASSIGNED_PROBES`). Not listed: the bespoke classes #: GitHub issue #860 converts instead (:class:`~pcapkit.const.reg.apptype. #: apptype.AppType`, :class:`~pcapkit.const.http.status_code.StatusCode`, @@ -445,7 +442,7 @@ def _first_unassigned_value(cls: 'type') -> 'int': #: generic "first branch that converts, by source order" probe can actually #: exercise -- i.e. the registry's own bounds contain at least one value that #: is not already a declared member, *and* nothing earlier in the same -#: ``_missing_`` masks it. One reason holds registries out of this list: 13 +#: ``_missing_`` masks it. One reason holds registries out of this list: some #: have no reachable gap at all -- the same situation :data:`ALL_REGISTRIES` #: already documents for :class:`~pcapkit.const.hip.transport.Transport`: #: every value inside the guard's own bounds names a real member, so @@ -545,9 +542,8 @@ def _first_unassigned_value(cls: 'type') -> 'int': #: "Old Xerox Experimental values. Invalid as an Ethertype since 1983." both #: convert, because the label itself says nothing was assigned. Every *other* #: named block -- companies the historical list attributes a real code range -#: to -- kept minting under that original ruling, and #775's final round -#: converts every remaining branch; :data:`ETHERTYPE_FORMERLY_KEPT_PROBE` pins -#: one (``Xyplex``) as a regression guard that it does not mint either. +#: to -- converts too (#775); :data:`ETHERTYPE_FORMERLY_KEPT_PROBE` pins one +#: (``Xyplex``) as a regression guard that it does not mint. #: #: "Old Xerox Experimental" (0x0101-0x01FF) is a strict subset of the wider #: "IEEE802.3 Length Field" branch (0x0000-0x05DC). :meth:`pcapkit.vendor.reg. @@ -564,23 +560,23 @@ def _first_unassigned_value(cls: 'type') -> 'int': 0x8039: 'DEC_Unassigned', 0x0101: ETHERTYPE_OLD_XEROX_LABEL, } -#: The one probe the original ruling kept minting -- Xyplex, 0x0888 -- because -#: a proprietary protocol has no public name of its own, so the company name -#: serves as one (#775, #847). #775's final round converts it too, preserving -#: the hex-suffixed name exactly as the crawler renders it (PR #878 scoped it -#: this way: keeping the existing hex-suffixed name, since the conversion is -#: about not registering rather than renaming), so this pins the opposite of -#: what its name suggests: that this probe does not mint either. Kept as its +#: An attributed vendor block -- Xyplex, 0x0888 -- whose company name serves as +#: the name of a proprietary protocol that has none of its own (#775, #847). +#: It converts like the unassigned rows, keeping the hex-suffixed name exactly +#: as the crawler renders it (PR #878 scoped it this way: keeping the existing +#: hex-suffixed name, since the conversion is about not registering rather +#: than renaming), so despite its name this pins that the probe does not +#: mint. Kept as its #: own constant, distinct from :data:`ETHERTYPE_UNASSIGNED_PROBES`, because #: :class:`EtherTypeMixedMintTests` below wants it named individually in #: its own regression test. ETHERTYPE_FORMERLY_KEPT_PROBE = (0x0888, 'Xyplex_0x0888') -#: :class:`~pcapkit.const.ipx.socket.Socket` probes for the owner's original -#: #775/#847 ruling: ``Experimental`` and the three dynamically/statically -#: assigned labels convert, as notation for the reader rather than a final -#: concrete assigned name; ``Registered by Xerox``, the company one, kept -#: minting under it, and is pinned by :data:`IPX_SOCKET_FORMERLY_KEPT_PROBE`. +#: :class:`~pcapkit.const.ipx.socket.Socket` probes for the owner's #775/#847 +#: ruling: ``Experimental`` and the dynamically/statically assigned labels +#: convert, as notation for the reader rather than a final concrete assigned +#: name. ``Registered by Xerox``, the company one, is pinned by +#: :data:`IPX_SOCKET_FORMERLY_KEPT_PROBE`. IPX_SOCKET_UNASSIGNED_PROBES = { 0x0025: 'Experimental', 0x4001: 'Dynamically Assigned Socket Numbers', @@ -589,7 +585,7 @@ def _first_unassigned_value(cls: 'type') -> 'int': } #: The mirror of :data:`ETHERTYPE_FORMERLY_KEPT_PROBE` for #: :class:`~pcapkit.const.ipx.socket.Socket`: ``Registered by Xerox`` also -#: converts in #775's final round, keeping its hex-suffixed name. +#: converts (#775), keeping its hex-suffixed name. IPX_SOCKET_FORMERLY_KEPT_PROBE = (0x0010, 'Registered by Xerox_0x0010') @@ -599,7 +595,7 @@ def _first_unregistered_value(cls: 'type') -> 'Optional[int]': pseudo-member, read from the class's own compiled ``_missing_`` by AST rather than hardcoded. - Handles both shapes tier 2 produced: a per-range branch (``if lo <= value + Handles both shapes the generated ``_missing_`` takes: a per-range branch (``if lo <= value <= hi: ... return cls._unregistered_member(...)``), whose lower bound is the probe; and an unconditional mint straight after the guard (no range branch of its own), for which any in-bounds value not already a member @@ -848,8 +844,8 @@ def test_register_actually_mints_on_every_one_of_the_105_registries(self) -> Non outside any registry's assigned codes, so this cannot collide with anything either the CSV seeded or a sibling test minted. - Since #1816, ``register`` refuses a value outside the range a - registry's ``_missing_`` checks, and most bound themselves to 16 bits + ``register`` refuses a value outside the range a registry's + ``_missing_`` checks (#1816), and most bound themselves to 16 bits or fewer. So the fixed value is used where the range reaches it, and elsewhere the highest value in range that no member carries. A registry whose whole range is assigned has nothing left to mint, and @@ -886,10 +882,9 @@ class UnregisteredMemberHelperTests(unittest.TestCase): """:meth:`_unregistered_member` in isolation, on every one of tier 1's registries -- not only those whose own ``_missing_`` happens to call it. It is part of the shared template regardless, so every registry carries - it whether or not its own ``_missing_`` uses it yet (the other 83 keep - their own bespoke ``process()``-driven ``_missing_``, out of tier 1's - scope), and this pins that the helper itself behaves identically on all - of them. + it whether or not its own ``_missing_`` uses it (the rest keep their + own bespoke ``process()``-driven ``_missing_``, out of tier 1's scope), + and this pins that the helper itself behaves identically on all of them. """ @@ -1007,10 +1002,10 @@ def is_manufactured(name_arg: 'Optional[ast.expr]') -> 'bool': than being hardcoded exemptions -- so a combining shape hidden inside either would still be caught. - Round 5's review found that a call's *arguments* need a plainer rule - than its receiver does: a bare ``value``/``key`` passed as data to - ``str(...)``, ``operator.mod(...)``, ``.format_map(...)``, or nested - inside a set/generator/list-comprehension/``map(...)`` argument to + A call's *arguments* need a plainer rule than its receiver does: a bare + ``value``/``key`` passed as data to ``str(...)``, ``operator.mod(...)``, + ``.format_map(...)``, or nested inside a + set/generator/list-comprehension/``map(...)`` argument to ``.join(...)``, is not to be cleared the way a receiver is, since an argument being the value at all is already how each of those shapes glues it to something else. Every argument of any call is therefore walked bluntly for @@ -1023,7 +1018,7 @@ def is_manufactured(name_arg: 'Optional[ast.expr]') -> 'bool': the enum machinery's own attribute rather than through the conventional ``value``/``key`` parameter name, which this function has no way to recognise without hardcoding attribute names as well as - parameter names -- a broader heuristic than round 5 asked for. Nothing + parameter names -- a broader heuristic than the sweep needs. Nothing under :mod:`pcapkit.const` writes a ``name`` argument this way. """ @@ -1059,7 +1054,7 @@ def is_manufactured(name_arg: 'Optional[ast.expr]') -> 'bool': # safe one (a false positive is loud and gets a fixture; a false # negative ships quietly), and no real call site under # ``pcapkit/const``/``pcapkit/vendor`` is affected either way - # (measured at round 5: the sweep stayed at 1012/0). It genuinely + # (measured: the sweep stayed at 1012/0). It genuinely # does over-flag: ``value.upper()`` alone is exempt (it is the # receiver case just above), but the exact same expression as an *argument* -- # ``str(value.upper())``, ``helper(value.upper())`` -- flags, so @@ -1101,27 +1096,26 @@ def is_manufactured(name_arg: 'Optional[ast.expr]') -> 'bool': #: expected)`` where ``source`` is a full ``return cls._unregistered_member( #: value, )`` statement parsed for its own ``name`` argument. IS_MANUFACTURED_FIXTURES = ( - # The manufactured shapes the round-2 review named explicitly -- - # each embeds ``value``, the thing being looked up, into the label. + # The manufactured shapes -- each embeds ``value``, the thing being looked up, into the label. ('percent-s-value', "cls._unregistered_member(value, 'x_%s' % value)", True), ('percent-x-value', "cls._unregistered_member(value, 'x_%x' % value)", True), ('concat-str-value', "cls._unregistered_member(value, 'x_' + str(value))", True), ('format-method-value', "cls._unregistered_member(value, 'x_{}'.format(value))", True), # An f-string embedding value is the same shape as the four above. ('fstring-value', "cls._unregistered_member(value, f'x_{value}')", True), - # Round-3 review's gap: a wrapping call around a combining shape -- - # hoisting a .upper()/.zfill()/str()/.join() outside the %/+/f-string - # must not silently disable the guard. + # A wrapping call around a combining shape -- hoisting a + # .upper()/.zfill()/str()/.join() outside the %/+/f-string -- must not + # silently disable the guard. ('percent-value-then-upper', "cls._unregistered_member(value, ('x_%s' % value).upper())", True), ('percent-value-then-zfill', "cls._unregistered_member(value, ('x_%d' % value).zfill(8))", True), ('str-wrapping-percent', "cls._unregistered_member(value, str('x_%s' % value))", True), ('fstring-then-upper', "cls._unregistered_member(value, f'x_{value}'.upper())", True), ('join-list-with-value', "cls._unregistered_member(value, ''.join(['x_', str(value)]))", True), - # Round-5 review's gap: on the generic call-recursion path, a bare - # ``ast.Name`` argument (as opposed to the outermost expression) was - # treated as exempt the same way ``value`` alone is at the top level -- - # which missed the value being glued in as *data* to a call rather than - # merely transformed. None of these nine exists under pcapkit/const/ + # On the generic call-recursion path, a bare ``ast.Name`` argument (as + # opposed to the outermost expression) is not exempt the way ``value`` + # alone is at the top level -- that would miss the value being glued in + # as *data* to a call rather than merely transformed. None of these + # exists under pcapkit/const/ # (latent, not live), but the census that found them is exactly # what this fixture set exists to keep honest. ('join-set-with-value', "cls._unregistered_member(value, ''.join({'x_', str(value)}))", True), @@ -1167,7 +1161,7 @@ def test_self_check(self) -> None: #: The one call-chain every hex-suffixed manufactured name shares, on every -#: branch #775's final round deliberately kept unrenamed rather than +#: branch #775 deliberately keeps unrenamed rather than #: converting to the bare placeholder every other converted registry uses #: (``Xyplex_0x0888``, ``Registered by Xerox_0x0010``, ...; see that #: sweep's own docstring, and the module docstring's closing paragraph, for @@ -1183,8 +1177,8 @@ def test_self_check(self) -> None: def _is_hex_suffixed_unregistered_name(name_arg: 'Optional[ast.expr]') -> bool: - """Whether ``name_arg`` is *exactly* the one shape #775's final round - preserved unrenamed on :class:`~pcapkit.const.reg.ethertype.EtherType`'s + """Whether ``name_arg`` is *exactly* the one shape #775 preserves + unrenamed on :class:`~pcapkit.const.reg.ethertype.EtherType`'s range branches and :class:`~pcapkit.const.ipx.socket.Socket`'s one -- a ``%``-format :class:`~ast.BinOp` whose left operand is a string constant ending in the literal ``'_0x%s'``, and whose right operand is @@ -1219,8 +1213,8 @@ def _is_hex_suffixed_unregistered_name(name_arg: 'Optional[ast.expr]') -> bool: class UnregisteredMemberNameIsBareTests(unittest.TestCase): """The naming rule settled under GitHub issue #775's tier 1: a non-minting pseudo-member carries the registry's own ``unassigned``/``reserved`` name - bare, with no numeric value appended. Minted members keep their number, - because there it is part of the real name (``Motorola_0x8705`` names one + bare, with no numeric value appended. An attributed vendor block keeps its + number, because there it is part of the real name (``Motorola_0x8705`` names one EtherType inside Motorola's block). Asked whether the non-minting path should honour the registry's own name directly or keep appending the numeric value, the owner chose the bare name -- safe precisely because a @@ -1236,9 +1230,8 @@ class UnregisteredMemberNameIsBareTests(unittest.TestCase): *manufactured* numeric-suffixed label: a bare string constant containing ``%`` (the ``'Unassigned_%d' % value`` mint shape, spelled out as a literal), or an equivalent ``%``-formatted :class:`~ast.BinOp` or - f-string. It therefore covers every call site tier 1's follow-up - touched, every one tier 2's #775/#847 ruling added, and those #860 - step 2's PR 1 added across :class:`~pcapkit.const.http. + f-string. It therefore covers every call site the #775/#847 ruling and + #860's step 2 convert: :class:`~pcapkit.const.http. status_code.StatusCode`, :class:`~pcapkit.const.ftp.return_code. ReturnCode`, :class:`~pcapkit.const.ftp.return_code.ResponseKind`, :class:`~pcapkit.const.ftp.return_code.GroupingInformation`, @@ -1251,10 +1244,9 @@ class UnregisteredMemberNameIsBareTests(unittest.TestCase): those ``__new__``-carrying overrides makes internally -- not a fresh call site with a label of its own, just relaying whatever the true call site already passed, so it is walked and counted here too rather - than specially excluded -- plus the sites round 2 review found still - minting on :class:`OptionType`'s own ``get()`` (its int/namespace path - and its ``str`` path, both independent of ``_missing_``). PR 2 then added - the ``_missing_`` branches on + than specially excluded -- plus the sites on :class:`OptionType`'s own + ``get()`` (its int/namespace path and its ``str`` path, both independent + of ``_missing_``), and the ``_missing_`` branches on :class:`~pcapkit.const.reg.apptype.apptype.AppType` itself, the one more inside its own ``get()``, and the one ``super()._unregistered_member(...)`` forward its own override makes; see @@ -1287,13 +1279,13 @@ class UnregisteredMemberNameIsBareTests(unittest.TestCase): A call site still using ``extend_enum(...)`` instead of ``_unregistered_member(...)`` at all -- :class:`~pcapkit.const.mh. cga_type.CGAType`, plus ``register``/``register_alias`` on every registry - (including :class:`~pcapkit.const.reg.apptype.apptype.AppType`'s own, - added by PR 2) -- is out of scope and untouched by this sweep entirely, + (including :class:`~pcapkit.const.reg.apptype.apptype.AppType`'s own) + -- is out of scope and untouched by this sweep entirely, since it never calls ``_unregistered_member`` in the first place; minting there is the explicit, caller-named path the sweep is not about. - GitHub issue #775's final round converts the last minting branches -- - every one of :class:`~pcapkit.const.reg.ethertype.EtherType`'s and + GitHub issue #775 converts the attributed branches too -- every one of + :class:`~pcapkit.const.reg.ethertype.EtherType`'s and :class:`~pcapkit.const.ipx.socket.Socket`'s one -- and deliberately keeps each one's hex-suffixed name unchanged (PR #878 scoped it this way: keeping the existing hex-suffixed name, since the conversion is about not @@ -1344,12 +1336,10 @@ def test_every_unregistered_member_call_passes_a_non_manufactured_name(self) -> offenders.append(f'{path.relative_to(repo_root)}:{node.lineno}: {segment}') # Sanity: the sweep itself must actually be exercising something. - # The floor is the call-site total when #775's final round landed -- - # the sites tier 1's follow-up touched, those tier 2's #775/#847 - # ruling added, those #860 step 2's PR 1 added (true call sites, each - # override's own ``super()`` forward, and OptionType.get()'s own - # paths), AppType's from PR 2, and that final round's on EtherType - # and Socket -- so a walk that silently stops finding sites fails. + # The floor is the call-site total of the #775/#847 and #860 + # conversions -- true call sites, each override's own ``super()`` + # forward, OptionType.get()'s own paths, AppType's, and EtherType's + # and Socket's -- so a walk that silently stops finding sites fails. self.assertGreaterEqual(call_count, 1065, f'expected at least 1065 _unregistered_member call sites, found {call_count}') # And that the exemption is actually earning its keep, rather than a @@ -1418,17 +1408,16 @@ def test_repeated_lookup_does_not_grow_members(self) -> None: self.assertIsNot(first, second) -#: Registries whose ``_missing_`` legitimately keeps minting on some branches -#: (real attributed names), so the blanket "no extend_enum left" sweep below -#: does not apply to them. Empty: GitHub issue #775's final round converts -#: every branch on :class:`~pcapkit.const.reg.ethertype.EtherType` and -#: :class:`~pcapkit.const.ipx.socket.Socket`, the two tier 2 left *mixed*. -#: Kept as an empty-by-construction frozenset so the two subtractions below +#: Registries whose ``_missing_`` legitimately mints on some branches (real +#: attributed names), so the blanket "no extend_enum left" sweep below does +#: not apply to them. Empty: every branch on +#: :class:`~pcapkit.const.reg.ethertype.EtherType` and +#: :class:`~pcapkit.const.ipx.socket.Socket` converts, attributed ones +#: included (#775). Kept as an empty-by-construction frozenset so the two subtractions below #: stay self-documenting about *why* nothing is excluded, and so a future #: registry that becomes genuinely mixed has a named place to be added. #: :class:`EtherTypeMixedMintTests` and :class:`IPXSocketMixedMintTests` prove -#: the probe each once kept minting does not mint either, alongside the rows -#: that never did. +#: their attributed-vendor probes do not mint, alongside the unassigned rows. _MIXED_REGISTRIES = frozenset() # type: frozenset[tuple[str, str]] @@ -1438,8 +1427,8 @@ class RulingConversionSourceTests(unittest.TestCase): so no lookup can exercise the branch) -- and, as a completeness check, for the wholly-converted registries: none of their ``_missing_`` bodies may call :func:`~aenum.extend_enum`. - :data:`_MIXED_REGISTRIES` is empty, #775's final round having - converted both, so nothing is excluded here -- see its own docstring.""" + :data:`_MIXED_REGISTRIES` is empty, so nothing is excluded here -- see + its own comment.""" def test_no_reachable_gap_registries_no_longer_call_extend_enum(self) -> None: # EtherType belongs to RULING_CONVERTED_WITH_REACHABLE_GAP: its Old @@ -1471,8 +1460,8 @@ def test_every_wholly_converted_registry_no_longer_calls_extend_enum(self) -> No self.assertNotIn('extend_enum', source) def test_ethertype_and_socket_no_longer_call_extend_enum_either(self) -> None: - """The two registries tier 2 left mixed: GitHub issue #775's final - round leaves no ``extend_enum`` call on either, the same invariant + """EtherType and Socket carry no ``extend_enum`` call either (#775), + the same invariant :meth:`test_every_wholly_converted_registry_no_longer_calls_extend_enum` proves for the rest -- named separately because Socket is not a member of :data:`RULING_CONVERTED_REGISTRIES`, so the loop above alone @@ -1488,13 +1477,12 @@ def test_ethertype_and_socket_no_longer_call_extend_enum_either(self) -> None: class EtherTypeMixedMintTests(unittest.TestCase): - """:class:`~pcapkit.const.reg.ethertype.EtherType` is tier 2's mixed - case: tier 2's ruling converts ``DEC Unassigned`` and the "Old Xerox - Experimental..." row and leaves every other attributed vendor block (a - real company's own name for its own code) minting. GitHub issue #775's - final round converts those too, so no branch mints -- ``EtherType(0x0888)`` - resolves to an unregistered ``Xyplex_0x0888`` -- and the class keeps its - tier-2 name. :meth:`test_formerly_attributed_vendor_block_ + """:class:`~pcapkit.const.reg.ethertype.EtherType`'s unassigned rows and + attributed vendor blocks. The #775/#847 ruling converts ``DEC + Unassigned`` and the "Old Xerox Experimental..." row, and #775 converts + every attributed vendor block (a real company's own name for its own + code) too, so no branch mints -- ``EtherType(0x0888)`` resolves to an + unregistered ``Xyplex_0x0888``. :meth:`test_formerly_attributed_vendor_block_ no_longer_mints` below pins that one named probe (``Xyplex``).""" def setUp(self) -> None: @@ -1516,13 +1504,12 @@ def test_unassigned_rows_do_not_mint(self) -> None: self.assertNotIn(value, EtherType._value2member_map_) # type: ignore[attr-defined] def test_formerly_attributed_vendor_block_no_longer_mints(self) -> None: - """``Xyplex`` (0x0888), the real attributed name tier 2's ruling - *kept* minting: GitHub issue #775's final round converts it, - preserving the hex-suffixed name exactly as the crawler renders it -- - PR #878 scoped it this way: keeping the existing hex-suffixed name, - since the conversion is about not registering rather than renaming. - Same shape as :meth:`test_unassigned_rows_do_not_mint` above, for the - one probe tier 2 kept as the exception.""" + """``Xyplex`` (0x0888), a real attributed name, does not mint (#775): + it resolves to an unregistered member with the hex-suffixed name + exactly as the crawler renders it -- PR #878 scoped it this way: + keeping the existing hex-suffixed name, since the conversion is about + not registering rather than renaming. Same shape as + :meth:`test_unassigned_rows_do_not_mint` above.""" from pcapkit.const.reg.ethertype import EtherType value, name = ETHERTYPE_FORMERLY_KEPT_PROBE @@ -1542,12 +1529,11 @@ def test_formerly_attributed_vendor_block_no_longer_mints(self) -> None: class IPXSocketMixedMintTests(unittest.TestCase): - """:class:`~pcapkit.const.ipx.socket.Socket` is tier 2's other mixed - case: tier 2's ruling converts ``Experimental`` and the three - dynamically/statically assigned labels and leaves ``Registered by - Xerox``, the company one, minting. GitHub issue #775's final round - converts it too, so ``Socket(0x0010)`` resolves to an unregistered - ``Registered by Xerox_0x0010``; see + """:class:`~pcapkit.const.ipx.socket.Socket`'s unassigned rows and its + attributed one. The #775/#847 ruling converts ``Experimental`` and the + dynamically/statically assigned labels, and #775 converts ``Registered + by Xerox``, the company one, too, so ``Socket(0x0010)`` resolves to an + unregistered ``Registered by Xerox_0x0010``; see :meth:`test_registered_by_xerox_no_longer_mints` below.""" def setUp(self) -> None: @@ -1569,10 +1555,10 @@ def test_unassigned_rows_do_not_mint(self) -> None: self.assertNotIn(value, Socket._value2member_map_) # type: ignore[attr-defined] def test_registered_by_xerox_no_longer_mints(self) -> None: - """``Registered by Xerox`` (0x0010), the company name tier 2's ruling - *kept* minting: GitHub issue #775's final round converts it, - preserving the hex-suffixed name exactly as the crawler renders it -- - same ruling, same reasoning as + """``Registered by Xerox`` (0x0010), the company name, does not mint + (#775): it resolves to an unregistered member with the hex-suffixed + name exactly as the crawler renders it -- same ruling, same reasoning + as :meth:`EtherTypeMixedMintTests. test_formerly_attributed_vendor_block_no_longer_mints`.""" from pcapkit.const.ipx.socket import Socket @@ -1630,7 +1616,7 @@ def setUp(self) -> None: reimport_once_per_class(self, restore=True) def test_unassigned_value_resolves_without_minting(self) -> None: - """Swept across all 5: resolves, does not mint, repeated lookup is + """Swept across every one of them: resolves, does not mint, repeated lookup is equal but not identical -- the same shape as :class:`UnassignedRangeDoesNotMintTests` above, generalised to registries whose ``_unregistered_member`` is not the generic one.""" @@ -1673,10 +1659,10 @@ def test_statuscode_unregistered_member_displays_correctly(self) -> None: self.assertEqual(str(member), '[105] Unassigned') def test_statuscode_every_unassigned_range_resolves_without_minting(self) -> None: - """All 8 of :meth:`~pcapkit.const.http.status_code.StatusCode. + """Every one of :meth:`~pcapkit.const.http.status_code.StatusCode. _missing_`'s ``if`` branches, not just the first -- each is its own source line and its own converted call, so covering only one leaves - seven untested.""" + the rest untested.""" from pcapkit.const.http.status_code import StatusCode for value in (105, 209, 227, 309, 419, 432, 452, 512): @@ -1770,7 +1756,7 @@ def test_optiontype_members_ns_is_not_corrupted(self) -> None: def test_optiontype_get_int_path_no_longer_mints(self) -> None: """Guards against :meth:`OptionType.get` minting directly on a miss (bypassing ``_missing_`` entirely) -- a second, independent mint - site, on the wire-facing path round 2 review measured live + site, on the wire-facing path (:meth:`pcapkit.protocols.misc.pcapng.PCAPNG._make_pcapng_options` calls ``get()`` with raw option-code bytes). The owner's ruling names ``get`` explicitly -- the same reason @@ -1843,27 +1829,26 @@ def test_optiontype_get_str_path_no_longer_mints(self) -> None: class BespokeOpenVocabularyUnmintConvertedTests(unittest.TestCase): - """The 3 open-vocabulary ``StrEnum`` classes -- unlike every other unmint - branch converted above or on tier 2, none of these three minted a - synthetic numeric placeholder under a procedural label - (``Unassigned_%d``, ``Unknown_%d``); each minted the literal, exact - string it was asked to resolve, as its own name. That initially read as - a case for keeping them minting (the label was never manufactured), but - GitHub issue #860 settled it the other way: ``get()`` does not mint, + """The open-vocabulary ``StrEnum`` classes -- unlike every other unmint + branch above or of tier 2, a miss here would not mint a synthetic + numeric placeholder under a procedural label (``Unassigned_%d``, + ``Unknown_%d``); it would mint the literal, exact string it was asked to + resolve, as its own name. That reads as a case for minting (the label is + not manufactured), but GitHub issue #860 settles it the other way: ``get()`` does not mint, because only ``register()`` creates a new registry entry, and for these registries only IANA-registered values are legitimate -- ``get()`` simply does not have enough information to build a new one. Concretely: :class:`~pcapkit.const.ftp.command.Command` needs ``feat``/``desc``/``type``/``conf`` and :class:`~pcapkit.const.http.method.Method` needs ``safe``/``idempotent``, neither of which a bare wire string carries, so - minting registers a permanently hollowed-out member for each. + minting would register a permanently hollowed-out member for each. :class:`~pcapkit.const.ftp.command.FEATCode` has no custom ``__new__`` - at all and needed no override. + at all and needs no override. Both ``_missing_`` and each class's own ``get()`` (a *second*, independent mint site bypassing ``_missing_`` entirely, the same shape - as :class:`~pcapkit.const.pcapng.option_type.OptionType`'s) are - converted, since the owner's reasoning names ``get`` explicitly and + as :class:`~pcapkit.const.pcapng.option_type.OptionType`'s) refuse to + mint, since the owner's reasoning names ``get`` explicitly and leaving it minting while ``_missing_`` does not would be a direct contradiction. ``Command``/``Method``'s ``_unregistered_member`` override canonicalises the member's *name* to upper case and keeps the @@ -2447,9 +2432,9 @@ def test_get_second_mint_site_no_longer_mints(self) -> None: ``_missing_``'s ranges either, so it reaches :meth:`AppType. get`'s own, second, independent mint site -- distinct from the one inside ``_missing_``, and easy to miss when - converting only the latter (exactly what round 2 review of #860 step - 2 PR 1 caught on :class:`~pcapkit.const.pcapng.option_type. - OptionType` for the same reason).""" + converting only the latter (the same second mint site + :class:`~pcapkit.const.pcapng.option_type.OptionType`'s own ``get()`` + carries).""" from pcapkit.const.reg.apptype.tcp import TCP before = len(TCP.__members__) diff --git a/tests/const/test_const_ethertype_862_unit.py b/tests/const/test_const_ethertype_862_unit.py index 4c5cecf13b..ecccebd951 100644 --- a/tests/const/test_const_ethertype_862_unit.py +++ b/tests/const/test_const_ethertype_862_unit.py @@ -61,7 +61,7 @@ #: on 2026-09-27). Row order matches the live table: the wide IEEE802.3 range #: first, the narrower Old Xerox range it contains second, then the #: unrelated Xyplex range -- exactly the order that reproduces the defect -#: against unfixed code. +#: without the fix. ETHERTYPE_862_FIXTURE_CSV = [ 'Ethertype (decimal),Ethertype (hex),Exp. Ethernet (decimal),Exp. Ethernet (octal),Description,Reference', '0000-1500,0000-05DC,-,-,IEEE802.3 Length Field,[IEEE Std 802.3]', @@ -167,10 +167,10 @@ def test_both_ranges_still_render_with_their_own_body(self) -> None: # Reordering must not lose either range or swap their bodies. # # NOTE: the second assertion checks for the ``_unregistered_member`` - # form, not ``extend_enum``: #775's final round converts every range - # branch on this crawler -- see + # form, not ``extend_enum``: every range branch on this crawler + # resolves through ``_unregistered_member`` (#775) -- see # tests/const/test_const_enum_no_mint.py's own docstring. The - # hex-suffixed name is kept, because PR #878 scoped that conversion + # hex-suffixed name is kept, because PR #878 scopes that conversion # as not registering rather than renaming. _enum, miss = self._process_fixture() text = '\n'.join(miss) @@ -203,11 +203,11 @@ def setUp(self) -> None: def test_nested_ranges_are_ordered_narrowest_first(self) -> None: """Regression pin against two specific wrong implementations of "insert ahead of a containing range" -- not a general correctness - proof for arbitrary nesting depth or arrival order. That exhaustive + proof for arbitrary nesting depth or arrival order. An exhaustive proof (every arrival order at 3 and 4 levels, plus randomised - fuzzing) was done separately in the review of PR #865, #862's fix, - and does not live in this repository as a test; this method only - pins the two specific shortcuts that review considered and rejected. + fuzzing) does not live in this repository as a test (see PR #865, + #862's fix); this method only pins the two specific shortcuts PR #865 + considers and rejects. Against these three nested ranges (OUTER contains MIDDLE contains INNER), the correct result is always ``[INNER, MIDDLE, OUTER]``, but diff --git a/tests/const/test_const_ftp_command_get_key_type_1824_unit.py b/tests/const/test_const_ftp_command_get_key_type_1824_unit.py index d3a29e42d1..86817316d1 100644 --- a/tests/const/test_const_ftp_command_get_key_type_1824_unit.py +++ b/tests/const/test_const_ftp_command_get_key_type_1824_unit.py @@ -1,14 +1,13 @@ # -*- coding: utf-8 -*- """``Command.get`` refuses a non-``str`` key the way ``Method.get`` does. -GitHub issue #1824. :meth:`~pcapkit.const.ftp.command.Command.get` was a -:class:`staticmethod` that called ``key.upper()`` on whatever it was given, so -``Command.get(b'\\x01')`` built an unregistered member keyed by the -:class:`bytes` object, and ``Command.get(-1)`` raised a builtin -:exc:`AttributeError` whether or not ``default`` was supplied. -:meth:`~pcapkit.const.http.method.Method.get` had the same shape until #908, -which made it a :class:`classmethod` delegating to -:meth:`~pcapkit.corekit.enum.EnumLookup.get`. +GitHub issue #1824. Guards against :meth:`~pcapkit.const.ftp.command.Command.get` +being a :class:`staticmethod` that calls ``key.upper()`` on whatever it is +given, under which ``Command.get(b'\\x01')`` builds an unregistered member keyed by the +:class:`bytes` object, and ``Command.get(-1)`` raises a builtin +:exc:`AttributeError` whether or not ``default`` is supplied. +:meth:`~pcapkit.const.http.method.Method.get` is a :class:`classmethod` +delegating to :meth:`~pcapkit.corekit.enum.EnumLookup.get` (#908). ``Command.get`` follows it: a non-``str`` key goes to the base, whose call to ``Command._validate_value`` (added by #1823) raises diff --git a/tests/const/test_const_ftp_featcode_case_903_unit.py b/tests/const/test_const_ftp_featcode_case_903_unit.py index 4233d80cb7..d20b771033 100644 --- a/tests/const/test_const_ftp_featcode_case_903_unit.py +++ b/tests/const/test_const_ftp_featcode_case_903_unit.py @@ -89,11 +89,11 @@ from pcapkit.const.ftp.command import Command, FEATCode from pcapkit.const.pcapng.tls_key_label import TLSKeyLabel -#: The 5 placeholder ("pseudo FEAT code") members, registered in lower case +#: The placeholder ("pseudo FEAT code") members, registered in lower case #: per RFC 5797 Section 2, with the ``<...>`` value form the crawler emits. PSEUDO_CODES = ('base', 'hist', 'secu', 'feat', 'nat6') -#: The 10 genuine FEAT keywords, registered in upper case per the same section. +#: The genuine FEAT keywords, registered in upper case per the same section. REAL_CODES = ('AUTH', 'HOST', 'UTF8', 'MDTM', 'MLST', 'PBSZ', 'PROT', 'REST', 'SIZE', 'TVFS') @@ -114,7 +114,7 @@ def test_a_lower_case_placeholder_resolves_from_upper_case(self) -> 'None': self.assertIs(FEATCode.get(key), FEATCode.base) # type: ignore[attr-defined] def test_every_placeholder_resolves_from_either_case(self) -> 'None': - """All 5 of them, not just the one the issue names.""" + """All of them, not just the one the issue names.""" for name in PSEUDO_CODES: member = FEATCode[name] for key in (name, name.upper(), name.capitalize()): @@ -155,8 +155,8 @@ def test_get_all_inherits_the_fold(self) -> 'None': (FEATCode.AUTH,)) # type: ignore[attr-defined] def test_exact_hits_are_unchanged_for_every_member(self) -> 'None': - """The fold is a fallback: an exact name or value still resolves the - way it did before, through the base, for all 15 members.""" + """The fold is a fallback: an exact name or value resolves through + the base, for every member.""" self.assertEqual(len(FEATCode._member_names_), 15) for member in FEATCode: with self.subTest(member=member.name): diff --git a/tests/const/test_const_linktype_209_unit.py b/tests/const/test_const_linktype_209_unit.py index ad99cf3bb4..381095f7de 100644 --- a/tests/const/test_const_linktype_209_unit.py +++ b/tests/const/test_const_linktype_209_unit.py @@ -1,44 +1,41 @@ # -*- coding: utf-8 -*- """``LinkType(209)`` must resolve to the current name, not the legacy one. -GitHub issue #844: :mod:`pcapkit.const.reg.linktype` declared two members for -value ``209`` -- ``IPMB_LINUX`` first, ``I2C_LINUX`` second -- so +GitHub issue #844: guards against :mod:`pcapkit.const.reg.linktype` declaring +``IPMB_LINUX`` ahead of ``I2C_LINUX`` for value ``209``, under which :class:`~aenum.IntEnum`'s "first member defined for a value is canonical" rule -made ``LinkType(209).name`` resolve to ``'IPMB_LINUX'``, the name tcpdump's own +makes ``LinkType(209).name`` resolve to ``'IPMB_LINUX'``, the name tcpdump's own registry (and the generated comment above it) labels "Legacy names (do not -use)". Traced to commit ``cfdf2ab5c7`` (2024-05-04): a regeneration inserted -the legacy row above the current one, because tcpdump's +use)". Traced to commit ``cfdf2ab5c7`` (2024-05-04), a regeneration that +inserted the legacy row above the current one, because tcpdump's ``http://www.tcpdump.org/linktypes.html`` table lists them in exactly that order -- ``LINKTYPE_IPMB_LINUX`` (209, marked legacy in its notes column) ahead of ``LINKTYPE_I2C_LINUX`` (209, the current name). 209 is the *only* value the table double-assigns, and that legacy note is the only occurrence of -the word "legacy" anywhere in it (measured directly against a live fetch while -fixing this). +the word "legacy" anywhere in it (measured directly against a live fetch). -The fix is in the generator, :mod:`pcapkit.vendor.reg.linktype`, not in the -generated file: :meth:`~pcapkit.vendor.reg.linktype.LinkType.process` -sinks any row whose notes column mentions "legacy" into a bucket appended +The generator, :mod:`pcapkit.vendor.reg.linktype`, holds the ordering rather +than the generated file: :meth:`~pcapkit.vendor.reg.linktype.LinkType.process` +sinks a row whose notes column mentions "legacy" into a bucket appended *after* every other row, so the current name is always the first one defined for a shared value regardless of the source table's own row order. -GitHub issue #852, a follow-up to the cross-review of #848, pointed out that -the original predicate tested the notes column for the word "legacy" alone, -with no check that the row's value was actually claimed by another row -- -meaning a future *current* row whose notes happened to mention "legacy" about -some unrelated code would be wrongly sunk, and a single ``USER0``-style range -row would sink all sixteen expanded members at once instead of none. +GitHub issue #852 (from the cross-review of #848) adds that a predicate testing +the notes column for the word "legacy" alone, with no check that the row's value +is actually claimed by another row, would wrongly sink a future *current* row +whose notes happen to mention "legacy" about some unrelated code, and would sink +all sixteen expanded members of a single ``USER0``-style range row at once +instead of none. :meth:`~pcapkit.vendor.reg.linktype.LinkType.process` only sinks a single-value row when its value is a genuine duplicate elsewhere in the same table, and never routes a range row through the sink at all. -The cross-review of #852 itself then caught a second-order regression: the -first cut of the duplicate-value pre-scan counted only single-value rows, so a -value duplicated *across a range boundary* -- a single-value row sharing a -value with one member of a ``USER0``-style range -- was invisible to it, and -that row's legacy alias was not sunk even when correctly worded. The -pre-scan expands ``a–b`` ranges the same way the main loop does -before counting, so no duplicate is missed regardless of which branch the -value comes from. +A duplicate-value pre-scan counting only single-value rows would miss a value +duplicated *across a range boundary* -- a single-value row sharing a value with +one member of a ``USER0``-style range -- and leave that row's legacy alias +unsunk even when correctly worded. The pre-scan expands ``a–b`` ranges the +same way the main loop does before counting, so no duplicate is missed +regardless of which branch the value comes from. This module pins both the symptom and the root cause: @@ -48,7 +45,7 @@ - :class:`LinkTypeGeneratorLegacyOrderingTests` -- against :meth:`~pcapkit.vendor.reg.linktype.LinkType.process` directly, fed a two-row fixture reproducing tcpdump's own HTML for value 209 byte-for-byte - (captured from a live fetch while diagnosing this issue). Needs no network: + (captured from a live fetch). Needs no network: the fixture *is* the table row, not a fetch of it, so this stays reliable in CI while still exercising the exact code path a real crawl runs. A regeneration that drops the fix reintroduces the wrong order here before it @@ -59,8 +56,8 @@ one regardless of which table position it occupies. - :class:`LinkTypeGeneratorRangeSinkIsolationTests` -- issue #852 item 2: a single range row's wording must never sink its whole expansion. -- :class:`LinkTypeGeneratorCrossRangeDuplicateTests` -- the #852 cross-review - regression: a single-value row's legacy wording must still be honoured when +- :class:`LinkTypeGeneratorCrossRangeDuplicateTests` -- #852's cross-range + case: a single-value row's legacy wording must still be honoured when the value it duplicates belongs to a range row, not another single-value one. @@ -84,7 +81,7 @@ #: ``dark={'test': ('html5lib',), 'gate': ('html5lib',)}``. #: #: ``html5lib`` is the one that is actually missing on ``test``: ``requests`` and -#: ``bs4`` have shipped in the ``test`` extra since #507, but only +#: ``bs4`` are in the ``test`` extra (#507), but only #: ``beautifulsoup4[html5lib]`` -- the ``vendor`` and ``dev`` extras -- provides a #: parser (their entries in :file:`pyproject.toml`, and the ``test`` extra's comment #: on ``beautifulsoup4`` saying so deliberately). @@ -432,7 +429,7 @@ def test_legacy_worded_range_row_still_lands_entirely_in_enum(self) -> None: #: A single-value legacy row for 900, followed by a range row that expands to #: include 900 among its members (``USER0``..``USER3`` == 900-903). This is -#: the #852 cross-review's own repro: value 900 is a genuine duplicate, but +#: #852's cross-range case: value 900 is a genuine duplicate, but #: only because one of its two occurrences comes from a *range* expansion -- #: a duplicate-value pre-scan that counts only single-value rows never sees #: it, so ``OLDUSER``'s "legacy" wording is silently ignored and the @@ -462,19 +459,18 @@ def test_legacy_worded_range_row_still_lands_entirely_in_enum(self) -> None: @unittest.skipUnless(HAS_VENDOR_DEPS, 'requests, bs4 and/or html5lib not installed') class LinkTypeGeneratorCrossRangeDuplicateTests(unittest.TestCase): - """The #852 cross-review's regression: a value duplicated *across* a range boundary. + """#852's cross-range case: a value duplicated *across* a range boundary. - The first cut of the duplicate-value pre-scan built ``dup_values`` from + Guards against a duplicate-value pre-scan that builds ``dup_values`` from only the single-value rows (filtering the notes column's number on - ``str.isdigit()``), so a range row's expanded values never entered the - count. A single-value row sharing one of *those* values -- ``900``, + ``str.isdigit()``), under which a range row's expanded values never enter + the count. A single-value row sharing one of *those* values -- ``900``, shared here with the first member of a ``USER0``..``USER3`` range - covering 900-903 -- was therefore never recognised as a duplicate, and - its correctly-worded "legacy" notes were silently ignored: it landed in + covering 900-903 -- is then never recognised as a duplicate, and its + correctly-worded "legacy" notes are silently ignored: it lands in ``enum`` instead of ``legacy``, ahead of the range it duplicates, making - the deprecated ``OLDUSER`` canonical for value 900 instead of ``USER0``. - That is exactly the failure mode #844 and #852 both exist to close, - reopened by narrowing the predicate. :meth:`process`'s pre-scan + the deprecated ``OLDUSER`` canonical for value 900 instead of ``USER0`` + -- exactly the failure mode #844 and #852 exist to close. :meth:`process`'s pre-scan expands ``a–b`` ranges the same way the main loop does before counting, so this duplicate is caught regardless of which branch either occurrence comes from. @@ -573,23 +569,22 @@ def _signed_or_underscored_duplicate_table_html(legacy_number: str, current_numb @unittest.skipUnless(HAS_VENDOR_DEPS, 'requests, bs4 and/or html5lib not installed') class LinkTypeGeneratorSignedOrUnderscoredValueTests(unittest.TestCase): - """The second #852 cross-review's regression: ``_expand``'s recognition set was too narrow. + """#852's signed and underscored values: ``_expand`` must recognise what the main loop does. - The pre-scan's single-value branch tested ``str.isdigit()``, which is - narrower than the main loop's own acceptance test, ``int(temp)`` -- + Guards against the pre-scan's single-value branch testing ``str.isdigit()``, + which is narrower than the main loop's own acceptance test, ``int(temp)`` -- ``int()`` also accepts a leading sign (``+209``, ``-5``) and PEP 515 underscore grouping (``2_09``). A value the main loop accepts but ``_expand`` does not is invisible to ``dup_values``, so a correctly "legacy"-worded row sharing that value silently stops being sunk -- the - same class of under-count the range-expansion fix above closes, just for - a different reason ``_expand`` can fail to recognise a value. + same class of under-count as the cross-range case above, just for a + different reason ``_expand`` can fail to recognise a value. Not reachable against tcpdump's table as vendored here: every committed member matches a plain unsigned decimal, so this is a latent - hole rather than a live defect, exactly as #844 and #852's original - word-only rule was. :meth:`process`'s ``_expand`` tries ``int(temp)`` - directly, before the en-dash range check, so it recognises the same - values the main loop does. + hole rather than a live defect, as #852's word-only rule is. + :meth:`process`'s ``_expand`` tries ``int(temp)`` directly, before the + en-dash range check, so it recognises the same values the main loop does. """ def setUp(self) -> None: diff --git a/tests/const/test_const_method_case_sensitive_896_unit.py b/tests/const/test_const_method_case_sensitive_896_unit.py index 77362c5dfe..aaf5c6b6ff 100644 --- a/tests/const/test_const_method_case_sensitive_896_unit.py +++ b/tests/const/test_const_method_case_sensitive_896_unit.py @@ -174,7 +174,7 @@ def test_get_lowercase_does_not_resolve_to_the_standard_member(self) -> None: """The issue's own repro: ``Method.get('get')`` must stop resolving to :attr:`Method.GET`. - Fails against the pre-fix override (``name = key.upper()`` folded + Fails against an override that folds case (``name = key.upper()`` folded into the ``_member_map_`` membership test): there, ``Method.get('get') is Method.GET`` is :data:`True`. """ diff --git a/tests/const/test_const_method_value_lookup_908_unit.py b/tests/const/test_const_method_value_lookup_908_unit.py index 238849f74f..dc04092b84 100644 --- a/tests/const/test_const_method_value_lookup_908_unit.py +++ b/tests/const/test_const_method_value_lookup_908_unit.py @@ -113,9 +113,9 @@ def test_value_lookup_stays_case_sensitive(self) -> None: def test_an_exact_name_match_still_wins_and_ignores_a_supplied_default(self) -> None: """The base's own precedence (name before value) survives the - delegation, and ``default`` is never consulted on a hit -- matching - the pre-#908 behaviour where a resolved ``key`` ignored ``default`` - entirely.""" + delegation, and ``default`` is never consulted on a hit -- a resolved + ``key`` ignores ``default`` entirely, with or without #908's + delegation.""" from pcapkit.const.http.method import Method self.assertIs(Method.get('GET', default='IGNORED'), Method.GET) @@ -154,8 +154,9 @@ def test_unregistered_member_keeps_the_callers_own_casing(self) -> None: class DefaultSignatureTests(unittest.TestCase): - """``default`` keeps its pre-#908 meaning rather than widening to the - base's ``NO_DEFAULT`` sentinel -- see the module docstring's reasoning.""" + """``default`` keeps its own meaning, the unregistered member's value, + rather than widening to the base's ``NO_DEFAULT`` sentinel -- see the + module docstring's reasoning.""" def test_default_still_names_the_unregistered_members_value(self) -> None: """Omitted or ``None``, ``default`` uses ``key`` itself; supplied, @@ -289,7 +290,7 @@ def test_the_docstring_documents_the_value_error(self) -> None: # NOTE: ``inspect.getsource(Method.get)`` rather than # ``Method.get.__func__``, which a ``staticmethod`` does not carry -- - # so against the pre-#908 module this would die with + # so against a ``staticmethod`` ``get`` (#908) this would die with # :exc:`AttributeError` before reaching a single assertion, and would # be pinning ``classmethod``-ness (already pinned by # :class:`DefaultSignatureTests`) rather than the docstring it is diff --git a/tests/const/test_const_mh_lla_code_1848_unit.py b/tests/const/test_const_mh_lla_code_1848_unit.py index 913eb09c1d..fb08df287b 100644 --- a/tests/const/test_const_mh_lla_code_1848_unit.py +++ b/tests/const/test_const_mh_lla_code_1848_unit.py @@ -1,17 +1,17 @@ # -*- coding: utf-8 -*- """``LLACode`` names codes ``0`` and ``2`` as :rfc:`5568#section-6.4.3` does. -GitHub issue #1848. Code ``0`` was misspelt ``Wilcard``, and code ``2``, the -*Link-Layer Address of the MN*, was named ``MH``. Both are renamed, to -``Wildcard`` and ``MN``, and neither old name survives as an alias, as the -renames of #1090 and #1715 left none. +GitHub issue #1848. Code ``0`` is ``Wildcard``, which the module misspelt +``Wilcard``, and code ``2``, the *Link-Layer Address of the MN*, is ``MN``, +which it named ``MH``. Neither misspelling is an alias, as #1090 and #1715 +keep no alias for a renamed member either. """ import unittest class LLACodeRenameTests(unittest.TestCase): - """The RFC's names resolve both ways, and the shipped names are gone.""" + """The RFC's names resolve both ways, and the misspellings resolve neither.""" def test_code_0_is_wildcard(self) -> 'None': from pcapkit.const.mh.lla_code import LLACode diff --git a/tests/const/test_const_register_validates_every_registry_1816_unit.py b/tests/const/test_const_register_validates_every_registry_1816_unit.py index eb30850916..de90c41a38 100644 --- a/tests/const/test_const_register_validates_every_registry_1816_unit.py +++ b/tests/const/test_const_register_validates_every_registry_1816_unit.py @@ -2,10 +2,11 @@ """Every registry's ``register`` refuses what its ``get`` refuses. GitHub issue #1816. :meth:`~pcapkit.corekit.enum.EnumRegistry.register` routes a -new value through :meth:`~pcapkit.corekit.enum.EnumLookup._validate_value`, but -only ``AppType`` (#1792) overrode that hook, so everywhere else ``register`` -minted a value ``get`` refuses: ``Hardware.register(70000, 'x')`` minted, and -``Hardware.get(70000)`` then returned the new member. Every registry overrides +new value through :meth:`~pcapkit.corekit.enum.EnumLookup._validate_value`. It +guards against only ``AppType`` (#1792) overriding that hook, under which +``register`` everywhere else mints a value ``get`` refuses: +``Hardware.register(70000, 'x')`` mints, and ``Hardware.get(70000)`` then +returns the new member. Every registry overrides the hook. A generated one uses the check its ``_missing_`` makes; ``TLSKeyLabel`` and ``ExtensionHeader`` have no ``_missing_``, so they check the member type and the 8-bit Next Header field respectively. diff --git a/tests/const/test_const_registry_protocol.py b/tests/const/test_const_registry_protocol.py index 235d1b137b..1c40f2da53 100644 --- a/tests/const/test_const_registry_protocol.py +++ b/tests/const/test_const_registry_protocol.py @@ -38,21 +38,19 @@ #: Repository root, for reading generated sources as text. REPO_ROOT = pathlib.Path(__file__).resolve().parents[2] -#: The batch converted onto :class:`~pcapkit.corekit.enum.EnumRegistry`: the -#: six bespoke-template registries whose members carry no extra attributes, so -#: the shared ``_unregistered_member`` -- which sets only ``_name_`` and -#: ``_value_`` -- builds a complete member for them. ``tcp/flags`` joined the -#: original five once measurement showed its exclusion rationale did not hold: -#: the PR description had grouped it with the five templates below on the -#: strength of "each attach extra attributes in ``__new__``", but -#: ``grep -c 'def __new__'`` reports ``0`` for both ``pcapkit/vendor/tcp/flags.py`` -#: and ``pcapkit/const/tcp/flags.py`` -- it never defined one, so it is the same -#: shape as the four ``mh/*_flag`` registries, not the five below. Its own -#: ``_missing_`` still does its own 16-bit range check ending in -#: ``super()._missing_(value)``, unchanged by the conversion: that call chain -#: resolves through :mod:`aenum`'s own :class:`~aenum.Flag` machinery either -#: way, since :class:`~pcapkit.corekit.enum.EnumRegistry` never defines -#: ``_missing_`` itself. The remaining five bespoke templates (``ftp/command``, +#: The bespoke-template registries converted onto +#: :class:`~pcapkit.corekit.enum.EnumRegistry` whose members carry no extra +#: attributes, so the shared ``_unregistered_member`` -- which sets only +#: ``_name_`` and ``_value_`` -- builds a complete member for them. ``tcp/flags`` +#: is one of them: ``grep -c 'def __new__'`` reports ``0`` for both +#: ``pcapkit/vendor/tcp/flags.py`` and ``pcapkit/const/tcp/flags.py``, so it is +#: the same shape as the four ``mh/*_flag`` registries, not the templates below +#: that "each attach extra attributes in ``__new__``". Its own ``_missing_`` +#: does its own 16-bit range check ending in ``super()._missing_(value)``, +#: whichever base it has: that call chain resolves through :mod:`aenum`'s own +#: :class:`~aenum.Flag` machinery either way, since +#: :class:`~pcapkit.corekit.enum.EnumRegistry` never defines ``_missing_`` +#: itself. The remaining bespoke templates (``ftp/command``, #: ``ftp/return_code``, ``http/method``, ``http/status_code``, #: ``pcapng/option_type``) do each define a custom ``__new__`` attaching further #: attributes, so an unregistered member of theirs would be missing them; they @@ -86,8 +84,8 @@ def _unused_value(cls: 'Any') -> 'int': """First non-negative integer no member of ``cls`` carries. Read from ``_value2member_map_`` rather than by calling ``cls(value)``: - after tier 1 a declared-but-unassigned value resolves to an unregistered - member instead of raising, so a successful call proves nothing about + a declared-but-unassigned value resolves to an unregistered member + instead of raising (#775 tier 1), so a successful call proves nothing about membership -- the same reason ``register_alias`` itself tests that table. """ @@ -248,8 +246,8 @@ class TCPFlagsConversionTests(unittest.TestCase): ``grep -c 'def __new__'`` reports ``0`` for both ``pcapkit/vendor/tcp/flags.py`` and ``pcapkit/const/tcp/flags.py``, the same shape as the four ``mh/*_flag`` registries in :data:`CONVERTED`, so - it belongs there rather than among the five templates excluded for - actually defining one. + it belongs there rather than among the templates excluded for actually + defining one. """ def setUp(self) -> None: @@ -260,7 +258,7 @@ def test_tcp_flags_now_inherits_the_base(self) -> None: Resolves ``EnumRegistry`` freshly from the just-reimported ``pcapkit.corekit.enum`` rather than the module-scope import above -- - ``setUp`` gave this class its own ``pcapkit`` import, so ``Flags`` now + ``setUp`` gave this class its own ``pcapkit`` import, so ``Flags`` inherits a *new* ``EnumRegistry`` class object, and identity against the stale outer one would fail for a reason that says nothing about the product (see ``ProtocolIsInheritedTests.setUpClass`` above, which @@ -499,10 +497,10 @@ def test_register_over_a_taken_value_raises_value_error_and_does_not_alias(self) ``__members__`` pointing at it, and mints nothing -- reachable under the wrong method, unannounced, and contradicting the method's own docstring contract that ``register`` is what mints and nothing else - does so silently. Reproduced against this exact shape on - ``pcapkit.const.reg.apptype.apptype.TransportProtocol`` (a different, - non-:class:`~pcapkit.corekit.enum.EnumRegistry` registry, since - that one is not gated the same way) before this guard existed: + does so silently. Reproduced against this exact shape, without the + guard, on ``pcapkit.const.reg.apptype.apptype.TransportProtocol`` (a + different, non-:class:`~pcapkit.corekit.enum.EnumRegistry` registry, + since that one is not gated the same way): ``TransportProtocol.register(6, 'TOTALLY_NEW_NAME')`` returned ``TransportProtocol.tcp`` unchanged and minted nothing. This pins the fix on the base class every :data:`CONVERTED` registry actually uses. @@ -695,20 +693,20 @@ class _Str(EnumRegistry, StrEnum): ('pcapkit.const.hip.parameter', 'Parameter', 'pcapkit/const/hip/parameter.py'), ) -#: Const modules tier 3 deliberately leaves alone. The first six override +#: Const modules tier 3 deliberately leaves alone. The bespoke ones override #: their crawler's own ``process()``/``context()`` with a bespoke ``get()``/ #: ``_missing_`` pair that does not share :mod:`pcapkit.vendor.default`'s #: template at all -- mixing :class:`~pcapkit.corekit.enum.EnumRegistry` in #: without further care would silently change behaviour rather than just -#: move it, since four of the six are :class:`~aenum.StrEnum` registries whose +#: move it, since several are :class:`~aenum.StrEnum` registries whose #: own ``__new__`` attaches further attributes the base's generic #: ``_unregistered_member`` does not set. GitHub issue #860 fixes the base #: ``get()``'s own str-key-never-tries-the-value-path limitation -- measured #: on a synthetic registry in :class:`StrEnumValueFallbackTests` below -- and -#: issue #860's step 2 converts all six (``FEATCode``, ``Command``, ``Method``, -#: ``ReturnCode``/``ResponseKind``/``GroupingInformation``, ``StatusCode`` -#: and ``OptionType`` in PR 1; ``AppType`` and its four transport subclasses -#: in PR 2), each with its own ``_unregistered_member`` override where a +#: issue #860's step 2 converts them all (``FEATCode``, ``Command``, +#: ``Method``, ``ReturnCode``/``ResponseKind``/``GroupingInformation``, +#: ``StatusCode``, ``OptionType``, and ``AppType`` and its transport +#: subclasses), each with its own ``_unregistered_member`` override where a #: custom ``__new__`` needs one. They stay excluded from *this* file's #: tier-3 sweep regardless, because the exclusion here is about their #: bespoke ``process()``/``get()`` shape not sharing the generated template, @@ -718,13 +716,13 @@ class _Str(EnumRegistry, StrEnum): #: generated template, and several also keep a bespoke #: ``_unregistered_member`` (``Command``, ``Method``, ``OptionType``, #: ``ReturnCode``, ``StatusCode``) or a bespoke ``get`` of their own -#: (``Command``, ``Method``, ``OptionType``; ``AppType`` and its four -#: transport subclasses keep all four ``get``/``get_all``/``register``/ -#: ``register_alias``, added in PR 2) -- but not every one does: +#: (``Command``, ``Method``, ``OptionType``; ``AppType`` and its transport +#: subclasses keep ``get``/``get_all``/``register``/``register_alias`` of +#: their own) -- but not every one does: #: ``FEATCode``, ``ResponseKind`` and ``GroupingInformation`` resolve every #: protocol method to the base's generic implementation unchanged. #: ``TransportProtocol`` alone is excluded for -#: the separate reason it always was: it never mixes in +#: a separate reason: it never mixes in #: :class:`~pcapkit.corekit.enum.EnumRegistry` at all, by the owner's own #: ruling against extending it (GitHub PR #836) -- there is no registry #: protocol here to inherit or shadow. @@ -746,21 +744,21 @@ class _Str(EnumRegistry, StrEnum): #: (a) too, but on narrower grounds worth spelling out, since -- unlike the #: rest of that group -- it does not carry a bespoke ``get()``/``register()`` #: at all; its own ``get``/``get_all``/``register``/``register_alias`` are -#: exactly the base's generic implementation, same as every one of the 107 -#: :data:`GENERATED_SAMPLE`-shaped registries below. What excludes it is +#: exactly the base's generic implementation, same as every +#: :data:`GENERATED_SAMPLE`-shaped registry below. What excludes it is #: simply that this whole census, and its #: ``class \w+(EnumRegistry, IntEnum):`` regex a few lines down, is scoped to #: the :class:`~aenum.IntEnum` batch tier 3 of #775 converted. #: ``TLSKeyLabel`` is :class:`~aenum.StrEnum`-valued -- RFC 9850's labels are -#: strings, matched by value -- and it was generated after that batch -#: closed, so it was never a candidate for it either way. Excluding it here +#: strings, matched by value -- so it is outside that batch either way. +#: Excluding it here #: is not a judgement about its own inherited-protocol cleanliness; #: :mod:`tests.const.test_const_enum_get`, #: :mod:`tests.const.test_const_enum_lookup` and #: :mod:`tests.const.test_const_enum_builtin_parity`'s dynamic #: ``pkgutil.walk_packages`` sweeps already discover and cover it -#: generically -- it is simply outside what this file's historical, -#: IntEnum-only census counts. +#: generically -- it is simply outside what this file's IntEnum-only census +#: counts. EXCLUDED_FROM_INTENUM_CENSUS = frozenset({ # (a) str-valued: not an IntEnum, so excluded from the regex census # regardless of override. @@ -793,7 +791,7 @@ class _Str(EnumRegistry, StrEnum): class GeneratedTemplateSourceInheritsTests(unittest.TestCase): """The default-template registries must inherit the protocol too, - not just the six bespoke ones :class:`GeneratedSourceInheritsTests` + not just the bespoke ones :class:`GeneratedSourceInheritsTests` above (tier 2) already covers.""" def test_generated_sample_declares_the_base(self) -> None: @@ -861,7 +859,7 @@ def test_every_generated_const_module_is_accounted_for(self) -> None: class GeneratedProtocolIsInheritedTests(unittest.TestCase): """The generated sample's protocol methods must resolve to the base class, exactly as :class:`ProtocolIsInheritedTests` above pins for the - six bespoke ones.""" + bespoke ones.""" if TYPE_CHECKING: registries: 'list[Any]' @@ -1145,13 +1143,12 @@ class StrEnumValueFallbackTests(unittest.TestCase): """GitHub issue #860, step 1: guards against the base ``get()``'s ``isinstance(key, str)`` branch trying only ``cls._member_map_[key]`` -- a *name* lookup -- and never falling back to treating a ``str`` key as a - *value*, unlike the integer path. Found during #858's review on a - synthetic registry rather than a real one - (:class:`~pcapkit.corekit.enum.EnumRegistry` itself came from tier 1, - #855, which converted no :class:`~aenum.StrEnum` registry). The bespoke - ``StrEnum`` const registries mix in this base through #860's separate - step 2 -- ``FEATCode``, ``Command`` and ``Method`` in PR 1, ``AppType`` - and its transport subclasses in PR 2 -- and + *value*, unlike the integer path. It shows on a synthetic registry + rather than a real one, since #855 converts no :class:`~aenum.StrEnum` + registry to :class:`~pcapkit.corekit.enum.EnumRegistry`. The bespoke + ``StrEnum`` const registries -- ``FEATCode``, ``Command``, ``Method``, + and ``AppType`` with its transport subclasses -- mix in this base + through #860's separate step 2, and :data:`EXCLUDED_FROM_INTENUM_CENSUS` above holds them out of *this* file's tier-3 sweep for a different reason: they are :class:`~aenum.StrEnum`, not :class:`~aenum.IntEnum`, which is reason @@ -1399,8 +1396,8 @@ def test_negative_one_defaults_on_a_shipped_registry_raise_and_mint_nothing(self Neither is a registered value of any shipped registry, whose domains all start at ``0``, so ``default`` does not resolve: there is no ``cls(default)`` fallback left to attempt, and the name lookup's own - :exc:`KeyError` propagates, as with no default at all; the pre-#864 - tree raised :exc:`ValueError` from ``cls(-1)`` instead. This does *not* + :exc:`KeyError` propagates, as with no default at all; without #864 + the lookup raises :exc:`ValueError` from ``cls(-1)`` instead. This does *not* discriminate #857, which raised the same :exc:`KeyError`; :meth:`test_missing_name_with_default_negative_one_is_now_a_real_default` does. @@ -1472,12 +1469,11 @@ def test_the_issues_own_case_no_longer_mints_and_raises_instead(self) -> None: ``0x1234`` (4660) falls in no ``_missing_`` range, so the lookup falls to ``default``; ``0x0888`` (2184) falls inside the ``Xyplex`` - range #775's *original* ruling deliberately kept minting -- but only - when reached as a ``key``. (#775's final round converts that range - too; see - :meth:`GetDefaultNoMintTests.test_key_path_minting_is_preserved`.) - Without #864's fix this mints for that reason; with it, ``default`` - does not reach ``cls(default)`` at all, so ``0x0888`` + range, which resolves without minting when reached as a ``key`` + (#775; see + :meth:`GetDefaultNoMintTests.test_key_path_minting_is_preserved`). + Without #864's fix the lookup reaches ``cls(default)``; with it, + ``default`` does not reach ``cls(default)`` at all, so ``0x0888`` not being an *already-registered* value propagates the ``0x1234`` lookup's own failure instead of minting anything.""" from pcapkit.const.reg.ethertype import EtherType @@ -1497,13 +1493,12 @@ def test_the_issues_own_case_no_longer_mints_and_raises_instead(self) -> None: def test_key_path_minting_is_preserved(self) -> None: """``EtherType.get(0x0888)`` -- no ``default`` at all -- falls through ``_missing_``, the one path by which #864 ruled ``get`` may mint. - #775's original mint/unmint criterion classed ``Xyplex`` as a - kept-minting, real-attributed-name range; GitHub issue #775's final - round converts that range (and every other one minting on - ``EtherType``/:class:`~pcapkit.const.ipx.socket.Socket`) to - :meth:`~pcapkit.corekit.enum.EnumRegistry._unregistered_member`, - preserving the hex-suffixed name unchanged -- so ``key`` resolution - through ``get()`` takes the same non-minting path, which this pins. + ``Xyplex`` is a real-attributed-name range, and like every other range + on ``EtherType``/:class:`~pcapkit.const.ipx.socket.Socket` it resolves + through :meth:`~pcapkit.corekit.enum.EnumRegistry._unregistered_member` + (GitHub issue #775), keeping the hex-suffixed name -- so ``key`` + resolution through ``get()`` takes the same non-minting path, which + this pins. #864's own point -- that ``default`` never reaches ``_missing_`` at all -- holds either way.""" from pcapkit.const.reg.ethertype import EtherType diff --git a/tests/const/test_const_str_payload_870_unit.py b/tests/const/test_const_str_payload_870_unit.py index f844c33f4d..06eaa4a436 100644 --- a/tests/const/test_const_str_payload_870_unit.py +++ b/tests/const/test_const_str_payload_870_unit.py @@ -7,8 +7,8 @@ member's own declared value. ``_value_`` was set correctly (``Method.GET.value == 'GET'``), but the :class:`str` the member itself *is* was not: ``str(Method.GET) == ''``, ``len(str(Method.GET)) == 0`` and, most visibly, -``Method.GET == 'GET'`` was :data:`False` for every one of the 40 declared -members, as measured, and true since the class was first written -- GitHub +``Method.GET == 'GET'`` was :data:`False` for every declared member, as +measured, and true since the class was first written -- GitHub pull request #869 fixed the same inconsistency for an *unregistered* member's own str payload (:meth:`~pcapkit.const.http.method.Method._unregistered_member` calls the @@ -68,7 +68,7 @@ class MethodStrPayload870RegressionTests(unittest.TestCase): - """The exact repro from GitHub issue #870, and the full sweep of Method's own 40 members.""" + """The exact repro from GitHub issue #870, and the full sweep of Method's own members.""" def setUp(self) -> None: reimport_once_per_class(self) diff --git a/tests/integration/test_cli_subprocess.py b/tests/integration/test_cli_subprocess.py index 221b267cb7..b65aebc392 100644 --- a/tests/integration/test_cli_subprocess.py +++ b/tests/integration/test_cli_subprocess.py @@ -142,8 +142,8 @@ def test_layer_option_stops_the_printed_chain_where_it_names(self) -> None: Frame 4 of :file:`http6.cap` is the shortest unambiguous witness: it parses to all four layers unlimited, so each limit shortens the chain the - verbose output prints. Every one of these printed the full chain while the - limits were inert. + verbose output prints. With the limits inert, every one of these prints the + full chain. """ capture = sample_path('http6.cap') diff --git a/tests/integration/test_engine_runtime.py b/tests/integration/test_engine_runtime.py index a79a0d5cda..8e01fcb59d 100644 --- a/tests/integration/test_engine_runtime.py +++ b/tests/integration/test_engine_runtime.py @@ -48,9 +48,9 @@ def test_dpkt_engine_returns_dpkt_packets_and_toolkit_chain(self) -> None: @unittest.skipUnless(HAS_SCAPY, 'scapy not installed') def test_scapy_engine_returns_scapy_packets(self) -> None: - # #406: this asserted ``Raw`` -- the engine imported only ``scapy.sendrecv``, - # so scapy's layer registries were empty and every frame came back - # undissected. ``Ether`` is what the capture actually holds: frame 1 of + # #406: guards against the engine importing only ``scapy.sendrecv``, under + # which scapy's layer registries are empty and every frame comes back + # undissected, as ``Raw``. ``Ether`` is what the capture actually holds: frame 1 of # in.pcap is an Ethernet frame carrying an ICMPv6 neighbour solicitation over # IPv6, which is exactly what the two tests above independently report for the # same frame -- 'Ethernet:IPv6:IPv6_ICMP' from the default engine and diff --git a/tests/integration/test_files_output_naming.py b/tests/integration/test_files_output_naming.py index bf16d0325d..a89aec0a84 100644 --- a/tests/integration/test_files_output_naming.py +++ b/tests/integration/test_files_output_naming.py @@ -1,17 +1,17 @@ # -*- coding: utf-8 -*- """What ``files=True`` actually writes to disk. -#358: every per-frame report landed with two dots before its extension -- -``Frame 1..json``, ``Global Header..txt``, ``Section Header 1..plist``. The name -was composed as ``f'{name}.{ext._fext}'`` while ``_fext`` still carried its own -leading dot, so the separator was supplied twice. +#358: guards against every per-frame report landing with two dots before its +extension -- ``Frame 1..json``, ``Global Header..txt``, ``Section Header +1..plist`` -- as it does when the name is composed as ``f'{name}.{ext._fext}'`` +while ``_fext`` carries its own leading dot, supplying the separator twice. The unit-tier half of this lives in :file:`tests/foundation/test_extraction.py` and pins :meth:`Extractor.make_name ` to returning a bare extension. This module checks the thing the user sees: the names on disk, for both engines and every format that reaches a file. A test of ``make_name`` alone -would keep passing if an engine started supplying the dot twice again, so the +would keep passing if an engine supplied the dot twice, so the two are complementary rather than redundant. """ @@ -86,11 +86,11 @@ def test_pcapng_per_frame_names_carry_exactly_one_dot(self) -> None: self.assertNotIn('..', name) def test_single_file_output_still_carries_exactly_one_dot(self) -> None: - """The non-``files`` branch, which was already right and must stay right. + """The non-``files`` branch, which must keep exactly one dot. - ``make_name`` appends the extension itself here, so moving the dot out of - ``_fext`` had to move it into this branch's f-string; a mistake would - show up as ``report`` with no suffix, or ``report..json``. + ``make_name`` appends the extension itself here, with the dot in this + branch's f-string rather than in ``_fext``; a mistake would show up as + ``report`` with no suffix, or ``report..json``. """ for fmt, suffix in FORMATS.items(): diff --git a/tests/integration/test_frame_iteration.py b/tests/integration/test_frame_iteration.py index 41e06a8876..5506f8780b 100644 --- a/tests/integration/test_frame_iteration.py +++ b/tests/integration/test_frame_iteration.py @@ -110,18 +110,18 @@ def stack(frame: 'Any') -> 'list[Any]': class ExtractionLimitTests(EndToEndTestCase): """``layer`` and ``protocol``, which stop parsing part way up the stack. - Both were inert until GH-356. ``ProtocolBase.__init__`` read the limits from - ``_layer``/``_protocol``, while every producer -- the two engines building - the outermost protocol, all four ``_import_next_layer`` implementations + Guards against both being inert (GH-356): ``ProtocolBase.__init__`` reading + the limits from ``_layer``/``_protocol``, while every producer -- the engines + building the outermost protocol, every ``_import_next_layer`` implementation recursing into the next one, and the public ``pcapkit.extract(layer=..., protocol=...)`` that the CLI's ``-L``/``-P`` - feed -- passed them without the underscore. They landed in ``**kwargs``, were - dropped, ``_sigterm`` stayed :data:`False` the whole way up, and the parse - always ran to the top of the stack whatever the caller asked for. + feed -- passes them without the underscore. They land in ``**kwargs``, are + dropped, ``_sigterm`` stays :data:`False` the whole way up, and the parse + always runs to the top of the stack whatever the caller asked for. So these tests assert on *where the parse stopped*, not on the option being - stored: a test that only checked ``extractor._exlyr`` would have passed - throughout the bug's lifetime. + stored: a test that only checked ``extractor._exlyr`` passes with the limits + inert. """ @@ -274,9 +274,9 @@ def test_pcapng_engine_honours_the_limits_too(self) -> None: It builds its outermost protocol in ``pcapkit.foundation.engines.pcapng.PCAPNG.read_frame`` rather than - through ``PCAP.read_frame``, so it has to be checked separately: the - protochains for :file:`test.pcapng` were byte-identical to the unlimited - run while the limits were inert. + through ``PCAP.read_frame``, so it has to be checked separately: with the + limits inert, the protochains for :file:`test.pcapng` are byte-identical + to the unlimited run. """ chains = {} @@ -290,7 +290,7 @@ def test_pcapng_engine_honours_the_limits_too(self) -> None: unlimited = chains[()] self.assertNotEqual(chains[(('layer', 'link'),)], unlimited) self.assertNotEqual(chains[(('layer', 'internet'),)], unlimited) - # Anything the unlimited run took past the link layer must be shorter now. + # Anything the unlimited run took past the link layer must be shorter here. self.assertTrue(any(len(short.split(':')) < len(long.split(':')) for short, long in zip(chains[(('layer', 'link'),)], unlimited))) diff --git a/tests/integration/test_traceflow_end_to_end.py b/tests/integration/test_traceflow_end_to_end.py index 8c7860f16e..6f850d3e41 100644 --- a/tests/integration/test_traceflow_end_to_end.py +++ b/tests/integration/test_traceflow_end_to_end.py @@ -122,9 +122,9 @@ def test_every_frame_lands_in_exactly_one_flow(self) -> None: # its four-way close: the final acknowledgement then arrives after the # buffer has been popped and forms a flow of its own, so each conversation # comes back as a nine-frame flow plus a one-frame tail -- 109 of them - # here, which is what the review caught. The assertions below are what - # would notice it again: a stray tail is a single-frame flow, and it is a - # flow a later connection reusing those endpoints can be absorbed into. + # here. The assertions below are what notice it: a stray tail is a + # single-frame flow, and it is a flow a later connection reusing those + # endpoints can be absorbed into. self.assertEqual(len(flows), 111) self.assertEqual(sum(1 for flow in flows if len(flow.index) == 1), 0, 'a flow of one frame is a stray tail, not a conversation') diff --git a/tests/protocols/application/test_ftp_unit.py b/tests/protocols/application/test_ftp_unit.py index 913c9abf8d..feb52f0eaa 100644 --- a/tests/protocols/application/test_ftp_unit.py +++ b/tests/protocols/application/test_ftp_unit.py @@ -97,11 +97,11 @@ def test_command_get_is_case_insensitive(self) -> None: The rule is in section 5.3 (COMMANDS, which gives the command syntax), not section 4.1 (FTP COMMANDS, which only lists the per-command - semantics); #582 cited 4.1 and a cross-review caught it. Verified against - the RFC text: the sentence sits between the 5.3 and 5.4 headings. The + semantics). Verified against the RFC text: the sentence sits between + the 5.3 and 5.4 headings. The *link* above says ``#section-5`` rather than ``#section-5.3`` because RFC 959 renders anchors only for its eight top-level sections -- GitHub issue - #944 -- so ``#section-5.3`` was a live link to nothing. The + #944 -- so ``#section-5.3`` would be a live link to nothing. The ``:rfc:`959#section-4``` citations in :mod:`pcapkit.const.ftp.command` are a different claim -- the command *kind* (access control, transfer parameter, service) -- and are correct. diff --git a/tests/protocols/application/test_http_unit.py b/tests/protocols/application/test_http_unit.py index fa6e878d04..f333e62b7b 100644 --- a/tests/protocols/application/test_http_unit.py +++ b/tests/protocols/application/test_http_unit.py @@ -771,10 +771,10 @@ def test_guess_version_leaves_a_mid_stream_frame_to_the_fall_through(self) -> No reserved bit clear" test, because that is what misfires on binary HTTP/1 bodies. So a bare frame falls through to the trial parse, where the HTTP/2 parser's own length and type consistency rules decide: a - self-consistent frame is read as HTTP/2 (unchanged behaviour, pinned so - the new identification step is not mistaken for a replacement of the - fall-through), and one whose declared length its buffer does not back is - refused rather than answered. + self-consistent frame is read as HTTP/2 (pinned so the identification + step is not mistaken for a replacement of the fall-through), and one + whose declared length its buffer does not back is refused rather than + answered. """ import io @@ -2567,11 +2567,10 @@ def test_httpv2_schema_selector_and_frame_post_process_flags(self) -> None: def test_make_http_data_restores_end_stream_from_the_frame(self) -> None: """``_make_http_data`` must read ``END_STREAM`` back off ``frame.flags``. - It was the only one of the six ``_make_http_*`` methods that never - looked at ``frame.flags``, so a DATA frame parsed with ``END_STREAM`` - set rebuilt with the bit clear and a parse -> reconstruct round trip - lost it silently (#652). ``PADDED`` survived only because it is - re-derived from ``pad_len``. + Guards against it ignoring ``frame.flags``: a DATA frame parsed with + ``END_STREAM`` set would rebuild with the bit clear, and a parse -> + reconstruct round trip would lose it silently (#652). ``PADDED`` survives either way, + because it is re-derived from ``pad_len``. """ import io @@ -2658,7 +2657,7 @@ def test_a_flagless_frame_seeds_its_flags_as_an_enum(self) -> None: with self.assertRaises(TypeError): FrameType.Flags(0) - # The five frame schemas that inherit that memberless enum therefore + # The frame schemas that inherit that memberless enum therefore # keep the plain ``int`` -- and must not raise on the way. unassigned = UnassignedFrame(data=b'x') unassigned.post_process({ diff --git a/tests/protocols/application/test_httpv2_payload_length_unit.py b/tests/protocols/application/test_httpv2_payload_length_unit.py index 0e322afbd3..f3448bf9f8 100644 --- a/tests/protocols/application/test_httpv2_payload_length_unit.py +++ b/tests/protocols/application/test_httpv2_payload_length_unit.py @@ -261,8 +261,8 @@ def test_unpadded_data_frame_keeps_its_whole_payload(self) -> None: def test_padded_data_frame_keeps_its_payload_and_drops_its_padding(self) -> None: """A padded ``DATA`` frame's payload survives, padding excluded. - The arm that already worked, asserted so that a fix cannot repair the - unpadded case by breaking this one. :data:`PADDING` is non-zero, so a + The padded arm, asserted so that a fix cannot repair the unpadded case + by breaking this one. :data:`PADDING` is non-zero, so a callback that stopped subtracting it would hand back ``DATA_PAYLOAD + PADDING`` and be caught here rather than pass on an equal length. @@ -370,14 +370,14 @@ def test_a_padded_headers_frame_of_only_padding_reads_an_empty_fragment(self) -> """``pad_len`` accounting for the whole area leaves ``fragment`` empty. The zero-length boundary, and it is load-bearing rather than decorative. - A cross-review on a second model found that without this case -- and its - ``PUSH_PROMISE`` and ``PRIORITY`` counterparts below -- the module was - satisfied by a wrong fix returning ``max(computed, 1)`` for these two - fields, since every other fixture here has a non-empty fragment and so - never asks the callback for ``0``. Measured under that mutation: this + Without this case -- and its ``PUSH_PROMISE`` and ``PRIORITY`` + counterparts below -- the module is satisfied by a wrong fix + returning ``max(computed, 1)`` for these two fields, since every other + fixture here has a non-empty fragment and so never asks the callback + for ``0``. Measured under that mutation: this frame's ``fragment`` came back ``b'\\xde'`` -- one padding octet leaked in - -- alongside a swallowed ``SchemaWarning: packet length < 0: -1``, and all - 15 of the other tests still passed. :meth:`FramePayloadMixin.parse` + -- alongside a swallowed ``SchemaWarning: packet length < 0: -1``, and every + other test in the module still passed. :meth:`FramePayloadMixin.parse` requiring silence is what turns that warning into a failure. """ @@ -457,7 +457,7 @@ def test_a_padded_push_promise_frame_of_only_padding_is_empty(self) -> None: The zero-length boundary for this frame -- see :meth:`HTTPv2HeadersFramePayloadLengthUnitTests.\ test_a_padded_headers_frame_of_only_padding_reads_an_empty_fragment` - for the wrong fix that passed the module before this case existed. The + for the wrong fix that passes the module without the zero-length cases. The promised stream identifier is asserted as well, since it is the field immediately ahead of the fragment and the one an offset error would borrow from. @@ -631,8 +631,8 @@ def test_the_unpadded_arm_does_not_consult_pad_len(self) -> None: def test_the_padded_arm_subtracts_exactly_the_padding_length(self) -> None: """With ``PADDED`` set the callback returns ``__length__ - pad_len``. - The arm that always worked, pinned at the level of the number so that a - change to the unpadded arm cannot quietly move this one. ``pad_len``'s + The padded arm, pinned at the level of the number so that a change to + the unpadded arm cannot quietly move this one. ``pad_len``'s own octet is *not* subtracted here, because the field that read it has already decremented ``__length__`` by one. @@ -653,8 +653,8 @@ def test_the_padded_arm_reaches_zero_and_is_not_clamped(self) -> None: ``0`` is a legitimate answer -- a frame whose padding fills its whole payload area has a zero-length payload -- so the callback must be able to return it. Pinned as a number because a clamp is the shape of wrong fix - the wire tests above could not see until their zero-length cases were - added: ``max(computed, 1)`` satisfies every non-empty fixture and only + the wire tests above see only through their zero-length cases: + ``max(computed, 1)`` satisfies every non-empty fixture and only shows up here and at that boundary. """ diff --git a/tests/protocols/application/test_ngap_unit.py b/tests/protocols/application/test_ngap_unit.py index b31f451a1f..1cf52773fd 100644 --- a/tests/protocols/application/test_ngap_unit.py +++ b/tests/protocols/application/test_ngap_unit.py @@ -170,7 +170,7 @@ def test_unknown_procedure_and_ie_resolve_without_raising_or_aliasing(self) -> N ProtocolIE(65536) def test_get_of_an_unresolvable_string_key_raises_rather_than_minting(self) -> None: - """GitHub issue #880's own repro, re-run against the fixed classes: + """GitHub issue #880's own repro: ``get()`` on an unrecognised name must raise rather than mint a member at a shared default -- the defect that let a second unknown name alias onto the first's member. Real callers in :meth:`NGAP.read` diff --git a/tests/protocols/application/test_ospf_cryptographic_authentication_unit.py b/tests/protocols/application/test_ospf_cryptographic_authentication_unit.py index 614fa33b04..779c2e96b0 100644 --- a/tests/protocols/application/test_ospf_cryptographic_authentication_unit.py +++ b/tests/protocols/application/test_ospf_cryptographic_authentication_unit.py @@ -68,7 +68,6 @@ def test_ospf_is_grouped_with_the_application_layer(self) -> None: self.assertNotIn(name, application) def test_auth_data_selector_picks_the_cryptographic_schema(self) -> None: - # Moved from tests/protocols/link/test_link_unit.py. from pcapkit.const.ospf.authentication import Authentication from pcapkit.corekit.fields.misc import SchemaField from pcapkit.corekit.fields.strings import BytesField diff --git a/tests/protocols/application/test_ospf_dead_pattern_1844_unit.py b/tests/protocols/application/test_ospf_dead_pattern_1844_unit.py index 3cc300293a..e53df5e70c 100644 --- a/tests/protocols/application/test_ospf_dead_pattern_1844_unit.py +++ b/tests/protocols/application/test_ospf_dead_pattern_1844_unit.py @@ -1,13 +1,12 @@ # -*- coding: utf-8 -*- """The unused Ethernet-address pattern in the OSPF module. -GitHub issue #1844. :mod:`pcapkit.protocols.application.ospf` defined a -``PAT_MAC_ADDR`` that nothing read, a leftover that came along when OSPF moved -to the application layer in #1031, and it imported :mod:`re` only to compile -it. The constant and the import are gone. This pins that the constant stays -gone and that the module does not import :mod:`re` without using it. The -copies of the pattern in the Ethernet, ARP and PCAP-NG modules have readers -and are untouched. +GitHub issue #1844. Guards against +:mod:`pcapkit.protocols.application.ospf` defining a ``PAT_MAC_ADDR`` that +nothing reads, and importing :mod:`re` only to compile it. These tests pin +that the constant is absent and that the module does not import :mod:`re` +without using it. The copies of the pattern in the Ethernet, ARP and PCAP-NG +modules have readers, and stay. """ from __future__ import annotations diff --git a/tests/protocols/application/test_ospf_unit.py b/tests/protocols/application/test_ospf_unit.py index 2e0ce42a2d..25e1b3a6aa 100644 --- a/tests/protocols/application/test_ospf_unit.py +++ b/tests/protocols/application/test_ospf_unit.py @@ -1,9 +1,5 @@ # -*- coding: utf-8 -*- -"""Unit tests for :mod:`pcapkit.protocols.application.ospf`. - -Relocated from ``tests/protocols/link/test_link_unit.py`` with the module -itself, under :issue:`719`. -""" +"""Unit tests for :mod:`pcapkit.protocols.application.ospf`.""" from __future__ import annotations import importlib.util diff --git a/tests/protocols/application/test_rarp_unit.py b/tests/protocols/application/test_rarp_unit.py index d8213492c5..d9995ea044 100644 --- a/tests/protocols/application/test_rarp_unit.py +++ b/tests/protocols/application/test_rarp_unit.py @@ -1,9 +1,5 @@ # -*- coding: utf-8 -*- -"""Unit tests for :mod:`pcapkit.protocols.application.rarp`. - -Relocated from ``tests/protocols/link/test_link_unit.py`` with the module -itself, under :issue:`719`. -""" +"""Unit tests for :mod:`pcapkit.protocols.application.rarp`.""" from __future__ import annotations import importlib.util diff --git a/tests/protocols/internet/test_hip_fixed_bits_unit.py b/tests/protocols/internet/test_hip_fixed_bits_unit.py index 632cea05a7..b33fcc2b97 100644 --- a/tests/protocols/internet/test_hip_fixed_bits_unit.py +++ b/tests/protocols/internet/test_hip_fixed_bits_unit.py @@ -4,10 +4,10 @@ GitHub issue #1459. The leading bit of the ``Packet Type`` octet and the low-order bit of the ``Version`` octet are reserved for SHIM6 compatibility: they "MUST be set as shown when sending and MUST be ignored when receiving" -(:rfc:`7401#section-5.1`). ``HIP.read`` raised ``ProtocolError`` unless they -read ``0`` and ``1``. +(:rfc:`7401#section-5.1`). These tests guard against ``HIP.read`` raising +``ProtocolError`` unless they read ``0`` and ``1``. -The fix stops validating them, carries them as ``packet_fixed`` and +``HIP`` does not validate them: it carries them as ``packet_fixed`` and ``version_fixed``, and lets ``make()`` write them, defaulting to the RFC-prescribed values. diff --git a/tests/protocols/internet/test_hip_locator_flags_unit.py b/tests/protocols/internet/test_hip_locator_flags_unit.py index c2692d0ae2..7fda6bd6cf 100644 --- a/tests/protocols/internet/test_hip_locator_flags_unit.py +++ b/tests/protocols/internet/test_hip_locator_flags_unit.py @@ -2,10 +2,11 @@ """HIP ``LOCATOR_SET`` keeps the seven reserved bits of each locator's flags octet. GitHub issue #1365. :rfc:`8046#section-4` lays the fourth octet of a locator out -as ``Reserved`` (7 bits) followed by ``P`` (1 bit). ``Locator.flags`` declared only -``preferred: (7, 1)``, so the seven high-order bits were dropped on rebuild: flags -``0xfe`` parsed and rebuilt through ``from_data`` as ``0x00``. The fix names them -``reserved: (0, 7)``, per the #654 per-site policy, and carries them through +as ``Reserved`` (7 bits) followed by ``P`` (1 bit). Declaring only +``preferred: (7, 1)`` in ``Locator.flags`` drops the seven high-order bits on +rebuild: flags ``0xfe`` parse and rebuild through ``from_data`` as ``0x00``. +``Locator.flags`` names them ``reserved: (0, 7)``, per the #654 per-site policy, +and carries them through :class:`~pcapkit.protocols.data.internet.hip.Locator` and ``make()``. Every case builds its own octets in memory and reads no capture. Classes are diff --git a/tests/protocols/internet/test_hip_locator_set_length_unit.py b/tests/protocols/internet/test_hip_locator_set_length_unit.py index 0fe5d562eb..39c58f7d52 100644 --- a/tests/protocols/internet/test_hip_locator_set_length_unit.py +++ b/tests/protocols/internet/test_hip_locator_set_length_unit.py @@ -45,7 +45,8 @@ SPI then plain 9 **60** 56 =================== ============== ====== ========================= -Four of the seven were not even multiples of eight. A sweep over n = 1..5 of +The SPI-bearing and mixed shapes were not even multiples of eight, nor was the +empty one. A sweep over n = 1..5 of plain locators passes by cancellation without the fix and would pass a half-fix too, so it discriminates nothing; the empty set, the SPI-bearing variant and a mixed set in both orders are the shapes that do. @@ -136,7 +137,7 @@ def rfc_total(length: 'int') -> 'int': @unittest.skipUnless(HAS_RUNTIME, 'runtime dependencies not installed') class HIPLocatorSetLengthTests(unittest.TestCase): - """``LOCATOR_SET``'s ``Length`` unit and padding source, over seven shapes.""" + """``LOCATOR_SET``'s ``Length`` unit and padding source, over every shape.""" def setUp(self) -> None: reimport_once_per_class(self) @@ -195,14 +196,14 @@ def test_every_shape_packs_the_rfc_total(self) -> None: self.assertEqual(raw[2:4], contents.to_bytes(2, 'big')) def test_the_three_plain_shapes_keep_the_octet_counts_main_emitted(self) -> None: - """n = 1, 2, 5 still pack 32, 56 and 128 octets. - - The one part of this parameter that was already right, and the reason - #651 and #664 left it alone. Those three are the shapes where the two - defects cancelled exactly, so they are also the three that a correct fix - must not move -- if they change, something has gone wrong even though - every other shape improved. Only the two ``Length`` octets differ from - ``f0999858e``: ``0004`` became ``0018`` at n = 1, and so on. + """n = 1, 2, 5 pack 32, 56 and 128 octets. + + These are the shapes where the two defects cancel exactly, and the + reason #651 and #664 left this parameter alone, so they are also the + three a correct fix must not move -- if they change, something has gone + wrong whatever the other shapes do. Only the two ``Length`` octets + differ from ``f0999858e``: ``0018`` at n = 1 where it packed ``0004``, + and so on. """ for locators, total, length in (([PLAIN], 32, 24), ([PLAIN, PLAIN], 56, 48), @@ -288,8 +289,8 @@ def test_every_shape_parses_back_to_the_locators_it_was_built_from(self) -> None than what it read. The residual warnings are asserted to be absent *at the parameter level* - only for the empty shape. Every nested :class:`Locator` still raises six - of them, one per field, because ``SchemaField`` seeds a nested schema's + only for the empty shape. Every nested :class:`Locator` raises one of + them per field, because ``SchemaField`` seeds a nested schema's ``__length__`` at the ``-1`` placeholder :meth:`Schema.pack ` documents; that is unrelated to ``Length`` units, and it fires once per diff --git a/tests/protocols/internet/test_hip_r1_counter_width_unit.py b/tests/protocols/internet/test_hip_r1_counter_width_unit.py index 5142cce1dc..32c4f2f77f 100644 --- a/tests/protocols/internet/test_hip_r1_counter_width_unit.py +++ b/tests/protocols/internet/test_hip_r1_counter_width_unit.py @@ -10,45 +10,47 @@ :class:`~pcapkit.protocols.schema.internet.hip.R1CounterParameter` are affected: ``R1_Counter`` (128, HIPv1) and ``R1_COUNTER`` (129). -``counter`` was a :class:`~pcapkit.corekit.fields.numbers.UInt32Field`. The -parameter therefore *declared* the correct ``len=12`` -- 4 octets of ``Reserved`` -plus 8 of counter, which is the ``Length 12`` the RFC states -- and *packed* only -eight of those twelve, for a record of 12 octets where :rfc:`7401` -Section 5.2.1's ``Total Length = 11 + Length - (Length + 3) % 8`` makes it 16. +Guards against ``counter`` being a +:class:`~pcapkit.corekit.fields.numbers.UInt32Field`. The parameter would then +*declare* the correct ``len=12`` -- 4 octets of ``Reserved`` plus 8 of counter, +which is the ``Length 12`` the RFC states -- and *pack* only eight of those +twelve, for a record of 12 octets where :rfc:`7401` Section 5.2.1's +``Total Length = 11 + Length - (Length + 3) % 8`` makes it 16. Four short, at ``4 (mod 8)``, in both directions: a conformant receiver reading ``Length = 12`` consumes 16 octets and takes four octets of the next parameter as this one's tail, and a real peer's 16-octet ``R1_COUNTER`` leaves its last four octets to be read as the following parameter's ``Type``. -Why the suite could not see it +Why a round trip cannot see it ------------------------------ -Three things hid it at once, and the third is the reason this module asserts +Three things can hide it at once, and the third is the reason this module asserts against the RFC's arithmetic rather than against a round trip. -1. ``HIP_COPIES = 2`` in :mod:`examples.generators.options` puts two copies of - each parameter in a frame, and two four-octet shortfalls sum to eight, so the - record area stays 8-aligned and ``HIP.make``'s ``total_length // 8 + 4`` - comes out exact. +1. With two copies of each parameter in a frame (``HIP_COPIES = 2`` in + :mod:`examples.generators.options`), two four-octet shortfalls sum to eight, + so the record area stays 8-aligned and ``HIP.make``'s + ``total_length // 8 + 4`` comes out exact. 2. A padding rule that aligns the parameter *contents* rather than the record (the #651 defect) appends, at ``Length = 12``, exactly the four surplus octets - this parameter was missing. So correcting the padding does not break + this parameter would be missing. So correcting the padding does not break ``R1_COUNTER``; it stops compensating for it. -3. The round trip cannot see it at all, because pcapkit's reader consumes exactly - the twelve octets its writer wrote. Only a comparison against the RFC's own +3. The round trip cannot see it at all, because pcapkit's reader would consume + exactly the twelve octets its writer wrote. Only a comparison against the RFC's own stride -- or a real peer -- can tell the difference, which is what :meth:`test_r1_counter_record_is_the_rfc_total_at_both_codes` does, with the formula spelled out locally rather than taken from the library. -The fixture could not see it either, for a fourth reason --------------------------------------------------------- +A zero counter hides it from the fixture too +-------------------------------------------- -``R1_COUNTER`` had no entry in ``_hip_overrides()``, so the generator built it -with ``counter = 0``. An RFC-only walk over :file:`options-internet.pcap` read -``Length = 12``, advanced 16 over a 12-octet record, landed four octets inside the -second copy, and found a phantom ``Type = 0, Length = 0`` record in that copy's -own zeroed contents -- whose "padding" was four zero octets, so its zero-padding -check passed and the walk ended tidily on the area boundary. Measured on +Without an entry in ``_hip_overrides()``, the generator builds ``R1_COUNTER`` +with ``counter = 0``. Given the four-octet field, an RFC-only walk over +:file:`options-internet.pcap` reads ``Length = 12``, advances 16 over a 12-octet +record, lands four octets inside the second copy, and finds a phantom +``Type = 0, Length = 0`` record in that copy's own zeroed contents -- whose +"padding" is four zero octets, so its zero-padding check passes and the walk +ends tidily on the area boundary. Measured on ``f0999858e``: patching one counter to ``aabbccdd`` turns that silence into ``frame 101: type 0 padding not zeroed: aabbccdd``. A zero-valued field cannot discriminate a width defect from a correct one, so diff --git a/tests/protocols/internet/test_hip_reserved_bits_unit.py b/tests/protocols/internet/test_hip_reserved_bits_unit.py index f7ab4f417c..12673fb72d 100644 --- a/tests/protocols/internet/test_hip_reserved_bits_unit.py +++ b/tests/protocols/internet/test_hip_reserved_bits_unit.py @@ -1,16 +1,17 @@ # -*- coding: utf-8 -*- """HIP keeps the reserved bits of the ``Version`` octet and the route ``Flags``. -GitHub issue #1417. Three HIP ``BitField`` namespaces left bits unnamed, so those -bits were packed as zero on rebuild (start, length): +GitHub issue #1417. Guards against HIP ``BitField`` namespaces leaving bits +unnamed, under which those bits pack as zero on rebuild. The sites, as (start, +length): * ``HIP.ver``: (4, 3) -- the ``RES`` bits between ``Version`` and the fixed low-order ``1`` (:rfc:`7401#section-5.1`); * ``RouteDstParameter.flags`` and ``RouteViaParameter.flags``: (2, 14) -- the reserved bits after ``S`` and ``M`` (:rfc:`6028#section-5`). -The fix names a ``reserved`` sub-field at each site, per the #654 per-site -policy, and carries it through the data model and ``make()``. +Each site names a ``reserved`` sub-field, per the #654 per-site policy, and +carries it through the data model and ``make()``. 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/protocols/internet/test_hip_unit.py b/tests/protocols/internet/test_hip_unit.py index 1ec42f8dcf..5cf24e78ec 100644 --- a/tests/protocols/internet/test_hip_unit.py +++ b/tests/protocols/internet/test_hip_unit.py @@ -241,9 +241,9 @@ def test_hip_register_parameter_warns_on_overwrite(self) -> None: registry[Parameter.ESP_INFO] = original # An unregistered code carries no entry, so registering one is not an - # overwrite. The setattr form could not tell the two apart: it keyed the - # warning on ``hasattr(cls, f'_read_param_{name}')``, which is true of - # every shipped handler as well as of anything a user had installed. + # overwrite. A setattr form cannot tell the two apart: keyed on + # ``hasattr(cls, f'_read_param_{name}')``, its warning fires for every + # shipped handler as well as for anything a user installed. custom = Parameter.Unassigned_65501 self.assertNotIn(custom, registry) try: @@ -270,11 +270,11 @@ def test_hip_register_parameter_dispatches_a_registered_callable_pair(self) -> N :data:`ParameterConstructor` declare -- ``(schema, *, version, options)`` and ``(code, param=None, *, version, **kwargs)`` -- i.e. as plain callables rather than as methods with an implicit ``self``. That is the - calling convention every other dispatch family uses, and the reason the - registry form replaced ``setattr``: installing the callable on the class - made it a descriptor, so dispatch passed ``self`` as the first positional - argument and a handler written to the declared signature could not be - called at all. + calling convention every other dispatch family uses, and the reason + dispatch goes through the registry rather than ``setattr``: a callable + installed on the class becomes a descriptor, so dispatch would pass + ``self`` as the first positional argument and a handler written to the + declared signature could not be called at all. """ from pcapkit.const.hip.parameter import Parameter @@ -338,10 +338,11 @@ def make_param(code, param=None, *, version, contents=b'', **kwargs): def test_hip_parameter_registry_covers_every_shipped_handler(self) -> None: """Every ``_read_param_*`` / ``_make_param_*`` pair must be reachable. - The setattr form derived the handler name from ``code.name.lower()``, so - a handler was reachable by construction and "what is registered?" had no - direct answer. Under the registry the mapping is explicit data, which - means a handler added without its registry entry becomes dead code -- + A setattr form, deriving the handler name from ``code.name.lower()``, + makes a handler reachable by construction and leaves "what is + registered?" with no direct answer. In the registry the mapping is + explicit data, which means a handler added without its registry entry + becomes dead code -- this pins the two sides together. """ @@ -366,7 +367,7 @@ def test_hip_parameter_registry_covers_every_shipped_handler(self) -> None: # ``R1_Counter`` (128, HIPv1) and ``R1_COUNTER`` (129, HIPv2) are distinct # codes whose names differ only in case, so both have to be keyed - # explicitly -- ``code.name.lower()`` collapsed them for free. + # explicitly -- ``code.name.lower()`` would collapse them. self.assertEqual(registry[Parameter.R1_Counter], 'r1_counter') self.assertEqual(registry[Parameter.R1_COUNTER], 'r1_counter') @@ -383,9 +384,8 @@ def test_hip_unregistered_parameter_code_does_not_mutate_the_registry(self) -> N :attr:`HIP.__parameter__ ` is a :class:`collections.defaultdict` on a class attribute shared by every instance in the process, so ``registry[code]`` would insert each - code it missed -- the leak #428 swept out of the sixteen registries that - already existed. This one is new, so the guard has to be pinned here too: - the reads go through + code it missed -- the leak #428 guards the other registries against, so + it is pinned here too: the reads go through :meth:`~pcapkit.protocols.protocol.ProtocolBase._lookup_registry`. Code 65499 is unassigned in IANA's registry and 4650 is @@ -2043,8 +2043,8 @@ def test_hip_transport_format_list_parameter_round_trips_through_the_maker(self) whatever ``item_type`` the ``formats`` field declares. A hand-built ``TransportFormatListParameter(type=..., len=..., formats=...)`` picks a self-consistent ``len`` by hand and so cannot expose a - maker/schema disagreement -- which is why the one-octet - ``item_type`` survived review once already. Building through the + maker/schema disagreement, such as a one-octet ``item_type``. + Building through the maker instead pins the two to agree: with one-octet items and two real entries, the maker's ``len=4`` reads back as ``len // 1 == 4`` entries -- ``[10, 20, 0, 0]``, two spurious trailing zeros -- while @@ -2530,8 +2530,8 @@ def test_hip_solution_parameter_length_is_two_whole_octet_fields(self) -> None: discriminators: at a multiple of 8 the defect's ``ceil(bits / 4)`` and the floor variant both coincide with the correct width, which is exactly why every byte-aligned fixture this library ships passed through the - defect unharmed. They are asserted to confirm the repair changes nothing - on the path that already worked. + defect unharmed. They are asserted to confirm the byte-aligned path + builds the same width as well. """ import math @@ -2969,8 +2969,8 @@ def test_hip_puzzle_and_solution_size_from_version_under_hipv1(self) -> None: schema, version=1, options=options, ).random, value) - # HIPv2: unchanged, still derived from the value when nothing - # declares the width. This is what makes the v1 column a fix rather + # HIPv2: derived from the value when nothing declares the + # width. This is what makes the v1 column a fix rather # than a blanket constant. self.assertEqual(proto._make_param_puzzle( Parameter.PUZZLE, version=2, index=1, lifetime=1, @@ -3174,14 +3174,14 @@ def pre_651_total(length: int) -> int: # Which parameters this governs, stated rather than implied: every padding # site in ``schema/internet/hip.py`` and every reported record length in - # ``internet/hip.py`` routes through these two helpers -- 45 of the 46 - # parameter schemas and 48 of the 49 reported lengths. The forty-sixth is + # ``internet/hip.py`` routes through these two helpers -- every parameter + # schema and every reported length but one. The exception is # ``LOCATOR_SET``, which uses # :func:`~pcapkit.protocols.schema.internet.hip.locator_set_padding_len` # instead (#679): the same arithmetic, read off a snapshot of the parameter's # own ``Length`` rather than off ``pkt['len']``, which its nested - # locators shadow while they pack. The counts and the identity of that - # one callback are asserted directly in + # locators shadow while they pack. The identity of that one callback is + # asserted directly in # :meth:`test_hip_padding_sites_are_the_two_known_callbacks_and_nothing_else`, # so a third expression cannot appear unnoticed. # @@ -3204,8 +3204,8 @@ def pre_651_total(length: int) -> int: # the three spot values worth naming, so a regression reads as a number # rather than as a loop index self.assertEqual(hip_schema.parameter_total_len(4), 8) # SEQ: no padding - self.assertEqual(hip_schema.parameter_total_len(8), 16) # was 12, short by 4 - self.assertEqual(hip_schema.parameter_total_len(20), 24) # SOLUTION: was 28 + self.assertEqual(hip_schema.parameter_total_len(8), 16) # 12 without #651, short by 4 + self.assertEqual(hip_schema.parameter_total_len(20), 24) # SOLUTION: 28 without #651 self.assertEqual(hip_schema.parameter_padding_len({'len': 4}), 0) self.assertEqual(hip_schema.parameter_padding_len({'len': 8}), 4) @@ -3220,7 +3220,7 @@ def pre_651_total(length: int) -> int: hip_schema.parameter_padding_len({'len': -1}) def test_hip_padding_sites_are_the_two_known_callbacks_and_nothing_else(self) -> None: - """#651/#664/#679: two padding callbacks cover all 46, and no third exists. + """#651/#664/#679: two padding callbacks cover every parameter, and no third exists. Every HIP padding site goes through :func:`~pcapkit.protocols.schema.internet.hip.parameter_padding_len` @@ -3232,7 +3232,7 @@ def test_hip_padding_sites_are_the_two_known_callbacks_and_nothing_else(self) -> and #664 documented the exclusion rather than leaving it to look accidental. - **With the pair fixed (#679), the exclusion is not a hold-back.** + **The exclusion is not a hold-back (#679).** ``LocatorSetParameter.padding`` uses :func:`~pcapkit.protocols.schema.internet.hip.locator_set_padding_len`, which defers to ``parameter_padding_len`` for the arithmetic and differs @@ -3302,13 +3302,13 @@ def test_hip_padding_sites_are_the_two_known_callbacks_and_nothing_else(self) -> 'given a snapshot it has no nested schemas to need.' ) - # Three schemas declare no padding field at all, which is correct rather - # than an omission and is unchanged by #651: their contents are a fixed - # 20 octets (a two-octet port, two reserved and a 16-octet address), and + # These schemas declare no padding field at all, which is correct rather + # than an omission: their contents are a fixed 20 octets (a two-octet + # port, two reserved and a 16-octet address), and # 11 + 20 - (20 + 3) % 8 == 24 == 4 + 20, so the RFC asks for no padding. - # Measured on this tree and on b34f132f6: all three pack to 24 octets. - # Listed explicitly because a *fourth* name appearing here would mean a - # parameter had quietly lost its padding field. + # Measured on b34f132f6: each packs to 24 octets. Listed explicitly + # because another name appearing here would mean a parameter had quietly + # lost its padding field. self.assertEqual( sorted(unpadded), ['RegFromParameter', 'RelayFromParameter', 'RelayToParameter']) diff --git a/tests/protocols/internet/test_ipv4_tso_total_length_1547_runtime.py b/tests/protocols/internet/test_ipv4_tso_total_length_1547_runtime.py index 219cfdd8ec..43045b7238 100644 --- a/tests/protocols/internet/test_ipv4_tso_total_length_1547_runtime.py +++ b/tests/protocols/internet/test_ipv4_tso_total_length_1547_runtime.py @@ -3,9 +3,10 @@ GitHub issue #1547. A capture taken on a host that offloads TCP segmentation (TSO/GSO) records its outgoing IPv4 headers with ``total_length`` 0, since the -NIC fills it in later. :meth:`IPv4.read ` -handed the next layer ``0 - hdr_len`` octets, so TCP fell back to -:class:`~pcapkit.protocols.misc.raw.Raw` and the flow was never traced. A Total +NIC fills it in later. These tests guard against +:meth:`IPv4.read ` handing the next +layer ``0 - hdr_len`` octets, under which TCP falls back to +:class:`~pcapkit.protocols.misc.raw.Raw` and the flow is never traced. A Total Length of 0 means the rest of the captured frame, with no trailer, as dpkt (``ip.py``) and Wireshark ("presumed TSO") read it, and the rebuild writes the 0 back. Following the owner's ruling to match Wireshark, any other Total Length @@ -156,7 +157,7 @@ def test_make_with_a_zero_total_length_round_trips(self) -> None: self.assertEqual(IPv4.from_data(parsed.info).data, made.data) self.assertEqual(IPv4.from_data(parsed.info.to_dict()).data, made.data) - # Without an explicit Total Length it is computed, as before. + # Without an explicit Total Length it is computed. computed = IPv4(protocol='TCP', payload=segment) self.assertEqual(struct.unpack('!H', computed.data[2:4])[0], 20 + len(segment)) @@ -223,7 +224,8 @@ def spy(self: 'Any', dict_: 'Any', proto: 'Any', length: 'Any' = None, **kwargs: def test_a_total_length_past_the_capture_is_unchanged(self) -> None: # The other direction, which Wireshark reports as "IPv4 total length # exceeds packet length (64 bytes)" while still dissecting TCP: the - # declared length is kept for the rebuild (#1155), and #1547 leaves it be. + # declared length is kept for the rebuild (#1155), and #1547's TSO + # reading does not apply to it. from pcapkit.protocols.internet.ipv4 import IPv4 from pcapkit.protocols.link.ethernet import Ethernet from pcapkit.protocols.transport.tcp import TCP diff --git a/tests/protocols/internet/test_ipv4_unit.py b/tests/protocols/internet/test_ipv4_unit.py index 7df46eab99..6a8186d0fb 100644 --- a/tests/protocols/internet/test_ipv4_unit.py +++ b/tests/protocols/internet/test_ipv4_unit.py @@ -68,7 +68,7 @@ class DummyDict(dict): def test_ipv4_make_data_scales_offset_and_defaults_missing_options(self) -> None: """Regression test for #494. - Two defects live in :meth:`IPv4._make_data + Guards against two defects in :meth:`IPv4._make_data `: the fragment ``offset`` was handed back in octets to a parameter that :meth:`IPv4.make ` takes in @@ -308,13 +308,12 @@ def header(value: 'object') -> 'Schema_IPv4': def test_schema_pack_packs_a_second_real_protocol_as_payload(self) -> None: """A real protocol nested in another one, not ``Raw`` and not ``NoPayload``. - This closes a hole in the three tests above rather than describing a - defect of its own. Every one of them hands the payload field a + This closes a hole in the tests above rather than describing a defect + of its own. Every one of them hands the payload field a :class:`~pcapkit.protocols.misc.raw.Raw` or a :class:`~pcapkit.protocols.misc.null.NoPayload`, because those are what - *parsing* yields, so between them they exercise exactly two of the more - than forty :class:`~pcapkit.protocols.protocol.ProtocolBase` descendants - the widened + *parsing* yields, so between them they exercise exactly two of the + :class:`~pcapkit.protocols.protocol.ProtocolBase` descendants the check in :meth:`Schema.pack ` has to accept. That is enough coverage to be defeated by a fix that is not one: replacing the @@ -324,10 +323,9 @@ def test_schema_pack_packs_a_second_real_protocol_as_payload(self) -> None: elif isinstance(data, (Protocol, Raw, NoPayload)): - -- special-casing precisely the classes the tests use -- passes all four - of them, measured, while leaving the general defect in place for the - other protocols. #536's own reviewer found that by falsification, and - this test is what makes the shortcut fail. + -- special-casing precisely the classes the tests use -- passes all of + them, measured, while leaving the general defect in place for the + other protocols. This test is what makes the shortcut fail. So the payload here is a ``UDP``, and the assertions are chosen to be unsatisfiable by any finite list of special cases: @@ -556,10 +554,9 @@ def test_ipv4_make_opt_sec_sizes_the_bitmap_from_the_bit_count(self) -> None: ``IndexError: list assignment index out of range`` -- not an in-library exception, and out of a ``_make_opt_*`` helper. - That is why :file:`examples/generators/options.py` passed *two* - authorities to this option; with the arithmetic fixed, one is a - legitimate argument again, which is the thing this test is really - asserting. + One authority is a legitimate argument, which is the thing this test is + really asserting; :file:`examples/generators/options.py` passes two as a + coverage choice. The same off-by-one under-sized by a whole octet at every exact multiple of eight, which is checked here too. No shipped enumeration member has @@ -612,22 +609,22 @@ def test_ipv4_make_opt_sec_rejects_a_termination_bit_as_an_authority(self) -> No ``Enum_ProtectionAuthority`` member ``7`` is named ``Field_Termination_Indicator`` -- it is not an authority at all, yet it - was a member of the enumeration the writer accepted as one. Passing it - produced an option encoding zero authorities. + is a member of the enumeration, so a writer accepting every member as an + authority encodes an option with zero authorities for it. - The writer and the reader disagreed about whether index 7 is data, and - this test records which of the two won. ``_read_opt_sec`` loops over + The writer and the reader must agree about whether index 7 is data, and + this test records which of the two wins. ``_read_opt_sec`` loops over ``range(7)`` per octet and maps octet ``base`` bit ``bit`` to authority ``base * 8 + bit``, so the authority numbering it produces *skips* 7, 15 and 23 -- those positions are termination bits and nothing else. The - reader is right and the writer was wrong, so the rejection is on the - write side and ``range(7)`` is left alone; widening it would make the - reader report a terminator as an authority. + reader is right, so the rejection is on the write side and + ``range(7)`` is left alone; widening it would make the reader report a + terminator as an authority. Rejecting the whole congruence class rather than only the named ``7`` follows from that: 15 and 23 are termination bits for exactly the same reason and are just as undeliverable, they simply have no name in the - enumeration yet. Asserting 15 is what stops the fix being read as a + enumeration. Asserting 15 is what stops the fix being read as a special case of one value. """ @@ -727,8 +724,8 @@ def test_ipv4_sid_option_is_four_octets_wide_on_the_wire(self) -> None: option) # Four octets is already 32-bit aligned, so the padding branch is not - # reached at all -- no NOP and no EOOL, where before there was one of - # each. + # reached at all -- no NOP and no EOOL, where a six-octet SID draws one + # of each. options, total_length = proto._make_ipv4_options(parsed.info.options) self.assertEqual([type(entry).__name__ for entry in options], ['SIDOption']) @@ -760,9 +757,8 @@ def test_ipv4_timestamp_option_refuses_a_bool_as_an_address(self) -> None: as the ``1`` it is, so #500's guard in :meth:`_IPAddressField.pre_process ` is never - reached. This was the fifth site of that defect -- #481, #500, #539 and - #540 are the first four -- and the fix is the same one #539 used for its - seven: route the conversion through + reached. It is the same defect as #481, #500, #539 and #540, with the + same fix #539 used: route the conversion through :func:`~pcapkit.corekit.fields.ipaddress.parse_ip_address`. Both halves are asserted, because a guard that rejects everything is not a @@ -929,11 +925,8 @@ def test_ipv4_timestamp_option_is_buildable_through_make(self) -> None: :class:`~pcapkit.utilities.warnings.UnknownFieldWarning` and carries on, so every timestamp was dropped in silence and ``ts_data`` stayed bound to its class-level ``ListField``, which ``post_process`` then tried to iterate: - ``TypeError: 'ListField' object is not iterable``. The IPv4 Timestamp - option was unbuildable through ``make`` for as long as that stood, which is - what the ``ipv4-option/TS`` entry of - :data:`tests.protocols.test_option_roundtrip_unit.EXPECTED_FAILURES` - recorded. + ``TypeError: 'ListField' object is not iterable``. Without the fix the + IPv4 Timestamp option cannot be built through ``make`` at all. The absence of the warning is asserted as well as the presence of the timestamps, because the warning is the whole reason the defect was silent: @@ -1085,10 +1078,10 @@ def test_ipv4_properties_read_and_make_cover_packet_paths(self) -> None: options=[(OptionNumber.SID, {'sid': 5})], payload=b'data', ) - # 20 of header, the 4-octet SID option, and 4 of payload. This read 32 - # with ihl=7 while ``SIDOption.sid`` was 32 bits wide: the option packed - # to six octets, which is not 32-bit aligned, so the option area picked - # up a NOP and an EOOL and grew to eight. See #534. + # 20 of header, the 4-octet SID option, and 4 of payload. Without the + # #534 fix this reads 32 with ihl=7: a 32-bit ``SIDOption.sid`` packs the + # option to six octets, which is not 32-bit aligned, so the option area + # picks up a NOP and an EOOL and grows to eight. self.assertEqual(made.length, 28) self.assertEqual(made.vihl['ihl'], 6) self.assertEqual(made.ttl, 64) @@ -1114,9 +1107,9 @@ def test_ipv4_register_option_warns_on_overwrite(self) -> None: registry[OptionNumber.EOOL] = original # An unregistered code carries no entry, so registering one is not an - # overwrite. The setattr form could not tell the two apart: it keyed the - # warning on ``hasattr(cls, f'_read_opt_{name}')``, which is true of every - # shipped handler as well as of anything a user had already installed. + # overwrite. A setattr form cannot tell the two apart: keyed on + # ``hasattr(cls, f'_read_opt_{name}')``, its warning fires for every + # shipped handler as well as for anything a user installed. custom = OptionNumber.get(31) self.assertNotIn(custom, registry) try: @@ -1143,11 +1136,11 @@ def test_ipv4_register_option_dispatches_a_registered_callable_pair(self) -> Non :data:`OptionConstructor` declare -- ``(schema, *, options)`` and ``(code, option=None, **kwargs)`` -- i.e. as plain callables rather than as methods with an implicit ``self``. That is the calling convention - every other dispatch family uses, and the reason the registry form - replaced ``setattr``: installing the callable on the class made it a - descriptor, so dispatch passed ``self`` as the first positional argument - and a handler written to the declared signature could not be called at - all. + every other dispatch family uses, and the reason dispatch goes through + the registry rather than ``setattr``: a callable installed on the class + becomes a descriptor, so dispatch would pass ``self`` as the first + positional argument and a handler written to the declared signature + could not be called at all. """ from pcapkit.const.ipv4.option_number import OptionNumber @@ -1211,10 +1204,11 @@ def make_option(code, option=None, *, data=b'', **kwargs): def test_ipv4_option_registry_covers_every_shipped_handler(self) -> None: """Every ``_read_opt_*`` / ``_make_opt_*`` pair must be reachable. - The setattr form derived the handler name from ``code.name.lower()``, so - a handler was reachable by construction and "what is registered?" had no - direct answer. Under the registry the mapping is explicit data, which - means a handler added without its registry entry becomes dead code -- + A setattr form, deriving the handler name from ``code.name.lower()``, + makes a handler reachable by construction and leaves "what is + registered?" with no direct answer. In the registry the mapping is + explicit data, which means a handler added without its registry entry + becomes dead code -- this pins the two sides together. """ @@ -1248,9 +1242,8 @@ def test_ipv4_unregistered_option_code_does_not_mutate_the_registry(self) -> Non :attr:`IPv4.__option__ ` is a :class:`collections.defaultdict` on a class attribute shared by every instance in the process, so ``registry[code]`` would insert each - code it missed -- the leak #428 swept out of the sixteen registries that - already existed. This one is new, so the guard has to be pinned here too: - the reads go through + code it missed -- the leak #428 guards the other registries against, so + it is pinned here too: the reads go through :meth:`~pcapkit.protocols.protocol.ProtocolBase._lookup_registry`. Option number 31 is unassigned in IANA's registry and 134 is ``CIPSO``, @@ -1379,7 +1372,7 @@ def test_ipv4_option_constructors_cover_common_and_error_branches(self) -> None: ['bytes', 'SIDOption']) # Which is why the alignment branch gets a case of its own rather than - # riding along on SID's old width. The padding terminator is an EOOL + # relying on SID to reach it. The padding terminator is an EOOL # *option*, not the bare wire code -- that is what #506 fixed. A SEC # option carrying a two-octet bitmap packs to five, so it is one: three # octets of padding, two NOPs and the EOOL. diff --git a/tests/protocols/internet/test_ipv6_calipso_pad_unit.py b/tests/protocols/internet/test_ipv6_calipso_pad_unit.py index 274ba998e8..d2a16b2b19 100644 --- a/tests/protocols/internet/test_ipv6_calipso_pad_unit.py +++ b/tests/protocols/internet/test_ipv6_calipso_pad_unit.py @@ -77,7 +77,7 @@ def test_make_writes_the_pad_and_counts_it_in_the_length(self) -> None: })]) self.assertEqual(made.data, bytes.fromhex(HEADERS['cmpt_len 2, zero pad'][0])) with self.subTest(protocol=cls.__name__, pad=None): - # without a pad the option is the fixed header alone, as before + # without a pad the option is the fixed header alone made = cls(next=59, options=[(Option.CALIPSO, {'domain': 1, 'level': 5})]) self.assertEqual(made.data, bytes.fromhex(HEADERS['cmpt_len 0, no pad'][0])) diff --git a/tests/protocols/internet/test_ipv6_ext_after_fragment_unit.py b/tests/protocols/internet/test_ipv6_ext_after_fragment_unit.py index 5dd91a68e7..e7c8ee9ebe 100644 --- a/tests/protocols/internet/test_ipv6_ext_after_fragment_unit.py +++ b/tests/protocols/internet/test_ipv6_ext_after_fragment_unit.py @@ -1,14 +1,15 @@ # -*- coding: utf-8 -*- """An IPv6 extension header behind a first fragment dissects as without it. -GitHub issue #1539: :class:`~pcapkit.protocols.internet.ipv6.IPv6` ended its -extension header walk at every Fragment header, and handed what followed to the -upper-layer dispatch. That dispatch neither passes ``extension=True`` nor -``version=6``, so Shim6 failed :class:`~pcapkit.protocols.internet.ipv6_ext.IPv6_Ext`'s -version gate, and a zero-filled HIP header or a Routing header of type 3 raised -with no :class:`IPv6_Ext` to stand in. Each became a plain -:class:`~pcapkit.protocols.misc.raw.Raw` leaf, so the TCP behind it was never -dissected, though it was without the Fragment header. +GitHub issue #1539: guards against :class:`~pcapkit.protocols.internet.ipv6.IPv6` +ending its extension header walk at every Fragment header and handing what +follows to the upper-layer dispatch. That dispatch neither passes +``extension=True`` nor ``version=6``, so Shim6 fails +:class:`~pcapkit.protocols.internet.ipv6_ext.IPv6_Ext`'s version gate, and a +zero-filled HIP header or a Routing header of type 3 raises with no +:class:`IPv6_Ext` to stand in. Each becomes a plain +:class:`~pcapkit.protocols.misc.raw.Raw` leaf, so the TCP behind it is never +dissected, though it is without the Fragment header. :rfc:`8200#section-4.5` puts every header through the upper-layer one in the first fragment, at offset zero, so the walk goes on past its Fragment header. @@ -122,8 +123,9 @@ def test_first_fragment_dissects_as_without_it(self) -> None: self.assert_rebuilds(ipv6) def test_first_fragment_walks_a_dedicated_header(self) -> None: - # Destination Options reached TCP behind a first fragment before, but as - # an upper layer holding it, not as an extension header beside it + # Without the #1539 fix, Destination Options reaches TCP behind a first + # fragment too, but as an upper layer holding it, not as an extension + # header beside it from pcapkit.protocols.internet.ipv6_frag import IPv6_Frag from pcapkit.protocols.internet.ipv6_opts import IPv6_Opts diff --git a/tests/protocols/internet/test_ipv6_ext_unit.py b/tests/protocols/internet/test_ipv6_ext_unit.py index 4395d6646e..5795ee8336 100644 --- a/tests/protocols/internet/test_ipv6_ext_unit.py +++ b/tests/protocols/internet/test_ipv6_ext_unit.py @@ -313,7 +313,7 @@ def test_version_gate_rejects_non_ipv6_construction(self) -> None: alias=int(ExtensionHeader.HOPOPT)) def test_shim6_registration_does_not_leak_into_ipv4_parsing(self) -> None: - """GitHub issue #891 cross-review: registering ``IPv6_Ext`` + """GitHub issue #891: registering ``IPv6_Ext`` into the shared ``Internet.__proto__`` for Shim6 must not make an IPv4 packet whose protocol byte happens to be 140 walk an IPv6-style extension-header chain. The version gate on ``__post_init__`` sends @@ -334,7 +334,7 @@ def test_shim6_registration_does_not_leak_into_ipv4_parsing(self) -> None: # payload. self.assertEqual(str(ipv4.protochain), 'IPv4:Shim6') - # -- item 1 (cross-review): an unimplemented terminal code must not ------ + # -- #891 item 1: an unimplemented terminal code must not -------------- # -- collapse the whole packet ------------------------------------------- def test_unimplemented_terminal_code_stops_the_walk_not_the_packet(self) -> None: @@ -349,7 +349,7 @@ def test_unimplemented_terminal_code_stops_the_walk_not_the_packet(self) -> None hop limit) intact rather than losing it to a further-out ``@beholder``. ``254`` is the same code path (also has no dedicated parser, also resolves to ``Raw``); this file covers one to keep the - test proportionate, per the review's own framing. + test proportionate. GitHub issue #925: the example is 253, not ``BIT-EMU`` (147). Unlike 253/254, 147 is not in IANA's authoritative *IPv6 Extension Header @@ -390,11 +390,11 @@ class IPv6ExtSharedBaseContractTests(unittest.TestCase): the shared base of every IPv6 extension header, *and* the concrete fallback parser for one this package does not implement. - That merge removes a safety net. Before it, each implemented - header inherited :class:`~pcapkit.protocols.internet.internet.Internet` - directly, whose ``name``/``alias``/``protocol``/``length`` are generic or - abstract and whose ``__index__`` is :func:`abc.abstractmethod` -- so - forgetting one was loud. After it, the same omission silently inherits this + Serving both roles removes a safety net. A header inheriting + :class:`~pcapkit.protocols.internet.internet.Internet` directly gets a + ``name``/``alias``/``protocol``/``length`` that is generic or abstract and + an ``__index__`` that is :func:`abc.abstractmethod` -- so forgetting one is + loud. Under ``IPv6_Ext``, the same omission silently inherits this module's own *fallback-role* answers, which are wrong for a header that has an identity: it reports itself as ``IPv6 Extension Header`` / ``IPv6-Ext``, reads its length off a data model that is not its own, and ``__index__`` @@ -474,8 +474,8 @@ def _constant(klass: type, member: str) -> 'str | None': return None def test_the_family_is_exactly_the_eight_known_members(self) -> None: - """Importing :mod:`pcapkit.protocols.internet` must turn up all eight, - and only those eight. This is what keeps the per-subclass tests below + """Importing :mod:`pcapkit.protocols.internet` must turn up exactly + :attr:`KNOWN_MEMBERS`. This is what keeps the per-subclass tests below from passing vacuously on a walk that discovered nothing. """ self.assertEqual({klass.__qualname__ for klass in self._family()}, @@ -487,7 +487,7 @@ def test_the_family_is_exactly_the_eight_known_members(self) -> None: #: The maintainer's convention, from GitHub pull request #924: a header #: usable *only* as an extension header inherits ``IPv6_Ext`` alone; one #: usable as a standalone protocol names ``Internet`` (or ``IPsec``) as - #: well. The three here each travel as an IPv4 payload on a primary + #: well. Those named here each travel as an IPv4 payload on a primary #: source -- ``AH`` :rfc:`4302#section-3.1.1`, ``ESP`` #: :rfc:`4303#section-3.1.1`, ``HIP`` :rfc:`7401#appendix-C.2`, whose #: worked example is an IPv4 header carrying ``Next Header: 139``. @@ -505,7 +505,7 @@ def test_standalone_members_name_a_second_base_and_the_rest_do_not(self) -> None Asserted against ``__bases__`` rather than ``__mro__``, because every member reaches :class:`~pcapkit.protocols.internet.internet.Internet` transitively through ``IPv6_Ext`` -- so an ``__mro__`` check passes for - all eight and tests nothing. What the convention encodes is the + every member and tests nothing. What the convention encodes is the *declaration*: naming the base is how a deliberate standalone protocol is told apart from a header that merely inherits one. @@ -541,8 +541,8 @@ def test_every_subclass_defines_the_required_members_itself(self) -> None: """Checked against ``__dict__``, not :func:`getattr`. :func:`getattr` cannot tell an override from an inherited fallback-role - answer -- which is the entire failure mode -- so it would report all - five present on a subclass that defines none of them. + answer -- which is the entire failure mode -- so it would report every + required member present on a subclass that defines none of them. """ for klass in self._family(): for member in self.REQUIRED_OVERRIDES: @@ -582,7 +582,7 @@ def test_every_subclass_alias_survives_the_packet_dict_key_computation(self) -> ``IPv6_Route`` leaves ``_route``. Asserted as "no underscore, and the key is usable" rather than as - "contains a hyphen", because four of the eight legitimately carry none: + "contains a hyphen", because some legitimately carry none: ``HOPOPT``, ``MH``, ``AH`` and ``ESP`` have no ``IPv6`` prefix for the strip to bite on, so demanding a hyphen of them would be demanding a rename rather than testing an invariant. @@ -674,7 +674,7 @@ def test_base_role_protocol_delegates_past_the_fallback_answer(self) -> None: the ``__data__`` discriminator in :attr:`~pcapkit.protocols.internet.ipv6_ext.IPv6_Ext.protocol`, that ``super()`` lands on the *fallback-role* body, which reads - ``self._info.protocol`` -- a field none of the eight data models has. + ``self._info.protocol`` -- a field none of the family's data models has. Measured without the discriminator: ``AttributeError`` on every one. Both parenting shapes are exercised, since ``AH``/``ESP`` reach the base through ``IPsec``. diff --git a/tests/protocols/internet/test_ipv6_extension_unit.py b/tests/protocols/internet/test_ipv6_extension_unit.py index c19263951a..d084a668c8 100644 --- a/tests/protocols/internet/test_ipv6_extension_unit.py +++ b/tests/protocols/internet/test_ipv6_extension_unit.py @@ -98,12 +98,12 @@ def test_ipv6_route_register_routing_warns_on_overwrite(self) -> None: def test_unregistered_extension_codes_do_not_mutate_the_class_registry(self) -> None: """Parsing must not write to the shared IPv6 extension registries. - #425's defect on the three IPv6 extension header registries. Each is a - :class:`collections.defaultdict` on a class attribute shared by every - instance of its protocol in the process, so ``registry[code]`` inserted - every unrecognised option type and routing type a capture carried -- and - the value it inserted was ``'none'``, which the default factory returns - anyway. That made + Guards against #425's defect on the IPv6 extension header registries. + Each is a :class:`collections.defaultdict` on a class attribute shared by + every instance of its protocol in the process, so a ``registry[code]`` + read inserts every unrecognised option type and routing type a capture + carries -- and the value it inserts is ``'none'``, which the default + factory returns anyway. That makes :meth:`~pcapkit.protocols.internet.hopopt.HOPOPT.register_option` and its siblings warn about overwriting something nobody registered. @@ -386,7 +386,7 @@ def test_ipv6_route_make_dst_rejects_a_bool(self) -> None: ``IPv6_Route.make``'s ``dst`` is converted to an :class:`~ipaddress.IPv6Address` ahead of dispatching to the per-type constructor; a bare ``ipaddress.ip_address`` there converts ``dst=True`` to ``::1`` with no - exception at all. This is the most reachable of the four + exception at all. This is the most reachable of the #540 sites, being on the public ``make`` entry point rather than a private helper. """ @@ -1677,15 +1677,13 @@ def test_ipv6_route_rpl_fixed_area_is_four_octets(self) -> None: def test_ipv6_route_rpl_length_guard_follows_rfc6554_address_arithmetic(self) -> None: """The reader's length guard must judge ``Hdr Ext Len`` in 8-octet units. - See #564; the guard is the defect #489 flagged and deliberately left, - for want of a working RPL round trip to validate a replacement - against. It read ``if header.length % 16 != 0``, which is wrong twice - over: ``header.length`` is ``Hdr Ext Len``, *"the length of the + See #564 and #489. A guard reading ``if header.length % 16 != 0`` is + wrong twice over: ``header.length`` is ``Hdr Ext Len``, *"the length of the Routing header in 8-octet units"* (:rfc:`6554#section-3`), not an octet count; and a fixed multiple-of-16 bound assumes 16-octet addresses, which an SRH only carries when ``CmprI`` and ``CmprE`` are - both 0. Together those rejected every realistic header -- the bound - admitted only ``Hdr Ext Len`` of 0, 16, 32 ..., i.e. nothing under + both 0. Together those reject every realistic header -- the bound + admits only ``Hdr Ext Len`` of 0, 16, 32 ..., i.e. nothing under 136 octets. :rfc:`6554#section-4.2` gives the arithmetic that *is* available:: @@ -1696,7 +1694,7 @@ def test_ipv6_route_rpl_length_guard_follows_rfc6554_address_arithmetic(self) -> whole. Every case here is a **hand-built wire form**, parsed through the - public constructor. That matters for the same reason it did in #489: a + public constructor. That matters for the reason #489 gives: a construct-then-parse round trip can pass on two mistakes cancelling out, whereas octets written out by hand from the RFC diagram cannot agree with a wrong reader. @@ -1815,13 +1813,12 @@ def test_ipv6_route_rpl_construction_path_decodes_its_own_addresses(self) -> Non ``IPv6_Route.read`` hands ``_read_data_type_rpl`` the schema ``make`` just built rather than a freshly parsed one. On that path ``RPL.post_process`` sees ``addresses`` as the ``list[bytes]`` the - constructor was handed (the #556 case) and returned early without - setting ``ip`` -- so the reader raised a bare, out-of-library + constructor was handed (the #556 case); returning early there without + setting ``ip`` makes the reader raise a bare, out-of-library ``AttributeError: 'RPL' object has no attribute 'ip'``. - It was masked for as long as the ``% 16`` guard rejected every - constructed header first, which is why it only became reachable once - that guard was corrected in the same pass. + A ``% 16`` length guard masks it, by rejecting every constructed header + first. """ from pcapkit.const.ipv6.routing import Routing @@ -2535,20 +2532,20 @@ def test_ipv6_opts_and_hopopt_option_registries_agree(self) -> None: self.assertEqual(hopopt_options[key].__name__, opts_options[key].__name__) def test_ipv6_route_source_route_make_accepts_tuple_and_list_addresses(self) -> None: - """#480 fixed ``Schema.pack``'s ``ListField`` branch to accept a tuple as - well as a list, but the one consumer that ships a ``tuple[...]``-typed - field against that branch shipped with no test of its own: + """#480: ``Schema.pack``'s ``ListField`` branch accepts a tuple as well + as a list, pinned here for the consumer that declares a + ``tuple[...]``-typed field against that branch: ``SourceRoute.ip`` on the data model (``tuple[IPv6Address, ...]`` in pcapkit/protocols/data/internet/ipv6_route.py) backed by the - ``ListField`` in ``SourceRoute`` here in the schema module. Before + ``ListField`` in ``SourceRoute`` here in the schema module. Without #480, building a Type 0 (deprecated Source Route) routing header through :meth:`IPv6_Route.make ` with a tuple of source addresses raised - ``ProtocolUnbound`` where the same call with a list succeeded; this + IPv6_Route.make>` with a tuple of source addresses raises + ``ProtocolUnbound`` where the same call with a list succeeds; this pins the ``make`` -> ``pack`` path for both, through the public construction API rather than by building ``Schema_SourceRoute`` directly, and checks the two forms produce byte-identical output -- - the actual invariant #480 established (see #476). + the invariant #480 sets (see #476). Boundaries: zero addresses, one, and two, so a fix that special-cases "empty" or stops one item short of the general case is still caught. @@ -2601,7 +2598,7 @@ def make_and_pack(ip): packed_tuple = make_and_pack(as_tuple) packed_list = make_and_pack(as_list) - # the actual invariant #480 established: tuple and list of the + # the invariant #480 sets: tuple and list of the # same addresses pack identically self.assertEqual(packed_tuple, packed_list) self.assertEqual(packed_tuple, expected) diff --git a/tests/protocols/internet/test_mh_unit.py b/tests/protocols/internet/test_mh_unit.py index bc963e04ed..bfddc26af8 100644 --- a/tests/protocols/internet/test_mh_unit.py +++ b/tests/protocols/internet/test_mh_unit.py @@ -89,7 +89,7 @@ def test_mh_register_extension_warns_on_overwrite(self) -> None: def test_unregistered_mh_codes_do_not_mutate_the_class_registries(self) -> None: """Parsing must not write to ``__message__``, ``__option__`` or ``__extension__``. - #425's defect on MH's three registries. Each is a + #425's defect on MH's registries. Each is a :class:`collections.defaultdict` on a class attribute shared by every :class:`~pcapkit.protocols.internet.mh.MH` instance in the process, so ``registry[code]`` inserted every unrecognised message type, option type @@ -679,8 +679,8 @@ def make_extension(code: CGAExtension, extension: data.UnknownExtension | None = ) self.assertEqual(proto.read(extension=True).data, b'read') # this constructor returns raw bytes, and 6 + 4 octets is 6 short of - # 16, so the body is padded to align the header; before that it emitted - # 10 octets while declaring 8 + # 16, so the body is padded to align the header; without the fix it + # emits 10 octets while declaring 8 self.assertEqual(proto.make(type=custom_packet, data={'value': b'dict'}).data, b'dict' + b'\x00' * 6) unknown_message = data.UnknownMessage( @@ -765,8 +765,8 @@ def make_extension(code: CGAExtension, extension: data.UnknownExtension | None = # NOTE: The data model's ``length`` is the whole option, the schema's is # its ``Option Length`` field. A 4-octet ``PadN`` spends 2 octets on its # type and length fields, so ``Option Length`` is 2 and the schema packs - # back to the 4 octets the data model described. Asserting 4 here was - # asserting the two-octet overshoot. + # back to the 4 octets the data model described. Asserting 4 here would + # assert the two-octet overshoot. remade = proto._make_opt_pad( Option.Pad1, data.PadOption(type=Option.PadN, length=4, data=b'\x00\x00'), @@ -1704,7 +1704,7 @@ def test_mh_get_default_works_uniformly_through_the_inherited_method(self) -> No cls.get('bogus') def test_mh_local_enums_raise_and_do_not_alias(self) -> None: - """GitHub issue #880: the four RFC-inline helper enums stay immutable. + """GitHub issue #880: the RFC-inline helper enums stay immutable. Companion to :meth:`test_mh_rfc5568_local_enums_cover_unregistered_value_sets`, which covers :class:`FastBindingAcknowledgmentStatus` and @@ -1712,8 +1712,8 @@ def test_mh_local_enums_raise_and_do_not_alias(self) -> None: never had wire-level coverage in that test -- :class:`LocalizedRoutingStatus` (:rfc:`6705#section-10.2`) and :class:`LMAAddressCode` (:rfc:`5949#section-6.2.2`) -- plus three - properties the issue measured that are not specific to any one of the - four: the ``get()``-level aliasing defect, the ``_missing_`` range + properties the issue measured that are not specific to any one of + them: the ``get()``-level aliasing defect, the ``_missing_`` range invariant, and the consequence of the raise once it reaches a real capture -- which turns out to differ by wrapping protocol, see the last two subtests. @@ -1833,17 +1833,17 @@ def wrap_in_ipv4(mh_payload: bytes) -> IPv4: # all -- it walks its own ``@beholder``-decorated override # (``pcapkit/protocols/internet/ipv6.py:_import_next_layer``). # - # Before GitHub issue #891, that override let any exception from - # MH's own parser reach ``@beholder`` unchanged, which substituted - # plain ``Raw`` for the MH slot; ``Raw``'s info carries no - # ``.next``, so ``proto = info.next`` + # Without GitHub issue #891's catch, that override lets any + # exception from MH's own parser reach ``@beholder`` unchanged, + # which substitutes plain ``Raw`` for the MH slot; ``Raw``'s info + # carries no ``.next``, so ``proto = info.next`` # (``pcapkit/protocols/internet/ipv6.py:_decode_next_layer``) - # raised ``AttributeError`` on it regardless -- and it was *that*, + # raises ``AttributeError`` on it regardless -- and it is *that*, # unrelated, exception that a further-out ``@beholder`` - # (Ethernet's) actually caught, degrading the whole IPv6 packet, - # header fields and MH message alike, to ``Raw``. + # (Ethernet's) catches, degrading the whole IPv6 packet, header + # fields and MH message alike, to ``Raw``. # - # #891 gives ``IPv6._import_next_layer`` its own catch, one layer + # ``IPv6._import_next_layer`` has its own catch (#891), one layer # in from ``@beholder``: for an extension header whose wire # format RFC 6564 guarantees (Mobility Header among them), the MH # slot becomes @@ -2059,10 +2059,10 @@ def test_mh_opaque_message_body_cannot_be_padded(self) -> None: """An opaque message body is padded to alignment, with a warning. A message whose body is raw bytes carries no mobility options, so the - padding goes into the body itself. Warning and returning it unchanged was + padding goes into the body itself. Warning and returning it unchanged is not enough: ``length`` is ``(len(data) + 6) // 8 - 1``, which floors, so the - header shipped declaring 8 octets while emitting 10, 12 or 14, and a parser - reads 8 and misinterprets the rest. + header would ship declaring 8 octets while emitting 10, 12 or 14, and a + parser reads 8 and misinterprets the rest. """ from pcapkit.const.mh.packet import Packet @@ -2250,7 +2250,7 @@ def test_mh_experimental_cga_extensions_round_trip(self) -> None: The ``data.length`` assertion below is ``len(payload) + 4``, not ``len(payload) + 2`` (#512): the latter contradicts the - ``len(schema.pack())`` assertion four lines above it -- the same test + ``len(schema.pack())`` assertion above it -- the same test would prove the wire is ``len(payload) + 4`` and then assert the parsed length two octets less. The ``+ 4`` is the wire's own count: a CGA extension's Extension Type and Extension Data Length are two octets @@ -2283,8 +2283,8 @@ def test_mh_cga_extension_length_counts_the_four_octet_header(self) -> None: All three readers -- :meth:`~pcapkit.protocols.internet.mh.MH._read_ext_none`, :meth:`~pcapkit.protocols.internet.mh.MH._read_ext_multiprefix` and - :meth:`~pcapkit.protocols.internet.mh.MH._read_ext_exp` -- passed their - ``Extension Data Length`` through + :meth:`~pcapkit.protocols.internet.mh.MH._read_ext_exp` -- must not pass + their ``Extension Data Length`` through :meth:`~pcapkit.protocols.internet.mh.MH._mh_option_length`, which adds the ``2`` octets an :rfc:`6275#section-6.2` mobility option's Option Type and Option Length occupy. A CGA extension's header is twice that: per @@ -2294,9 +2294,10 @@ def test_mh_cga_extension_length_counts_the_four_octet_header(self) -> None: :rfc:`5535#section-5` says the same from the other side: its ``Ext Len`` is the length *"not including the first 4 octets"*. - So every parsed extension reported a length two octets short of what it - consumed. ``len(schema.pack())`` is the arbiter here -- the write side has - always measured the real thing, which is why only the read side was wrong. + Through that helper every parsed extension would report a length two + octets short of what it consumed. ``len(schema.pack())`` is the arbiter + here -- the write side measures the real thing, which is why only the + read side is at stake. """ from pcapkit.const.mh.cga_extension import CGAExtension from pcapkit.protocols.internet.mh import MH @@ -2370,11 +2371,11 @@ def test_mh_option_and_extension_length_helpers_do_not_share_a_header_width(self :meth:`~pcapkit.protocols.internet.mh.MH._mh_option_length` is correct and stays correct -- an :rfc:`6275#section-6.2` mobility option's Option Type and - Option Length are one octet each. What was wrong was reusing it for CGA - extensions, whose two header fields are 16 bits each + Option Length are one octet each. What it must not do is serve CGA + extensions too, whose two header fields are 16 bits each [:rfc:`4581#section-2`]. This pins both contracts side by side so the next reader picks the right one, and pins the ``+2`` against being "fixed" to - ``+4`` to suit the three callers that were the actual defect. + ``+4`` to suit the CGA extension readers, where the defect of #512 lies. """ from pcapkit.protocols.internet.mh import MH @@ -2400,14 +2401,15 @@ def test_mh_pad_option_reader_delegates_to_the_option_length_helper(self) -> Non #509 extracted :meth:`~pcapkit.protocols.internet.mh.MH._mh_option_length` so that "a future - fix to this arithmetic only has to happen once", and converted the equivalent - branch in both ``hopopt.py`` and ``ipv6_opts.py``. The ``mh.py`` branch was - left open-coding ``clen + 2``, so the arithmetic still existed twice in the - very file that introduced the helper. + fix to this arithmetic only has to happen once", and the equivalent branch of + ``hopopt.py`` and ``ipv6_opts.py`` routes through each module's own helper. + Guards against the + ``mh.py`` branch open-coding ``clen + 2``, which keeps the arithmetic twice in + the very file that holds the helper. A value assertion cannot catch that -- ``clen + 2`` and the helper return the - same number, which is why the conversion is behaviour-neutral and why - ``test_mh_padding_options_parse_from_the_wire`` passed throughout. What is + same number, so the conversion is behaviour-neutral and + ``test_mh_padding_options_parse_from_the_wire`` passes either way. What is testable is the *delegation*: patch the helper and a converted reader follows it, while an open-coded one ignores it. """ @@ -2623,7 +2625,7 @@ def test_mh_pmipv6_options_round_trip_byte_for_byte(self) -> None: self.assertEqual(rebuilt, raw) def test_mh_message_types_round_trip_byte_for_byte(self) -> None: - """Every one of the 24 registered message types must survive a round trip.""" + """Every registered message type must survive a round trip.""" from pcapkit.const.mh.option import Option from pcapkit.const.mh.packet import Packet from pcapkit.const.reg.transtype import TransType @@ -3011,7 +3013,7 @@ def test_mh_bid_rejects_an_out_of_range_binding_priority(self) -> None: """``BID-PRI`` is a 7-bit field, so ``0x80`` cannot be packed. In the same maker as #508's care-of-address conversion, and immediately - above it, so it is worth pinning that the new conversion did not move the + above it, so it is worth pinning that the conversion does not move the check out from under a valid input. """ from pcapkit.const.mh.option import Option @@ -3070,7 +3072,7 @@ def test_mh_lmaa_rejects_a_bytes_address_of_the_wrong_width(self) -> None: def test_mh_target_careof_address_suboption_round_trips_both_families(self) -> None: """The sub-option length is derived from the family, so both widths need pinning. - ``_make_fid_suboption`` is one of #508's seven sites and the only one reached + ``_make_fid_suboption`` is one of #508's sites and the only one reached through the sub-option dispatcher rather than an option maker, so its legitimate output is worth asserting byte for byte alongside the bool rejection. @@ -3178,7 +3180,7 @@ def test_mh_mn_id_option_converts_int_identifier_per_subtype(self) -> None: from that width without converting it -- ``BytesField`` then receives the ``int`` itself, and ``struct.pack()`` cannot do anything with that (#467) -- and against rejecting ``int`` outright for every subtype but - ``IPv6_Address``, as 9b26fa387 did. See #467, #468. + ``IPv6_Address``, the shape of 9b26fa387. See #467, #468. """ import io @@ -3492,13 +3494,13 @@ def test_mh_nested_suboptions_build_from_raw_kwargs(self) -> None: ``kwargs`` branch, and a field whose name collides with one of the maker's own parameters never arrives there. - That is what happened. The makers took the data model as a parameter named - ``data``, and the vendor-specific quality-of-service attribute of + Guards against the makers taking the data model as a parameter named + ``data``: the vendor-specific quality-of-service attribute of :rfc:`7222#section-4.2.11` has a *field* called ``data``, so a caller's - ``data=`` bound to the model parameter and ``kwargs.get('data')`` always saw - nothing. The payload was dropped with no exception and the length written as - though it were empty -- ``vendor`` and ``subtype`` arrived intact, which - made it look like a partial success. The parameter is ``option``, which no + ``data=`` binds to the model parameter and ``kwargs.get('data')`` always sees + nothing. The payload is dropped with no exception and the length written as + though it were empty -- ``vendor`` and ``subtype`` arrive intact, which + makes it look like a partial success. The parameter is ``option``, which no sub-option field is called. """ from pcapkit.const.mh.ani_suboption import ANISuboption diff --git a/tests/protocols/link/test_l2tp_data_export_1797_unit.py b/tests/protocols/link/test_l2tp_data_export_1797_unit.py index 1b816a9b05..ee0385477d 100644 --- a/tests/protocols/link/test_l2tp_data_export_1797_unit.py +++ b/tests/protocols/link/test_l2tp_data_export_1797_unit.py @@ -1,15 +1,16 @@ # -*- coding: utf-8 -*- """The L2TP data model is exported with its link-layer siblings (#1797). -:mod:`pcapkit.protocols.data.link` re-exported the data models of ARP, Ethernet, -Loopback and VLAN but not :mod:`pcapkit.protocols.data.link.l2tp`, although -:mod:`pcapkit.protocols.schema.link` exports the schema of the same name. The -omission was not a decision: the data model arrived in ``cdb5fef3f`` without -touching the package, and no commit has ever named ``L2TP`` in it. +:mod:`pcapkit.protocols.data.link` re-exports the data models of its children, +:mod:`pcapkit.protocols.data.link.l2tp` among them, just as +:mod:`pcapkit.protocols.schema.link` exports the schema of the same name. +Its absence (#1797) was not a decision: the data model's commit, +``cdb5fef3f``, did not touch the package, and no commit up to #1797 named +``L2TP`` in it. -:mod:`tests.project.test_public_api` could not see the gap. Its converse -assertion reads the attributes a package *holds*, and a name the package never -imported is not one of them -- so these cases check the package against the +:mod:`tests.project.test_public_api` cannot see a missing re-export. Its +converse assertion reads the attributes a package *holds*, and a name the +package never imports is not one of them -- so these cases check the package against the ``__all__`` of each of its children instead. Nothing here reads a capture, so this belongs to the unit tier. diff --git a/tests/protocols/link/test_l2tp_version_unit.py b/tests/protocols/link/test_l2tp_version_unit.py index c859a5d8fc..bda014c31e 100644 --- a/tests/protocols/link/test_l2tp_version_unit.py +++ b/tests/protocols/link/test_l2tp_version_unit.py @@ -16,15 +16,14 @@ :class:`L2TPv2` implements; it is a different header that only an ``L2TPv3`` class can read, and no such class exists in this tree. -What *was* genuinely missing is the guard that makes that reasoning -enforceable. +What these tests pin is the guard that makes that reasoning enforceable. :attr:`L2TPv2.version ` is annotated ``Literal[2]``, returns a hard-coded ``2``, and its -docstring already promises that *"a datagram carrying any other value is a -different protocol reached through a different class"* -- but -:meth:`~pcapkit.protocols.link.l2tpv2.L2TPv2.read` stored the wire nibble -unchecked, so the class reported ``version == 2`` while its own parsed data -reported ``3`` for the same octets. These tests hold the two to the same +docstring promises that *"a datagram carrying any other value is a +different protocol reached through a different class"* -- so a +:meth:`~pcapkit.protocols.link.l2tpv2.L2TPv2.read` that stores the wire nibble +unchecked has the class report ``version == 2`` while its own parsed data +reports ``3`` for the same octets. These tests hold the two to the same answer. Every case builds its own octets in memory and reads no capture under @@ -151,10 +150,10 @@ def test_a_version_nibble_other_than_two_is_refused(self) -> None: def test_no_accepted_datagram_disagrees_with_the_version_property(self) -> None: """``L2TPv2.version`` and ``info.version`` cannot report different numbers. - Before this guard the class answered ``version == 2`` from a hard-coded - ``Literal[2]`` property while ``info.version`` carried whatever the - wire said, so one datagram had two versions depending on which - attribute a consumer read. + Without this guard the class answers ``version == 2`` from a hard-coded + ``Literal[2]`` property while ``info.version`` carries whatever the + wire says, so one datagram has two versions depending on which + attribute a consumer reads. """ from pcapkit.utilities.exceptions import ProtocolError @@ -238,11 +237,11 @@ def test_l2tpv3_over_ip_degrades_to_raw_rather_than_a_fabricated_header(self) -> def test_l2tpv3_over_udp_1701_no_longer_parses_as_l2tpv2(self) -> None: """:rfc:`3931` §4.1.2 puts L2TPv3 on port 1701 too, and that is a live capture. - This is the part of the change that fixes a real dissection rather than - guarding a hypothetical registration: port 1701 is bound, so - before the version guard a v3 datagram arriving on it was dissected as - L2TPv2 and reported ``tunnelid=0x1234``/``sessionid=0x5678`` read out - of v3's Control Connection ID. :rfc:`3931` §3.2.1 requires ``Ver`` to + This is the case that guards a real dissection rather than a + hypothetical registration: port 1701 is bound, so without the version + guard a v3 datagram arriving on it is dissected as L2TPv2 and reports + ``tunnelid=0x1234``/``sessionid=0x5678`` read out of v3's Control + Connection ID. :rfc:`3931` §3.2.1 requires ``Ver`` to be 3, so the nibble is exactly the signal that this is not v2. """ diff --git a/tests/protocols/misc/pcap/test_frame_length_runtime.py b/tests/protocols/misc/pcap/test_frame_length_runtime.py index 700c4e5a3d..f8a0fb0d13 100644 --- a/tests/protocols/misc/pcap/test_frame_length_runtime.py +++ b/tests/protocols/misc/pcap/test_frame_length_runtime.py @@ -348,13 +348,9 @@ def test_block2frame_puts_the_on_wire_length_in_len(self) -> None: self.assertGreaterEqual(frame.len, frame.cap_len) # NOTE: no assertion that ``cap_len == len(frame.packet)`` here, - # which is the PCAP side's independent check. The PCAP-NG - # reader leaves the block's own ``packet`` field empty -- every - # block in ``test.pcapng`` comes back with ``packet == b''`` - # while carrying a ``captured_len`` in the hundreds -- so - # ``block2frame`` hands on nothing to measure. That is a separate - # defect from #618 and is not fixed here; asserting it would pin - # the wrong reading of the two. + # which is the PCAP side's independent check. The octets a + # PCAP-NG block carries are #646's concern, pinned in + # tests/protocols/test_pcapng_regression.py rather than here. if block.captured_len != block.original_len: truncated += 1 diff --git a/tests/protocols/misc/test_fromtimestamp_windows_range_2099_unit.py b/tests/protocols/misc/test_fromtimestamp_windows_range_2099_unit.py index 3a6d482282..309eef2164 100644 --- a/tests/protocols/misc/test_fromtimestamp_windows_range_2099_unit.py +++ b/tests/protocols/misc/test_fromtimestamp_windows_range_2099_unit.py @@ -3,9 +3,9 @@ :meth:`datetime.datetime.fromtimestamp` with a ``tz`` breaks the instant down with the C library's ``gmtime()``, and the Windows CRT's ``gmtime_s`` takes -nothing before 1970 or after 23:59:59, December 31, 3000, UTC. On the first -Windows run (#2095) a PCAP-NG packet dated in year 1 or year 9999 therefore took -the invalid-timestamp fallback and was dated 1970. +nothing before 1970 or after 23:59:59, December 31, 3000, UTC. Without the +fix, a PCAP-NG packet dated in year 1 or year 9999 therefore takes the +invalid-timestamp fallback on Windows and is dated 1970 (#2095). Windows is emulated here by a ``datetime`` whose ``fromtimestamp`` raises what CPython raises there, ``OSError(EINVAL)``, outside that range. Every reader is diff --git a/tests/protocols/misc/test_pcap_magic_lookup_1739_unit.py b/tests/protocols/misc/test_pcap_magic_lookup_1739_unit.py index 9f7912f466..7ef1dce819 100644 --- a/tests/protocols/misc/test_pcap_magic_lookup_1739_unit.py +++ b/tests/protocols/misc/test_pcap_magic_lookup_1739_unit.py @@ -1,16 +1,17 @@ # -*- coding: utf-8 -*- -"""The PCAP magic number is looked up in its one home as it was compared before. C.f. #1739. +"""The PCAP magic number is looked up in its one home, as a comparison would read it. C.f. #1739. -Moving the four magic numbers to -:data:`pcapkit.protocols.schema.misc.pcap.header.MAGIC_NUMBER` turned three -comparisons into lookups, which pins two behaviours the literals had: +The magic numbers live in +:data:`pcapkit.protocols.schema.misc.pcap.header.MAGIC_NUMBER`, and the code +that reads them looks them up there, which pins two behaviours a comparison +against literals has: * :meth:`Header._make_magic ` raises :exc:`KeyError` for a nanosecond flag that is neither, not a :exc:`StopIteration` that a generator would turn into :exc:`RuntimeError`; * :func:`~pcapkit.protocols.schema.misc.pcap.header.magic_number_callback` and :meth:`Header.read ` match a - :obj:`bytearray` magic number, which compared equal to the literals, rather + :obj:`bytearray` magic number, which compares equal to the literals, rather than reject it as unhashable. Every value is built here, so the module is in the unit tier. diff --git a/tests/protocols/misc/test_pcapng_unit.py b/tests/protocols/misc/test_pcapng_unit.py index 263aa52c28..65b0e36f77 100644 --- a/tests/protocols/misc/test_pcapng_unit.py +++ b/tests/protocols/misc/test_pcapng_unit.py @@ -447,7 +447,7 @@ def test_pcapng_context_timestamp_and_address_helpers(self) -> None: self.assertEqual(ts_datetime, datetime.datetime.fromtimestamp(12, tz)) self.assertEqual(ts_decimal, decimal.Decimal(12)) self.assertEqual(ts_datetime.timestamp(), float(ts_decimal)) - # and the round trip closes, which it could not while the two disagreed + # and the round trip closes, which needs the two to agree self.assertEqual(pcapng._make_timestamp(ts_decimal), (0, 2000)) self.assertEqual(pcapng._read_mac_addr(b'\x00\x01\x02\x03\x04\x05'), '00:01:02:03:04:05') @@ -1407,16 +1407,15 @@ def test_pcapng_record_and_secrets_readers_cover_dispatch(self) -> None: self.assertEqual(PCAPNG.__dict__['__secrets__'][SecretsType.TLS_Key_Log], 'tls') def test_tls_key_label_tracks_rfc_9850_plus_documented_rsa_exception(self) -> None: - # GitHub issue #886: the canonical definition moved from a hand-written - # class in this module to a generated one under - # :mod:`pcapkit.const.pcapng.tls_key_label`. The re-export once left at - # ``pcapkit.protocols.misc.pcapng.TLSKeyLabel`` was dropped under #1744; + # GitHub issue #886: the canonical definition is the generated one under + # :mod:`pcapkit.const.pcapng.tls_key_label`, with no re-export at + # ``pcapkit.protocols.misc.pcapng.TLSKeyLabel`` (#1744); # tests/project/test_no_moved_name_re_exports_1744_unit.py pins that. from pcapkit.const.pcapng.tls_key_label import TLSKeyLabel - # RFC 9850 :rfc:`9850#section-4.2`'s "TLS SSLKEYLOGFILE Labels" registry - # (10 rows as of 2026-09-28), plus the NSS-historical ``RSA`` label this - # module documents and retains -- it has no row of its own. + # RFC 9850 :rfc:`9850#section-4.2`'s "TLS SSLKEYLOGFILE Labels" registry, + # plus the NSS-historical ``RSA`` label this module documents and + # retains -- it has no row of its own. expected = { 'RSA', 'CLIENT_RANDOM', @@ -1448,8 +1447,7 @@ def test_tls_key_label_tracks_rfc_9850_plus_documented_rsa_exception(self) -> No self.assertIs(TLSKeyLabel('RSA'), TLSKeyLabel.RSA) # Matching stays by value, not name -- lower-case never resolves, and - # an unrecognised label still raises rather than minting, unchanged - # from before this class was generated. + # an unrecognised label raises rather than minting. with self.assertRaises(ValueError): TLSKeyLabel('client_random') with self.assertRaises(ValueError): @@ -2847,8 +2845,8 @@ def test_pcapng_read_block_packet_resolves_linktype_without_info(self) -> None: def _all_block_schemas(): """Every PCAP-NG block schema, populated well enough to be packed. - Returns a list of ``(label, block type, schema)`` triples covering all - eleven block schemas of + Returns a list of ``(label, block type, schema)`` triples covering every + block schema of :mod:`pcapkit.protocols.schema.misc.pcapng`, each with the block total length its own fields imply. @@ -3086,15 +3084,15 @@ def test_pcapng_section_header_block_round_trips_through_bytes(self) -> None: self.assertEqual(statistics[0].interface_id, 0) ########################################################################## - # Signed numeric fields (#366, uncovered by the SHB repro). + # Signed numeric fields (#366). ########################################################################## def test_signed_number_fields_pack_negative_values(self) -> None: - # Uncovered while fixing #366: ``NumberField.pre_process`` masks the - # value against the *unsigned* bit mask, which turns any negative value - # into a pattern ``struct.pack`` rejects for a signed template. No - # signed field in the library could write a negative value, which is - # what a PCAP-NG section length of -1 (not specified) needs. + # #366: guards against ``NumberField.pre_process`` masking the value + # against the *unsigned* bit mask, which turns any negative value into a + # pattern ``struct.pack`` rejects for a signed template. Under it no + # signed field in the library can write a negative value, which is what + # a PCAP-NG section length of -1 (not specified) needs. from pcapkit.corekit.fields.numbers import (Int8Field, Int16Field, Int32Field, Int64Field, UInt32Field) @@ -3120,11 +3118,11 @@ def test_signed_number_fields_pack_negative_values(self) -> None: ########################################################################## def test_pcapng_make_block_uses_its_own_option_namespace(self) -> None: - # Regression for #365: all eight ``_make_block_*`` methods passed - # ``namespace='shb'``, which is not a namespace anywhere -- neither on - # ``OptionType`` nor in the schema option registry -- so every raw-bytes - # option was misclassified as ``shb_unknown_*`` and ``OptionType`` was - # permanently extended with a member per unrecognised code. + # Regression for #365: guards against the ``_make_block_*`` methods + # passing ``namespace='shb'``, which is not a namespace anywhere -- + # neither on ``OptionType`` nor in the schema option registry -- so every + # raw-bytes option is misclassified as ``shb_unknown_*`` and + # ``OptionType`` permanently extended with a member per unrecognised code. from pcapkit.const.pcapng.block_type import BlockType from pcapkit.const.pcapng.option_type import OptionType from pcapkit.const.pcapng.secrets_type import SecretsType @@ -3364,12 +3362,11 @@ def test_pcapng_big_endian_sample_section_options_are_intact(self) -> None: # further up the stack, where subclassing FileNotFoundError is what keeps # the outer handler working. # - # It also drops a dependency on pytest's working directory, which the - # relative os.path.join('examples', ...) this replaces quietly had. Run - # from anywhere but the repository root that path never resolved, so this - # test skipped with "run make_samples.py first" on a tree that in fact had - # every fixture -- a silent hole rather than a failure. Measured from /tmp: - # this version passes where the previous one skipped. + # It also avoids depending on pytest's working directory. A relative + # os.path.join('examples', ...) resolves only from the repository root, + # so run from anywhere else this test would skip with "run + # make_samples.py first" on a tree that in fact has every fixture -- a + # silent hole rather than a failure. from pcapkit.const.pcapng.option_type import OptionType from pcapkit.interface import extract @@ -3599,8 +3596,8 @@ def test_an_option_declaring_more_than_its_area_keeps_the_area(self) -> None: """#594's vector, one block: 65,535 declared against a four-octet area. The area is exactly the option header, so there is nothing left for the - payload. Before any bound this read ``b''`` off an exhausted stream and - zero-padded it to 65,535 octets -- from a 40-octet block. + payload. Without a bound this reads ``b''`` off an exhausted stream and + zero-pads it to 65,535 octets -- from a 40-octet block. """ options = over_declared(UNKNOWN_OPT, b'', 0xFFFF) @@ -3813,8 +3810,8 @@ def test_packing_an_option_is_not_shortened_by_an_unknown_length(self) -> None: """An option still packs when no length is known. :meth:`Schema.pack` leaves ``__length__`` at ``-1`` when no length is - known. This passes with or without the bound's negative-remainder guard, - and the cross-review of #676 is what established that: both + known. This passes with or without the bound's negative-remainder guard + (#676): both :class:`~pcapkit.corekit.fields.strings.BytesField` and :class:`~pcapkit.corekit.fields.strings.StringField` repair a negative width in ``pre_process``, resetting it to ``len(value)``, so the pack @@ -3859,10 +3856,10 @@ def test_a_block_declaring_more_room_than_it_holds_keeps_what_it_holds(self) -> Block Total Length is checked only against its own trailing copy, never against the file, so a block may declare 1,000,000 octets while holding 36. Its option area is then sized at 999,964, and an option declaring - 65,535 is comfortably under that: unbounded, this synthesised 65,535 + 65,535 is comfortably under that: unbounded, this synthesises 65,535 octets of zeros from a 36-octet buffer -- 1,820x, with no warning at - all. Found by the cross-review of #676, which is why the area is bounded - by the octets the block holds. + all. That is why the area is bounded by the octets the block holds + (#676). The block still parses, and reports the length it declared; its option area is the four octets it holds, kept as captured, and the trailing @@ -4138,8 +4135,8 @@ def test_a_tail_too_short_for_a_block_is_reported_as_end_of_stream(self) -> None than clamped, because at the end of the file no block is being read: the clamp-and-warn of #676 keeps a truncated *block* parsing, where inventing a whole block out of zero padding would only fabricate a frame. It is the - same signal ``prepare`` already raises for a remainder of exactly zero -- - one, two and three octets merely fell through it. + same signal ``prepare`` raises for a remainder of exactly zero, extended + to one, two and three octets. """ from pcapkit.protocols.misc.pcapng import PCAPNG @@ -4302,17 +4299,16 @@ def test_every_length_callback_in_the_module_is_floored_at_zero(self) -> None: self.assertGreaterEqual(len(exercised), 70) def test_a_field_sized_by_the_remaining_length_whole_is_left_alone(self) -> None: - """The three decryption-secrets payloads are exempt, and measurably so. + """The decryption-secrets payloads are exempt, and measurably so. ``UnknownSecrets.data``, ``TLSKeyLog.data`` and ``WireGuardKeyLog.data`` read ``__length__`` whole rather than subtracting from it, and :meth:`Schema.pack ` leaves it at ``-1`` when no length is known -- so flooring them at zero packs - *nothing*. Measured: it emptied both secrets payloads, and the two - ``EXPECTED_FAILURES`` entries for them in - ``tests/protocols/test_option_roundtrip_unit.py`` then came back ``OK`` - rather than ``MISMATCH``, because an empty payload compares equal to an - empty payload. A regression that reads as a fix, which is why this is + *nothing*. Measured: it empties the secrets payloads, which the + round-trip sweep in ``tests/protocols/test_option_roundtrip_unit.py`` + reads as ``OK``, because an empty payload compares equal to an empty + payload. A regression that reads as a fix, which is why this is pinned rather than left to the reader. """ @@ -4467,11 +4463,11 @@ def _assert_journal_kept(self, entry: bytes, reason: str): return caught def test_a_journal_binary_field_cut_short_of_its_length_is_kept_as_captured(self) -> None: - """The reviewer's case: fewer than eight octets for a 64-bit length. + """Fewer than eight octets for a 64-bit length. ``struct.unpack(' NUL is valid UTF-8, so a text field's value may end in one. Given its own trailing newline, this line is one ``readline()`` call away from - the block's own padding -- exactly as it always was -- so #794's fix, + the block's own padding, so #794's fix, which only triggers when that newline is missing, must leave it untouched. @@ -4736,7 +4732,7 @@ def test_a_terminated_journal_value_ending_in_a_nul_octet_is_kept_whole(self) -> if item.category.__name__ == 'SchemaWarning'], []) def test_a_terminator_less_line_whose_raw_tail_is_whitespace_is_kept_as_captured(self) -> None: - """Cross-review on #795: a whitespace tail is not a newline. + """#795: a whitespace tail is not a newline. ``bytes.strip()`` would take the whitespace and expose NUL octets before it as if they were padding. The line has no terminating newline, @@ -4879,10 +4875,11 @@ class NamedBuffer(io.BufferedReader): class PCAPNGOptionRegistryGuardTests(unittest.TestCase): """Registering an option schema over another must say so. - ``Option.register`` wrote into its namespace dictionaries with no check at - all, so a second registration for the same code replaced the first in - silence and every later lookup got the replacement. #681 added the same guard - to ``register_protocol``; this is the sibling on the schema side. + Guards against ``Option.register`` writing into its namespace dictionaries + with no check at all, under which a second registration for the same code + replaces the first in silence and every later lookup gets the replacement. + #681 guards ``register_protocol`` the same way; this is the sibling on the + schema side. """ @@ -4939,8 +4936,7 @@ def test_re_registering_an_option_code_reports_the_collision(self) -> None: The guard is identity-based, matching every sibling registrar: presence alone is not the test, only whether the incumbent differs from the replacement. ``if_name`` already names ``IF_NameOption``, so replacing - it with ``UnknownOption`` is a real displacement, and it is reported - exactly as it was before the guard changed. + it with ``UnknownOption`` is a real displacement, and it is reported. """ from pcapkit.const.pcapng.option_type import OptionType @@ -4966,7 +4962,7 @@ def test_an_opt_namespace_collision_names_every_namespace_it_displaced(self) -> The ``opt`` namespace fans a single registration out across every namespace there is, and a warning per namespace would report one mistake - seven times. Naming them in one message says the same thing and stays + once per namespace. Naming them in one message says the same thing and stays greppable. """ @@ -5044,10 +5040,9 @@ def test_repeated_code_list_registering_the_same_class_is_silent(self) -> None: ``__init_subclass__`` loops over ``code`` with no deduplication, so a repeated entry reaches :meth:`Option.register` twice with the *same* - class -- the second call finds itself already the incumbent. Before - the identity guard this still hit the presence-only check and warned - about an overwrite that never happened, since the class was - overwriting itself. + class -- the second call finds itself already the incumbent. A + presence-only check would warn about an overwrite that never happens, + since the class is overwriting itself. """ from pcapkit.const.pcapng.option_type import OptionType @@ -5076,8 +5071,8 @@ def test_repeated_code_registration_still_warns_once_for_a_genuine_displacement( already names a different incumbent: two calls to :meth:`Option.register` with the same new class. The first call genuinely displaces ``IF_NameOption`` and must warn; the second finds itself already the - incumbent and must not warn again. The presence-only guard warned on - both, reporting the same displacement twice. + incumbent and must not warn again. A presence-only guard warns on both, + reporting the same displacement twice. """ from pcapkit.const.pcapng.option_type import OptionType diff --git a/tests/protocols/schema/test_bitfield_coverage_unit.py b/tests/protocols/schema/test_bitfield_coverage_unit.py index 4de86add31..ffbe81539a 100644 --- a/tests/protocols/schema/test_bitfield_coverage_unit.py +++ b/tests/protocols/schema/test_bitfield_coverage_unit.py @@ -43,8 +43,8 @@ #: Bit fields allowed to leave bits unnamed, as ``schema.field`` (relative to #: :mod:`pcapkit.protocols.schema`) -> the ``(start, length)`` bit ranges left -#: uncovered. Every known site has been fixed, so this is empty, and it must -#: stay empty unless a new gap is filed as a GitHub issue. An entry must match +#: uncovered. It is empty, and must stay empty unless a new gap is filed as a +#: GitHub issue. An entry must match #: exactly: once its site is fixed the entry has to go, and a site that changes #: shape has to be looked at again. #: diff --git a/tests/protocols/schema/test_enum_schema_registry_unit.py b/tests/protocols/schema/test_enum_schema_registry_unit.py index 2563b79262..2cc04b4282 100644 --- a/tests/protocols/schema/test_enum_schema_registry_unit.py +++ b/tests/protocols/schema/test_enum_schema_registry_unit.py @@ -481,8 +481,8 @@ class Replacement(BaseSchema, code=Code.one): def test_declaring_a_subclass_with_fresh_codes_stays_quiet(self) -> None: """Every ordinary schema declaration must stay silent. - This is the case that governs whether ``import pcapkit`` is noisy: 326 - registry writes happen during import, and a guard that warned on a + This is the case that governs whether ``import pcapkit`` is noisy: + hundreds of registry writes happen during import, and a guard that warned on a first-time declaration would fire on the great majority of them. Covers the iterable form of ``code`` as well, which is the branch that shares the guard. @@ -503,8 +503,8 @@ class Several(BaseSchema, code=(Code.two, Code.three)): self.assertEqual(self._registry_warnings(caught, RegistryWarning), []) - # The iterable branch was restructured into a single loop to carry the - # guard once; assert it still registers every code it was given. + # The iterable branch carries the guard in a single loop; assert it + # registers every code it is given. self.assertIs(BaseSchema.registry[Code.one], Single) self.assertIs(BaseSchema.registry[Code.two], Several) self.assertIs(BaseSchema.registry[Code.three], Several) diff --git a/tests/protocols/schema/test_schema_metaclass_abc_cache_unit.py b/tests/protocols/schema/test_schema_metaclass_abc_cache_unit.py index 0588e28c0a..854311d634 100644 --- a/tests/protocols/schema/test_schema_metaclass_abc_cache_unit.py +++ b/tests/protocols/schema/test_schema_metaclass_abc_cache_unit.py @@ -49,15 +49,15 @@ class keyword collision described in that module. Bypassing it also skipped ``False`` for ``dict`` -- but caches it in the shared object's *negative* cache, and since that object is ``Mapping``'s own, a plain ``dict`` then stops answering ``isinstance(x, Mapping)`` at all, anywhere in the process, - until the cache token advances. Measured directly against the pre-fix code - (not merely inferred): asking ``isinstance({}, Schema)`` and then + until the cache token advances. Measured directly on a shared cache (not + merely inferred): asking ``isinstance({}, Schema)`` and then ``isinstance({}, Mapping)`` returns ``False`` for the second question too. This is the more dangerous direction, since it breaks code with no connection to schemas at all -- ordinary library code such as :meth:`Info.from_dict ` asking ``isinstance(dict_, (dict, collections.abc.Mapping))`` is enough to trigger - either direction, which is why the defect only ever showed up in the full - suite and not in a single test file run alone. + either direction, which is why the defect shows up in the full suite and + not in a single test file run alone. """ @@ -67,9 +67,9 @@ def setUp(self) -> None: # first assertion below true for the wrong reason. # :func:`reimport_once_per_class` resets it on every call. The classes # are the class's own import, kept across its tests (GitHub issue - # #1538): on the #439 defect every schema class shares ``Mapping``'s - # cache, so that reset reaches it too, and reverting the fix fails the - # same tests either way. A couple of tests deliberately poison the + # #1538): with the #439 defect present every schema class shares + # ``Mapping``'s cache, so that reset reaches it too, and the same tests + # fail either way. A couple of tests deliberately poison the # shared caches, so ``restore=True`` resets them again on the way out # of the class, and nothing outside it reads what was left behind. reimport_once_per_class(self, restore=True) @@ -101,11 +101,11 @@ def test_a_plain_dict_is_not_a_schema_after_mapping_is_asked_first(self) -> None def test_asking_schema_first_does_not_break_mapping_for_a_plain_dict(self) -> None: """The reverse direction: ``Schema`` first must not corrupt ``Mapping``. - Before this class had its own ``_abc_impl``, asking ``isinstance({}, - Schema)`` computed a fresh (correct) ``False`` for ``dict`` and cached - it in the *negative* cache of the object it shared with + Without its own ``_abc_impl``, asking ``isinstance({}, Schema)`` + computes a fresh (correct) ``False`` for ``dict`` and caches it in the + *negative* cache of the object it shares with :class:`collections.abc.Mapping` -- after which a plain :class:`dict` - stopped answering ``isinstance(x, Mapping)`` at all. This is the + stops answering ``isinstance(x, Mapping)`` at all. This is the direction most likely to be missed: it has nothing to do with ``Schema`` being asked about a dict that should not match it, and everything to do with an *unrelated* stdlib question breaking as a @@ -347,8 +347,7 @@ def test_an_ordinary_class_keyword_is_unaffected(self) -> None: `), and it does not appear anywhere in :attr:`~pcapkit.protocols.schema.schema.SchemaMeta._RESERVED_CLASS_KWARGS`, - so it must keep working exactly as before -- as must the renamed - ``ns=`` keyword itself. + so it must keep working -- as must the ``ns=`` keyword itself. """ from pcapkit.const.pcapng.option_type import OptionType as Enum_OptionType diff --git a/tests/protocols/schema/test_schema_unit.py b/tests/protocols/schema/test_schema_unit.py index 3b2d9deb9c..d329d8ed05 100644 --- a/tests/protocols/schema/test_schema_unit.py +++ b/tests/protocols/schema/test_schema_unit.py @@ -321,9 +321,9 @@ class HeaderSchema(Schema): spare: int = UInt8Field() trailer: list[int] = ListField(length=2, item_type=UInt8Field()) - # the guard read ``hasattr(cls, '__init__')``, which every class satisfies - # through :obj:`object`, so the generated method was never installed and - # ``__init__`` stayed bound to ``Schema.__update__`` + # a guard reading ``hasattr(cls, '__init__')``, which every class + # satisfies through :obj:`object`, never installs the generated method, + # and ``__init__`` stays bound to ``Schema.__update__`` self.assertIsNot(HeaderSchema.__init__, Schema.__update__) self.assertEqual(HeaderSchema.__init__.__qualname__, 'HeaderSchema.__init__') @@ -505,7 +505,7 @@ def test_schema_option_field_unpack_reports_a_field_relative_offset(self) -> Non :class:`~pcapkit.corekit.fields.misc.SchemaField` hands the live file straight down. Reporting the raw stream position then names an offset outside the field: the same three-octet area that fails at offset 3 below - reported offset 5 when the stream was two octets in. + would report offset 5, the stream being two octets in. """ from pcapkit.utilities.exceptions import FieldValueError diff --git a/tests/protocols/test_construction_keyword_check_unit.py b/tests/protocols/test_construction_keyword_check_unit.py index db06673c43..11df0900fa 100644 --- a/tests/protocols/test_construction_keyword_check_unit.py +++ b/tests/protocols/test_construction_keyword_check_unit.py @@ -15,10 +15,10 @@ declared seq = 1 built info.seq = 0 warnings captured: [] -Twenty-five generated fixture frames carried sequence number ``0`` while the -generator said ``1``, one octet per frame, and the only reason anybody found out -was that somebody read the mapping against the signature. Issues #541 and #556 -were the same family of silence. +Without the fix, twenty-five generated fixture frames carry sequence number +``0`` while the generator says ``1``, one octet per frame, and nothing but +reading the mapping against the signature reveals it. Issues #541 and #556 are +the same family of silence. The schema layer is not so permissive: :meth:`Schema.__update__ ` warns @@ -298,13 +298,13 @@ def test_the_exception_is_the_one_the_library_already_uses(self) -> None: one question rather than adding a second. The honest caveat, recorded so it is a decision rather than an oversight: - all five of those reject a *class* keyword at ``__init_subclass__`` time, + all of those reject a *class* keyword at ``__init_subclass__`` time, which is a narrower kind of "unexpected keyword" than a field value passed to a constructor. And ``UnsupportedCall`` carries :exc:`AttributeError`, where a reader expecting the stdlib's ``unexpected keyword argument`` would reach for :exc:`TypeError` -- and where an ``except AttributeError`` written - for duck-typing could swallow this. Consistency with the five precedents was - preferred to a sixth spelling of the same idea, but the trade is real. + for duck-typing could swallow this. Consistency with those precedents is + preferred to another spelling of the same idea, but the trade is real. """ from pcapkit.protocols.transport.tcp import TCP @@ -492,7 +492,7 @@ def test_out_of_band_keywords_are_accepted_while_constructing(self) -> None: name. A regression here would refuse a caller that is setting a parse limit on something it is constructing. - ``packet`` is the fifth, and is there for a different reason -- see + ``packet`` is one more, and is there for a different reason -- see :meth:`test_from_data_survives_the_injected_packet_keyword`. """ @@ -575,7 +575,7 @@ def test_a_dispatcher_may_decline_the_check(self) -> None: # The opt-out is not inherited. Both versioned classes declare their # keywords in full, and exempting them along with their base would forgo - # the check on the only two HTTP classes that can carry it -- measured + # the check on the HTTP classes that can carry it -- measured # without the ``klass is cls`` guard, every HTTP subclass comes back # unchecked. for versioned, keyword in ((HTTPv1, 'http_version'), (HTTPv2, 'sid')): @@ -680,24 +680,25 @@ class SentinelTests(unittest.TestCase): That marker is :data:`~pcapkit.corekit.sentinels.ABSENT`, an instance of :class:`~pcapkit.corekit.sentinels.AbsentType` following :class:`~pcapkit.corekit.sentinels.NoValueType`, which is how this library - already spells a singleton marker. It was a bare ``object()`` when #640 was - first raised; a bare ``object()`` has no name in a traceback, no informative - :func:`repr`, and no type a checker can hold anyone to. + already spells a singleton marker. It is not a bare ``object()`` (#640): a + bare ``object()`` has no name in a traceback, no informative :func:`repr`, + and no type a checker can hold anyone to. - The four tests here split two and two, and the split is worth knowing before + The tests here split into two pairs, and the split is worth knowing before reading them: :meth:`test_the_absent_marker_is_an_instance_of_its_own_type` and :meth:`test_the_absent_marker_cannot_be_confused_with_another_singleton` - pin the marker's *shape*. Both name ``ABSENT``, so on the bare - ``object()`` they fail at the import -- which makes them a check that the - rename happened, with the substance behind the gate. + pin the marker's *shape*. Both name ``ABSENT``, so against a bare + ``object()`` marker they fail at the import -- which makes them a check + on the name, with the substance behind the gate. :meth:`test_the_absent_marker_never_reaches_the_accepted_names` and :meth:`test_the_three_answers_stay_distinct_across_the_swap` - pin the *behaviour*, and **both pass on the bare** ``object()`` **too**. - That is the point of them rather than a weakness: they are the evidence - that the swap changed nothing, which a shape assertion cannot give. What + pin the *behaviour*, and **both pass against a bare** ``object()`` + **too**. That is the point of them rather than a weakness: they are the + evidence that the marker's type changes no behaviour, which a shape + assertion cannot give. What they catch is the mistake this kind of swap actually makes -- comparing against a second instance of the right class, which reads correctly and typechecks -- measured by mutating the ``.get`` default to a fresh @@ -717,7 +718,7 @@ def test_the_absent_marker_is_an_instance_of_its_own_type(self) -> None: self.assertIsInstance(ABSENT, AbsentType) - # The point of #640's review comment: ``type(object())`` is ``object``, + # The point of #640: ``type(object())`` is ``object``, # which says nothing about what the value is for. self.assertIsNot(type(ABSENT), object) diff --git a/tests/protocols/test_dispatch_bindings_unit.py b/tests/protocols/test_dispatch_bindings_unit.py index 6eb99dbceb..55610eeff0 100644 --- a/tests/protocols/test_dispatch_bindings_unit.py +++ b/tests/protocols/test_dispatch_bindings_unit.py @@ -115,9 +115,9 @@ def extract(self, *frames: bytes): def test_stacked_service_and_customer_tags_both_parse_and_stay_distinct(self) -> None: """A Q-in-Q frame yields both tags, under their own info names. - This is the acceptance test for the S-Tag work: before the service tag - was bound, the whole of ``S-Tag + C-Tag + IPv4 + TCP`` collapsed into a - single opaque ``Raw`` payload hanging off the Ethernet header. + This is the acceptance test for the S-Tag binding: without it, the + whole of ``S-Tag + C-Tag + IPv4 + TCP`` collapses into a single opaque + ``Raw`` payload hanging off the Ethernet header. """ from pcapkit.const.reg.ethertype import EtherType @@ -180,10 +180,10 @@ def test_single_customer_tag_is_unchanged_by_the_service_tag_binding(self) -> No def test_ospf_parses_when_dispatched_from_ip_protocol_89(self) -> None: """OSPF is reachable from ``TransType.OSPFIGP`` and parses its header. - The dissector could not parse anything at all before: ``read`` consulted - the schema *class* rather than the parsed header, and ``alias`` -- which - the dispatch reads while ``read`` is still running -- reached for an - ``_info`` that does not exist yet. + Guards against a dissector that parses nothing at all: a ``read`` that + consults the schema *class* rather than the parsed header, and an + ``alias`` -- which the dispatch reads while ``read`` is still running -- + reaching for an ``_info`` that does not exist yet. """ from pcapkit.const.ospf.authentication import Authentication diff --git a/tests/protocols/test_dispatch_default_resolution_unit.py b/tests/protocols/test_dispatch_default_resolution_unit.py index 15171efb08..8ac8f320bf 100644 --- a/tests/protocols/test_dispatch_default_resolution_unit.py +++ b/tests/protocols/test_dispatch_default_resolution_unit.py @@ -4,24 +4,24 @@ :class:`~pcapkit.corekit.module.ModuleDescriptor` the registry's default factory produces -- normally :class:`~pcapkit.protocols.misc.raw.Raw`. That resolution is deliberately **not** written back into the registry, because the registry is a -class-level :class:`collections.defaultdict` and recording a miss in it is the -defect GitHub issue #421 reported and pull request #426 fixed at this layer, -pull request #428 extended to the option, chunk and block registries, and pull -request #560 fixed at the schema layer. The cost of not writing it back is that +class-level :class:`collections.defaultdict` and recording a miss in it is +GitHub issue #421's defect, guarded at this layer by pull request #426, at the +option, chunk and block registries by pull request #428, and at the schema layer +by pull request #560. The cost of not writing it back is that every unrecognised frame resolves the same descriptor again. A registered descriptor is not written back either (GitHub issue #1559), so an extraction of :file:`many_interfaces.pcapng` performs 296 resolutions, 48 of them fallbacks. -Proposed by @Ts-Boom in GitHub pull request #563, which paid that cost with a -class-level cache of resolved classes and no invalidation -- so a -:func:`importlib.reload` left it serving the pre-reload class forever. These +GitHub pull request #563 proposes paying that cost with a class-level cache of +resolved classes and no invalidation, under which an :func:`importlib.reload` +leaves it serving the pre-reload class forever. These tests pin the shape taken instead: the repeats are cheap because :attr:`ModuleDescriptor.klass ` reads :data:`sys.modules` rather than re-entering :func:`importlib.import_module`, and nothing anywhere retains the class. -Three properties, and the split is deliberate -- the first says the cost is -gone, the other two say what it was not allowed to cost: +The properties split deliberately -- the first pins the saving, the others say +what it is not allowed to cost: :meth:`DefaultDescriptorResolutionTests.test_repeated_miss_does_not_re_enter_the_import_machinery` The saving itself, as a call count rather than as a timing. Fails without @@ -80,7 +80,7 @@ def _dummy_protocol(self) -> type: ``__proto__`` is a fresh, empty :class:`collections.defaultdict` declaring the same :class:`~pcapkit.corekit.module.ModuleDescriptor` fallback the real registries do. Fresh, so that a miss recorded by - accident is visible as a key rather than hidden among the ~90 + accident is visible as a key rather than hidden among the many registrations a real table carries. """ diff --git a/tests/protocols/test_dispatch_reachability_unit.py b/tests/protocols/test_dispatch_reachability_unit.py index ebdbe4a69e..3c77ba9f73 100644 --- a/tests/protocols/test_dispatch_reachability_unit.py +++ b/tests/protocols/test_dispatch_reachability_unit.py @@ -1,26 +1,22 @@ # -*- coding: utf-8 -*- """Every class that declares a dispatch code is reachable under it. -GitHub issue #548 asked for this directly: - - No test asserts that every ``TransType`` member with an implementing class - is reachable through dispatch, which is why this survived. Such a test - would be broadly useful -- it would catch the next missing registration - rather than just this one. +GitHub issue #548 asks for this directly: a test asserting that every +``TransType`` member with an implementing class is reachable through dispatch +catches the next missing registration, not just one. :file:`test_dispatch_registry_unit.py` walks the problem from the other end: it takes each ``__proto__`` entry that *exists* and checks the class it -names can parse a packet. That cannot see a class nobody registered at all, -which is exactly how :class:`~pcapkit.protocols.application.ospf.OSPF` shipped -reachable from no table (fixed in #436). This module walks it from the class -side instead -- every +names can parse a packet. That cannot see a class nobody registered at all +-- #436's :class:`~pcapkit.protocols.application.ospf.OSPF` case, a class +reachable from no table. This module walks it from the class side instead -- every :class:`~pcapkit.protocols.protocol.ProtocolBase` descendant whose :meth:`~pcapkit.protocols.protocol.Protocol.__index__` returns an enum member is *claiming* to be reachable under that code, so the claim is checked. Which registry a code belongs in is not restated here. It is read from :data:`~pcapkit.foundation.registry.protocols._CODE_DESTINATIONS`, the enum -type -> destination table #570 shipped to back ``code=``, so the audit and the +type -> destination table that backs ``code=`` (#570), so the audit and the registration mechanism cannot drift apart: an enum type added to that table gets audited here the same day. @@ -160,9 +156,9 @@ def test_every_indexed_class_is_reachable_under_the_code_it_declares(self) -> No f'{gap.found!r}' for gap in gaps)) # A walk that found nothing to check would also report no gaps, so pin - # the floor. 23 claims verified when this landed -- the 21 distinct - # classes plus ``InARP`` and ``DRARP`` reaching their siblings' entries, - # and ``Ethernet`` counted once for ``Frame`` and once for ``PCAPNG``. + # a floor of 23 claims (measured: the 21 distinct classes plus ``InARP`` + # and ``DRARP`` reaching their siblings' entries, and ``Ethernet`` + # counted once for ``Frame`` and once for ``PCAPNG``). self.assertGreaterEqual(checked, 23) def test_the_audit_detects_a_code_that_nothing_dispatches(self) -> None: diff --git a/tests/protocols/test_dispatch_registry_unit.py b/tests/protocols/test_dispatch_registry_unit.py index 782cde84d7..0e1c523b6e 100644 --- a/tests/protocols/test_dispatch_registry_unit.py +++ b/tests/protocols/test_dispatch_registry_unit.py @@ -3,19 +3,16 @@ GitHub issue #496: the ``__proto__`` dispatch registries decide which :class:`~pcapkit.protocols.protocol.Protocol` subclass parses the next layer. -#496 named the original tables; #904 added -``Internet.__proto__[TransType.Shim6]``, and #1574 added the loopback table. -Nothing walked all of them. +#496 names the tables, joined by #904's +``Internet.__proto__[TransType.Shim6]`` and #1574's loopback table. :file:`test_registry_runtime.py` checks that a table entry *resolves* to the right class object, and :file:`test_dispatch_bindings_unit.py` checks that a hand-picked set of codes actually parse a packet into the class the table names -- the property that -matters, since a resolvable entry whose target cannot parse a packet is -exactly what shipped once: :class:`~pcapkit.protocols.application.ospf.OSPF` was -reachable from no table at all, per that module's own docstring. +matters, since an entry can resolve to a target that cannot parse a packet. -This module closes the gap the same way -:file:`test_option_roundtrip_unit.py` closed it for the option/parameter +This module covers every table the same way +:file:`test_option_roundtrip_unit.py` covers the option/parameter registries: :func:`examples.generators.dispatch.cases` walks every ``__proto__`` table directly, so a code registered tomorrow gets a case here tomorrow -- or fails :meth:`DispatchRegistryTests @@ -194,10 +191,10 @@ def test_registry_currently_matches_pinned_target(self) -> None: registry size unchanged, code still present, class wrong. Measured: pointing ``Internet.__proto__[TransType.AH]`` at :class:`~pcapkit.protocols.misc.raw.Raw` instead of - :class:`~pcapkit.protocols.internet.ah.AH` changed nothing - :meth:`test_cases_cover_every_table_named_in_the_issue` or the old, - registry-derived version of :meth:`test_dispatch_reaches_target_or_is_a_recorded_degrade` - looked at, and both kept passing. This does not: it compares + :class:`~pcapkit.protocols.internet.ah.AH` changes nothing + :meth:`test_cases_cover_every_table_named_in_the_issue` looks at, nor + anything a registry-derived dispatch check would, and both pass. This + test does not: it compares :attr:`Case.registered` -- what the table says right now -- against :attr:`Case.target` -- what it is pinned to say -- and is independent of whether the target dissector can actually parse anything, which is @@ -224,12 +221,10 @@ def test_cases_cover_every_table_named_in_the_issue(self) -> None: """ cases = self.dispatch.cases() - # #904: 38 -> 39, and the internet count 16 -> 17 below with it -- one - # new entry, Internet.__proto__[TransType.Shim6], registered at - # IPv6_Ext where it previously had none at all (defaulted to - # Raw). Diffed against origin/main: no other table changed shape. - # #1574: 39 -> 47 -- LinkType.NULL and LinkType.LOOP in both link-type - # tables, and the four address families of the new Loopback.__proto__. + # The tables of #496, including #904's Internet.__proto__[TransType.Shim6] + # registered at IPv6_Ext rather than defaulting to Raw, plus #1574's + # LinkType.NULL and LinkType.LOOP in both link-type tables and the four + # address families of Loopback.__proto__. self.assertEqual(len(cases), 47, 'expected exactly 47 entries across the seven __proto__ ' 'tables named in GitHub issue #496 and the loopback table ' diff --git a/tests/protocols/test_option_roundtrip_unit.py b/tests/protocols/test_option_roundtrip_unit.py index e8b330b179..89c935e193 100644 --- a/tests/protocols/test_option_roundtrip_unit.py +++ b/tests/protocols/test_option_roundtrip_unit.py @@ -10,10 +10,9 @@ That third step is what this module exists for. A construct-then-parse test passes for a ``_make_*`` that takes only keyword arguments and cannot consume the data model its own ``_read_*`` produced -- which is a defect that ships, and -one this suite had no way to see. Sixteen HIP parameters used to be in exactly -that state, until :class:`~pcapkit.protocols.schema.schema.Schema`'s -``ListField`` pack branch was widened to accept the ``tuple`` their own data -models declare, not just ``list``. See #476. +one such a test cannot see. HIP parameters are the example: their data models +declare a ``tuple``, which :class:`~pcapkit.protocols.schema.schema.Schema`'s +``ListField`` pack branch has to accept as well as a ``list`` (#476). The case list and the cycle both live in :file:`examples/generators/options.py`, next to the generator that turns the @@ -29,25 +28,21 @@ leave the cycle closed -- the generator constructing and reconstructing the same wrong octets, which match each other and so match the assertion. Those are pinned as tests of their own rather than as entries, since an entry would have to -record ``'OK'`` as a failure. HIP's parameter padding was the standing example: -it aligned the contents to eight rather than the record, so every parameter was -``4 (mod 8)``, the header ``len`` field could not represent a lone one, and the -generator's ``HIP_COPIES`` put two in a packet to make the arithmetic come out. -A round trip could never see any of it, because pcapkit's writer and reader -shared the error. #651 fixed it against :rfc:`7401` Section 5.2.1 rather than -against a round trip, and +record ``'OK'`` as a failure. HIP's parameter padding is the example: a rule +aligning the contents to eight rather than the record makes every parameter +``4 (mod 8)``, so the header ``len`` field cannot represent a lone one, and a +round trip cannot see it, because pcapkit's writer and reader share the rule. +#651 pins the padding against :rfc:`7401` Section 5.2.1 rather than against a +round trip, and :meth:`OptionRoundTripTests.test_a_hip_packet_carrying_one_parameter_round_trips` -is the same pin, now asserting the case closes. +asserts that the case closes. -IPv4's ``SID`` option width was the other one, tracked as #534 and pinned here by -a ``test_a_parsed_sid_option_re_emits_two_octets_too_wide`` that no longer exists: -``SIDOption.sid`` has been narrowed to a 16-bit field, so a four-octet option read -off the wire now re-emits as the same four octets. The assertion that says so is +IPv4's ``SID`` option width is another (#534): the generator constructs and +reconstructs through the same ``_make_opt_sid``, so the width is asserted against +wire octets instead, by :meth:`IPv4UnitTests.test_ipv4_sid_option_is_four_octets_wide_on_the_wire `, -in the IPv4 unit suite beside the rest of that option's coverage rather than here --- what kept it in this module was the ``EXPECTED_FAILURES`` entry it stood in -for, and with the defect fixed there is nothing for it to stand in for. +in the IPv4 unit suite beside the rest of that option's coverage. Why the table is asserted in both directions -------------------------------------------- @@ -62,16 +57,12 @@ Nothing here is a workaround for the defects it records. Every ``_make_*`` argument in the generator's tables is a legitimate value for that option, and none of the assertions below has been loosened to make a failing case pass. HIP's -``HIP_COPIES`` used to be the one place where an argument was chosen to route -*around* a defect rather than into it, putting two copies of each parameter in a -packet so a lone one's unrepresentable length never had to be constructed. One -copy has been representable since #651, and by the time #672 and #679 landed the -pair was routing around nothing that a single copy did not already hit -- see the -constant's note in the generator for the measurement -- so #689 dropped it to -one. The single-parameter case is asserted directly by +``HIP_COPIES`` puts one copy of each parameter in a packet (#689) -- see the +constant's note in the generator -- so no argument routes around a lone +parameter's length. The single-parameter case is asserted directly by :meth:`OptionRoundTripTests.test_a_hip_packet_carrying_one_parameter_round_trips`, -which is also where the two-copy shape is still exercised now that this table no -longer produces it. +which is also where the two-copy shape is exercised, as this table does not +produce it. This module is unit tier: it constructs its own octets and reads no capture, so it runs on a fresh checkout with nothing generated. @@ -130,287 +121,50 @@ class Gap(NamedTuple): defect: 'str' -#: Every case whose cycle does not close, and the defect that stops it. It is -#: empty while every case round-trips; the comments below record the entries -#: deleted as their defects were fixed. +#: Every case whose cycle does not close, and the defect that stops it. Empty: +#: every case round-trips. #: -#: Grouped by defect rather than by family, because the failures cluster far more -#: tightly by cause than by protocol: most Multipath TCP subtypes failed on the -#: same missing ``MPTCP`` schema fields in ``schema/transport/tcp.py``, and -#: ``PCAPNG.__post_init__`` alone failed every PCAP-NG option with an "only one" -#: guard. +#: Group entries by defect rather than by family, because failures cluster far +#: more tightly by cause than by protocol. EXPECTED_FAILURES = { - - # -- TCP ------------------------------------------------------------------ - - # #541 declared real ``kind``/``length`` fields on ``MPTCP``, which got six - # of these seven far enough to construct and pack, and #566/#567 closed - # the rest of the chain: #566 gave ``MPTCP.subtype`` the same treatment - # (it was ``TYPE_CHECKING``-only, an annotation rather than a field, and - # the only thing that ever set it was ``_MPTCP.post_process``, which runs - # on a real byte-level unpack -- not on the schema a ``_make_mptcp_*`` - # maker returns in memory, which is what ``TCP``'s convenience - # constructor, ``TCP(options=[(code, kwargs)])``, reads straight back - # through ``_read_mptcp_*`` with no round trip in between); #567 fixed - # MP_CAPABLE's own ``length``/``rkey`` arithmetic on top of that. Six of - # the seven (MP_CAPABLE, ADD_ADDR, REMOVE_ADDR, MP_PRIO, DSS, MP_FAIL) now - # read ``'OK'`` and so have no entry below any more. - # - # MP_FASTCLOSE joined them in #576, and its entry is gone with them. Fixing - # ``subtype`` in #566 had got it *past* the ``AttributeError`` this table - # used to record and into a second, unrelated defect: three sites disagreed - # on its length, and the maker's *correct* value (12, from :rfc:`8684` - # section 3.5 figure 14 -- section 3.5 is Fast Close; the entry that used to - # sit here cited 3.7, which is Fallback) failed the parser's wrong check of - # 16, while the schema packed only 11 for want of a reserved octet. All - # three read 12 now. Note that REMOVE_ADDR, MP_PRIO and DSS in the list - # above read ``'OK'`` throughout that period *without* being correct -- this - # suite only checks the cycle is self-consistent, which a wrong length can - # be, so #576 covers them per option in - # :mod:`tests.protocols.transport.test_tcp_mptcp_length_arithmetic_unit` - # against RFC 8684 rather than against the cycle. - - # MP_JOIN was the last of the eight MPTCP subtypes with a registered maker to - # keep an entry here, recording ``CONSTRUCT`` with ``"no attribute '_flags'"`` - # against ``_make_mptcp_join``. Its entry is gone with #587, which was a - # statement-ordering defect rather than a length one and so deliberately - # outlived #576/#585: ``_make_mptcp_join`` branches on ``self._flags`` to - # choose between the three MP_JOIN layouts of RFC 8684 section 3.2, and - # ``TCP.make`` assigned that attribute *after* it had already built the - # options. #587 hoists the flag resolution above the ``_make_tcp_options`` - # call, leaving only the ``offset`` computation -- which genuinely needs the - # options' ``total_length`` -- after it. - # - # The cycle closes for whichever layout this suite's flags select, the SYN - # form of figure 5, since ``examples.generators.options``' ``TCP_BASE`` sets - # ``syn`` and leaves ``ack`` clear. The other two forms, and the silent - # wrong-layout outcome that made a zero-valued default the wrong fix, are - # covered per layout in - # :mod:`tests.protocols.transport.test_tcp_mptcp_join_flag_ordering_unit` - # -- this suite can only ever exercise one MP_JOIN form, there being one - # case per registry code. - - # -- IPv4, whose option padding is now fixed ------------------------------ - - # ``_make_ipv4_options`` used to append a bare enumeration member to the - # option list as its end-of-list padding, where ``OptionField.pack`` accepts - # only bytes or a Schema, so every option whose packed length is not a - # multiple of four failed to construct. #506 appends an ``EOOL`` option - # *schema* instead, the way the ``NOP`` options beside it always did, so - # ``LSR``, ``RR`` and ``SSR`` round-trip and have no entry here any more -- - # and they could not have been routed around, their length being - # ``3 + counts * 4`` and so never a multiple of four for any argument. - # - # ``SID`` reached that same branch for a second reason of its own, which #506 - # did not address and #534 now has: ``SIDOption.sid`` was a ``UInt32Field`` - # where RFC 791 section 3.1 gives the Stream ID 16 bits, so the option - # re-emitted as ``880400000037`` where the wire holds ``88040037``. Six not - # being a multiple of four, it then reached the padding branch above and grew - # a ``NOP`` and an ``EOOL``. The field is now ``UInt16Field``, the option - # re-emits as the four octets it was read as, and the padding branch is not - # reached at all. - # - # Neither state was expressible here. The cycle closed either way, because - # the generator constructs and reconstructs through the same - # ``_make_opt_sid`` and so compared six octets against six -- and a ``Gap`` - # naming ``'OK'`` is the one entry - # :meth:`test_round_trip_is_identity_or_a_recorded_gap` reads as "no entry - # needed". Which is why the width is asserted against *wire* octets instead, - # now in :meth:`IPv4UnitTests.test_ipv4_sid_option_is_four_octets_wide_on_the_wire - # `. - - # ``_make_opt_ts`` used to be recorded here too. It passed ``data=`` where the - # schema field is ``ts_data``; ``Schema.__update__`` only warns about an - # unknown field name and carries on, so the value was dropped and the - # attribute stayed bound to the class-level ``ListField`` descriptor -- which - # ``post_process`` then tried to iterate, ``'ListField' object is not - # iterable``. The IPv4 Timestamp option was unbuildable through ``make`` for - # as long as that stood. #552 passes ``ts_data=`` and corrects the - # ``TYPE_CHECKING`` ``__init__`` stub that advertised ``data`` and is what the - # maker was written against, so ``ipv4-option/TS`` round-trips and has no - # entry here any more. - - # -- Quick-Start, in all three protocols that carry it -------------------- - - # ``ipv4-option/QS``, ``hopopt-option/Quick_Start`` and - # ``ipv6-opts-option/Quick_Start`` used to be recorded here. ``_read_opt_qs`` - # read ``schema.func``, which is declared only under ``TYPE_CHECKING`` and set - # only by ``_QSOption.post_process`` on the parse path, so the schema - # ``_make_opt_qs`` builds raised ``AttributeError`` when ``__post_init__`` - # re-read it. The readers now take the function from ``flags['func']``, as - # they already took ``rate`` (#1091). HOPOPT and IPv6-Opts also sized the - # nested schema with ``SchemaField(length=5)`` against an eight-octet option, - # which decoded the nonce wrongly; #552 had fixed only IPv4's copy, and #1091 - # fixes the other two. All three now round-trip. - - # -- The non-progress loop, now fully fixed ------------------------------- - - # Both of these used to *hang* rather than fail, which is why the cycle is - # run under a deadline at all. #432 landed the progress guard in - # ``OptionField``/``ListField`` and fixed the ``_SMFDPDOption`` sizing in - # *both* schema modules symmetrically -- 26 lines each, ``'len': (1, 8)`` to - # ``(8, 8)`` and the ``+ 2`` on the selector's ``SchemaField`` -- so - # ``hopopt-option/SMF_DPD`` round-trips and has no entry here at all. - # - # ``IPv6-Opts`` used to still fail, and not because it missed that fix. The - # two modules differed in exactly one line of code, older than #432: - # ``ipv6_opts.SMFIdentificationBasedDPDOption`` declared a second, redundant - # ``test`` ``ForwardMatchField`` that ``hopopt``'s does not. The enclosing - # ``_SMFDPDOption`` already has one, in both modules, and it is that outer - # field the selector reads -- nothing read the nested copy. But a - # ``ForwardMatchField`` does not consume the stream while still occupying a - # slot in ``__buffer__``, so the nested schema over-reported its own size by - # one octet, and ``OptionField`` then mis-counted the option area against - # the header. Measured on the *same* octets, ``1100080100010100``: - # - # hopopt __fields__ = [type, len, info, tid, id] len(schema) = 3 - # ipv6_opts __fields__ = [type, len, test, info, tid, id] len(schema) = 4 + # A cycle can close on wrong octets, since this suite checks only that it is + # self-consistent. Such defects are pinned against their specifications + # elsewhere: # - # HOPOPT(...) -> options=[SMF_DPD, PadN] - # IPv6_Opts(...) -> ProtocolError: IPv6-Opts: invalid format + # * the Multipath TCP option lengths, per option against RFC 8684, in + # :mod:`tests.protocols.transport.test_tcp_mptcp_length_arithmetic_unit` + # (#576); + # * the three MP_JOIN layouts of RFC 8684 section 3.2, of which this suite + # reaches only the SYN form that ``examples.generators.options``' + # ``TCP_BASE`` selects, in + # :mod:`tests.protocols.transport.test_tcp_mptcp_join_flag_ordering_unit` + # (#587); + # * the IPv4 ``SID`` option width, against wire octets, in + # :meth:`IPv4UnitTests.test_ipv4_sid_option_is_four_octets_wide_on_the_wire + # ` + # (#534); + # * HIP's parameter padding, against :rfc:`7401` Section 5.2.1, in + # :meth:`test_a_hip_packet_carrying_one_parameter_round_trips` (#651). # - # This was identical on 3.10.20 and 3.14.7, so it was never - # interpreter-dependent. Fixed by #441/PR #449, which dropped the stray - # field -- ``ipv6-opts-option/SMF_DPD`` now round-trips too and has no - # entry here either. + # The RPL source route header (#564) round-trips here. Its fixed area is + # pinned against :rfc:`6554#section-3` by + # :meth:`IPv6ExtensionUnitTests.test_ipv6_route_rpl_fixed_area_is_four_octets + # `, + # which records that without the fix a constructed header is 41 octets + # against the 48 its ``Hdr Ext Len`` of 5 declares. The caveat #489 + # records holds: none of it has been checked against a real RPL capture. # - # The deadline in the sweep stays regardless. It is protection against the - # *next* non-progress defect, not against this one. - - # -- IPv6-Route ----------------------------------------------------------- - - # ``Source_Route`` and ``Type_2_Routing_Header`` used to be recorded here: - # ``IPv6_Route.make`` wrote a raw octet count into ``length`` (``Hdr Ext - # Len``) on the dict and Data paths, a different-and-also-wrong expression - # on the bytes and Schema paths, and separately, ``ipv6_route_data_selector`` - # (pcapkit/protocols/schema/internet/ipv6_route.py) sized the nested - # routing-data schema 4 octets short of the wire, missing the "Reserved" - # field every routing type's data starts with -- so a hand-built, - # spec-correct header failed to parse independently of anything ``make`` - # produced. #487 fixed both: one shared helper - # (``IPv6_Route._make_hdr_ext_len``) computes ``Hdr Ext Len`` in the - # 8-octet units :rfc:`8200#section-4.4` specifies, on every ``make`` - # branch, and ``ipv6_route_data_selector`` accounts for the 4-octet - # offset. Both cases round-trip now; entries deleted rather than left - # behind, per the note at the top of this table. - - # ``RPL_Source_Route_Header`` used to be recorded here too, behind a stack - # of four defects that had to come off in order -- which is why it outlived - # #487 by several rounds: - # - # 1. ``post_process`` assumed ``addresses`` was bytes -- true after - # unpacking, false while packing, where it is still the list the - # constructor was handed. Fixed by #556, which unmasked the rest. - # 2. The reader's length guard read ``header.length`` (``Hdr Ext Len``, - # in 8-octet units) as an octet count and assumed 16-octet addresses, - # which an SRH only carries when ``CmprI`` and ``CmprE`` are both 0 -- - # the same unit confusion #487 fixed for Source Route and Type 2, - # flagged but deliberately left by #489 for want of a working round - # trip to validate a replacement against. - # 3. Behind that, the schema's fixed area -- ``cmpr_i`` + ``cmpr_e`` + - # ``pad`` -- packed to 5 octets where :rfc:`6554#section-3` gives 4, - # ``CmprI``/``CmprE``/``Pad`` being 4-bit fields sharing one 32-bit - # word with a 20-bit ``Reserved``. Measured before the fix: a - # constructed header of 41 octets against the 48 its own ``Hdr Ext - # Len`` of 5 declared. - # 4. And behind *that*, once the guard stopped rejecting every - # constructed header, ``_read_data_type_rpl`` raised a bare - # ``AttributeError`` on the construction path, because - # ``post_process`` set ``ip`` only when it had parsed octets. - # - # #564 took all four off together -- the guard could not be validated - # against a header whose width was still wrong -- replacing the ``% 16`` - # bound with :rfc:`6554#section-4.2`'s own address-count arithmetic. The - # case round-trips now; entry deleted rather than left behind, per the note - # at the top of this table. The caveat #489 recorded does survive: none of - # it has been checked against a real RPL capture. - - # -- Mobility Header ------------------------------------------------------ - # - # ``mh-extension/{Multi_Prefix,Exp_FFFD,Exp_FFFE,Exp_FFFF}`` all round-trip - # cleanly now that #445, #437 and #446 are all applied together: #445 let - # ``CGAParameter.extensions`` size itself instead of raising - # ``KeyError: 'length'``, #437 (merged as registry completion) both - # registered the three experimental codes and fixed - # ``_make_ext_multiprefix``'s bogus length arithmetic, and #446/#456 fixed - # the ``ForwardMatchField`` double-count that stopped ``CGAParametersOption - # .parameters`` from sizing correctly. No entries needed here any more. - - # -- HIP ------------------------------------------------------------------ - - # ``hip-parameter/R1_Counter`` used to be here: ``HIP.__parameter__``'s - # hand-written dict entries already mapped both 128 and 129 to - # ``_read_param_r1_counter``/``_make_param_r1_counter`` (not a - # name-normalisation rule -- each code has its own literal entry), so both - # existed for code 128. But the *schema* registry was keyed by the - # ``code=`` of the class statement, and ``R1CounterParameter`` declared - # only 129 -- so code 128 parsed as an ``UnassignedParameter``. Fixed by - # #690, registering ``R1CounterParameter`` under both codes. - - # ``ENCRYPTED`` used to have an entry here, for two defects at once: that - # ``_make_param_encrypted`` passed ``cipher=``, a keyword - # ``EncryptedParameter`` does not accept, so the cipher id was dropped with - # an ``UnknownFieldWarning`` and never reached the wire; and that the - # ``data`` length callback omitted the four octets ``reserved`` had already - # taken out of ``len``, so ``len`` grew by four on every round trip - # (measured: 4 -> 8). The first was fixed by #556. The second was fixed - # alongside #651, because the two four-octet errors cancelled at four of the - # eight residues of ``Length`` -- measured, ``Length % 8`` in {0, 5, 6, 7} -- - # so correcting the padding on its own would have turned "right at four - # residues" into "four octets too long at all eight". With both gone the - # cycle closes and the entry is deleted rather than kept as documentation of - # a defect that is no longer there. - - # ``HOST_ID`` and ``HIP_TRANSFORM`` were the last two entries here, and - # neither was helped by a second copy -- each failed at two copies as well as - # at one. #1128 built ``HIP_TRANSFORM`` at the HIPv1 its constructor demands, - # through the generator's ``HIP_VERSION``, instead of the version 2 the table - # had been using; #1118 made ``HOST_ID``'s DI-Type/DI-Length word the single - # 16-bit one of :rfc:`7401` Section 5.2.9, where the schema had declared four - # octets for it. Every HIP case round-trips now, at either setting of - # ``HIP_COPIES``, so no entries are needed here any more. - - # -- HTTP/2 --------------------------------------------------------------- - - # ``SchemaField.pack`` used to give a nested frame schema a fresh packet - # context whose only link to the parent was ``__packet__``, and six frame - # schemas reach for the HTTP/2 header's ``flags`` bitfield directly -- - # either from a ConditionalField test or from ``FrameType.post_process``. - # #445 makes a name absent from the nested schema fall through to the - # parent instead of raising, which fixed that for all six. RST_STREAM, - # GOAWAY and WINDOW_UPDATE already passed, declaring no flag members at - # all; the other five each got past ``flags`` and hit their own, - # unrelated defect in turn -- and every one of those has since been - # fixed and merged too, so none of the six needs an entry any more: - # - # - PUSH_PROMISE, PING: round-tripped cleanly as soon as #445 landed. - # - DATA, HEADERS, CONTINUATION: hit ``decorators.py``'s ``@prepare`` - # treating a zero-length nested unpack (a frame with no payload) as - # end-of-file. Filed as #458, fixed and merged as #461 (``prepare`` now - # distinguishes a *declared* zero length from a *derived* one). - # - SETTINGS: hit ``SettingsFrame.settings`` declaring - # ``item_type=SettingPair`` (the raw schema class) instead of - # ``SchemaField(schema=SettingPair)``. Filed as #459, fixed and merged - # as #462. - # - # PRIORITY needs no entry either: ``make`` declared payload + 9 while the - # reader demanded 9, until #1121 moved both to RFC 9113's payload-only - # Length. - - # -- PCAP-NG -------------------------------------------------------------- - - # PCAP-NG entries used to be recorded here, for the defects - # that #1267-#1270 fixed. ``PCAPNG.__post_init__`` carried the make pass's - # option count into its own re-parse, so every option with an "only one" - # guard tripped it. ``if_IPv6addr`` declared 8 octets for 17. - # ``isb_starttime``/``isb_endtime`` read an interface ID that only the parse - # path set. And an EPB, SPB or PB rebuilt from its data model lost its - # packet data. All of them round-trip now. - - # The two key-log secrets failed for #1271 (``os.sep`` line endings and a - # ``datetime.now()`` body), then for #1279: ``_make_block_dsb`` never padded - # the secrets data, so a log whose length is not a multiple of four gave a - # Block Total Length that is not either. Both are fixed. + # The deadline in the sweep protects against a non-progress defect, under + # which a cycle hangs rather than fails (#432, #441). ``SMF_DPD`` is the + # case: a stray nested ``test`` ``ForwardMatchField`` in ``ipv6_opts`` + # over-reports the option's size by one octet. Measured on the same octets, + # ``1100080100010100``, ``len(schema)`` is 3 in ``hopopt`` against 4 in + # ``ipv6_opts``, so ``HOPOPT`` reads ``options=[SMF_DPD, PadN]`` where + # ``IPv6_Opts`` raises ``ProtocolError: IPv6-Opts: invalid format`` -- + # identical on 3.10.20 and 3.14.7, so not interpreter-dependent. + # :meth:`IPv6ExtensionUnitTests.test_ipv6_opts_identification_based_dpd_null_tid_option_area_matches + # ` + # pins it. } @@ -622,42 +376,33 @@ def test_round_trip_is_identity_or_a_recorded_gap(self) -> None: def test_a_hip_packet_carrying_one_parameter_round_trips(self) -> None: """A HIP packet carrying exactly one parameter is accepted by its own reader. - This assertion used to run the other way, as - ``test_a_single_hip_parameter_cannot_be_constructed``: it required the - library to *reject* its own single-parameter packets, which it did, and - pinned the defect the generator's ``HIP_COPIES`` (then ``2``) routed - around so that routing around it did not also bury it. - ``HIP.make`` computes the header's ``len`` as ``total_length // 8 + 4``, which is lossless only when the parameter octets are a multiple of eight. - Every padding site in the two HIP modules aligned the *contents* to - eight and ignored the four-octet type-and-length header, so one - parameter was always ``4 (mod 8)``; the floor division dropped those four - octets and ``_read_hip_param``, which compares the recovered length - exactly, raised. #651 made the padding :rfc:`7401` Section 5.2.1's - ``Total Length = 11 + Length - (Length + 3) % 8``, under which a lone - parameter is 8-aligned by construction, so the case now closes and this - test says so positively rather than recording the raise. - - Both halves of the old test are kept, because the pair was its control: - one copy and two copies must *both* work, and must both come back as the + A padding rule that aligns the *contents* to eight and ignores the + four-octet type-and-length header makes one parameter always + ``4 (mod 8)``; the floor division drops those four octets and + ``_read_hip_param``, which compares the recovered length exactly, + raises. :rfc:`7401` Section 5.2.1's + ``Total Length = 11 + Length - (Length + 3) % 8`` (#651) makes a lone + parameter 8-aligned by construction, so the case closes, and this test + says so positively rather than recording a raise. + + One copy and two copies must *both* work, and must both come back as the octets they went out as. ``SEQ`` is the case that discriminates hardest, at ``Length = 4``: the record needs no padding at all - (``11 + 4 - (4 + 3) % 8 == 8``), so the old rule's four appended octets - were pure surplus rather than a shortfall, and a reader that pads by any - non-zero amount lands in the wrong place at the start of the second copy. - - ``HIP_COPIES`` dropped to ``1`` in #689, once #672 and #679 had closed - the last two codes a second copy was still routing around -- see the - constant's note in the generator. That makes the ``copies=1`` subtest - below redundant with the main sweep, which now exercises - ``hip-parameter/SEQ`` at exactly one copy itself; it is kept anyway - because it pins the exact wire layout (``len(built)``, the header's + (``11 + 4 - (4 + 3) % 8 == 8``), so a contents-aligning rule's four + appended octets are pure surplus rather than a shortfall, and a reader + that pads by any non-zero amount lands in the wrong place at the start of + the second copy. + + ``HIP_COPIES`` is ``1`` (#689) -- see the constant's note in the + generator -- so the ``copies=1`` subtest below overlaps the main sweep, + which exercises ``hip-parameter/SEQ`` at exactly one copy itself; it is + kept because it pins the exact wire layout (``len(built)``, the header's ``len`` byte, the reparsed count) rather than only the round-trip - identity the sweep checks. The ``copies=2`` subtest is no longer - redundant with anything -- the generator does not build that shape any - more -- so this is the one place left that still asserts the pair - round-trips. + identity the sweep checks. The generator does not build the two-copy + shape, so the ``copies=2`` subtest is the one place that asserts the + pair round-trips. """ from pcapkit.const.hip.parameter import Parameter diff --git a/tests/protocols/test_padding_kept_as_captured_unit.py b/tests/protocols/test_padding_kept_as_captured_unit.py index 13d60bd232..def760bb9f 100644 --- a/tests/protocols/test_padding_kept_as_captured_unit.py +++ b/tests/protocols/test_padding_kept_as_captured_unit.py @@ -1,9 +1,9 @@ # -*- coding: utf-8 -*- """Padding and reserved octets held by a ``PaddingField`` rebuild as captured. -This guards against GitHub issue #1223, where -:class:`~pcapkit.corekit.fields.strings.PaddingField` packed zeros whatever it -had read, so non-zero padding or reserved octets parsed silently and came back +This guards against GitHub issue #1223: a +:class:`~pcapkit.corekit.fields.strings.PaddingField` that packs zeros whatever +it has read, so non-zero padding or reserved octets parse silently and come back zeroed. The field packs the octets it was given, and zeros when it has none, which is what a freshly built packet writes. Each protocol below carries the octets through its reader, data model and @@ -59,7 +59,7 @@ '138813891122334400000000' '06000009' '00ff0005aaeeeeee'), } -#: HIP parameters whose reserved octets are ``ff`` (the 12 sites listed on +#: HIP parameters whose reserved octets are ``ff`` (the sites listed on #: #1223), and one whose trailing padding is not zero. HIP_PARAMS = { 'R1_COUNTER': '0081000cffffffff00000000aabbccdd', @@ -110,13 +110,13 @@ class Padded(Schema): self.assertEqual(PaddingField(length=2).pack(None, {}), bytes(2)) def test_wrong_width_padding_is_zero_filled_or_truncated(self) -> None: - """A ``ConditionalField``-wrapped padding keeps ``main``'s ``struct`` semantics.""" + """A ``ConditionalField``-wrapped padding zero-fills or truncates as ``struct`` does.""" from pcapkit.const.http.frame import Frame from pcapkit.protocols.application.httpv2 import HTTP from pcapkit.protocols.link.l2tpv2 import L2TPv2 # the L2TPv2 ``padding`` contract: zero-filled or truncated to ``offset`` - # octets (bytes measured on ``main``) + # octets (bytes measured) for padding, wire in ((b'\xaa', '0202000000000003aa00007878'), (b'\xaa\xbb\xcc\xdd\xee', '0202000000000003aabbcc7878'), (None, '02020000000000030000007878')): @@ -131,7 +131,7 @@ def test_wrong_width_padding_is_zero_filled_or_truncated(self) -> None: self.assertEqual(HTTP(type=Frame.DATA, sid=1, frame=frame).data.hex(), wire) def test_makers_tolerate_a_data_model_without_the_new_fields(self) -> None: - """A stand-in lacking the new attributes rebuilds with zero padding.""" + """A stand-in lacking the padding attributes rebuilds with zero padding.""" from types import SimpleNamespace from pcapkit.const.ipv6.option import Option diff --git a/tests/protocols/test_parser_registrar_identity_unit.py b/tests/protocols/test_parser_registrar_identity_unit.py index 8a34a0c406..e5dc3f4f2c 100644 --- a/tests/protocols/test_parser_registrar_identity_unit.py +++ b/tests/protocols/test_parser_registrar_identity_unit.py @@ -22,7 +22,7 @@ from tests._support import reimport_once_per_class #: ``(module, class, registrar, registry)`` for every parser registrar but -#: PCAP-NG's, which another change owns. +#: PCAP-NG's, which :mod:`tests.protocols.misc.test_pcapng_unit` covers. SITES = ( ('pcapkit.protocols.internet.ipv4', 'IPv4', 'register_option', '__option__'), ('pcapkit.protocols.internet.hopopt', 'HOPOPT', 'register_option', '__option__'), diff --git a/tests/protocols/test_pcapng_regression.py b/tests/protocols/test_pcapng_regression.py index d099b617de..a87b1ea541 100644 --- a/tests/protocols/test_pcapng_regression.py +++ b/tests/protocols/test_pcapng_regression.py @@ -405,7 +405,7 @@ def test_dumped_pcap_carries_the_payload_octets(self) -> None: self.assertGreaterEqual(orig_len, incl_len) # 24 octets of global header, then 16 of record header and the payload per - # record, with nothing left over. Without the #646 fix this file is 104 octets + # record, with nothing left over. Without the #646 fix this file is 88 octets # for these four blocks -- the global header and four record headers, and not # one octet of payload -- which is both the wrong size and, worse, a size a # reader cannot tell is wrong until it has already lost frame sync. diff --git a/tests/protocols/test_protocol_code_registration_unit.py b/tests/protocols/test_protocol_code_registration_unit.py index faa7ecf148..27a5ff4db2 100644 --- a/tests/protocols/test_protocol_code_registration_unit.py +++ b/tests/protocols/test_protocol_code_registration_unit.py @@ -101,9 +101,9 @@ def __index__(cls): def test_unrecognised_class_keyword_raises_unsupportedcall(self) -> None: """A misspelled keyword raises rather than being silently swallowed. - Before this hook existed, extra keywords fell into ``**kwargs`` and - were dropped by the bare ``super().__init_subclass__()`` -- no - exception, no warning. ``codes=`` (plural) is the natural typo for + Without this hook, extra keywords fall into ``**kwargs`` and are + dropped by a bare ``super().__init_subclass__()`` -- no exception, no + warning. ``codes=`` (plural) is the natural typo for ``code=``, and does not collide with :meth:`abc.ABCMeta.__new__`. """ from pcapkit.protocols.internet.internet import Internet diff --git a/tests/protocols/test_protocol_make_unit.py b/tests/protocols/test_protocol_make_unit.py index 74f8b2fe33..cbe939a438 100644 --- a/tests/protocols/test_protocol_make_unit.py +++ b/tests/protocols/test_protocol_make_unit.py @@ -194,7 +194,7 @@ def make_pair(self, a: 'Any' = left.Shared, a_namespace: 'Any' = None, def test_shared_name_resolves_per_argument(self) -> None: pair, _, _ = self._pair() self.assertEqual(pair.make_pair(pair, a='Shared', b='Shared'), (1, 2)) - # a name only one argument holds resolves as it did before + # and a name only one argument holds resolves normally self.assertEqual(pair.make_pair(pair, a='Shared', b='Only'), (1, 3)) def test_ipv4_shared_name_falls_back_per_argument(self) -> None: @@ -217,7 +217,7 @@ def make_alias(self, b: 'Any' = level.HIGH, b_namespace: 'Any' = None) -> 'int': value = b return self._make_index(value) - # ``value`` is a local, so ``b`` is found by its value as before + # ``value`` is a local, so ``b`` is found by its value self.assertEqual(Alias.make_alias(Alias, b='HIGH'), 7) def test_unlocated_call_matches_every_argument(self) -> None: diff --git a/tests/protocols/test_trial_parse_warnings_1580_unit.py b/tests/protocols/test_trial_parse_warnings_1580_unit.py index 52f138a6e0..d85775fcc3 100644 --- a/tests/protocols/test_trial_parse_warnings_1580_unit.py +++ b/tests/protocols/test_trial_parse_warnings_1580_unit.py @@ -289,7 +289,7 @@ def test_error_filter_chains_match_holding_nothing(self) -> None: A warning made an error is raised where the trial that kept it ends, inside the guard it was raised in when nothing was held, so the same layer, and only it, becomes ``Raw``. Holding until the outermost trial - ended instead turned the whole subtree into ``Raw``. + ends instead would turn the whole subtree into ``Raw``. """ import pcapkit.protocols.protocol as protocol_module diff --git a/tests/protocols/transport/test_sctp_unit.py b/tests/protocols/transport/test_sctp_unit.py index e37d26106e..ca2f1fefb8 100644 --- a/tests/protocols/transport/test_sctp_unit.py +++ b/tests/protocols/transport/test_sctp_unit.py @@ -34,18 +34,19 @@ def bitfield_packs_zero_bits() -> bool: """Whether :class:`~pcapkit.corekit.fields.strings.BitField` packs a clear bit. - ``BitField.pre_process`` seeds its working buffer with NUL bytes and then - writes the ASCII characters ``b'0'`` and ``b'1'`` into it, before collapsing - the buffer by truth-testing each byte -- and ``b'0'`` is ``0x30``, which is - truthy. Every named bit therefore comes out **set** regardless of its value, - so a flag word constructed with any bit clear does not survive a round trip. - - That is a library-wide defect in :mod:`pcapkit.corekit`, not an SCTP one, and - it is tracked separately. The tests that need clear bits on the - *construction* side are gated on this probe so that they run of their own - accord wherever :mod:`pcapkit.corekit` packs clear bits correctly. Parsing is unaffected -- - ``BitField.post_process`` is correct -- so the wire-conformance and scapy - cross-check tests below exercise mixed flags unconditionally. + It guards against a ``BitField.pre_process`` that seeds its working buffer + with NUL bytes and then writes the ASCII characters ``b'0'`` and ``b'1'`` + into it, before collapsing the buffer by truth-testing each byte -- ``b'0'`` + is ``0x30``, which is truthy, so every named bit comes out **set** regardless + of its value, and a flag word constructed with any bit clear does not survive + a round trip. + + That is a library-wide concern of :mod:`pcapkit.corekit`, not an SCTP one. + The tests that need clear bits on the *construction* side are gated on this + probe, so that they run wherever :mod:`pcapkit.corekit` packs clear bits + correctly. Parsing does not depend on it -- ``BitField.post_process`` reads + the bits separately -- so the wire-conformance and scapy cross-check tests + below exercise mixed flags unconditionally. """ from pcapkit.corekit.fields.strings import BitField @@ -163,9 +164,9 @@ def test_common_header_wire_conformance(self) -> None: def test_constructed_ports_carry_the_same_type_as_parsed_ones(self) -> None: """A bare :obj:`int` port is resolved on construction, not left as it is. - SCTP never tripped over this the way TCP and UDP did -- it keys its next - layer on the DATA chunk's PPID rather than on a port, so it never read - ``srcport.port`` off the schema -- but it did leave a constructed packet + SCTP keys its next layer on the DATA chunk's PPID rather than on a port, + so it never reads ``srcport.port`` off the schema the way TCP and UDP + do -- but an unresolved port would still leave a constructed packet holding an :obj:`int` where a parsed one holds an :class:`~pcapkit.const.reg.apptype.AppType`. @@ -245,8 +246,8 @@ def test_every_chunk_type_round_trips(self) -> None: from pcapkit.const.sctp.chunk import Chunk from pcapkit.const.sctp.parameter import Parameter - # Every flag in the flag-carrying chunks is set, so that the tests are - # not blocked on the BitField construction defect described by + # Every flag in the flag-carrying chunks is set, so that these tests do + # not depend on the BitField construction probe # :func:`bitfield_packs_zero_bits`; mixed flags are covered on the # parsing side by the wire-conformance and scapy tests below, and on the # construction side by the gated test that follows this one. diff --git a/tests/protocols/transport/test_tcp_http_dispatch_unit.py b/tests/protocols/transport/test_tcp_http_dispatch_unit.py index 1e5b56f3d3..df887f372f 100644 --- a/tests/protocols/transport/test_tcp_http_dispatch_unit.py +++ b/tests/protocols/transport/test_tcp_http_dispatch_unit.py @@ -67,10 +67,10 @@ def test_tcp_binds_the_http_proxy_on_both_http_ports(self) -> None: self.assertEqual(descriptor.name, 'HTTP') def test_tcp_and_udp_agree_on_the_http_ports(self) -> None: - """The TCP/UDP asymmetry #682 closed stays closed. + """TCP and UDP bind the HTTP ports alike (#682). - UDP already bound the proxy for both ports; TCP bound ``httpv1`` for the - same two. Pinning the equality rather than each table separately is what + UDP binds the proxy for both ports, and so must TCP, rather than + ``httpv1`` for the same two. Pinning the equality rather than each table separately is what makes a future one-sided edit fail here. """ diff --git a/tests/protocols/transport/test_tcp_mptcp_addaddr_unit.py b/tests/protocols/transport/test_tcp_mptcp_addaddr_unit.py index b3fe6e7bc6..94f2221892 100644 --- a/tests/protocols/transport/test_tcp_mptcp_addaddr_unit.py +++ b/tests/protocols/transport/test_tcp_mptcp_addaddr_unit.py @@ -214,11 +214,11 @@ def test_opt_argument_round_trips_through_the_maker(self) -> None: class TCPMPTCPUnpackUnitTests(unittest.TestCase): """Spec-correct MPTCP option octets parse into a :class:`TCP`, field by field. - Every case above builds an option and packs it; none of them ever asks the library - to *parse* one, so the identical off-by-two that broke packing -- ``MPTCPAddAddress`` - (and its siblings) reading the ``kind`` octet as their own first field, because - ``MPTCP`` supplied no real ``kind``/``length`` fields for anything ahead of it to - consume -- shipped unpinned on the read side. These two do not construct anything + Every case above builds an option and packs it; none of them asks the library to + *parse* one, so the off-by-two that breaks packing -- ``MPTCPAddAddress`` (and its + siblings) reading the ``kind`` octet as their own first field, when ``MPTCP`` + supplies no real ``kind``/``length`` fields for anything ahead of it to consume -- + needs pinning on the read side too. These two do not construct anything through a maker at all: :data:`ADD_ADDR_SPEC_OCTETS` and :data:`MP_PRIO_SPEC_OCTETS` are hand-built from the RFC 8684 figures, spliced into a real TCP segment by :func:`build_tcp_segment`, and parsed back through :class:`TCP` proper. diff --git a/tests/protocols/transport/test_tcp_mptcp_error_message_unit.py b/tests/protocols/transport/test_tcp_mptcp_error_message_unit.py index b18243f89c..cfc7f37a1c 100644 --- a/tests/protocols/transport/test_tcp_mptcp_error_message_unit.py +++ b/tests/protocols/transport/test_tcp_mptcp_error_message_unit.py @@ -194,7 +194,7 @@ def test_make_mptcp_dss_messages(self) -> None: self.assertNotIn(DOUBLED_PREFIX, str(caught.exception)) def test_no_doubled_separator_survives_anywhere_in_the_module(self) -> None: - """The source-level sweep, so a fifth site cannot be added unnoticed. + """The source-level sweep, so another site cannot be added unnoticed. A test that pins only the known strings would not notice another appearing, so the absence is asserted over the whole file -- and the diff --git a/tests/protocols/transport/test_tcp_mptcp_join_flag_ordering_unit.py b/tests/protocols/transport/test_tcp_mptcp_join_flag_ordering_unit.py index 639474038e..4744b28ae2 100644 --- a/tests/protocols/transport/test_tcp_mptcp_join_flag_ordering_unit.py +++ b/tests/protocols/transport/test_tcp_mptcp_join_flag_ordering_unit.py @@ -422,8 +422,8 @@ def test_each_layout_survives_the_wire(self) -> None: """Construct, pack, parse, and get the same values back, for all three forms. This is the cycle - :mod:`tests.protocols.test_option_roundtrip_unit` exercises generically and had - to record as an ``EXPECTED_FAILURES`` gap for MP_JOIN. Asserted per layout here + :mod:`tests.protocols.test_option_roundtrip_unit` exercises generically. Asserted + per layout here because that suite drives one case per registry code and so reaches only whichever form its fixture's flags select. diff --git a/tests/protocols/transport/test_tcp_mptcp_length_arithmetic_unit.py b/tests/protocols/transport/test_tcp_mptcp_length_arithmetic_unit.py index 3b76fce042..a1d317ff90 100644 --- a/tests/protocols/transport/test_tcp_mptcp_length_arithmetic_unit.py +++ b/tests/protocols/transport/test_tcp_mptcp_length_arithmetic_unit.py @@ -1,16 +1,15 @@ # -*- coding: utf-8 -*- -"""Multipath TCP option lengths, re-derived from RFC 8684 at the six sites #576 names. +"""Multipath TCP option lengths, re-derived from RFC 8684 at the sites #576 names. -GitHub issue #576, the follow-up #567 invited ("worth checking the sibling ``_make_mptcp_*`` -helpers' length arithmetic in the same pass") and #579 deliberately left alone, being scoped -to ``MPTCP.subtype`` and MP_CAPABLE. +GitHub issue #576: the check of the sibling ``_make_mptcp_*`` helpers' length arithmetic that +#567 suggests, outside the scope of #579 (``MPTCP.subtype`` and MP_CAPABLE). An MPTCP option carries its own octet count in its ``Length`` field, and three separate places have to agree on it: the ``_make_mptcp_*`` maker that *writes* that octet, the schema in :mod:`pcapkit.protocols.schema.transport.tcp` whose fields actually *pack* the octets, and -the ``_read_mptcp_*`` reader whose guard *accepts* it coming back. Each of the six sites below -had at least one of the three disagreeing with :rfc:`8684`, and one had all three disagreeing -with each other. +the ``_read_mptcp_*`` reader whose guard *accepts* it coming back. These tests guard against +any of the three disagreeing with :rfc:`8684` at each site below; at MP_FASTCLOSE all three can +disagree with each other. What each site is, and where its number comes from -------------------------------------------------- @@ -21,34 +20,34 @@ where they are followed by 12 reserved bits. * **MP_FASTCLOSE** -- :rfc:`8684` section 3.5, figure 14. Head 4 (``Kind`` 1 + ``Length`` 1 + - subtype-and-12-reserved-bits 2) + the receiver's key 8 = **12**. The maker already wrote 12; - :class:`~pcapkit.protocols.schema.transport.tcp.MPTCPFastclose` packed **11**, having no - reserved field at all; and ``_read_mptcp_fastclose`` required **16**, a number neither of - the other two nor the RFC produces. Note section 3.5, not 3.7: 3.7 is Fallback (MP_FAIL), - and #576's own text -- and the ``EXPECTED_FAILURES`` entry it left behind -- cite 3.7 here - in error. + subtype-and-12-reserved-bits 2) + the receiver's key 8 = **12**, which the maker, + :class:`~pcapkit.protocols.schema.transport.tcp.MPTCPFastclose` and + ``_read_mptcp_fastclose`` all have to agree on: a schema with no reserved field packs + **11**, and a reader requiring **16** wants a number neither of the other two nor the RFC + produces. Note section 3.5, not 3.7: 3.7 is Fallback (MP_FAIL), and #576's own text cites + 3.7 here in error. * **MP_JOIN-SYN/ACK** -- :rfc:`8684` section 3.2, figure 6. Head 4 (``Kind`` 1 + ``Length`` 1 + subtype/rsv/``B`` 1 + ``Address ID`` 1) + the truncated HMAC 8 + the random number 4 = - **16**. The maker wrote **12**, which is ``_make_join_syn``'s own correct length for the - *SYN* form of figure 5 -- that one carries a 4-octet token where this one carries an - 8-octet HMAC -- copied across without recomputing. ``_read_join_synack`` independently - required **20**, contradicting its own docstring figure. + **16**. **12** is ``_make_join_syn``'s own correct length for the *SYN* form of figure 5 + -- that one carries a 4-octet token where this one carries an 8-octet HMAC -- so copying + it across without recomputing is the mistake to catch; a ``_read_join_synack`` requiring + **20** contradicts its own docstring figure. * **MP_JOIN-ACK** -- :rfc:`8684` section 3.2, figure 7. Head 4 (``Kind`` 1 + ``Length`` 1 + - subtype-and-12-reserved-bits 2) + the full 160-bit HMAC 20 = **24**. The maker wrote **8**. - Its reader already required 24, so nothing the maker produced could be parsed back at all. + subtype-and-12-reserved-bits 2) + the full 160-bit HMAC 20 = **24**, which the reader + requires, so a maker writing **8** produces nothing that parses back at all. * **REMOVE_ADDR** -- :rfc:`8684` section 3.4.2, figure 13, which states ``Length = 3 + n`` outright: head 3 (``Kind`` 1 + ``Length`` 1 + subtype-and-4-reserved-bits 1) + one octet per - Address ID. The maker wrote a constant **4**, correct for exactly one list length -- and + Address ID. A constant **4** is correct for exactly one list length -- and ``examples.generators.options``' fixture passes ``addr_id=[1]``, so the round-trip suite - exercised only that one. + exercises only that one. * **MP_PRIO** -- :rfc:`8684` section 3.3.8, figure 11: head 3 (``Kind`` 1 + ``Length`` 1 + subtype/rsv/``B`` 1) and **nothing else**, since section 5 of that document "specifies the removal of the AddrID field [RFC6824] in the MP_PRIO option", closing a theoretical attack in which a subflow could be forced into backup mode. So **3**, with :rfc:`6824`'s 4-octet - Address ID form still accepted as legacy. The maker wrote a constant **4**, and because + Address ID form still accepted as legacy. Because :attr:`~pcapkit.protocols.schema.transport.tcp.MPTCPPriority.addr_id` is conditional on - ``pkt['length'] == 4``, that constant satisfied its own predicate: a caller who gave no - Address ID got a phantom all-zero one anyway. + ``pkt['length'] == 4``, a maker writing a constant **4** satisfies its own predicate: a + caller who gives no Address ID gets a phantom all-zero one anyway. * **DSS** -- :rfc:`8684` section 3.3, figure 9. Head 4, then the Data ACK (4 octets, or 8 when ``a`` is set) if ``A``, the Data Sequence Number (4, or 8 when ``m`` is set), the Subflow Sequence Number (4), the Data-Level Length (2) and the Checksum (2) if ``M``. All flags set @@ -83,7 +82,7 @@ builds its template once at ``__init__`` with the placeholder length ``-1``, and recomputes ``_need_process`` from the width in force rather than latching it from that placeholder (#598), so a callable length packs and unpacks both widths; latched, the 8-octet form -``_make_mptcp_dss(DSS, ack=1 << 40)`` raised ``struct.error: required argument is not an +``_make_mptcp_dss(DSS, ack=1 << 40)`` raises ``struct.error: required argument is not an integer``. The schema selects between :class:`~pcapkit.corekit.fields.numbers.UInt32Field` and :class:`~pcapkit.corekit.fields.numbers.UInt64Field`, which each fix ``__template__`` at class level, through a :class:`~pcapkit.corekit.fields.misc.SwitchField` -- the pattern @@ -107,16 +106,16 @@ class level, through a :class:`~pcapkit.corekit.fields.misc.SwitchField` -- the Coverage -------- -One test class per site, not one test for all six, so a regression names the option it broke. +One test class per site, not one test for every site, so a regression names the option it broke. Each class asserts the **byte-exact** packed option from its maker rather than only its ``length`` octet -- a declared length and a packed length can disagree, which is the whole defect, so both are checked, and the octets themselves pin that the payload really is where -the figure puts it. Where the *reader* guard was also wrong (MP_FASTCLOSE, MP_JOIN-SYN/ACK) a +the figure puts it. Where the *reader* guard is also at stake (MP_FASTCLOSE, MP_JOIN-SYN/ACK) a second test splices hand-built, spec-correct octets into a real TCP segment and parses them back through :class:`~pcapkit.protocols.transport.tcp.TCP` proper, so a pack-side and a parse-side bug cannot cancel out behind a closed round trip -- the pattern -:mod:`tests.protocols.transport.test_tcp_mptcp_capable_length_unit` established for #567, and +:mod:`tests.protocols.transport.test_tcp_mptcp_capable_length_unit` uses for #567, and the blind spot :mod:`tests.protocols.test_option_roundtrip_unit`'s own docstring names ("a defect can leave the cycle closed"). @@ -229,7 +228,7 @@ def make(meth: 'str', subtype_name: 'str', **kwargs: 'object') -> 'bytes': class LengthInvariantMixin: - """The one invariant every site below broke: declared length is packed length.""" + """The one invariant every site below must keep: declared length is packed length.""" def assertLengthMatchesWire(self, packed: 'bytes', expected: 'int') -> 'None': # noqa: N802 """Assert ``packed`` is ``expected`` octets long and says so in its length octet. @@ -285,9 +284,8 @@ def test_spec_octets_parse_back(self) -> None: def test_public_constructor_round_trips(self) -> None: """MP_FASTCLOSE builds through ``TCP()`` and reports itself back. - This is the case ``tcp-mptcp/MP_FASTCLOSE`` records in - :data:`tests.protocols.test_option_roundtrip_unit.EXPECTED_FAILURES`: the maker's - *correct* length failing the parser's wrong check made the whole construction raise. + Guards against the maker's *correct* length failing a wrong parser check, which makes + the whole construction raise. """ from pcapkit.const.tcp.mp_tcp_option import MPTCPOption as Enum_MPTCPOption @@ -576,8 +574,8 @@ def test_generator_override_arguments_pack_twenty_octets(self) -> None: This is #576's filed measurement: ``ack=1, dsn=2, ssn=3, dl_len=4, checksum=b'\\x00\\x00'`` packed **12** octets against a declared 20, with neither ``ack`` nor ``dsn`` on the wire. The round-trip suite cannot see that, because - construct -> parse -> reconstruct produces the same wrong 12 octets every time and - the suite only checks the cycle is self-consistent. + construct -> parse -> reconstruct would produce the same wrong 12 octets every time + and the suite only checks the cycle is self-consistent. """ packed = make('_make_mptcp_dss', 'DSS', ack=1, dsn=2, ssn=3, dl_len=4, @@ -624,14 +622,14 @@ def test_ack_and_dsn_survive_a_real_byte_round_trip(self) -> None: class TCPMPTCPDSSExtendedFieldsUnitTests(LengthInvariantMixin, unittest.TestCase): - """Site 6, second half -- the 8-octet DSS forms, which could not be packed at all. + """Site 6, second half -- the 8-octet DSS forms, which a latched field cannot pack at all. - ``MPTCPDSS.ack``/``.dsn`` used ``NumberField(length=)``, and that field latches - ``_need_process = True`` at ``__init__`` from the placeholder length ``-1`` and never - clears it, so once ``__call__`` resolved a real width of 8 and rebuilt the template as - ``>Q``, ``pre_process`` handed :func:`struct.pack` bytes for an integer template. Every - test in this class raised ``struct.error: required argument is not an integer`` before the - fix -- not a wrong length, no length at all. + Guards against ``MPTCPDSS.ack``/``.dsn`` using a ``NumberField(length=)`` that + latches ``_need_process = True`` at ``__init__`` from the placeholder length ``-1`` and + never clears it: once ``__call__`` resolves a real width of 8 and rebuilds the template as + ``>Q``, ``pre_process`` hands :func:`struct.pack` bytes for an integer template. Every test + in this class then raises ``struct.error: required argument is not an integer`` -- not a + wrong length, no length at all. """ @@ -694,7 +692,7 @@ class TCPMPTCPJoinSYNIsCorrectUnitTests(LengthInvariantMixin, unittest.TestCase) """ def test_maker_packs_twelve_octets(self) -> None: - """``_make_join_syn`` packs 12 octets and declares 12, before and after #576.""" + """``_make_join_syn`` packs 12 octets and declares 12.""" packed = make('_make_join_syn', 'MP_JOIN', addr_id=1, token=2, nonce=3) self.assertLengthMatchesWire(packed, 12) diff --git a/tests/protocols/transport/test_tcp_mptcp_subtype_unit.py b/tests/protocols/transport/test_tcp_mptcp_subtype_unit.py index 5b125c1721..caf73e80fe 100644 --- a/tests/protocols/transport/test_tcp_mptcp_subtype_unit.py +++ b/tests/protocols/transport/test_tcp_mptcp_subtype_unit.py @@ -193,7 +193,7 @@ def test_dss_subtype_round_trips(self) -> None: """``DSS`` built through ``TCP()`` reports its own subtype. ``ack`` alone (no ``dsn``) keeps clear of the unrelated length-arithmetic defect - that #576 fixed, which is not what this test is about. + of #576, which is not what this test is about. """ from pcapkit.const.tcp.mp_tcp_option import MPTCPOption as Enum_MPTCPOption diff --git a/tests/protocols/transport/test_tcp_runtime.py b/tests/protocols/transport/test_tcp_runtime.py index df806466ab..3d274abc89 100644 --- a/tests/protocols/transport/test_tcp_runtime.py +++ b/tests/protocols/transport/test_tcp_runtime.py @@ -78,10 +78,10 @@ def test_tcp_unregistered_application_payload_falls_back_to_raw(self) -> None: ``Data_Raw.protocol`` is "the original enumeration of this protocol", and it is worth most precisely when the protocol is unknown -- it is then the only record of what the payload claimed to be. This frame is SSH, which - :mod:`pcapkit` does not decode, so port 22 is what it has to say. It used - to say :obj:`None`, which made the frame indistinguishable from "TCP - payload on a port we cannot name at all", while the equivalent SCTP and - IPv4 cases both named theirs. + :mod:`pcapkit` does not decode, so port 22 is what it has to say. + :obj:`None` would make the frame indistinguishable from "TCP payload on + a port we cannot name at all", while the equivalent SCTP and IPv4 cases + both name theirs. The chain still ends in ``Raw`` rather than in the port's name: the ports reach :meth:`Transport._decode_next_layer diff --git a/tests/protocols/transport/test_tcp_udp_unit.py b/tests/protocols/transport/test_tcp_udp_unit.py index 5130bac4d8..03aa83ce78 100644 --- a/tests/protocols/transport/test_tcp_udp_unit.py +++ b/tests/protocols/transport/test_tcp_udp_unit.py @@ -1114,9 +1114,9 @@ def mark(schema, length: int, subtype: MPTCPOption): # NOTE: length 5, not 4: :rfc:`8684` section 3.4.2 figure 13 gives # ``Length = 3 + n``, so a two-Address-ID REMOVE_ADDR is 5 octets. The # reader only requires ``>= 3`` and does not cross-check the list against - # the length, so 4 passed -- but it enshrined the very constant #576 - # removed from ``_make_mptcp_remove``, which is worth not leaving in a - # test as though it were correct. + # the length, so 4 would pass -- but it is the constant #576 keeps out of + # ``_make_mptcp_remove``, which is worth not leaving in a test as though + # it were correct. remove = mark( MPTCPRemoveAddress(test={'subtype': MPTCPOption.REMOVE_ADDR.value, 'reserved': 0}, addr_id=[1, 2]), 5, @@ -1481,11 +1481,11 @@ def test_unregistered_option_kind_does_not_mutate_the_class_registry(self) -> No nothing registers. ``__option__`` is a :class:`collections.defaultdict` on a class attribute shared by every :class:`~pcapkit.protocols.transport.tcp.TCP` instance in the process, so - ``__option__[kind]`` inserted the kind it missed -- and the value it - inserted was ``'donone'``, exactly what the default factory returns - anyway. The entry bought nothing and cost a + a ``__option__[kind]`` read inserts the kind it misses -- and the value + it inserts is ``'donone'``, exactly what the default factory returns + anyway. The entry buys nothing and costs a :func:`~pcapkit.foundation.registry.protocols.register_tcp_option` call - afterwards a warning about an overwrite that never happened. + afterwards a warning about an overwrite that never happens. The segment is the issue's, with its data offset corrected from 7 words to 6: the issue supplies 24 octets but declared a 28-octet header. The @@ -1584,14 +1584,14 @@ def test_a_flagless_segment_seeds_its_connection_flags_as_an_enum(self) -> None: :data:`True`, so ``assertEqual(proto.connection, 0)`` passes with either seed and cannot discriminate -- and neither can a membership test alone, on the flagful rows. Hence ``assertIs(type(...), Enum_Flags)``, which is what the - :attr:`~pcapkit.protocols.transport.tcp.TCP.connection` property has always - advertised it returns. + :attr:`~pcapkit.protocols.transport.tcp.TCP.connection` property advertises + it returns. The last block pins the consequence rather than the type. ``_read_mptcp_join`` chooses between :rfc:`8684` section 3.2's three MP_JOIN layouts by testing ``self._flags`` for SYN and ACK, and falls through to the library's own ``ProtocolError`` when neither is set. On an :obj:`int` it would die on a bare - :exc:`TypeError` two lines earlier instead. That branch is not reachable from + :exc:`TypeError` at the flag test instead. That branch is not reachable from a caller -- :func:`~pcapkit.protocols.schema.transport.tcp.mptcp_data_selector` rejects a flagless MP_JOIN with a ``FieldError`` before the dispatcher runs -- so the defect is latent, but latent only by virtue of a guard in a different diff --git a/tests/vendor/test_ftp_return_code_unit.py b/tests/vendor/test_ftp_return_code_unit.py index 657d7e9745..c35b8e5e38 100644 --- a/tests/vendor/test_ftp_return_code_unit.py +++ b/tests/vendor/test_ftp_return_code_unit.py @@ -1,18 +1,18 @@ # -*- coding: utf-8 -*- """Regression tests for :mod:`pcapkit.vendor.ftp.return_code`'s empty-row guard. -GitHub issue #518. Fixing the shared ``User-Agent`` got this crawler past the -Wikipedia 403 it had been taking, and it then died one step further on: -:meth:`~pcapkit.vendor.ftp.return_code.ReturnCode.process` read ``line[0]`` on -every row of the registry table, and revision 1354125851 of the article ends that -table with ```` -- which is what MediaWiki renders a -trailing ``|-`` row separator in the wikitext as. It carries no cells at all, so -``line[0]`` raised ``IndexError: list index out of range`` and took the whole -crawler down. The guard is ``if len(line) < 2: continue``. - -That guard is the most fragile line in the #518 change and it had no test: the -suite's other cases all run against the hand-maintained IPX table or a mocked -fetch, so none of them ever sees a cell-less row. This module is that test. +GitHub issue #518. Past the Wikipedia 403 that the shared ``User-Agent`` +addresses, this crawler meets a cell-less row: revision 1354125851 of the article +ends the registry table with ```` -- which is what +MediaWiki renders a trailing ``|-`` row separator in the wikitext as. It carries +no cells at all, so a :meth:`~pcapkit.vendor.ftp.return_code.ReturnCode.process` +that reads ``line[0]`` on every row raises ``IndexError: list index out of +range`` and takes the whole crawler down. The guard is +``if len(line) < 2: continue``. + +That guard needs a test of its own: the suite's other cases all run against the +hand-maintained IPX table or a mocked fetch, so none of them sees a cell-less +row. **The fixture is built inline and parsed with the same ``html5lib`` the crawler uses**, rather than fetched. Two reasons: the unit tier makes no network call, and diff --git a/tests/vendor/test_re_sub_positional_flag_unit.py b/tests/vendor/test_re_sub_positional_flag_unit.py index 6935ca3c89..b3b975a156 100644 --- a/tests/vendor/test_re_sub_positional_flag_unit.py +++ b/tests/vendor/test_re_sub_positional_flag_unit.py @@ -205,16 +205,16 @@ def test_no_positional_flag_argument_in_vendor_tree(self) -> None: class RuntimeDeprecationWarningTests(unittest.TestCase): """The interpreter-level symptom, pinned directly on the exact call shape. - Every one of the thirteen sites shares the same pattern, ``r'\\r*\\n'``, - which contains no ``^``/``$``/``\\A``/``\\Z`` anchor -- so moving the flag - to ``flags=`` cannot change *what* it matches (it was already a no-op for - this pattern), only removes the ``count=8`` cap and the warning below. + Every affected site shares the same pattern, ``r'\\r*\\n'``, which + contains no ``^``/``$``/``\\A``/``\\Z`` anchor -- so moving the flag to + ``flags=`` cannot change *what* it matches (it is a no-op for this + pattern), only removes the ``count=8`` cap and the warning below. """ - #: The pattern shared by all thirteen sites, reproduced verbatim rather + #: The pattern shared by every affected site, reproduced verbatim rather #: than imported, since the point is to pin the runtime symptom - #: independent of which file happens to still contain it. + #: independent of which file contains it. PATTERN = r'\r*\n' #: The CPython version that began emitting a ``DeprecationWarning`` when @@ -266,9 +266,9 @@ def test_the_boundary_constant_is_load_bearing(self) -> None: # disagree with it either way: # - already warns (3.13+): push the boundary one minor version # higher, so the branch flips to "no warning expected" while the - # real behaviour still warns -- the mirror image of the wild - # failure, where the boundary was wrongly read as 3.12 instead - # of 3.13 and a real 3.12 interpreter landed on the wrong side. + # real behaviour still warns -- the mirror image of reading the + # boundary as 3.12 instead of 3.13, which puts a real 3.12 + # interpreter on the wrong side. # - does not warn yet (below 3.13): pull the boundary down to this # interpreter's own version, so the branch flips to "warning # expected" while the real behaviour still does not warn. diff --git a/tests/vendor/test_vendor_dest_path_unit.py b/tests/vendor/test_vendor_dest_path_unit.py index 198a8e652c..d0f0504fb4 100644 --- a/tests/vendor/test_vendor_dest_path_unit.py +++ b/tests/vendor/test_vendor_dest_path_unit.py @@ -93,7 +93,7 @@ only pytest jobs of :file:`.github/workflows/unit-tests.yml` that collect anything under :file:`tests/vendor/` at all, since ``integration`` and ``pypcap-parity`` never reach it and ``engine-tests`` runs only each engine's -own test files since #849 -- unlike :file:`tests/vendor/test_user_agent_unit.py`, +own test files (#849) -- unlike :file:`tests/vendor/test_user_agent_unit.py`, whose own gate also lists ``html5lib`` and so skips on both. """ @@ -350,7 +350,7 @@ def test_nested_module_resolves_under_const(self) -> None: 'dropped inside vendor/const/apptype/') def test_module_outside_vendor_root_raises_instead_of_self_truncating(self) -> None: - # The destructive case a reviewer caught: const/ and vendor/ are + # The destructive case: const/ and vendor/ are # siblings at equal depth, so for a module that is NOT under # vendor_root, os.path.relpath(module, vendor_root)'s leading '..' # segments cancel out exactly against const_root -- reproducing diff --git a/tests/vendor/test_vendor_missing_body_unit.py b/tests/vendor/test_vendor_missing_body_unit.py index 1c41b974c6..a415a6e06e 100644 --- a/tests/vendor/test_vendor_missing_body_unit.py +++ b/tests/vendor/test_vendor_missing_body_unit.py @@ -141,7 +141,7 @@ def test_the_hard_coded_body_ends_in_a_return(self) -> None: Not every emitted line has to ``return`` -- the range-guarded shape in :meth:`pcapkit.vendor.default.Vendor.process` legitimately emits - an ``if`` header and a ``#:`` comment before its ``return``, and 53 + an ``if`` header and a ``#:`` comment before its ``return``, and many crawlers do exactly that (see the module docstring). What has to be true is narrower: a hard-coded body must *end* on a ``return``, so control never falls off the end of ``miss`` into a fallthrough. diff --git a/tests/vendor/test_vendor_ngap_unit.py b/tests/vendor/test_vendor_ngap_unit.py index 01f1083b9e..e917f7625e 100644 --- a/tests/vendor/test_vendor_ngap_unit.py +++ b/tests/vendor/test_vendor_ngap_unit.py @@ -2,7 +2,7 @@ """Unit tests for :mod:`pcapkit.vendor.ngap`'s pycrate-sourced crawlers. GitHub issue #880's owner ruling for the ``ngap.py`` half of the defect: unlike -the four closed :mod:`pcapkit.protocols.internet.mh` helper enums #877 ruled +the closed :mod:`pcapkit.protocols.internet.mh` helper enums #877 ruled on, :class:`~pcapkit.const.ngap.procedure_code.ProcedureCode` and :class:`~pcapkit.const.ngap.protocol_ie.ProtocolIE` are genuinely open per 3GPP TS 38.413, so their real assignment is sourced from |pycrate|_ through a diff --git a/tests/vendor/test_vendor_reference_formats_1664_unit.py b/tests/vendor/test_vendor_reference_formats_1664_unit.py index 387b5d6f28..ead4574b04 100644 --- a/tests/vendor/test_vendor_reference_formats_1664_unit.py +++ b/tests/vendor/test_vendor_reference_formats_1664_unit.py @@ -185,7 +185,7 @@ ) #: ``tls-parameters/tls-sslkeylogfile-labels.csv``. The crawler's own comment -#: records ``[RFC9850]`` for every row as of 2026-09-28; ``[RFC 9850]`` is live. +#: records ``[RFC9850]`` for every row; ``[RFC 9850]`` is the live form. TLS_LABEL_OLD = ('Value,Description,Reference,Comment\r\n' 'CLIENT_RANDOM,Master secret in TLS 1.2 and earlier,[RFC9850],\r\n') TLS_LABEL_NEW = TLS_LABEL_OLD.replace('[RFC', '[RFC ') diff --git a/tests/vendor/test_vendor_reg_apptype_generator_unit.py b/tests/vendor/test_vendor_reg_apptype_generator_unit.py index 2d7033e399..51b40e9576 100644 --- a/tests/vendor/test_vendor_reg_apptype_generator_unit.py +++ b/tests/vendor/test_vendor_reg_apptype_generator_unit.py @@ -11,8 +11,9 @@ ``sphinx/ext/autodoc/_dynamic/_loader.py`` -- the wrapper around :func:`typing.get_type_hints` that catches the ``NameError`` a ``TYPE_CHECKING``-only forward ref would otherwise raise), each call -re-resolving the *whole* class, which made documenting this one registry cost -378.8s on CI -- the single largest stall in the entire docs build. The +re-resolving the *whole* class, which with annotated members makes documenting +this one registry cost 378.8s on CI (measured) -- the single largest stall in the +entire docs build. The annotation carries no information a lookup needs: member construction reads the positional tuple, not ``__annotations__``. Measured by #769, dropping it removes 86,838 bytes (7/8 bytes per member, depending on whether its class name is 3 or @@ -26,8 +27,8 @@ ``__getitem__``) to reach a member plain attribute access returns in 24 ns, saving 7 bytes each (``TransportProtocol.get('tcp')`` is 28 characters, ``TransportProtocol.tcp`` is 21), for 167,587 bytes (163.7 KiB) of generated -source as measured by #769. The generator only ever emits the five declared -names (``TRANSPORTS + ('undefined',)``), so the attribute form cannot miss. +source as measured by #769. The generator only ever emits the declared names +(``TRANSPORTS + ('undefined',)``), so the attribute form cannot miss. GitHub issue #770: a bare ``undefined = 0`` in the ``BASE`` template is an int literal, so mypy -- which has no :mod:`aenum` plugin and so treats this class @@ -113,7 +114,7 @@ class AppTypeGeneratorShapeTests(unittest.TestCase): - """Pins to the three mechanical changes #744, #768 and #770 made.""" + """Pins to the mechanical shapes of #744, #768 and #770.""" def test_flag_emits_attribute_access_not_get_calls(self) -> None: """GitHub issue #768, against the generator method directly. @@ -166,9 +167,9 @@ def test_no_transport_protocol_get_calls_survive_in_generated_output(self) -> No Every *generator-emitted* site -- one per member's ``proto`` argument plus one per ``cls.__transport__ is ...`` range-row test and - its own ``extend_enum(...)`` call -- was rewritten, so the generated - registries have none. The base module keeps the one legitimate - survivor, ``_dispatch``'s ``TransportProtocol.get(proto.lower())``, + its own ``extend_enum(...)`` call -- uses attribute access, so the + generated registries have none. The base module keeps one legitimate + call, ``_dispatch``'s ``TransportProtocol.get(proto.lower())``, which resolves a *caller*-supplied string at run time and so cannot be a fixed attribute access -- exactly the "other callers" #768 says :meth:`~pcapkit.const.reg.apptype.apptype.TransportProtocol.get` diff --git a/tests/vendor/test_vendor_snapshot_restore_unit.py b/tests/vendor/test_vendor_snapshot_restore_unit.py index 140a2c9fb4..2257a70e41 100644 --- a/tests/vendor/test_vendor_snapshot_restore_unit.py +++ b/tests/vendor/test_vendor_snapshot_restore_unit.py @@ -91,7 +91,7 @@ class directly would exercise :meth:`Vendor.__init__` in isolation, which is ``.const.py.*.bak`` is left behind in the directory, measured directly as ``['.const.py..bak', 'const.py']``. The interrupt itself still propagates out of ``run()`` either way, since ``run()``'s own -``except Exception:`` never caught it either; what the mutant changes is +``except Exception:`` does not catch it either; what the mutant changes is only whether anything got restored on the way out. This is why the method asserts three things rather than two: the exception's propagation, the content, and the directory listing -- the first alone is silent about @@ -194,11 +194,11 @@ def setUp(self) -> None: ``__module__`` and ``__name__``, so the message names exactly the class that *was* raised. - ``vendor_main`` and ``Vendor`` were already immune to this, because they - were resolved here in ``setUp`` rather than at module level. - ``VendorRuntimeWarning`` was not; it joins them, resolved through - :func:`importlib.import_module` so this always compares against the - same generation the crawler actually raises. + ``vendor_main``, ``Vendor`` and ``VendorRuntimeWarning`` are immune to + this, because they are resolved here in ``setUp`` rather than at module + level -- the warning class through :func:`importlib.import_module`, so + this always compares against the same generation the crawler actually + raises. """ import pcapkit.vendor.__main__ as vendor_main