From 0ccaa39c6f302b720011541f015be8ecab245c5d Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Sun, 25 Oct 2026 10:09:31 -0400 Subject: [PATCH] docs(tests): state foundation, toolkit and root tests' behaviour without change-relative narrative (#1103) Rewrite comments and docstrings that described behaviour relative to a fix ("now", "used to", "before this change") as timeless statements of behaviour or of the defect guarded against. Prose only; AST unchanged once docstrings are stripped. --- tests/_dependency_gates.py | 10 +- tests/conftest.py | 39 +++-- .../test_dpkt_raw_null_linktypes_unit.py | 2 +- tests/foundation/engines/test_engine_base.py | 45 +++--- .../test_engine_verbose_callback_1874_unit.py | 2 +- .../foundation/engines/test_pcapng_engine.py | 8 +- .../test_pcapng_spb_zero_snaplen_1558_unit.py | 2 +- .../test_pypcap_unusable_module_2132_unit.py | 2 +- .../foundation/engines/test_pyshark_engine.py | 2 +- tests/foundation/engines/test_scapy_engine.py | 2 +- .../foundation/reassembly/data/test_models.py | 4 +- tests/foundation/reassembly/test_ip.py | 6 +- ...est_ip_cut_non_final_fragment_1567_unit.py | 2 +- .../test_ip_last_fragment_offset_1565_unit.py | 2 +- .../test_ip_length_limit_1585_unit.py | 2 +- .../test_ip_oversized_datagram_1566_unit.py | 9 +- .../test_ip_tso_total_length_1555_unit.py | 8 +- .../reassembly/test_reassembly_base.py | 6 +- tests/foundation/reassembly/test_tcp.py | 31 ++-- tests/foundation/reassembly/test_timeout.py | 6 +- tests/foundation/registry/test_foundation.py | 16 +- tests/foundation/registry/test_protocols.py | 53 ++++--- tests/foundation/test_extraction.py | 12 +- tests/foundation/test_extraction_no_eof.py | 46 +++--- ...xtraction_non_seekable_stream_1586_unit.py | 10 +- tests/foundation/test_extraction_ownership.py | 15 +- .../test_extraction_unnamed_stream_unit.py | 2 +- .../test_pcapng_unique_block_names_unit.py | 12 +- .../traceflow/test_traceflow_base.py | 6 +- .../test_traceflow_label_unique_unit.py | 2 +- .../test_traceflow_snapped_capture_runtime.py | 10 +- tests/integration/_helpers.py | 10 +- tests/integration/test_engine_parity.py | 9 +- tests/integration/test_engine_runtime.py | 11 +- .../integration/test_reassembly_end_to_end.py | 4 +- .../test_reassembly_engine_parity.py | 6 +- tests/test_docstring_contract.py | 30 ++-- tests/test_final_enforcement.py | 11 +- tests/test_support_helpers.py | 12 +- tests/test_tier_guard.py | 138 ++++++++---------- tests/test_tier_guard_xdist.py | 38 ++--- ...kt_ipv4_reassembly_input_1615_1616_unit.py | 2 +- .../test_dpkt_reassembly_rawbytes_unit.py | 6 +- .../test_dpkt_tcp_ns_flags_1618_unit.py | 2 +- .../toolkit/test_engine_pcap_trace_runtime.py | 8 +- .../test_pcap_jumbogram_rfc2675_1687_unit.py | 4 +- ...pcap_tcp_segment_ipv6_version_1666_unit.py | 4 +- tests/toolkit/test_pcap_tcp_segment_unit.py | 7 +- tests/toolkit/test_pyshark_unit.py | 13 +- tests/toolkit/test_scapy_linktype_unit.py | 4 +- tests/toolkit/test_scapy_unit.py | 14 +- .../test_tcp_carrier_walk_1606_1617_unit.py | 2 +- .../test_tunnelled_tcp_flow_key_1581_unit.py | 2 +- 53 files changed, 337 insertions(+), 374 deletions(-) diff --git a/tests/_dependency_gates.py b/tests/_dependency_gates.py index 4edc8a56f6..265e2951fa 100644 --- a/tests/_dependency_gates.py +++ b/tests/_dependency_gates.py @@ -178,8 +178,8 @@ # :file:`pyproject.toml` extra declares it at all -- so # :func:`extras_providing` resolves it to the empty set and every pytest # job reaching a ``HAS_MYPY`` gate is reported as a :class:`Gap`. That is - # the honest answer rather than the :exc:`KeyError` a missing entry used to - # raise: the gate really is dark in CI, and + # the honest answer rather than a :exc:`KeyError` for a missing entry: the + # gate really is dark in CI, and # :data:`DEPENDENCY_GATE_EXCLUSIONS`'s ``HAS_MYPY`` entry is where the reason # for declining to close it lives (#779). Spelled as a requirement string # anyway, so the day some extra does carry mypy the gap closes itself @@ -190,8 +190,8 @@ # (#766), so :func:`extras_providing` resolves it to ``{'test'}``, every # pytest job installs it, and ``HAS_ISORT`` is a satisfied gate with no # :data:`DEPENDENCY_GATE_EXCLUSIONS` entry at all -- which is the whole point - # of listing it here: the guard now *counts* a gate it could not see before, - # and finds nothing to report. + # of listing it here: the guard *counts* a gate it could not see without + # the entry, and finds nothing to report. # # Why the opposite ruling from ``mypy``, whose gap is declined above: that # entry's reason turns on lint.yml already running mypy over the whole @@ -539,7 +539,7 @@ class Job(NamedTuple): That is deliberate rather than an oddity to special-case away: :func:`dependency_gate_gaps` aggregates by ``(flag, job.name)``, and a gate reached by *any* of a job's per-engine variants is exactly what "this job - reaches this gate" has to mean once one job's cells no longer all install + reaches this gate" has to mean when one job's cells do not all install and select the same thing. """ diff --git a/tests/conftest.py b/tests/conftest.py index cc54cbaaf0..129889ec34 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -137,23 +137,22 @@ def pytest_sessionstart(session: 'pytest.Session') -> 'None': happens here decides what every test is restored *to*. Left cold, the region is empty at the pin and restored to empty after every test, which makes each of the tests that does not purge for itself -- :mod:`tests.project` - is the bulk of them -- pay a fresh ``import pcapkit`` it used to get from the - warm table. Measured over ``tests/project/``: 1.59s with no + is the bulk of them -- pay a fresh ``import pcapkit`` it would otherwise get + from the warm table. Measured over ``tests/project/``: 1.59s with no guard at all, **9.34s** guarded from a cold table, **0.35s** guarded from a warm one. The last is the fastest of the three because the import it would otherwise do inside the first test has moved here instead. - This used **not** to help a class that purges in - :meth:`~unittest.TestCase.setUpClass`, for the reason - :func:`_pin_module_snapshot` explains: such a class purges *after* this hook - and *before* the first snapshot the old, entry-scoped fixture took, so that - snapshot was whatever the class left -- warming the table earlier changed - nothing about it. Restoring to a *pinned* snapshot before each test as well - as after, rather than to one taken at that test's own entry, is what makes - the warm-up matter to a purging class too now: the pin below still captures - this import, and every later test -- including a purging class's own first - one -- is restored to it going in as well as coming out, regardless of - whether that class reloads anything for itself. + It helps a class that purges in :meth:`~unittest.TestCase.setUpClass` too. + Such a class purges *after* this hook and *before* its first test, so a + snapshot taken at that test's own entry would be whatever the class left -- + warming the table earlier would change nothing about it, as + :func:`_pin_module_snapshot` explains. Restoring to a *pinned* snapshot + before each test as well as after is what makes the warm-up matter to a + purging class: the pin below captures this import, and every later test -- + including a purging class's own first one -- is restored to it going in as + well as coming out, regardless of whether that class reloads anything for + itself. So a class that purges and reimports for itself, as :class:`tests.integration._helpers.EndToEndTestCase` does, still runs its tests on the pin. Only a module object a class attribute holds a direct @@ -269,13 +268,11 @@ def restore_module_table() -> 'Iterator[None]': that will be written, which is the difference between the failure being unlikely and being impossible. - The others were fixed at their call sites when this guard landed, but - :mod:`tests.cli.test_main` was not: it kept its own purge loop, which - purges and restores nothing, and this fixture went on quietly healing it for - every test. That is issue #688 -- it took an audit rather than a failing run - to find, because a guard that repairs a leak also hides it. It is now fixed - at its call site too, which is what makes the paragraph above true of all - three rather than of two. + A guard that repairs a leak also hides it. Issue #688 -- a purge loop in + :mod:`tests.cli.test_main` that purged and restored nothing, which this + fixture quietly healed for every test -- took an audit rather than a + failing run to find. That is why every leak is fixed at its call site as + well. Deliberately *not* fixed by moving ``test_protochain.py`` so it sorts elsewhere, or by leaning on a neighbouring test's purge to heal the state. @@ -603,7 +600,7 @@ def _tier_guard_lock(exclusive: 'bool') -> 'Iterator[None]': writes a real violation to disk, under :data:`~tests._tiers.PROBE_MODULE_NAME`, to reproduce the tier guard's own collection-time diagnostic end to end. Serially the two never overlap; under the ``-n auto --dist load`` this - suite's CI jobs now use, pytest-xdist may hand the two tests to different + suite's CI jobs use, pytest-xdist may hand the two tests to different worker processes at the same moment, and the scan can then observe the probe mid-life and report a violation nobody committed -- a real defect in the test suite's own concurrency, not a false positive, since the guard is diff --git a/tests/foundation/engines/test_dpkt_raw_null_linktypes_unit.py b/tests/foundation/engines/test_dpkt_raw_null_linktypes_unit.py index 0965f59333..af3542a1a0 100644 --- a/tests/foundation/engines/test_dpkt_raw_null_linktypes_unit.py +++ b/tests/foundation/engines/test_dpkt_raw_null_linktypes_unit.py @@ -4,7 +4,7 @@ GitHub issues #1502 and #1574. :meth:`DPKT._get_protocol ` knew Ethernet, IPv4 and IPv6 only, so a ``LINKTYPE_RAW`` (101), ``LINKTYPE_NULL`` (0) or -``LINKTYPE_LOOP`` (108) frame was kept as raw data with a warning. Raw IP is now +``LINKTYPE_LOOP`` (108) frame was kept as raw data with a warning. Raw IP is read as IPv4 or IPv6 by its version nibble, and both loopback link types by the address family, as :class:`~pcapkit.protocols.link.loopback.Loopback` reads them, with the network layer where :mod:`pcapkit.toolkit.dpkt` looks for it. diff --git a/tests/foundation/engines/test_engine_base.py b/tests/foundation/engines/test_engine_base.py index 7b1240e3e1..a5521ae0ca 100644 --- a/tests/foundation/engines/test_engine_base.py +++ b/tests/foundation/engines/test_engine_base.py @@ -69,25 +69,22 @@ def read_frame(self) -> str: def test_engine_subclass_registration_is_opt_in(self) -> None: """Registration happens if and only if ``engine`` is given. - This is the #514 opt-in contract. Before it, ``__init_subclass__`` fell - back to ``cls.name`` when the keyword was absent, so *every* subclass of - the public class was registered -- which is why no built-in could - subclass it and every one of them inherited :class:`EngineBase` under an - alias instead. + This is the #514 opt-in contract. It guards against ``__init_subclass__`` + falling back to ``cls.name`` when the keyword is absent, which registers + *every* subclass of the public class. The ``Default`` case is the load-bearing half: it sets - ``__engine_name__``, so under the old fallback it registered under - ``'defaultengine'``. A class attribute is not a registry key, and it + ``__engine_name__``, so a fallback to ``cls.name`` would register it under + ``'defaultengine'``. A class attribute is not a registry key, so it registers nothing. - No version guard, and that is the point of the rename. The keyword was - ``name`` when opt-in registration landed, which made this half of the test - unrunnable below Python 3.11 -- ``mcls``/``name``/``bases``/``namespace`` - collide with :meth:`abc.ABCMeta.__new__`'s own parameters, which are - positional-or-keyword before 3.11 and positional-only from 3.11, so the - class statement failed in the metaclass before ``__init_subclass__`` ran. - ``engine`` is outside that set, so this runs everywhere. Verified on - 3.10.21 and 3.14.7. + No version guard, and that is why the keyword is ``engine`` rather than + ``name``: ``mcls``/``name``/``bases``/``namespace`` collide with + :meth:`abc.ABCMeta.__new__`'s own parameters, which are + positional-or-keyword before 3.11 and positional-only from 3.11, so below + Python 3.11 such a class statement fails in the metaclass before + ``__init_subclass__`` runs. ``engine`` is outside that set, so this runs + everywhere. Verified on 3.10.21 and 3.14.7. """ from pcapkit.foundation.engines.engine import Engine @@ -123,10 +120,9 @@ def test_engine_subclass_rejects_unrecognised_keyword(self) -> None: ``Engine`` spells the registry key ``engine`` while ``Reassembly`` and ``TraceFlow`` spell the same idea ``protocol``, so guessing the wrong one - is the expected mistake. It used to land in ``**kwargs``, get dropped by - the bare ``super().__init_subclass__()``, and leave the class registered - under its own class name. With registration now opt-in the same typo - would instead skip registration silently, which is quieter still. + is the expected mistake. Without the guard it lands in ``**kwargs``, is + dropped by a bare ``super().__init_subclass__()``, and, registration being + opt-in, the class silently goes unregistered. Unguarded by version on purpose: ``protocol`` is not one of the four names that collide with :meth:`abc.ABCMeta.__new__` below Python 3.11, so @@ -138,7 +134,7 @@ def test_engine_subclass_rejects_unrecognised_keyword(self) -> None: version-dependent: below 3.11 the metaclass collision fires before the guard, so it is a :exc:`TypeError` rather than an :exc:`~pcapkit.utilities.exceptions.UnsupportedCall`. Loud either way, - which is why the rename did not need to chase it. + which is why the guard needs no special case for it. """ from pcapkit.foundation.engines.engine import Engine @@ -179,8 +175,8 @@ def test_registration_is_not_inherited_by_a_subclass(self) -> None: ``final`` marker -- so that subclassing them again cannot register the subclass under a name nobody chose. That worked, but it is a property of *which base you inherited*, so it cannot be varied per subclass, and - under the old fallback a subclass of a registered class re-registered - itself under its own class name. + under a ``cls.name`` fallback a subclass of a registered class + re-registers itself under its own class name. Keying registration on the keyword instead gives both properties at once: the parent stays registered, the subclass does not re-register, and a @@ -188,8 +184,9 @@ def test_registration_is_not_inherited_by_a_subclass(self) -> None: ``HTTP``, ``L2TP`` and ``IP`` can be open to inheritance without the anti-re-registration guarantee being weakened. - Runs on every supported version since the keyword became ``engine``; - it was skipped below 3.11 while the keyword was ``name``. + Runs on every supported version: ``engine`` does not collide with + :meth:`abc.ABCMeta.__new__`'s own parameters below 3.11, as ``name`` + would. """ from pcapkit.foundation.engines.engine import Engine diff --git a/tests/foundation/engines/test_engine_verbose_callback_1874_unit.py b/tests/foundation/engines/test_engine_verbose_callback_1874_unit.py index 24333d4473..180d801b2c 100644 --- a/tests/foundation/engines/test_engine_verbose_callback_1874_unit.py +++ b/tests/foundation/engines/test_engine_verbose_callback_1874_unit.py @@ -4,7 +4,7 @@ :class:`~pcapkit.foundation.extraction.Extractor` keeps a callable ``verbose`` as its ``_vfunc``, but each third-party engine installed its own printer whenever verbose output was on, so the caller's callback was replaced and never -called. An engine now installs its printer only for ``verbose=True``. +called. An engine installs its printer only for ``verbose=True``. Each case runs a real extraction of :file:`in.pcap`, which git tracks, so the module is unit tier. It proves first that the requested engine ran: the diff --git a/tests/foundation/engines/test_pcapng_engine.py b/tests/foundation/engines/test_pcapng_engine.py index 1a99612cb6..6d64899dea 100644 --- a/tests/foundation/engines/test_pcapng_engine.py +++ b/tests/foundation/engines/test_pcapng_engine.py @@ -414,10 +414,10 @@ def test_packet_blocks_without_an_interface_description_are_format_errors(self) self.assertIn('interface description block', message) def test_out_of_range_interface_id_is_a_format_error(self) -> None: - # Regression for #367: a section with at least one IDB and a block naming - # an interface it does not describe used to escape as a bare - # ``IndexError`` out of ``PCAPNG._get_timezone``, because the engine's - # bounds guards ran only after the block had already been parsed. + # Regression for #367: guards against a section with at least one IDB and + # a block naming an interface it does not describe escaping as a bare + # ``IndexError`` out of ``PCAPNG._get_timezone``, which happens when the + # engine's bounds guards run only after the block has been parsed. from pcapkit.utilities.exceptions import FormatError blocks = { diff --git a/tests/foundation/engines/test_pcapng_spb_zero_snaplen_1558_unit.py b/tests/foundation/engines/test_pcapng_spb_zero_snaplen_1558_unit.py index 8851c51d3e..822001e411 100644 --- a/tests/foundation/engines/test_pcapng_spb_zero_snaplen_1558_unit.py +++ b/tests/foundation/engines/test_pcapng_spb_zero_snaplen_1558_unit.py @@ -5,7 +5,7 @@ 0's snaplen, and a snaplen of 0 means no limit. The default engine handed interface 0's snaplen to the parser as ``__packet__['snaplen']`` as it stood, so a 0 read as a limit of no octets, and the block's padding was read as packet -data: 57 data octets came out as 60. The engine now hands over no limit for a +data: 57 data octets came out as 60. The engine hands over no limit for a zero snaplen, and :class:`~pcapkit.protocols.misc.pcapng.PCAPNG` reads a zero snaplen handed in as no limit too. diff --git a/tests/foundation/engines/test_pypcap_unusable_module_2132_unit.py b/tests/foundation/engines/test_pypcap_unusable_module_2132_unit.py index 945d20096f..c1d0bcce2b 100644 --- a/tests/foundation/engines/test_pypcap_unusable_module_2132_unit.py +++ b/tests/foundation/engines/test_pypcap_unusable_module_2132_unit.py @@ -31,7 +31,7 @@ #: What the fake ``pcap`` package raises on import: the libpcap loader's message. LOADER_ERROR = 'Cannot find libpcap.so library' -#: The verdict ``PCAP_CT`` already gave on that failure, which ``PyPCAP`` shares. +#: The verdict ``PCAP_CT`` and ``PyPCAP`` both give on that failure. REASON = f'the `pcap` module is installed but unusable -- OSError: {LOADER_ERROR}' #: Hang guard for the child interpreter, in seconds. TIMEOUT = 120 diff --git a/tests/foundation/engines/test_pyshark_engine.py b/tests/foundation/engines/test_pyshark_engine.py index e15a5df368..c46adc6e2d 100644 --- a/tests/foundation/engines/test_pyshark_engine.py +++ b/tests/foundation/engines/test_pyshark_engine.py @@ -19,7 +19,7 @@ * **the** :program:`tshark` **binary** -- ``pyshark`` shells out to it and parses nothing itself. -What the running host provides is no longer assumed either way, and that is worth +What the running host provides is not assumed either way, and that is worth stating plainly rather than leaving to a silent skip: :program:`tshark` is absent on most of this file's legs, but #751's ``engine-tests`` job -- which collects this same module -- installs it via ``apt-get``, so neither "present" nor "absent" can diff --git a/tests/foundation/engines/test_scapy_engine.py b/tests/foundation/engines/test_scapy_engine.py index 4d06ba61a3..9015f03686 100644 --- a/tests/foundation/engines/test_scapy_engine.py +++ b/tests/foundation/engines/test_scapy_engine.py @@ -21,7 +21,7 @@ public API resets them, and ``sys.modules`` surgery does not un-run a ``bind_layers`` call. So an in-process assertion here would pass whenever something earlier in the run had imported a `Scapy`_ layer -- which is exactly -what happened before #406 was found. :mod:`tests.toolkit.test_scapy_unit` imports +how #406 went unnoticed. :mod:`tests.toolkit.test_scapy_unit` imports :mod:`scapy.layers.l2`, :mod:`~scapy.layers.inet` and :mod:`~scapy.layers.inet6` to build its fixtures, so ``pytest tests/toolkit tests/interface/test_misc.py`` saw correct dissection while ``pytest tests/interface/test_misc.py`` alone did diff --git a/tests/foundation/reassembly/data/test_models.py b/tests/foundation/reassembly/data/test_models.py index 65ac529678..47c2487b26 100644 --- a/tests/foundation/reassembly/data/test_models.py +++ b/tests/foundation/reassembly/data/test_models.py @@ -52,12 +52,12 @@ def test_ip_data_models_and_package_aliases(self) -> None: self.assertEqual(storage.tcp, ()) def test_completion_is_a_string_that_still_reads_as_the_old_bool(self) -> None: - """The contract ``Datagram.completed`` has to keep, now it is a StrEnum. + """The contract ``Datagram.completed`` has to keep as a StrEnum. Deriving from :class:`~pcapkit.utilities.compat.StrEnum` -- as :class:`~pcapkit.protocols.application.httpv1.Type` does -- buys serialisability and comparison against a plain string. What it must not - cost is the truthiness callers of the former :obj:`bool` field rely on, + cost is the truthiness callers rely on, as for a :obj:`bool` field, which needs ``__bool__`` overridden because every non-empty string is otherwise truthy. diff --git a/tests/foundation/reassembly/test_ip.py b/tests/foundation/reassembly/test_ip.py index d4de9dfe6a..9e71a90d7e 100644 --- a/tests/foundation/reassembly/test_ip.py +++ b/tests/foundation/reassembly/test_ip.py @@ -124,9 +124,9 @@ class IPOverlapConflictTests(unittest.TestCase): :rfc:`791` resolves *which* bytes win itself: "this procedure will use the more recently arrived copy in the data buffer" -- the opposite of TCP's first-write-wins (:rfc:`9293#section-3.10`, fixed for TCP by #443/#478). - So every case below still expects the *arriving* fragment's bytes in the - reassembled payload; what changes is that ``datagram.conflict`` now - records where that overwrite disagreed with what was already there. + So every case below expects the *arriving* fragment's bytes in the + reassembled payload, and ``datagram.conflict`` records where that + overwrite disagreed with what was already there. """ diff --git a/tests/foundation/reassembly/test_ip_cut_non_final_fragment_1567_unit.py b/tests/foundation/reassembly/test_ip_cut_non_final_fragment_1567_unit.py index aa33f79c95..0a9e90c59a 100644 --- a/tests/foundation/reassembly/test_ip_cut_non_final_fragment_1567_unit.py +++ b/tests/foundation/reassembly/test_ip_cut_non_final_fragment_1567_unit.py @@ -10,7 +10,7 @@ Every fragment but the last carries a multiple of 8 octets (:rfc:`8200#section-4.5`, :rfc:`791#section-3.2`), so only the final fragment's -partial last block is now counted received. That covers an IPv4 fragment whose +partial last block is counted received. That covers an IPv4 fragment whose Total Length declares a non-multiple of 8 on a non-final fragment as well. Every case builds its fragments in memory and reads no capture. Classes are diff --git a/tests/foundation/reassembly/test_ip_last_fragment_offset_1565_unit.py b/tests/foundation/reassembly/test_ip_last_fragment_offset_1565_unit.py index 02737c3830..31acd14435 100644 --- a/tests/foundation/reassembly/test_ip_last_fragment_offset_1565_unit.py +++ b/tests/foundation/reassembly/test_ip_last_fragment_offset_1565_unit.py @@ -7,7 +7,7 @@ fragment at the largest Fragment Offset, 8191, among them -- made :meth:`IP._detect_conflicts ` index past the table, and the whole extraction raised :exc:`IndexError`. The -table now has 8,192 entries, the partial last block included. +table has 8,192 entries, the partial last block included. Every case builds its fragments in memory and reads no capture. Classes are imported inside each test, after :func:`~tests._support.reimport_once_per_class`. diff --git a/tests/foundation/reassembly/test_ip_length_limit_1585_unit.py b/tests/foundation/reassembly/test_ip_length_limit_1585_unit.py index 01cab4df8a..1b9c2d32df 100644 --- a/tests/foundation/reassembly/test_ip_length_limit_1585_unit.py +++ b/tests/foundation/reassembly/test_ip_length_limit_1585_unit.py @@ -4,7 +4,7 @@ GitHub issue #1585. The data buffer holds 65,535 octets of data, but IPv4's Total Length counts the header as well, so fragments whose data covered octets 0-65,522 behind a 20-octet header made a 65,543-octet datagram, and it read -``COMPLETE``. Linux's ``ip_frag_reasm`` rejects it. The bound now counts the +``COMPLETE``. Linux's ``ip_frag_reasm`` rejects it. The bound counts the header as the family's length field does: IPv4's Total Length counts all of it, and IPv6's Payload Length counts the extension headers but not the fixed 40-octet header. A Total Length of 0 declares no length (#1555), so a datagram diff --git a/tests/foundation/reassembly/test_ip_oversized_datagram_1566_unit.py b/tests/foundation/reassembly/test_ip_oversized_datagram_1566_unit.py index 4a47332896..15b74c68cc 100644 --- a/tests/foundation/reassembly/test_ip_oversized_datagram_1566_unit.py +++ b/tests/foundation/reassembly/test_ip_oversized_datagram_1566_unit.py @@ -4,14 +4,13 @@ GitHub issue #1566. The data buffer holds 65,535 octets and ``RCVBT`` 8,192 blocks. Both completion checks tested ``all(RCVBT[0:(TDL + 7) // 8])``, and the slice stops silently at the table's end, so a final fragment declaring more data -than fits completed its datagram short. A datagram now completes only if its +than fits completed its datagram short. A datagram completes only if its total data length fits the buffer. A fragment the buffer's end clips counts as cut there: its partial last block is -a hole. An 8-octet non-final fragment at the largest offset therefore received -no block, and strict mode then dropped its datagram, while a 7-octet one was -reported. #1567 leaves the 7-octet one's block clear too, and strict mode now -reports a datagram with no run received, as lax mode does. +a hole. An 8-octet non-final fragment at the largest offset therefore receives +no block, and so does a 7-octet one (#1567). Strict mode reports a datagram with +no run received, as lax mode does. Every case builds its fragments in memory and reads no capture. Classes are imported inside each test, after :func:`~tests._support.reimport_once_per_class`. diff --git a/tests/foundation/reassembly/test_ip_tso_total_length_1555_unit.py b/tests/foundation/reassembly/test_ip_tso_total_length_1555_unit.py index b8fb91614f..f7966579e6 100644 --- a/tests/foundation/reassembly/test_ip_tso_total_length_1555_unit.py +++ b/tests/foundation/reassembly/test_ip_tso_total_length_1555_unit.py @@ -2,10 +2,10 @@ """IP reassembly reads a Total Length of 0 as the fragment's captured length. GitHub issue #1555. A capture taken on a host that offloads TCP segmentation -records its IPv4 headers with Total Length 0, and since #1547 the adapters hand -over the rest of the captured frame as the payload. The reassembler still took -the fragment's data length from ``tl - ihl``, i.e. ``-20``, so every such -datagram came out ``PARTIAL`` and empty. It now takes the payload's own length, +records its IPv4 headers with Total Length 0, and the adapters hand over the +rest of the captured frame as the payload (#1547). Taking the fragment's data +length from ``tl - ihl``, i.e. ``-20``, made every such datagram come out +``PARTIAL`` and empty. The reassembler takes the payload's own length instead, as Wireshark 4.6.9 does ("presumed TSO") before it reassembles anything. A non-final fragment's partial last block can then only be a snaplen cut, so it is left as a hole. diff --git a/tests/foundation/reassembly/test_reassembly_base.py b/tests/foundation/reassembly/test_reassembly_base.py index a45b5c283a..296598746d 100644 --- a/tests/foundation/reassembly/test_reassembly_base.py +++ b/tests/foundation/reassembly/test_reassembly_base.py @@ -181,9 +181,9 @@ def submit(self, buf: DummyBuffer, **kwargs: object) -> list[DummyDatagram]: def test_reassembly_subclass_rejects_unrecognised_keyword(self) -> None: """A misspelled class keyword raises instead of being swallowed. - It used to land in ``**kwargs``, get dropped by the bare - ``super().__init_subclass__()``, and leave the class registered under its - own class name -- no exception, no warning. + Guards against it landing in ``**kwargs``, being dropped by a bare + ``super().__init_subclass__()``, and leaving the class registered under + its own class name -- no exception, no warning. The keyword used here is deliberately *not* ``name``. Four class keyword names -- ``mcls``, ``name``, ``bases`` and ``namespace`` -- collide with diff --git a/tests/foundation/reassembly/test_tcp.py b/tests/foundation/reassembly/test_tcp.py index 3468253eff..c0c58b6046 100644 --- a/tests/foundation/reassembly/test_tcp.py +++ b/tests/foundation/reassembly/test_tcp.py @@ -38,7 +38,7 @@ def _packet(self, *, num: int, dsn: int, ack: int = 500, payload: bytes = b'', ``first`` defaults to ``dsn`` and ``last`` to ``dsn + len(payload) - 1`` -- absolute sequence numbers, with ``last`` inclusive -- because that is - what every :mod:`pcapkit.toolkit` module now passes. Tests that exercise + what every :mod:`pcapkit.toolkit` module passes. Tests that exercise the :rfc:`815` interval arithmetic on its own still override both. """ @@ -367,10 +367,10 @@ def _run(self, packets, **kwargs): def test_incomplete_datagram_survives_a_realistic_initial_sequence_number(self) -> None: """A permanently missing segment gives ``completed=False``, not silence. - Two of the five segments never arrive. Before the fix this produced no - datagram whatsoever: the hole bounds were around 3.2 billion while the - payload buffer was 50 octets long, so every slice came back empty and - ``submit`` dropped the datagram at its ``if data:`` guard without a + Two of the five segments never arrive. Without the fix this produces no + datagram whatsoever: the hole bounds are around 3.2 billion while the + payload buffer is 50 octets long, so every slice comes back empty and + ``submit`` drops the datagram at its ``if data:`` guard without a word. """ @@ -408,11 +408,11 @@ def test_the_answer_does_not_depend_on_the_initial_sequence_number(self) -> None def test_a_capture_without_the_handshake_reports_every_fragment(self) -> None: """A stream picked up mid-flight still reports all of its fragments. - Before the fix this returned a *partial* answer rather than nothing -- - one fragment out of three -- because the completeness test counted hole - descriptors (``len(HDL) > 2``) rather than asking whether a hole fell + Without the fix this returns a *partial* answer rather than nothing -- + one fragment out of three -- because the completeness test counts hole + descriptors (``len(HDL) > 2``) rather than asking whether a hole falls inside the received data. Without a SYN the list is one entry shorter, - so a two-gap stream slipped under the threshold. + so a two-gap stream slips under the threshold. """ packets = self._stream(syn=False, @@ -633,8 +633,8 @@ def test_fin_completes_a_stream_above_2_31_whatever_sys_maxsize(self) -> None: class TCPReassemblyConflictTests(unittest.TestCase): """Regression tests for GitHub issue #443. - Two segments claiming the same sequence range but carrying *different* - bytes used to resolve last-write-wins, silently, with ``completed`` still + Guards against two segments claiming the same sequence range but carrying + *different* bytes resolving last-write-wins, silently, with ``completed`` reporting the datagram whole. Per :rfc:`9293#section-3.10` ("we reconstruct the segment to contain just the new data") the resolution is first-write-wins instead: the already-buffered bytes are kept, the @@ -696,9 +696,9 @@ def test_identical_retransmission_stays_uncontested_and_unchanged(self) -> None: def test_conflicting_full_overlap_keeps_the_first_segment(self) -> None: """The issue's own reproduction: first-write-wins, and the range is recorded. - Before the fix this returned ``completed=True`` and + Without the fix this returns ``completed=True`` and ``payload=b'BBBBBBBBCCCC'`` -- the later, conflicting segment silently - won. The RFC-conformant answer keeps the first segment's bytes. + wins. The RFC-conformant answer keeps the first segment's bytes. """ base = self.BASE @@ -914,9 +914,8 @@ def test_a_genuine_gap_fill_through_the_overlap_merge_is_never_a_conflict(self) def test_a_simultaneous_head_prepend_and_tail_extension_in_one_overlap_call(self) -> None: """Full engulfment plus extension: a new head *and* a new tail in the same call. - Corrects a wrong comment that used to sit on the reach-back branch, - claiming a head-prepend and a tail-append "cannot both happen at - once". They can: the old buffer (``isn=100``, ``len=10``) is fully + A head-prepend and a tail-append can both happen at once: the old + buffer (``isn=100``, ``len=10``) is fully inside the arriving segment's range (``dsn=90``, ``len=30``), so the arriving segment supplies genuinely new bytes *before* ``isn`` (90..99) and genuinely new bytes *past* the old buffer's end diff --git a/tests/foundation/reassembly/test_timeout.py b/tests/foundation/reassembly/test_timeout.py index 07780d2077..693588e581 100644 --- a/tests/foundation/reassembly/test_timeout.py +++ b/tests/foundation/reassembly/test_timeout.py @@ -226,9 +226,9 @@ def run() -> list: class IPLooseModeTests(unittest.TestCase): """``strict=False``, where IP reports one contiguous payload, not the runs. - This branch had no test, and it is the one where the payload -- not merely the - ``completed`` flag -- changed: an unterminated datagram used to come back as - 65534 octets of preallocated buffer, reported *complete*. + This branch guards the payload, not merely the ``completed`` flag: without the + fix, an unterminated datagram comes back as 65534 octets of preallocated + buffer, reported *complete*. """ def setUp(self) -> None: diff --git a/tests/foundation/registry/test_foundation.py b/tests/foundation/registry/test_foundation.py index 6a2bc46ccc..abbf5d3401 100644 --- a/tests/foundation/registry/test_foundation.py +++ b/tests/foundation/registry/test_foundation.py @@ -169,15 +169,13 @@ def test_registration_accepts_pcapkit_own_builtin_classes(self) -> None: Deliberately does **not** mock, because the point is to exercise the gate. """ - # NOTE: GitHub issue #1016 moved this assertion. #513 was fixed by widening the - # public ``register_*`` guards to the ``*Base`` classes; #1016 narrowed them back - # to ``Engine``/``Reassembly``/``TraceFlow`` (the registration surface third - # parties extend), so the public door now refuses the built-ins -- see - # ``test_public_registration_rejects_base_only_classes`` below. What #513 bought - # -- that pcapkit's *own* classes are registrable through a real, unmocked gate -- - # is kept by pointing this test at the internal ``Extractor._register_internal_*`` - # path instead of the public wrappers. The subject classes and the read-back - # are unchanged. + # NOTE: the public ``register_*`` guards accept ``Engine``/``Reassembly``/ + # ``TraceFlow`` (the registration surface third parties extend), so the public + # door refuses the built-ins (#1016) -- see + # ``test_public_registration_rejects_base_only_classes`` below. What #513 + # guards -- that pcapkit's *own* classes are registrable through a real, + # unmocked gate -- is checked through the internal + # ``Extractor._register_internal_*`` path rather than the public wrappers. # # NOTE: imported inside the test because ``setUp`` puts the class's own ``pcapkit`` # import in ``sys.modules``, which is why every sibling test imports locally too. diff --git a/tests/foundation/registry/test_protocols.py b/tests/foundation/registry/test_protocols.py index b96a09744b..a6f8902142 100644 --- a/tests/foundation/registry/test_protocols.py +++ b/tests/foundation/registry/test_protocols.py @@ -92,10 +92,11 @@ def test_register_protocol_warns_when_a_colliding_name_overwrites(self) -> None: :class:`pcapkit.protocols.application.http.HTTP`, :class:`pcapkit.protocols.application.httpv1.HTTP` and :class:`pcapkit.protocols.application.httpv2.HTTP` all land on ``'HTTP'`` - and registering one replaces whichever was there. Before #675 the - replacement was silent -- no exception, no warning -- so a later bare-name - lookup simply returned a different class than it had before, with nothing - connecting the symptom to the registration that caused it. + and registering one replaces whichever was there. Guards against the + replacement being silent (#675) -- no exception, no warning -- so that a + later bare-name lookup simply returns a different class than it did + before, with nothing connecting the symptom to the registration that + caused it. """ from pcapkit.foundation.registry import protocols as registry @@ -148,8 +149,8 @@ def test_a_protocol_subclass_shadowing_a_builtin_name_warns(self) -> None: :class:`~pcapkit.protocols.protocol.Protocol` under a name a built-in already holds displaces the built-in's entry. That is the sharpest form of #675: there is no call to any registry function anywhere in the - user's code, so before the fix nothing at all marked the moment the - dispatch table changed meaning. + user's code, so without the fix nothing at all marks the moment the + dispatch table changes meaning. """ from pcapkit.foundation.registry import protocols as registry @@ -231,19 +232,15 @@ def test_register_protocol_stays_quiet_when_nothing_is_displaced(self) -> None: self.assertEqual(self._registry_warnings(caught, RegistryWarning), []) def test_sibling_registries_now_stay_quiet_on_an_identical_re_registration(self) -> None: - """GitHub issue #718: the siblings now share ``register_protocol``'s guard. - - This test used to pin the opposite: the code-keyed registries warned - on presence alone, even when the incumbent and the replacement were - the same object, and #695's commit message argued that was - deliberate -- the key is caller-supplied rather than derived from the - value, so "present and different" supposedly had nothing to fix there. - #718 found that reasoning did not hold (a caller replaying a - registration is not a mistake merely because the key happens to be - explicit), added the identity guard, and this test now pins *that*: a - repeat registration naming the exact same class stays silent, exactly + """GitHub issue #718: the siblings share ``register_protocol``'s guard. + + A repeat registration naming the exact same class stays silent, exactly as :func:`~pcapkit.foundation.registry.protocols.register_protocol` - already behaved. + does. Guards against the code-keyed registries warning on presence + alone, even when the incumbent and the replacement are the same object. + That the key is caller-supplied rather than derived from the value does + not justify the warning: a caller replaying a registration is not a + mistake merely because the key happens to be explicit. """ from pcapkit.const.reg.ethertype import EtherType @@ -277,11 +274,11 @@ def test_sibling_registries_now_stay_quiet_on_an_identical_re_registration(self) def test_sibling_registries_name_what_they_displaced(self) -> None: """Every code-keyed registrar says *what* it overwrote, not just that it did. - Independent of the identity guard the test above now pins (#718): a - genuine overwrite -- two different classes -- still warns and still - names both. The message used to be ``'protocol {code} already - registered, overwriting'`` and stop there, so a caller learned that - something had been displaced and never which class it was. That is the + Independent of the identity guard the test above pins (#718): a + genuine overwrite -- two different classes -- warns and names both. + Guards against a message reading ``'protocol {code} already + registered, overwriting'`` and stopping there, so a caller learns that + something has been displaced and never which class it was. That is the same diagnostic gap #675 closed for :func:`~pcapkit.foundation.registry.protocols.register_protocol`, and it is the part of #681 that does generalise. @@ -368,7 +365,7 @@ def test_register_protocol_disambiguates_classes_sharing_a_repr(self) -> None: builds a closure-local ``DummyProtocol`` on every call and every call after the first warns. - Before the fix the message was the same string for both operands -- + Without the fix the message is the same string for both operands -- ``overwriting with `` -- which is technically true and tells a reader nothing, because the registry *did* overwrite one object with a different one, but nothing @@ -522,7 +519,7 @@ def test_top_level_link_internet_and_transport_protocol_wrappers(self) -> None: register_protocol.assert_called_once_with(UnitProtocol) # NOTE: naming both transport protocols is the explicit way to reach both, - # one argument each, and it is the only way now that no member's ``proto`` + # one argument each, and it is the only way, since no member's ``proto`` # carries a composite to fan out from. with mock.patch.object(registry.TCP, 'register') as tcp_register: with mock.patch.object(registry.UDP, 'register') as udp_register: @@ -824,15 +821,15 @@ def test_register_apptype_explicit_null_string_is_a_class_name_not_the_sentinel( self.assertIn("'(null)'", str(caught.exception)) def test_register_apptype_null_sentinel_still_means_absent_with_a_class_module(self) -> None: - """GitHub issue #833's own repro, re-run against the new sentinel. + """GitHub issue #833's own repro, run against the :data:`NULL` sentinel. With a non-``str`` ``module``, the third positional is never ``class_`` -- it is the first ``*transport`` element instead, per the maintainer ruling in #815. :data:`NULL` passed there must still mean "nothing was named" (falling through to "no transport protocol given"), while the *string* ``'(null)'`` must be treated as an - ordinary, unrecognised transport name -- proving the two no longer - collide now that :data:`NULL` is not a :class:`str`. + ordinary, unrecognised transport name -- proving the two do not + collide, since :data:`NULL` is not a :class:`str`. """ from pcapkit.foundation.registry import protocols as registry diff --git a/tests/foundation/test_extraction.py b/tests/foundation/test_extraction.py index 4ddcd34035..c66dc890ab 100644 --- a/tests/foundation/test_extraction.py +++ b/tests/foundation/test_extraction.py @@ -183,8 +183,8 @@ def test_declared_engine_type_is_satisfied_by_the_built_in_engines(self) -> None Both built-in engines subclass :class:`~pcapkit.foundation.engines.engine.EngineBase` directly (so that they are not auto-registered), hence neither is an - :class:`~pcapkit.foundation.engines.engine.Engine`. The three declarations - used to say ``Engine`` and two ``cast`` calls hid the gap (#1022). + :class:`~pcapkit.foundation.engines.engine.Engine`. Guards against the + three declarations saying ``Engine``, a gap ``cast`` calls can hide (#1022). """ import pcapkit.foundation.extraction as module from pcapkit.foundation.engines.pcap import PCAP @@ -471,7 +471,7 @@ def test_register_engine_identity_guard(self) -> None: warned about an overwrite that never happened. This mirrors the identity guard GitHub issue #718 gave the code-keyed protocol registrars (see ``test_register_stays_quiet_on_same_object_reregistration`` - in ``tests/protocols/test_protocol_base_unit.py``): the guard now + in ``tests/protocols/test_protocol_base_unit.py``): the guard fires only when the incumbent differs from the replacement, and the registry write happens either way. @@ -704,7 +704,7 @@ def test_import_test_and_make_name_paths(self) -> None: ) # An extension already on ``fout`` is recognised and not appended - # twice -- the comparison is against ``f'.{ext}'`` now that ``ext`` + # twice -- the comparison is against ``f'.{ext}'`` since ``ext`` # is bare, and getting that wrong would give ``out.json.json``. self.assertEqual( Extractor.make_name(str(capture), str(temp / 'kept.json'), 'json')[1], @@ -1271,8 +1271,8 @@ class DictFrameTraceEndToEndTests(unittest.TestCase): The constructor-level cases above prove the format is kept; this proves the adapter's frame record is *sufficient* -- the tracer runs, the dumper is handed - the record and writes it. Before #1507 the adapter built a :obj:`dict`, the - format was swapped for JSON, and before that guard this raised + the record and writes it. Guards against the adapter building a :obj:`dict` + (#1507): unless the format is swapped for JSON, that raises ``AttributeError: 'dict' object has no attribute 'packet'`` from :meth:`pcapkit.dumpkit.pcap.PCAPIO._append_value`. diff --git a/tests/foundation/test_extraction_no_eof.py b/tests/foundation/test_extraction_no_eof.py index 245231273a..d306f9dabc 100644 --- a/tests/foundation/test_extraction_no_eof.py +++ b/tests/foundation/test_extraction_no_eof.py @@ -14,19 +14,19 @@ flag suppressed the error that *ended* the loop without supplying any other ending. -The flag is not pointless, which is why the fix is a termination condition rather +The flag is not pointless, which is why it gets a termination condition rather than a deletion. ``pcapkit/__main__.py`` sets ``no_eof = args.fin == '-'``, so ``pcapkit -`` reads a live capture off standard input, and reaching the end of -what has arrived so far genuinely is not the end of that capture. What was -missing was a way to tell a capture that has paused from one that is over. +what has arrived so far genuinely is not the end of that capture. What is +needed is a way to tell a capture that has paused from one that is over. -Measured on ``6c3d1b0d9`` before the fix, which is what these tests reproduce: +Measured without the fix, which is what these tests reproduce: * ``extract(in.pcap, nofile=True)`` returns with 6 frames; adding ``no_eof=True`` never returns, and ``faulthandler`` killed it at 10s with the stack in ``pcapkit/protocols/protocol.py`` ``__init__``. * The input's position at end of stream is *constant*: eight consecutive ends of - stream all at 605, of a 605-byte file. That is the signal the fix uses. + stream all at 605, of a 605-byte file. That is the signal the stopping rule uses. * A pipe whose writer is open but idle **blocks** rather than reporting end of stream -- frame 2 arrived at t=1.50s across a 1.5s pause -- so retrying was never what kept a live capture on a pipe alive. @@ -38,19 +38,18 @@ A **seekable** input does not block, so the two end-of-stream probes happen microseconds apart and a file still being appended to ends at whatever was -present when the extraction got there. Measured both ways, with the append -landing on a record boundary 0.6s in: ``6c3d1b0d9`` yields all six frames, and -this yields five. That is a real behaviour change, and it is deliberate -- the -previous behaviour was unbounded by construction, which is the defect #620 -reports, so some stopping rule had to be chosen. A timed grace period would only +present when the extraction got there. Measured with the append landing on a +record boundary 0.6s in: unbounded retrying yields all six frames, and the +stopping rule yields five. That is deliberate -- unbounded retrying is the defect +#620 reports, so some stopping rule had to be chosen. A timed grace period would only make the cut-off intermittent instead of absent, so following a growing file is left to a policy of its own. :class:`NoEOFOverAGrowingFileStopsAtWhatIsThere` pins the decision so that it stays a decision rather than becoming folklore. Worth knowing while reading that: an append landing *mid-record* fails with -``ValueError: read length must be non-negative or -1`` on **both** trees, so -``no_eof`` over a growing file only ever worked for a writer flushing whole -records. That is pre-existing and not addressed here. +``ValueError: read length must be non-negative or -1`` with or without the +stopping rule, so ``no_eof`` over a growing file only works for a writer flushing +whole records. That is a separate matter, not addressed here. Why the end-to-end cases run in a child process ----------------------------------------------- @@ -431,8 +430,8 @@ def test_call_form_with_no_eof_raises_once_the_input_is_finished(self) -> None: matches the plain form, which :file:`tests/integration/test_frame_iteration.py` already pins. - This is the case that proved an in-process deadline cannot bound: on - ``6c3d1b0d9`` it absorbed the ``SIGALRM`` into the parse path's + This is the case that proved an in-process deadline cannot bound: + without the fix it absorbed the ``SIGALRM`` into the parse path's ``except Exception`` and then ran unbounded. """ @@ -500,17 +499,18 @@ def readinto(self, buffer): class NoEOFOverAGrowingFileStopsAtWhatIsThere(ChildBoundedTestCase): """The deliberate narrowing: a seekable file being appended to is not followed. - This is the one case where the fix is a behaviour *change* rather than a - repair, so it is pinned rather than left to be discovered. A pipe is safe + This is the one case where the stopping rule narrows behaviour rather than + repairing it, so it is pinned rather than left to be discovered. A pipe is safe because its read blocks across a pause; a regular file reports end of stream at once, so the two probes fall microseconds apart and the extraction ends at the data present when it arrived. - Measured with the append landing on a record boundary 0.6s in: ``6c3d1b0d9`` - yields all six frames, this yields five. Following a writer needs a policy -- - a grace period, a poll interval, an explicit follow mode -- and a timed grace - would make the cut-off intermittent rather than absent, which is harder to - reason about than a crisp rule. So the rule is crisp and written down here. + Measured with the append landing on a record boundary 0.6s in: unbounded + retrying yields all six frames, the stopping rule five. Following a writer + needs a policy -- a grace period, a poll interval, an explicit follow mode -- + and a timed grace would make the cut-off intermittent rather than absent, + which is harder to reason about than a crisp rule. So the rule is crisp and + written down here. The case below sequences the append on the extractor's own end of stream, not on that 0.6s timer: a loaded runner took longer than 0.6s to get there, read @@ -534,7 +534,7 @@ def test_a_file_appended_to_after_the_read_yields_only_what_was_there(self) -> N a capture that has merely paused, so a frame appended there is read, and should be -- :class:`NoEOFStillRetriesAnInputThatGrows`. - The child is bounded like the others: on ``6c3d1b0d9`` the retry never + The child is bounded like the others: without the fix the retry never stops, so it reads the appended frame and then does not terminate, and the case doubles as another reproduction. diff --git a/tests/foundation/test_extraction_non_seekable_stream_1586_unit.py b/tests/foundation/test_extraction_non_seekable_stream_1586_unit.py index 41176f46c5..7c851e04ad 100644 --- a/tests/foundation/test_extraction_non_seekable_stream_1586_unit.py +++ b/tests/foundation/test_extraction_non_seekable_stream_1586_unit.py @@ -271,11 +271,11 @@ def test_no_eof_over_a_pipe_whose_writer_closed(self) -> None: def test_a_live_pipe_hands_over_each_frame_as_it_arrives(self) -> None: """A frame does not wait for the stream to go on past it. - ``pcapkit -`` reads a live capture. Measuring a record by reading a buffer's - worth past it held each frame back until that much more had arrived; and - before the fix, the second measurement of each record waited for at least one - more octet. The writer here sends one record, then waits for the reader to - have it before sending the rest. + ``pcapkit -`` reads a live capture. Guards against two defects: measuring a + record by reading a buffer's worth past it holds each frame back until that + much more has arrived, and a second measurement of each record waits for at + least one more octet. The writer here sends one record, then waits for the + reader to have it before sending the rest. """ for name, data, first in ( diff --git a/tests/foundation/test_extraction_ownership.py b/tests/foundation/test_extraction_ownership.py index 9614ecae0a..e5d2a57e78 100644 --- a/tests/foundation/test_extraction_ownership.py +++ b/tests/foundation/test_extraction_ownership.py @@ -39,8 +39,9 @@ than the Python wrapper around it, which is the thing that actually runs out. It is Linux-only and skipped elsewhere. -Both were measured against ``6c3d1b0d9`` before the fix: path-given leaked -exactly 1, stream-given left the caller's handle closed and unreadable. +Both guard against the defects of #610: a path-given extraction leaking +exactly one handle, and a stream-given one leaving the caller's handle closed +and unreadable. """ @@ -167,7 +168,7 @@ def test_path_given_extraction_leaks_no_handle(self) -> None: """Nothing is left open on the capture once ``extract`` has returned. Measured two ways, so a future change that defeats one still fails on the - other. Before #610 both reported exactly one handle left open. + other. Without the fix (#610), both report exactly one handle left open. """ import pcapkit @@ -248,8 +249,8 @@ def test_cleanup_is_safe_to_reach_twice(self) -> None: """``_cleanup`` runs twice for one extraction, and closing twice is fine. Its own docstring records the two routes -- the EOF path and - :meth:`Extractor.run` -- so now that the path-given case actually closes - the handle, the second call has to tolerate an already-closed one. + :meth:`Extractor.run` -- and since the path-given case closes the + handle, the second call has to tolerate an already-closed one. """ import pcapkit @@ -271,8 +272,8 @@ def test_caller_supplied_stream_stays_open_and_readable(self) -> None: ``closed`` alone would be a thin assertion, so the read is done as well: the failure this guards against is a caller losing access to a file it - still holds, and that is what actually demonstrates it. Before #610 the - handle came back closed and the read raised + still holds, and that is what actually demonstrates it. Without the fix + (#610), the handle comes back closed and the read raises ``ValueError: seek of closed file``. """ diff --git a/tests/foundation/test_extraction_unnamed_stream_unit.py b/tests/foundation/test_extraction_unnamed_stream_unit.py index 52d38abf7b..9583927b73 100644 --- a/tests/foundation/test_extraction_unnamed_stream_unit.py +++ b/tests/foundation/test_extraction_unnamed_stream_unit.py @@ -10,7 +10,7 @@ * :meth:`Extractor.make_name ` read ``fin.name``, which a :class:`io.BytesIO` does not have. The name only labels the input -- the output name comes from ``fout`` alone -- so a stream - without one is now labelled after its type, ````. + without one is labelled after its type, ````. * The magic number, and the PCAP-NG engine's block type, are read with ``peek``, which :class:`io.BufferedReader` has and :class:`typing.IO` does not. diff --git a/tests/foundation/test_pcapng_unique_block_names_unit.py b/tests/foundation/test_pcapng_unique_block_names_unit.py index e8f60318a0..27a67ffdd3 100644 --- a/tests/foundation/test_pcapng_unique_block_names_unit.py +++ b/tests/foundation/test_pcapng_unique_block_names_unit.py @@ -1,11 +1,11 @@ # -*- coding: utf-8 -*- """Regression tests for #1256: PCAP-NG block names are unique across sections. -The PCAP-NG engine used to number a non-packet block by the length of its -section's context list, which restarts with every Section Header Block. The -first Interface Description Block of the second section was therefore named -``Interface Description 1`` again, so a mapping dumper (JSON, plist) kept only -one of the two blocks and ``files=True`` overwrote the first block's file. +Guards against the PCAP-NG engine numbering a non-packet block by the length +of its section's context list, which restarts with every Section Header Block. +That names the first Interface Description Block of the second section +``Interface Description 1`` again, so a mapping dumper (JSON, plist) keeps only +one of the two blocks and ``files=True`` overwrites the first block's file. """ from __future__ import annotations @@ -83,7 +83,7 @@ def dumped(path: str) -> tuple[list[str], list[str]]: class SingleSectionSampleTests(unittest.TestCase): - """A single-section file is named exactly as before the fix.""" + """A single-section file's block names are unaffected by cross-section numbering.""" def test_single_section_names_are_unchanged(self) -> None: keys, files = dumped(sample_path('dhcp.pcapng')) diff --git a/tests/foundation/traceflow/test_traceflow_base.py b/tests/foundation/traceflow/test_traceflow_base.py index 05c03cea7b..f6be87a681 100644 --- a/tests/foundation/traceflow/test_traceflow_base.py +++ b/tests/foundation/traceflow/test_traceflow_base.py @@ -229,9 +229,9 @@ def submit(self) -> tuple[DummyIndex, ...]: def test_traceflow_subclass_rejects_unrecognised_keyword(self) -> None: """A misspelled class keyword raises instead of being swallowed. - It used to land in ``**kwargs``, get dropped by the bare - ``super().__init_subclass__()``, and leave the class registered under its - own class name -- no exception, no warning. + Guards against it landing in ``**kwargs``, being dropped by a bare + ``super().__init_subclass__()``, and leaving the class registered under + its own class name -- no exception, no warning. The keyword used here is deliberately *not* ``name``; see the sibling test in ``tests/foundation/reassembly/test_reassembly_base.py`` for why diff --git a/tests/foundation/traceflow/test_traceflow_label_unique_unit.py b/tests/foundation/traceflow/test_traceflow_label_unique_unit.py index cf09a3c37e..f0a0bce7cc 100644 --- a/tests/foundation/traceflow/test_traceflow_label_unique_unit.py +++ b/tests/foundation/traceflow/test_traceflow_label_unique_unit.py @@ -23,7 +23,7 @@ HAS_RUNTIME = all(importlib.util.find_spec(name) is not None for name in ('tbtrim', 'aenum', 'chardet', 'dictdumper')) -#: The label both colliding flows used to share. +#: The label both colliding flows share when labels are not made unique. LABEL = '192.0.2.1_12345-198.51.100.2_443-300.0' diff --git a/tests/foundation/traceflow/test_traceflow_snapped_capture_runtime.py b/tests/foundation/traceflow/test_traceflow_snapped_capture_runtime.py index aff57b947d..379d57e66f 100644 --- a/tests/foundation/traceflow/test_traceflow_snapped_capture_runtime.py +++ b/tests/foundation/traceflow/test_traceflow_snapped_capture_runtime.py @@ -4,11 +4,11 @@ Every frame of :file:`tcp.pcap` snapped by 4 octets, with its original length kept, cuts four of its seven TCP headers short: frame 1 inside its options and frames 2, 4 and 7 inside a Timestamps option. The TCP parser rejects those -headers by design (#1404), and the default engine's adapters used to skip the -frames, tracing ``(3, 5) (6,)``. :mod:`dpkt`, :mod:`scapy` and Wireshark's -``tcp.stream`` all trace ``(1, 2, 6, 7) (3, 4, 5)``, the flows of the full -capture, and so does the default engine now that its adapters read the fixed -20-octet header of a rejected segment. +headers by design (#1404), and adapters that skip those frames trace +``(3, 5) (6,)``. :mod:`dpkt`, :mod:`scapy` and Wireshark's ``tcp.stream`` all +trace ``(1, 2, 6, 7) (3, 4, 5)``, the flows of the full capture, and so does the +default engine, whose adapters read the fixed 20-octet header of a rejected +segment. The snapped capture is built in memory, as PCAP and as PCAP-NG. The module reads the generated :file:`tcp.pcap`, so it belongs to the fixture-dependent diff --git a/tests/integration/_helpers.py b/tests/integration/_helpers.py index a847d4ab2d..5262d807aa 100644 --- a/tests/integration/_helpers.py +++ b/tests/integration/_helpers.py @@ -155,12 +155,10 @@ def report_stems(directory: 'str') -> 'list[str]': Split at the *first* dot, which keeps the callers' assertions about *which* sections a report holds independent of what the extension happens to be. - This used to be neutrality about the number of dots as well: per-frame - reports were named ``f'{name}.{ext._fext}'`` while ``_fext`` still carried - its own leading dot, so they landed on disk as ``Frame 1..json`` (#358). - That is fixed -- the extension is bare now, and - :mod:`tests.integration.test_files_output_naming` asserts the real names -- - so nothing here is working around it any more. + The number of dots before the extension is not this helper's concern: + :mod:`tests.integration.test_files_output_naming` asserts the real names, + which guards against per-frame reports landing on disk as + ``Frame 1..json`` (#358). """ return sorted(entry.name.split('.', 1)[0] for entry in pathlib.Path(directory).iterdir()) diff --git a/tests/integration/test_engine_parity.py b/tests/integration/test_engine_parity.py index f5c522cfba..ef0dd1b943 100644 --- a/tests/integration/test_engine_parity.py +++ b/tests/integration/test_engine_parity.py @@ -115,11 +115,10 @@ def test_scapy_engine_returns_every_link_layer_octet(self) -> None: # Ethernet header as captured: the engine accounts for every # captured octet, whatever it calls the layers it splits it into. # - # #406: this comment used to say scapy "does not know this capture's - # link type and hands back one opaque ``Raw`` layer". That was the - # missing-layer-registry bug, not a property of the capture -- - # arp.pcap is link type 1 and scapy now dissects it as - # ``Ethernet / ARP / Padding``. The assertions below still pass + # #406: arp.pcap is link type 1 and scapy dissects it as + # ``Ethernet / ARP / Padding``. One opaque ``Raw`` layer would be + # the missing-layer-registry bug, not a property of the capture. + # The assertions below pass # either way, because ``bytes()`` of a dissected frame is the same # octet string as ``bytes()`` of an undissected one. captured = bytes(frame) diff --git a/tests/integration/test_engine_runtime.py b/tests/integration/test_engine_runtime.py index e0ae06bee1..a79a0d5cda 100644 --- a/tests/integration/test_engine_runtime.py +++ b/tests/integration/test_engine_runtime.py @@ -74,13 +74,12 @@ def test_scapy_engine_returns_scapy_packets(self) -> None: def test_pyshark_engine_is_refused_before_asyncio_can_break(self) -> None: """On 3.14 the engine is declined by the preflight, not by pyshark. - This test used to assert the raw :exc:`AttributeError` from inside - ``pyshark``, which was the honest description of the behaviour until :meth:`PyShark.unsupported_reason - ` began - answering the question first. The preflight now names the interpreter as - the cause and falls back, so the exception never happens -- and the - extraction succeeds with ``pcapkit``'s own parser rather than failing. + ` answers + the question first: it names the interpreter as the cause and falls back, + so the raw :exc:`AttributeError` from inside ``pyshark`` never happens -- + and the extraction succeeds with ``pcapkit``'s own parser rather than + failing. """ from pcapkit.interface import extract diff --git a/tests/integration/test_reassembly_end_to_end.py b/tests/integration/test_reassembly_end_to_end.py index 657d4d0d23..e90d620e54 100644 --- a/tests/integration/test_reassembly_end_to_end.py +++ b/tests/integration/test_reassembly_end_to_end.py @@ -277,8 +277,8 @@ class IPv6FragmentPayloadTests(EndToEndTestCase): Kept apart from :class:`IPv6FragmentReassemblyTests` because these are the assertions on the datagram's octets, which are what the two defects recorded - below used to get wrong while the frame collection and the ``completed`` flag - stayed right. + below get wrong while the frame collection and the ``completed`` flag stay + right. """ diff --git a/tests/integration/test_reassembly_engine_parity.py b/tests/integration/test_reassembly_engine_parity.py index dee8901d75..e4c6adb0c4 100644 --- a/tests/integration/test_reassembly_engine_parity.py +++ b/tests/integration/test_reassembly_engine_parity.py @@ -248,9 +248,9 @@ class IPv6DatagramHeaderTests(EndToEndTestCase): :rfc:`8200#section-4.5` -- "The Fragment header is not present in the reassembled packet", and "the Next Header field of the last header of the Unfragmentable Part is obtained from the Next Header field of the first - fragment's Fragment header". Every engine used to leave the field alone, so - the reassembled datagram advertised a Fragment header on a datagram that is - by definition no longer a fragment. + fragment's Fragment header". Guards against an engine leaving the field + alone, so that the reassembled datagram advertises a Fragment header on a + datagram that is by definition no longer a fragment. """ diff --git a/tests/test_docstring_contract.py b/tests/test_docstring_contract.py index 61a43e6e38..b46a731184 100644 --- a/tests/test_docstring_contract.py +++ b/tests/test_docstring_contract.py @@ -59,9 +59,9 @@ construction rather than by being listed. :meth:`Raw.__post_init__ ` shows -how sharp that trap is, and it caught a reviewing scanner during this change. -It documents ``error`` and ``alias``, has neither in its signature, and its body -assigns a *local* ``alias`` from ``self._info.protocol.name`` -- which reads +how sharp that trap is. It documents ``error`` and ``alias``, has neither in +its signature, and its body assigns a *local* ``alias`` from +``self._info.protocol.name`` -- which reads exactly like a docstring naming a parameter that does not exist. It is not: ``__post_init__`` forwards ``**kwargs`` to :meth:`~pcapkit.protocols.misc.raw.Raw.unpack`, which dispatches to :meth:`Raw.read `, and @@ -96,23 +96,13 @@ owning change lands, that test fails and the entry has to be removed rather than sitting there forever describing a bug that is gone. -That is not a hypothetical. The list was written against ``980e52f0c`` and had -an entry for ``pcapkit/vendor/ipx/socket.py``'s ``process``, which documented -``data`` where the parameter was ``soup``. Rebasing this change onto -``c8fd97bcd`` picked up #511, which retired the HTML scrape and renamed that -parameter to ``data`` as a side effect -- so the defect was gone, and this test -failed on exactly that subtest and nothing else. The entry was removed. - -It then happened a second time, to the sibling ``pcapkit/vendor/ipx/packet.py``, -and that entry's removal is in this change's diff. ``a8912b46f`` (#524) renamed -that ``process``'s parameter to ``data`` so its docstring became correct, and -``48c6f07f8`` (#535) added this list four commits later without re-running -against the newer base -- so ``main`` was red on that subtest from the moment -the guard itself landed. The guard was right both times; what failed was -re-running it after a rebase. Worth knowing that the failure is reported through -:meth:`~unittest.TestCase.subTest`, and ``pytest-subtests`` is not a dependency -here, so pytest prints the parent test as ``PASSED`` while exiting non-zero: -read the exit code, not the summary line. +That is not a hypothetical: #511 and #524 each fixed, as a side effect, a +``pcapkit/vendor/ipx`` ``process`` docstring this list carried as a defect, and +this test failed on exactly that subtest until the entry was removed. So re-run +it after a rebase. Worth knowing that the failure is reported through +:meth:`~unittest.TestCase.subTest`: pytest reports each failing subtest as +``SUBFAILED`` and counts it as failed, while the parent test is counted as +passed. Everything here reads :file:`pcapkit/` source and imports nothing but the standard library, so it reads no capture under :file:`examples/captures/` and diff --git a/tests/test_final_enforcement.py b/tests/test_final_enforcement.py index ff48fb06fb..91e6d4fe13 100644 --- a/tests/test_final_enforcement.py +++ b/tests/test_final_enforcement.py @@ -334,7 +334,7 @@ def test_a_descendant_that_predates_the_marker_may_still_be_finalised(self) -> N still needs. A ``getattr``-keyed check would do exactly that. **This is the test that matters most, because its failure mode is - silent.** Now that re-finalising only warns, a ``getattr`` here would + silent.** Since re-finalising only warns, a ``getattr`` here would not raise -- it would warn and hand back a class with no generated ``__init__``, whose construction then falls through to :meth:`Info.__update__` and takes a mapping instead of the declared @@ -443,7 +443,7 @@ def test_a_final_class_descending_from_a_base_state_ancestor_is_refused(self) -> """``__finalised__`` is read from each class's own ``__dict__`` (#1490). A class marked ``@final`` under a :attr:`~FinalisedState.BASE` ancestor - no longer reads ``BASE`` off it, so it reaches the branch that reads its + does not read ``BASE`` off it, so it reaches the branch that reads its own marker and is refused like any other mismarked class. """ @@ -746,7 +746,8 @@ def test_a_bare_final_info_class_is_refused_at_first_construction(self) -> None: applied to the class *object*, after creation, so that hook has already run and returned by the time the marker lands. First construction is the next moment this library is given control, and it is where the damage - used to surface -- as ``TypeError: 'int' object is not iterable`` out of + surfaces without the guard -- as + ``TypeError: 'int' object is not iterable`` out of :meth:`Info.__update__`, which names neither the class nor the mistake. """ @@ -767,7 +768,7 @@ class Mismarked(Info): self.assertIn('info_final', message) def test_a_bare_final_schema_is_refused_at_first_construction(self) -> None: - """The schema-side half, which used to return corrupt data instead.""" + """The schema-side half, which returns corrupt data without the guard.""" from pcapkit.protocols.schema.schema import Schema from pcapkit.utilities.compat import final from pcapkit.utilities.exceptions import SchemaError @@ -894,7 +895,7 @@ class MismarkedAfterBareSchema(Schema): def test_repeated_bare_info_construction_does_not_grow_excluded_without_bound(self) -> None: """The cost of the ``cls is not Info`` fix, pinned rather than assumed. - ``Info`` never reaches :attr:`~FinalisedState.BASE` now, so a bare + ``Info`` never reaches :attr:`~FinalisedState.BASE`, so a bare ``Info()`` would re-enter :func:`info_final` on *every* call rather than once, and ``cls.__excluded__.extend(cls.__builtin__)`` would append the same ~60 names again each time -- correctness untouched (``__excluded__`` diff --git a/tests/test_support_helpers.py b/tests/test_support_helpers.py index eb00ccfaef..ea3fa20b2e 100644 --- a/tests/test_support_helpers.py +++ b/tests/test_support_helpers.py @@ -749,8 +749,8 @@ class StandInsDoNotOutliveTheirTestTests(unittest.TestCase): way and the assertion would pass whether or not the test case had been fixed. - Before the fix this failed on any ordering, on the fake ``ProtocolBase`` and - the stub ``pcapkit`` that ``ProtoChainTests.setUp`` left behind. + Without the fix this fails on any ordering, on the fake ``ProtocolBase`` and + the stub ``pcapkit`` that ``ProtoChainTests.setUp`` leaves behind. """ @@ -833,7 +833,7 @@ class LoadedModulesDoNotOutliveTheirTestTests(unittest.TestCase): different helper. :func:`~tests._support.load_module` writes to :data:`sys.modules` twice over -- the module it was asked for, and a bare stub package for each parent of that module's dotted name, via - :func:`~tests._support.ensure_package` -- and before the fix neither write was + :func:`~tests._support.ensure_package` -- and without the fix neither write is taken back off again. What that cost: ``pcapkit``, ``pcapkit.corekit`` and ``pcapkit.utilities`` @@ -848,15 +848,15 @@ class LoadedModulesDoNotOutliveTheirTestTests(unittest.TestCase): reason the sibling class gives and with more force here: :func:`tests.conftest.restore_module_table` repairs the region after every test, so under :mod:`pytest` these assertions pass whether or not the helper - was ever fixed. That guard arrived with #662, it is why the two-file - :mod:`pytest` repro quoted in #674 no longer fails, and it is not a substitute + was ever fixed. That guard (#662) is why the two-file :mod:`pytest` repro + quoted in #674 passes under :mod:`pytest`, and it is not a substitute for the helper putting its own writes back -- the stdlib runner reads no ``conftest.py`` at all, and a suite-wide net that hides a helper's bug also hides the next one. The polluting cases are *borrowed* rather than written here, so what is pinned is the behaviour of call sites the suite actually has. Every one of them - leaked before the fix, and none of them looked wrong. + leaks without the fix, and none of them looks wrong. """ diff --git a/tests/test_tier_guard.py b/tests/test_tier_guard.py index c0ebd6c2a3..c783388152 100644 --- a/tests/test_tier_guard.py +++ b/tests/test_tier_guard.py @@ -1204,12 +1204,11 @@ def test_calls_and_findings_come_out_in_source_order(self) -> None: alone preserves that -- a stable sort leaves a tie in walk order. Only ``col_offset`` puts the pair back. - A tuple of two calls, which is what this module used to hold here, pins - neither half: :class:`ast.Tuple` keeps its elements in one field list, so - the walk yields them left to right already and the test passed with - ``col_offset`` dropped. :class:`ast.Dict` is the other shape that gets a - single line wrong, since all of its ``keys`` are walked before any of its - ``values``. + A tuple of two calls would pin neither half: :class:`ast.Tuple` keeps + its elements in one field list, so the walk yields them left to right + already and the test passes with ``col_offset`` dropped. + :class:`ast.Dict` is the other shape that gets a single line wrong, + since all of its ``keys`` are walked before any of its ``values``. """ module = write_module(self.tmp_path, 'test_order_unit.py', """ @@ -1538,11 +1537,11 @@ def test_a_bracket_inside_a_requirement_does_not_end_the_array(self) -> None: def test_all_is_narrowed_to_core_addons_by_910(self) -> None: """#910: ``all`` means core addons only, not every engine an end user might want. - The owner's ruling narrowed ``all`` to the core addons -- ``cli``, - ``crypto`` and ``pycrate`` (``NGAP``) stay, and the third-party - capture engines that used to be bundled here (``dpkt``, ``scapy``, - ``pyshark``, ``pypcapfile``) move out to their own on-demand extras, - covered by :meth:`test_dev_carries_what_all_used_to_for_the_toolchain`. + The owner's ruling limits ``all`` to the core addons -- ``cli``, + ``crypto`` and ``pycrate`` (``NGAP``) -- and the third-party capture + engines (``dpkt``, ``scapy``, ``pyshark``, ``pypcapfile``) have their + own on-demand extras instead, covered by + :meth:`test_dev_carries_what_all_used_to_for_the_toolchain`. """ declared = _dependency_gates.declared_requirements() @@ -1596,7 +1595,7 @@ def test_the_core_dependencies_are_read_from_project_not_build_system(self) -> N self.assertNotIn('setuptools', names) def test_every_extra_any_pytest_job_installs_is_declared(self) -> None: - """An install line naming an extra that no longer exists resolves to nothing.""" + """An install line naming an extra that does not exist resolves to nothing.""" if not _dependency_gates.WORKFLOW.is_file(): self.skipTest(f'{_dependency_gates.WORKFLOW} is not present') @@ -2175,20 +2174,15 @@ def test_the_mypy_gate_is_visible_and_dark_on_every_job_that_reaches_it(self) -> """#779: a lint-tier tool gated visibly, and declining the install line anyway. ``mypy`` is a :file:`Pipfile` ``[dev-packages]`` entry and is in no - :file:`pyproject.toml` extra, so before #779 it had no - :data:`~tests._dependency_gates.MODULE_PROVIDERS` entry at all and an - ``@unittest.skipUnless`` gate on it was not merely undesirable but - *impossible*: adding one to + :file:`pyproject.toml` extra, so without a + :data:`~tests._dependency_gates.MODULE_PROVIDERS` entry an + ``@unittest.skipUnless`` gate on it is not merely undesirable but + *impossible* (#779): adding one to :file:`tests/vendor/test_vendor_reg_apptype_generator_unit.py` and - nothing else made this class report 2 failures and 5 errors of its 10 - tests, every one of those errors a bare ``KeyError: 'mypy'`` out of + nothing else makes this class fail and error, every error a bare + ``KeyError: 'mypy'`` out of :func:`~tests._dependency_gates.extras_providing`, upstream of any :data:`~tests._dependency_gates.DEPENDENCY_GATE_EXCLUSIONS` filtering. - (#779 reports 4 errors of 9 tests for the same mutation; it was written - before #774 added - :meth:`test_the_vendor_extra_closes_every_job_reaching_the_vendor_gates`, - which is the tenth and the fifth error. Re-measured here rather than - copied.) The entry is what makes the gate expressible; the exclusion is what records that *closing* the gap would be the wrong fix, since a type checker belongs to :file:`.github/workflows/lint.yml` rather than to a @@ -2216,11 +2210,11 @@ def test_the_mypy_gate_is_visible_and_dark_on_every_job_that_reaches_it(self) -> self.assertIn(('HAS_MYPY', job), gaps) # integration and pypcap-parity select by fixture tier and never # collect tests/vendor/ at all -- the same reason they are absent from - # HAS_VENDOR_DEPS above. engine-tests (#849) no longer reaches it - # either: each matrix.engine cell now runs an explicit TEST_PATHS list + # HAS_VENDOR_DEPS above. engine-tests (#849) does not reach it + # either: each matrix.engine cell runs an explicit TEST_PATHS list # and none of those lists names tests/vendor/ -- see the HAS_MYPY - # exclusion's own reason for why that is a real, disclosed change - # rather than a mistake here. + # exclusion's own reason for why that is real and disclosed rather + # than a mistake here. for job in ('integration', 'pypcap-parity', 'engine-tests'): with self.subTest(does_not_reach=job): self.assertNotIn(('HAS_MYPY', job), gaps) @@ -2356,7 +2350,7 @@ def test_every_gated_flag_is_classified(self) -> None: module the same deliberate contract a required one already has, instead of an accident of whichever caller reaches it first. - Both messages below still earn their place now that + Both messages below earn their place even though :func:`~tests._dependency_gates._top_level_providers` names the module and the fix on its own (#779): what that cannot say is *which gate* wanted the module, and the location is most of what makes the failure @@ -2563,7 +2557,7 @@ def test_dropping_isort_from_the_test_extra_is_caught(self) -> None: gaps = {(gap.flag, gap.job): gap for gap in _dependency_gates.dependency_gate_gaps()} - # engine-tests (#849) no longer reaches tests/project/ at all -- see + # engine-tests (#849) does not reach tests/project/ at all -- see # test_the_isort_gate_is_visible_and_closed_by_the_test_extra above. # #1954: the module is a project pin, which project-tests runs instead of test. self.assertEqual( @@ -2587,23 +2581,17 @@ def test_dropping_isort_from_the_test_extra_is_caught(self) -> None: def test_removing_dpkt_is_caught_on_every_job_that_installs_it(self) -> None: """#729's defect, restaged. Every job installing ``DPKT`` via a comma list goes dark. - #751 added ``engine-tests`` and ``pypcap-parity`` to the jobs this test - used to name, each carrying its own ``DPKT`` for the same reason as the - original ones -- ``engine-tests`` for - :file:`tests/foundation/engines/test_runtime_engines.py`'s reused - ``HAS_RUNTIME``, ``pypcap-parity`` for + ``pypcap-parity`` (#751) is among them, carrying its own ``DPKT`` for :file:`examples/generators/make_samples.py`. - #849's rebuild moves engine-tests off the shared comma-joined extras - list this test doctors: DPKT is now chosen per matrix.engine cell by a - ``case "$ENGINE" in DPKT) EXTRA=DPKT; ... esac`` block, so the literal - ``'DPKT,'``/``',DPKT'`` substitution below no longer touches it at all - -- and, separately, engine-tests no longer reaches - test_runtime_engines.py regardless (see the HAS_RUNTIME exclusion), - so there is no longer a DPKT-via-engine-tests gap for this doctoring - to reopen, and it is not among the jobs checked here. (#1538 later - dropped that DPKT cell outright: it ran only test_dpkt_unit.py, which - ``test`` runs too.) + engine-tests is not: it chooses its extra per matrix.engine cell (#849) + rather than from the shared comma-joined extras list this test doctors, + so the literal ``'DPKT,'``/``',DPKT'`` substitution below does not touch + it -- and, separately, engine-tests does not reach + test_runtime_engines.py regardless (see the HAS_RUNTIME exclusion), so + there is no DPKT-via-engine-tests gap for this doctoring to reopen. Nor + has it a DPKT cell (#1538): that cell ran only test_dpkt_unit.py, which + ``test`` runs too. """ text = _dependency_gates.WORKFLOW.read_text(encoding='utf-8') @@ -2812,8 +2800,8 @@ def test_a_third_contested_name_going_undeclared_is_caught(self) -> None: def test_a_non_distribution_flag_is_not_reported_even_when_doctored_ambiguous(self) -> None: """:data:`~tests._dependency_gates.NON_DISTRIBUTION_FLAGS`, exercised for real. - ``HAS_PYSHARK`` (used to stand in for this mechanism elsewhere) never - reaches :func:`~tests._dependency_gates.ambiguous_satisfactions`'s + ``HAS_PYSHARK`` never reaches + :func:`~tests._dependency_gates.ambiguous_satisfactions`'s ``MUTUALLY_EXCLUSIVE_IMPORTS`` branch at all -- ``pyshark`` has one provider -- so naming it in ``NON_DISTRIBUTION_FLAGS`` would still report nothing whether or not this function's own skip line ran, @@ -2841,18 +2829,19 @@ def test_a_non_distribution_flag_is_not_reported_even_when_doctored_ambiguous(se self.assertNotIn(('pypcap-parity', 'HAS_PYPCAP'), findings) def test_deleting_an_engine_from_the_matrix_list_is_caught(self) -> None: - """#849's second cross-review, verbatim: ``_engine_matrix_variants`` never read ``engine:``. - - Before this test's fix, the function built its :class:`Job` set purely from the - ``case "$ENGINE" in ...`` arms and never checked them against the ``engine:`` - matrix list at all, so deleting ``- PyShark`` -- the ordinary way anyone retires a - cell -- left the case arms (still naming ``PyShark``) as the only source of truth. - Measured against the unfixed function: ``pytest_jobs`` raised nothing, the - returned :class:`Job` set still credited a ``PyShark`` engine-tests cell with - ``test_pyshark_engine.py``'s 3 ``HAS_PYSHARK`` methods, and + """#849's second cross-review: ``_engine_matrix_variants`` reads ``engine:``. + + Guards against the function building its :class:`Job` set purely from the + ``case "$ENGINE" in ...`` arms without checking them against the ``engine:`` + matrix list, so that deleting ``- PyShark`` -- the ordinary way anyone + retires a cell -- leaves the case arms (still naming ``PyShark``) as the + only source of truth. + Measured without the fix: ``pytest_jobs`` raised nothing, the returned + :class:`Job` set still credited a ``PyShark`` engine-tests cell with + ``test_pyshark_engine.py``'s ``HAS_PYSHARK`` methods, and ``dependency_gate_gaps`` reported no ``('HAS_PYSHARK', 'engine-tests')`` gap -- - green, while the retired cell no longer runs in CI at all. This is the hard - acceptance criterion: this exact doctoring must raise :class:`AssertionError` now. + green, while the retired cell did not run in CI at all. This is the hard + acceptance criterion: this exact doctoring must raise :class:`AssertionError`. """ doctored = doctored_workflow(self, ' - PyShark\n', '') @@ -2865,12 +2854,11 @@ def test_deleting_an_engine_from_the_matrix_list_is_caught(self) -> None: self.assertIn('PyShark', message) def test_a_second_baseline_install_line_is_caught(self) -> None: - """The pre-existing ``base_installs`` branch, exercised for the first time. + """The ``base_installs`` branch of ``_engine_matrix_variants``, exercised. - Grepping this file for ``_engine_matrix_variants`` before this change returned - nothing at all -- none of this function's ``raise AssertionError`` branches had a - test, this guard's own docstring notwithstanding ("the only honest way to show a - guard works is to break the thing it guards and watch it fire"). Doctoring in a + Each of that function's ``raise AssertionError`` branches needs a test, as this + guard's own docstring says ("the only honest way to show a guard works is to + break the thing it guards and watch it fire"). Doctoring in a second literal ``pip install -e '.[...]'`` baseline line makes the count the function checks for (exactly one) come out to two. @@ -2889,7 +2877,7 @@ def test_a_second_baseline_install_line_is_caught(self) -> None: self.assertIn('2', message) def test_deleting_every_test_paths_case_arm_is_caught(self) -> None: - """The pre-existing ``engine_paths`` empty branch, exercised for the first time. + """The empty ``engine_paths`` branch of ``_engine_matrix_variants``, exercised. Without a single ``ENGINE) TEST_PATHS="..."`` arm left, this guard has nothing to build a :class:`Job` from at all and must say so rather than returning an empty, @@ -2916,10 +2904,10 @@ def test_deleting_every_test_paths_case_arm_is_caught(self) -> None: self.assertIn('TEST_PATHS', message) def test_a_test_paths_arm_with_no_matching_extra_arm_is_caught(self) -> None: - """The pre-existing per-engine ``extra is None`` branch, exercised for the first time. + """``_engine_matrix_variants``'s per-engine ``extra is None`` branch, exercised. Commenting out one engine's ``EXTRA=`` arm -- rather than renaming it, which the - new matrix-list-vs-case-arms equality check above would catch first, for a + matrix-list-vs-case-arms equality check above would catch first, for a different reason -- leaves Scapy declared in the ``engine:`` list and still present in ``engine_paths`` (so the equality check sees the same engines on both sides and stays quiet), but with no ``EXTRA=`` arm of its own: exactly the @@ -3105,13 +3093,13 @@ def test_a_flag_no_extra_could_satisfy_is_not_reported_as_a_gap(self) -> None: {gap.flag for gap in _dependency_gates.dependency_gate_gaps()}) def test_an_unmapped_module_names_itself_and_the_table_to_add_it_to(self) -> None: - """#779's second defect: the table lookup used to die on a bare ``KeyError``. + """#779's second defect: the table lookup must not die on a bare ``KeyError``. Auditing gates is this module's whole purpose, so the worst available - answer to an unrecognised one was the one it gave: ``KeyError: 'mypy'`` - raised from a dict subscript three call sites deep, naming neither the - fix nor even the table that wanted the entry. All three subscripts now - go through :func:`~tests._dependency_gates._top_level_providers`, so + answer to an unrecognised one is ``KeyError: 'mypy'`` raised from a dict + subscript three call sites deep, naming neither the fix nor even the + table that wanted the entry. Every such subscript goes through + :func:`~tests._dependency_gates._top_level_providers`, so every entry point fails the same way and says the same thing -- :func:`~tests._dependency_gates.extras_providing` (which :func:`~tests._dependency_gates.dependency_gate_gaps` reaches), @@ -3200,16 +3188,16 @@ def test_an_excluded_module_under_a_contested_top_level_needs_its_own_entry(self def test_an_unrelated_flags_unmapped_exclusion_never_reaches_disqualification(self) -> None: """The ordering fix: the contested-scope check has to run *first*. - :func:`~tests._dependency_gates.ambiguous_satisfactions` used to - compute disqualified providers for every gate a job reaches, before - checking whether the gate's own required module was even in + Guards against :func:`~tests._dependency_gates.ambiguous_satisfactions` + computing disqualified providers for every gate a job reaches before + checking whether the gate's own required module is even in :data:`~tests._dependency_gates.MUTUALLY_EXCLUSIVE_IMPORTS`. A flag with a negated probe on some module with no :data:`~tests._dependency_gates.MODULE_PROVIDERS` entry, and nothing - to do with a contested name, would crash the entire function via + to do with a contested name, then crashes the entire function via :func:`~tests._dependency_gates._disqualified_providers` rather than being scoped out of it. This pins a synthetic flag shaped exactly - that way and shows it no longer crashes. + that way and shows it does not crash. """ with unittest.mock.patch.object( diff --git a/tests/test_tier_guard_xdist.py b/tests/test_tier_guard_xdist.py index c6bb3cb9ee..0e30182ee5 100644 --- a/tests/test_tier_guard_xdist.py +++ b/tests/test_tier_guard_xdist.py @@ -3,7 +3,7 @@ :func:`tests.conftest.pytest_collection_modifyitems` raises :exc:`pytest.UsageError` with a detailed diagnostic when a unit-tier module reads a generated capture. Under -``-n auto --dist load`` that message used to be lost entirely: each xdist worker +``-n auto --dist load`` that message can be lost entirely: each xdist worker runs this hook inside its own nested :program:`pytest` session (:mod:`xdist.remote`), and that session's ``pytest_collection_finish`` fires from a ``finally:`` regardless of the ``UsageError``, so the controller sees what looks @@ -13,13 +13,13 @@ ``INTERNALERROR`` instead of the diagnostic and describes the violation as a worker crash rather than as the tier-guard failure it is. -The fix is to set ``session.shouldfail`` before raising, but only inside a worker +So the hook sets ``session.shouldfail`` before raising, but only inside a worker (``hasattr(config, 'workerinput')``): that is the signal the controller already knows how to shut a worker down on cleanly, with no crash-item assertion, and it is a no-op outside a worker so the serial path is untouched. :class:`CollectionModifyItemsXdistTests` pins the hook's own logic directly and -runs unconditionally. :class:`XdistSubprocessTests` reproduces the original bug +runs unconditionally. :class:`XdistSubprocessTests` reproduces the bug end-to-end with a real violation and a real ``pytest -n auto --dist load`` invocation, and skips when :mod:`xdist` is not importable -- the repository venv this suite normally runs under deliberately has no xdist installed. @@ -34,7 +34,7 @@ :class:`XdistSubprocessTests` below takes the exclusive hold in ``setUp`` for as long as the probe sits on disk, and :func:`tests.conftest._tier_guard_reader_lock` takes the shared hold on behalf of every other unit-tier test, including -``SuiteIsCleanTests``, without that module's own source changing at all. +``SuiteIsCleanTests``, from :mod:`tests.conftest` alone. :class:`ProbeWriteLockTests` pins the mutex itself, independently of xdist. A third (#1756): a worker that dies mid-probe never runs its cleanup, and the @@ -47,12 +47,12 @@ only under the lock, and :class:`ProbeLeftoverCheckTests` pins that ordering. A fifth (#1977): GraalPy holds the GIL inside a blocking :func:`fcntl.flock`, so -:class:`ProbeWriteLockTests` deadlocked its own process and every worker queued -behind it. There the lock polls with ``LOCK_NB``, and :class:`ProbeLockNonBlockingTests` -pins that it never makes a blocking call. +a blocking lock deadlocks :class:`ProbeWriteLockTests`'s own process and queues +every worker behind it. On GraalPy the lock polls with ``LOCK_NB``, and +:class:`ProbeLockNonBlockingTests` pins that it never makes a blocking call. -Kept separate from :mod:`tests.test_tier_guard`, which another change is already -modifying: this module touches none of its code, only :mod:`tests.conftest`. +Kept separate from :mod:`tests.test_tier_guard`: this module exercises +:mod:`tests.conftest`, and none of :mod:`tests.test_tier_guard`'s code. """ from __future__ import annotations @@ -143,8 +143,8 @@ def _findings(self, session: '_FakeSession', config: '_FakeConfig') -> 'str': def test_worker_context_sets_shouldfail_before_raising(self) -> None: """Inside a worker, the diagnostic is also stashed on ``session.shouldfail``. - Fails on the code before this change: ``session.shouldfail`` stays at its - initial :data:`False` because nothing ever assigns to it. + Without the fix, ``session.shouldfail`` stays at its initial + :data:`False` because nothing ever assigns to it. """ session = _FakeSession() @@ -155,7 +155,7 @@ def test_worker_context_sets_shouldfail_before_raising(self) -> None: self.assertIn('a synthetic finding', message) def test_serial_context_leaves_shouldfail_untouched(self) -> None: - """Outside a worker, behaviour is exactly what it was before this change.""" + """Outside a worker, ``session.shouldfail`` is left untouched.""" session = _FakeSession() message = self._findings(session, _FakeConfig(is_worker=False)) @@ -190,7 +190,7 @@ class XdistSubprocessTests(unittest.TestCase): Skipped whenever the interpreter running this suite has no ``xdist`` to import -- true of the repository's own venv by design, so this class contributes nothing there and everything under a venv that has xdist - installed, such as the one used to verify this fix. + installed. """ @@ -259,9 +259,9 @@ def test_serial_run_reports_the_diagnostic_and_fails(self) -> None: self.assertNotIn('INTERNALERROR', output) def test_xdist_run_reports_the_diagnostic_and_fails(self) -> None: - """The reproduction this change exists for. + """The end-to-end reproduction of the lost diagnostic. - Fails on the code before this change: the run exits non-zero from an + Without the fix, the run exits non-zero from an ``INTERNALERROR`` (``assert not crashitem`` in ``xdist/dsession.py``, naming the probe's test id) and ``_DIAGNOSTIC_MARKER`` never appears. @@ -391,8 +391,8 @@ class ProbeWriteLockTests(unittest.TestCase): def test_a_concurrent_reader_waits_out_the_writer(self) -> None: """A shared hold cannot be granted while the exclusive one is live. - Fails on the code before this change: :func:`tests.conftest._tier_guard_lock` - does not exist yet, so there is nothing to import and nothing to wait on. + Without the fix, :func:`tests.conftest._tier_guard_lock` does not + exist, so there is nothing to import and nothing to wait on. """ reader_acquired = threading.Event() @@ -640,7 +640,7 @@ def _assert_lock_free(self) -> None: fcntl.flock(handle.fileno(), fcntl.LOCK_UN) def test_a_live_probe_is_waited_for_not_reported_as_a_leftover(self) -> None: - """Fails on the code before this change: ``setUp`` raises "already exists" + """Without the fix, ``setUp`` raises "already exists" at once, without ever asking for the lock the live probe's writer holds. """ @@ -754,7 +754,7 @@ def _held(self, suffix: 'str', operation: 'int') -> 'bool': return False def test_a_nested_run_does_not_queue_behind_a_waiting_writer(self) -> None: - """Fails on the code before #2082: the child times out at the gate.""" + """Without the fix (#2082), the child times out at the gate.""" order = [] # type: list[str] def writer() -> None: diff --git a/tests/toolkit/test_dpkt_ipv4_reassembly_input_1615_1616_unit.py b/tests/toolkit/test_dpkt_ipv4_reassembly_input_1615_1616_unit.py index 65248cd27f..10a08b12c9 100644 --- a/tests/toolkit/test_dpkt_ipv4_reassembly_input_1615_1616_unit.py +++ b/tests/toolkit/test_dpkt_ipv4_reassembly_input_1615_1616_unit.py @@ -2,7 +2,7 @@ """dpkt hands IPv4 reassembly a whole header. C.f. #1616. dpkt cut the input at the Total Length, so one shorter than the IHL cut the -header itself (#1616). It now cuts no shorter than the IHL. A header whose IHL +header itself (#1616). It cuts no shorter than the IHL. A header whose IHL runs past the captured octets, which dpkt still decodes, is no input at all. Every capture is built here, so the module is in the unit tier. diff --git a/tests/toolkit/test_dpkt_reassembly_rawbytes_unit.py b/tests/toolkit/test_dpkt_reassembly_rawbytes_unit.py index 18293e8ade..de6c1b1171 100644 --- a/tests/toolkit/test_dpkt_reassembly_rawbytes_unit.py +++ b/tests/toolkit/test_dpkt_reassembly_rawbytes_unit.py @@ -3,8 +3,8 @@ GitHub issue #1297. :mod:`pcapkit.toolkit.dpkt` built the header and payload of every reassembly and flow-tracing record by calling ``.pack()`` on the DPKT layer. DPKT recomputes a zeroed checksum or length while packing *and writes it -back into the packet*, so the records no longer matched the capture, and the -first adapter to run changed what every later one saw. The adapters now slice the +back into the packet*, so the records did not match the capture, and the +first adapter to run changed what every later one saw. The adapters slice the record octets that :class:`~pcapkit.foundation.engines.dpkt.DPKT` attaches to the frame. @@ -282,7 +282,7 @@ def test_no_adapter_changes_what_the_next_one_sees(self) -> None: self.assertEqual(info['packet'], wire) def test_options_transport_inputs_are_the_captured_octets(self) -> None: - """Before the fix, 33 of this capture's IPv4 datagrams came back rewritten. + """Without the fix, 33 of this capture's IPv4 datagrams come back rewritten. Each record is read by hand, and each input is the IHL's octets, then the rest up to the Total Length. The number of datagrams is counted here diff --git a/tests/toolkit/test_dpkt_tcp_ns_flags_1618_unit.py b/tests/toolkit/test_dpkt_tcp_ns_flags_1618_unit.py index f8e9cbcde3..e4e69eb6ae 100644 --- a/tests/toolkit/test_dpkt_tcp_ns_flags_1618_unit.py +++ b/tests/toolkit/test_dpkt_tcp_ns_flags_1618_unit.py @@ -5,7 +5,7 @@ NS bit (:rfc:`3540`). :func:`~pcapkit.toolkit.dpkt.tcp_reassembly` and :func:`~pcapkit.toolkit.dpkt.tcp_traceflow` read each flag by its position among that field's binary digits, padded to eight, so with NS set every flag was read -one bit low: a SYN as a FIN, an RST as a SYN, a PSH as an RST. They now mask +one bit low: a SYN as a FIN, an RST as a SYN, a PSH as an RST. They mask each flag out of the flags octet. Every capture is built here, so the module is in the unit tier. diff --git a/tests/toolkit/test_engine_pcap_trace_runtime.py b/tests/toolkit/test_engine_pcap_trace_runtime.py index a35a1816dd..6d660f40c1 100644 --- a/tests/toolkit/test_engine_pcap_trace_runtime.py +++ b/tests/toolkit/test_engine_pcap_trace_runtime.py @@ -4,12 +4,12 @@ Both engines expose each frame's captured octets, its timestamp, its original length and its link type, so their flow tracing adapters carry a PCAP frame record beside the frame mapping they always reported, and -:class:`~pcapkit.foundation.extraction.Extractor` no longer swaps the format for +:class:`~pcapkit.foundation.extraction.Extractor` does not swap the format for JSON. Every traced PCAP file is checked against the capture it was traced from: it holds its flow's records as captured, octets, original length and timestamp alike, the timestamp in the trace's resolution. The records are read from the capture by hand, so no engine's reading is the reference. Every JSON trace is -checked against the one main wrote before. +checked against pinned digests. The inputs no sample has are synthesised from :file:`tcp.pcap`: nanosecond PCAP and PCAP-NG (``if_tsresol=9``) captures, each with a timestamp that has @@ -354,8 +354,8 @@ def test_no_engine_warns_about_original_lengths(self) -> None: def test_dict_capable_trace_formats_are_unchanged(self) -> None: # Only the PCAP trace dumper reads the frame record, so the JSON trace of - # each flow is the very file main wrote before #1507: the adapters still - # report the engine's own mapping of the frame, value types included. + # each flow matches ``JSON_TRACE_SHA256`` (#1507): the adapters report + # the engine's own mapping of the frame, value types included. # Re-measure the digests if dictdumper's output ever changes. from pcapkit import extract diff --git a/tests/toolkit/test_pcap_jumbogram_rfc2675_1687_unit.py b/tests/toolkit/test_pcap_jumbogram_rfc2675_1687_unit.py index b08de3dca7..e2b024a63d 100644 --- a/tests/toolkit/test_pcap_jumbogram_rfc2675_1687_unit.py +++ b/tests/toolkit/test_pcap_jumbogram_rfc2675_1687_unit.py @@ -4,10 +4,10 @@ :rfc:`2675#section-3` lists four errors a receiver must detect: a Payload Length of 0 with a Hop-by-Hop Options header and no Jumbo Payload option, a non-zero Payload Length with the option, a Jumbo Payload Length below 65,536, and the -option with a Fragment header. The default engine's IPv6 parser now rejects +option with a Fragment header. The default engine's IPv6 parser rejects each, so it dissects nothing past the IPv6 header: no TCP reassembly or flow tracing input, and no IP reassembly input from the packet or what it carries. A -jumbogram RFC 2675 accepts is read as before, a capture cut short of its Jumbo +jumbogram RFC 2675 accepts is dissected, a capture cut short of its Jumbo Payload Length included. Every capture is built here, so the module is in the unit tier. diff --git a/tests/toolkit/test_pcap_tcp_segment_ipv6_version_1666_unit.py b/tests/toolkit/test_pcap_tcp_segment_ipv6_version_1666_unit.py index 051ec97f6d..30752e56ab 100644 --- a/tests/toolkit/test_pcap_tcp_segment_ipv6_version_1666_unit.py +++ b/tests/toolkit/test_pcap_tcp_segment_ipv6_version_1666_unit.py @@ -6,8 +6,8 @@ took the IPv4 branch of that fragment check by the header's Version field, and IPv6 then read any Version, so an IPv6 layer with Version 4 took it too and read an ``offset`` IPv6 does not have: the ``AttributeError`` ended the extraction, -PCAP and PCAP-NG alike (#1666). The branch now goes by the layer's class. And -IPv6 now rejects a Version other than 6 (#1680), so such a header is kept raw: +PCAP and PCAP-NG alike (#1666). The branch goes by the layer's class. And +IPv6 rejects a Version other than 6 (#1680), so such a header is kept raw: its frame hands over no input, and every frame after it is still read. scapy still reads those headers as IPv6 and hands their segments over, which diff --git a/tests/toolkit/test_pcap_tcp_segment_unit.py b/tests/toolkit/test_pcap_tcp_segment_unit.py index 37f949fb8c..cb6f4510b0 100644 --- a/tests/toolkit/test_pcap_tcp_segment_unit.py +++ b/tests/toolkit/test_pcap_tcp_segment_unit.py @@ -4,8 +4,8 @@ A capture snapped inside the TCP header leaves a Data Offset running past the captured octets, and the TCP parser rejects such a header by design (#1404), so the segment arrives as ``Raw``. The flow tracing and reassembly adapters of -:mod:`pcapkit.toolkit.pcap` and :mod:`pcapkit.toolkit.pcapng` used to skip it; -they now take the ports, numbers and flags from the fixed 20-octet header, as +:mod:`pcapkit.toolkit.pcap` and :mod:`pcapkit.toolkit.pcapng` take the ports, +numbers and flags from the fixed 20-octet header rather than skipping it, as :mod:`dpkt`, :mod:`scapy` and Wireshark do. Every frame is built here, written as PCAP and as PCAP-NG, and read back through @@ -188,7 +188,8 @@ def test_no_tcp_header_is_read_where_there_is_none(self) -> None: def test_a_dissection_stopped_at_the_internet_layer_traces_nothing(self) -> None: # The payload is Raw because the caller asked for no more, not because TCP - # rejected it, so the adapters leave it alone, as they did before #1518. + # rejected it, so the adapters leave it alone: reading a rejected + # segment's fixed header (#1518) does not apply. for container, write in (('pcap', pcap), ('pcapng', pcapng)): for kwargs in ({'layer': 'Internet'}, {'protocol': 'IPv4'}): with self.subTest(container=container, **kwargs): diff --git a/tests/toolkit/test_pyshark_unit.py b/tests/toolkit/test_pyshark_unit.py index 28d29fb822..d9e4a31311 100644 --- a/tests/toolkit/test_pyshark_unit.py +++ b/tests/toolkit/test_pyshark_unit.py @@ -176,10 +176,9 @@ def test_tcp_traceflow_resolves_ethernet_via_filter_name_mapping(self) -> None: Ethernet capture is ``eth``, not ``ETHERNET`` -- not a LinkType member name on its own. Per the ruling on #838, ``pcapkit.toolkit.pyshark.FILTER_NAME_TO_LINKTYPE`` translates that - filter name to :attr:`LinkType.ETHERNET`, so this no longer raises the - way it did prior to that table landing. The fake carries no - ``encap_type``, which is what selects the filter-name path at all - since #843. + filter name to :attr:`LinkType.ETHERNET`, so this resolves rather than + raising. The fake carries no ``encap_type``, which is what selects the + filter-name path at all (#843). """ from pcapkit.const.reg.linktype import LinkType @@ -462,9 +461,9 @@ def test_lookup_table_shapes(self) -> None: def test_tcp_traceflow_end_to_end_for_an_ethernet_capture(self) -> None: """With the link-layer name resolved, the rest of :func:`tcp_traceflow`'s ``TF_TCP_Packet`` construction is unaffected - by this change -- check every field it fills in from the fake, not - just ``protocol``, so a regression there is not masked by the two - tests above only checking the new mapping. + by it -- check every field it fills in from the fake, not just + ``protocol``, so a regression there is not masked by the two tests + above only checking the mapping. """ from pcapkit.const.reg.linktype import LinkType diff --git a/tests/toolkit/test_scapy_linktype_unit.py b/tests/toolkit/test_scapy_linktype_unit.py index 3b9602a3d1..aae0383c13 100644 --- a/tests/toolkit/test_scapy_linktype_unit.py +++ b/tests/toolkit/test_scapy_linktype_unit.py @@ -5,9 +5,9 @@ ``IP``, which is no :class:`~pcapkit.const.reg.linktype.LinkType` name, so the name lookup raised :exc:`~pcapkit.utilities.exceptions.MissingKeyError`, and a raw-IP packet whose first layer is ``IPv6`` resolved to ``LINKTYPE_IPV6`` whatever -its interface said. :class:`~pcapkit.foundation.engines.scapy.Scapy` now attaches +its interface said. :class:`~pcapkit.foundation.engines.scapy.Scapy` attaches the interface's link type with :func:`~pcapkit.toolkit.scapy.attach_linktype`, -and only a packet built by hand, which carries none, is still looked up by name. +and only a packet built by hand, which carries none, is looked up by name. """ from __future__ import annotations diff --git a/tests/toolkit/test_scapy_unit.py b/tests/toolkit/test_scapy_unit.py index ce33377add..c5c6ab7dfd 100644 --- a/tests/toolkit/test_scapy_unit.py +++ b/tests/toolkit/test_scapy_unit.py @@ -343,14 +343,14 @@ def test_tcp_reassembly_and_traceflow(self) -> None: def test_tcp_traceflow_raises_for_an_ip_rooted_packet(self) -> None: """An IP-rooted (Ether-less) packet's ``.name`` is ``'IP'``, which is - not a LinkType member name. Since #775 tier 1, ``Enum_LinkType.get()`` + not a LinkType member name. Per #775 tier 1, ``Enum_LinkType.get()`` with no default raises on an unresolvable name instead of minting one -- and per the ruling on #838, NULL and RAW are genuine DLTs with their own handler protocol classes, not stand-ins for "unknown link - type", so this call site no longer papers over the miss with a - ``LinkType.NULL`` default. This pins that the miss now raises + type", so this call site does not paper over the miss with a + ``LinkType.NULL`` default. This pins that the miss raises :exc:`~pcapkit.utilities.exceptions.MissingKeyError` naming the - offending value, and that the registry is still not grown by it. + offending value, and that the registry is not grown by it. Contrast an Ether-rooted packet, exercised in ``test_tcp_reassembly_and_traceflow`` above: ``(Ether()/IP()/TCP()). @@ -459,7 +459,7 @@ def test_ipv6_fragment_data_stops_at_the_payload_length(self) -> None: first, last = (toolkit.ipv6_reassembly(packet) for packet in packets[2:]) assert first is not None and last is not None self.assertEqual(bytes(first.payload), DATAGRAM[:40]) - # 44 octets and a ``tl`` of 84 before the fix + # 44 octets and a ``tl`` of 84 without the fix self.assertEqual(bytes(last.payload), DATAGRAM[40:]) self.assertEqual((last.ihl, last.tl), (40, 80)) @@ -500,8 +500,8 @@ def test_tcp_reassembly_is_the_stream_sent(self) -> None: extractor = self._extract(self._tcp_frames(), store=False, reassembly=True, tcp=True) scapy = {(str(datagram.id.src[0]), datagram.id.src[1]): datagram.payload for datagram in extractor.reassembly.tcp} - # ``b'X\x00\x00\x00\x00\x00\xef'`` before the fix: the first segment's - # padding took the place of the second's data, and its trailer followed + # ``b'X\x00\x00\x00\x00\x00\xef'`` without the fix: the first segment's + # padding takes the place of the second's data, and its trailer follows self.assertEqual(scapy.get(('192.0.2.1', 1)), b'XYZ') self.assertEqual(scapy.get(('2001:db8::1', 1)), b'six') diff --git a/tests/toolkit/test_tcp_carrier_walk_1606_1617_unit.py b/tests/toolkit/test_tcp_carrier_walk_1606_1617_unit.py index 810dfff316..09decb0382 100644 --- a/tests/toolkit/test_tcp_carrier_walk_1606_1617_unit.py +++ b/tests/toolkit/test_tcp_carrier_walk_1606_1617_unit.py @@ -10,7 +10,7 @@ A later IPv4 fragment carries no upper-layer header (:rfc:`791#section-3.2`), so the default engine reads no TCP in one, but `Scapy`_ binds IPv6 to IPv4 protocol 41, and AH to protocol 51, whatever the fragment offset, and its engine handed on -the TCP it then dissected (#1617). It now stops at such a fragment. +the TCP it then dissected (#1617). It stops at such a fragment. `dpkt`_ decodes protocol 0 as IPv4, so a Hop-by-Hop header there can read as an IPv4 packet dpkt cannot decode, or whose payload is no packet; neither ends the diff --git a/tests/toolkit/test_tunnelled_tcp_flow_key_1581_unit.py b/tests/toolkit/test_tunnelled_tcp_flow_key_1581_unit.py index 4886d17811..ea614a978b 100644 --- a/tests/toolkit/test_tunnelled_tcp_flow_key_1581_unit.py +++ b/tests/toolkit/test_tunnelled_tcp_flow_key_1581_unit.py @@ -5,7 +5,7 @@ *first* IP layer, which in a tunnel is the tunnel's own: every connection through one tunnel that shares a port pair merged into one flow, under the tunnel's endpoints. The default engine, scapy and pyshark did so (scapy found the inner -IPv4 layer of 4in6 only). Each now keys it by the IP layer nearest above the TCP +IPv4 layer of 4in6 only). Each keys it by the IP layer nearest above the TCP segment, as Wireshark's TCP conversations do. Every capture is built here, so the module is in the unit tier. pyshark runs