From 74efba3cec96f4d0637057ba1a716def16e2160d Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Fri, 16 Oct 2026 19:35:48 -0400 Subject: [PATCH 1/6] docs(foundation): restore Extractor._vfunc's annotation (#1839) - Uncomment the `_vfunc` annotation under its `#:` comment, so Sphinx attaches the description and the type the extraction page asks for. Typed `Callable[[Extractor, Any], Any]` rather than `VerboseHandler`, whose `Packet` union makes mypy reject the default and engine callbacks; mypy output on the extractor and the engines is unchanged. - Add a test that the comment attaches, read the way Sphinx's parser reads it, plus a cross-check against Sphinx itself where it is installed. Closes #1839 --- pcapkit/foundation/extraction.py | 5 +- ...t_extractor_vfunc_doc_comment_1839_unit.py | 124 ++++++++++++++++++ 2 files changed, 128 insertions(+), 1 deletion(-) create mode 100644 tests/project/test_extractor_vfunc_doc_comment_1839_unit.py diff --git a/pcapkit/foundation/extraction.py b/pcapkit/foundation/extraction.py index dd4acb10b..a75df7af2 100644 --- a/pcapkit/foundation/extraction.py +++ b/pcapkit/foundation/extraction.py @@ -177,8 +177,11 @@ class Extractor(Generic[_P]): #: the file object after extraction. _flag_s: 'bool' + # NOTE: not ``VerboseHandler``, whose frame parameter is the whole + # ``Packet`` union: mypy would then reject the default and the engines' + # callbacks, each of which handles only its own packet type (#1839). #: Verbose callback function. - #_vfunc: 'VerboseHandler' + _vfunc: 'Callable[[Extractor, Any], Any]' #: Frame number. _frnum: 'int' diff --git a/tests/project/test_extractor_vfunc_doc_comment_1839_unit.py b/tests/project/test_extractor_vfunc_doc_comment_1839_unit.py new file mode 100644 index 000000000..ad1c70b30 --- /dev/null +++ b/tests/project/test_extractor_vfunc_doc_comment_1839_unit.py @@ -0,0 +1,124 @@ +# -*- coding: utf-8 -*- +"""Pins the documentation comment on :attr:`Extractor._vfunc`. + +GitHub issue #1839: :file:`docs/source/pcapkit/foundation/extraction.rst` asks +for ``.. autoattribute:: _vfunc``, but the annotation under its ``#:`` comment +had been commented out. Sphinx attaches a ``#:`` comment only to the assignment +or annotation directly below it, so the page rendered a bare name with no type +and no description. + +The first test reads the comment the way :mod:`sphinx.pycode.parser` does and +runs everywhere; it reads source text and never imports the package. The second +asks Sphinx itself, and skips without it. + +""" + +from __future__ import annotations + +import ast +import pathlib +import re +import unittest +from typing import TYPE_CHECKING + +from tests.project.test_docs_ambiguous_xrefs_1775_unit import HAS_SPHINX + +if TYPE_CHECKING: + from typing import Iterable, Iterator, Optional + +ROOT = pathlib.Path(__file__).resolve().parents[2] + +#: The module that declares the attribute, by name and by path. +MODULE = 'pcapkit.foundation.extraction' +SOURCE = ROOT / 'pcapkit' / 'foundation' / 'extraction.py' + +#: The documented attribute, and the description its page should render. +CLASS = 'Extractor' +ATTR = '_vfunc' +TEXT = 'Verbose callback function.' + +#: A documentation comment line, as :mod:`sphinx.pycode.parser` matches it. +COMMENT = re.compile(r'^\s*#: ?(.*)$') + + +def class_statements(body: 'Iterable[ast.stmt]') -> 'Iterator[ast.stmt]': + """Yield every statement of a class body, including those nested in a block. + + An ``if TYPE_CHECKING:`` block in a class body declares class attributes, + so the walk goes into compound statements; it stops at a function or class, + which opens a new scope. + + """ + for stmt in body: + if isinstance(stmt, (ast.FunctionDef, ast.AsyncFunctionDef, ast.ClassDef)): + continue + yield stmt + for field in ('body', 'orelse', 'finalbody'): + yield from class_statements(getattr(stmt, field, [])) + for handler in getattr(stmt, 'handlers', []): + yield from class_statements(handler.body) + + +def doc_comment(source: 'str', cls: 'str', attr: 'str') -> 'Optional[list[str]]': + """Return the ``#:`` lines Sphinx would attach to ``cls.attr``. + + These are the consecutive ``#:`` lines directly above the first assignment + or annotation of ``attr`` in the body of ``cls``. The result is :obj:`None` + when the class body neither assigns nor annotates ``attr``. + + """ + lines = source.splitlines() + classes = [node for node in ast.walk(ast.parse(source)) + if isinstance(node, ast.ClassDef) and node.name == cls] + if not classes: + raise AssertionError(f'no class {cls!r} in {SOURCE}') + + for stmt in class_statements(classes[0].body): + if isinstance(stmt, ast.AnnAssign): + targets = [stmt.target] # type: list[ast.expr] + elif isinstance(stmt, ast.Assign): + targets = stmt.targets + else: + continue + if not any(isinstance(target, ast.Name) and target.id == attr for target in targets): + continue + + found = [] # type: list[str] + for line in reversed(lines[:stmt.lineno - 1]): + match = COMMENT.match(line) + if match is None: + break + found.append(match.group(1)) + return found[::-1] + return None + + +class VerboseCallbackDocCommentTests(unittest.TestCase): + """The ``#:`` comment above :attr:`Extractor._vfunc` attaches to it.""" + + source = '' + + @classmethod + def setUpClass(cls) -> 'None': + cls.source = SOURCE.read_text(encoding='utf-8') + + def test_comment_sits_directly_above_the_annotation(self) -> 'None': + comment = doc_comment(self.source, CLASS, ATTR) + self.assertIsNotNone(comment, f'{CLASS} neither annotates nor assigns {ATTR}') + self.assertIn(TEXT, ' '.join(comment or [])) + + @unittest.skipUnless(HAS_SPHINX, 'Sphinx and sphinx-autodoc-typehints are not installed') + def test_sphinx_attaches_the_comment(self) -> 'None': + from sphinx.pycode import ModuleAnalyzer # pylint: disable=import-outside-toplevel + + docs = ModuleAnalyzer.for_string(self.source, MODULE).find_attr_docs() + lines = docs.get((CLASS, ATTR)) + self.assertIsNotNone(lines, f'Sphinx attaches no comment to {CLASS}.{ATTR}') + found = '\n'.join(lines or []).strip() + self.assertIn(TEXT, found) + # The first test reads the comment the same way, so it is measuring this. + self.assertEqual('\n'.join(doc_comment(self.source, CLASS, ATTR) or []).strip(), found) + + +if __name__ == '__main__': + unittest.main() From e7d2465dd68e6af1a19707fd9c958510cfb0fd11 Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Fri, 16 Oct 2026 20:12:29 -0400 Subject: [PATCH 2/6] docs(foundation): give _vfunc and VerboseHandler a precise signature (#1839) - Type `VerboseHandler` as `Callable[['Extractor', Packet], object]` and annotate `_vfunc` with it. The return is `object`, not `None`: every caller discards it, and `None` would make mypy reject a user callback that returns a value. - Narrow the frame to the engine's own packet type with `cast` inside the default callback and each engine's callback, so they accept the alias's `Packet` union. Runtime output is unchanged. - Pin the annotation and the alias's signature in the #1839 test. Refs #1839 --- pcapkit/foundation/engines/dpkt.py | 2 +- pcapkit/foundation/engines/pcap_ct.py | 2 +- pcapkit/foundation/engines/pypcap.py | 2 +- pcapkit/foundation/engines/pypcapfile.py | 2 +- pcapkit/foundation/engines/pyshark.py | 2 +- pcapkit/foundation/engines/scapy.py | 2 +- pcapkit/foundation/extraction.py | 15 +-- ...t_extractor_vfunc_doc_comment_1839_unit.py | 119 ++++++++++++------ 8 files changed, 98 insertions(+), 48 deletions(-) diff --git a/pcapkit/foundation/engines/dpkt.py b/pcapkit/foundation/engines/dpkt.py index 508f5bdae..37edce47e 100644 --- a/pcapkit/foundation/engines/dpkt.py +++ b/pcapkit/foundation/engines/dpkt.py @@ -458,7 +458,7 @@ def run(self) -> 'None': if ext._flag_v: from pcapkit.toolkit.dpkt import packet2chain # isort:skip ext._vfunc = lambda e, f: print( - f'Frame {e._frnum:>3d}: {packet2chain(f)}' # pylint: disable=protected-access + f'Frame {e._frnum:>3d}: {packet2chain(cast("DPKTPacket", f))}' # pylint: disable=protected-access ) # extract global header diff --git a/pcapkit/foundation/engines/pcap_ct.py b/pcapkit/foundation/engines/pcap_ct.py index 51f42ef19..5e1f81df3 100644 --- a/pcapkit/foundation/engines/pcap_ct.py +++ b/pcapkit/foundation/engines/pcap_ct.py @@ -403,7 +403,7 @@ def run(self) -> 'None': if ext._flag_v: from pcapkit.toolkit.pcap_ct import packet2chain # isort:skip ext._vfunc = lambda e, f: print( - f'Frame {e._frnum:>3d}: {packet2chain(f[1], data_link=self._dlink)}' # pylint: disable=protected-access + f'Frame {e._frnum:>3d}: {packet2chain(cast("RawFrame", f)[1], data_link=self._dlink)}' # pylint: disable=protected-access ) # pylint: disable=logging-fstring-interpolation # extract & analyse file diff --git a/pcapkit/foundation/engines/pypcap.py b/pcapkit/foundation/engines/pypcap.py index 2121d9782..6eebc29fa 100644 --- a/pcapkit/foundation/engines/pypcap.py +++ b/pcapkit/foundation/engines/pypcap.py @@ -316,7 +316,7 @@ def run(self) -> 'None': if ext._flag_v: from pcapkit.toolkit.pypcap import packet2chain # isort:skip ext._vfunc = lambda e, f: print( - f'Frame {e._frnum:>3d}: {packet2chain(f[1], data_link=self._dlink)}' # pylint: disable=protected-access + f'Frame {e._frnum:>3d}: {packet2chain(cast("RawFrame", f)[1], data_link=self._dlink)}' # pylint: disable=protected-access ) # pylint: disable=logging-fstring-interpolation # extract & analyse file diff --git a/pcapkit/foundation/engines/pypcapfile.py b/pcapkit/foundation/engines/pypcapfile.py index 42a28a6f7..75c5a118e 100644 --- a/pcapkit/foundation/engines/pypcapfile.py +++ b/pcapkit/foundation/engines/pypcapfile.py @@ -224,7 +224,7 @@ def run(self) -> 'None': if ext._flag_v: from pcapkit.toolkit.pypcapfile import packet2chain # isort:skip ext._vfunc = lambda e, f: print( - f'Frame {e._frnum:>3d}: {packet2chain(f, data_link=self._dlink)}' # pylint: disable=protected-access + f'Frame {e._frnum:>3d}: {packet2chain(cast("PCAPFilePacket", f), data_link=self._dlink)}' # pylint: disable=protected-access ) # pylint: disable=logging-fstring-interpolation # extract & analyse file diff --git a/pcapkit/foundation/engines/pyshark.py b/pcapkit/foundation/engines/pyshark.py index 2113ce1ef..7a3dbbb31 100644 --- a/pcapkit/foundation/engines/pyshark.py +++ b/pcapkit/foundation/engines/pyshark.py @@ -259,7 +259,7 @@ def run(self) -> 'None': if ext._flag_v: ext._vfunc = lambda e, f: print( f'Frame {e._frnum:>3d}: ' # pylint: disable=protected-access - f'{cast("BaseLayer", _frame_layer(f)).protocols}' + f'{cast("BaseLayer", _frame_layer(cast("PySharkPacket", f))).protocols}' ) # extract global header diff --git a/pcapkit/foundation/engines/scapy.py b/pcapkit/foundation/engines/scapy.py index 4509cc30b..0f406757c 100644 --- a/pcapkit/foundation/engines/scapy.py +++ b/pcapkit/foundation/engines/scapy.py @@ -387,7 +387,7 @@ def run(self) -> 'None': if ext._flag_v: from pcapkit.toolkit.scapy import packet2chain # isort:skip ext._vfunc = lambda e, f: print( - f'Frame {e._frnum:>3d}: {packet2chain(f)}' # pylint: disable=protected-access + f'Frame {e._frnum:>3d}: {packet2chain(cast("ScapyPacket", f))}' # pylint: disable=protected-access ) # extract global header diff --git a/pcapkit/foundation/extraction.py b/pcapkit/foundation/extraction.py index a75df7af2..c4d4a820c 100644 --- a/pcapkit/foundation/extraction.py +++ b/pcapkit/foundation/extraction.py @@ -85,7 +85,9 @@ PCAPFilePacket, tuple[float, bytes]] Protocols = Union[str, ProtocolBase, Type[ProtocolBase]] - VerboseHandler = Callable[['Extractor', Packet], Any] + # NOTE: every caller discards the callback's result. ``object`` rather than + # ``None`` still accepts one that returns something, e.g. ``sys.stdout.write``. + VerboseHandler = Callable[['Extractor', Packet], object] __all__ = ['Extractor'] @@ -177,11 +179,8 @@ class Extractor(Generic[_P]): #: the file object after extraction. _flag_s: 'bool' - # NOTE: not ``VerboseHandler``, whose frame parameter is the whole - # ``Packet`` union: mypy would then reject the default and the engines' - # callbacks, each of which handles only its own packet type (#1839). #: Verbose callback function. - _vfunc: 'Callable[[Extractor, Any], Any]' + _vfunc: 'VerboseHandler' #: Frame number. _frnum: 'int' @@ -1287,9 +1286,11 @@ def __init__(self, # user-facing output on stdout, not diagnostics -- the frame # chains are a feature of the tool, so they stay on ``print`` # rather than becoming log records a consumer has to configure - # a handler to see (and on a different stream at that). + # a handler to see (and on a different stream at that). Of the + # built-in engines only PCAP and PCAPNG keep this callback, so + # its frame is theirs; the others install their own. self._vfunc = lambda e, f: print( - f'Frame {e._frnum:>3d}: {f.protochain}' # pylint: disable=protected-access + f'Frame {e._frnum:>3d}: {cast("Frame | PCAPNG", f).protochain}' # pylint: disable=protected-access ) else: self._vfunc = lambda e, f: None diff --git a/tests/project/test_extractor_vfunc_doc_comment_1839_unit.py b/tests/project/test_extractor_vfunc_doc_comment_1839_unit.py index ad1c70b30..4c7e14fdf 100644 --- a/tests/project/test_extractor_vfunc_doc_comment_1839_unit.py +++ b/tests/project/test_extractor_vfunc_doc_comment_1839_unit.py @@ -1,5 +1,5 @@ # -*- coding: utf-8 -*- -"""Pins the documentation comment on :attr:`Extractor._vfunc`. +"""Pins the documentation comment and the type of :attr:`Extractor._vfunc`. GitHub issue #1839: :file:`docs/source/pcapkit/foundation/extraction.rst` asks for ``.. autoattribute:: _vfunc``, but the annotation under its ``#:`` comment @@ -7,9 +7,12 @@ or annotation directly below it, so the page rendered a bare name with no type and no description. -The first test reads the comment the way :mod:`sphinx.pycode.parser` does and -runs everywhere; it reads source text and never imports the package. The second -asks Sphinx itself, and skips without it. +The annotation is the ``VerboseHandler`` alias, whose signature is pinned too: +the frame parameter is the whole ``Packet`` union, and the return is +:class:`object`, because every caller discards it. + +Every test reads source text and never imports the package. The one that asks +Sphinx itself skips without it; the others run everywhere. """ @@ -24,7 +27,9 @@ from tests.project.test_docs_ambiguous_xrefs_1775_unit import HAS_SPHINX if TYPE_CHECKING: - from typing import Iterable, Iterator, Optional + from typing import Iterable, Iterator, Optional, Union + + Binding = Union[ast.Assign, ast.AnnAssign] ROOT = pathlib.Path(__file__).resolve().parents[2] @@ -37,16 +42,20 @@ ATTR = '_vfunc' TEXT = 'Verbose callback function.' +#: The alias that types the attribute, and the signature it should spell. +ALIAS = 'VerboseHandler' +SIGNATURE = "Callable[['Extractor', Packet], object]" + #: A documentation comment line, as :mod:`sphinx.pycode.parser` matches it. COMMENT = re.compile(r'^\s*#: ?(.*)$') -def class_statements(body: 'Iterable[ast.stmt]') -> 'Iterator[ast.stmt]': - """Yield every statement of a class body, including those nested in a block. +def scope_statements(body: 'Iterable[ast.stmt]') -> 'Iterator[ast.stmt]': + """Yield every statement of a module or class body, including nested blocks. - An ``if TYPE_CHECKING:`` block in a class body declares class attributes, - so the walk goes into compound statements; it stops at a function or class, - which opens a new scope. + An ``if TYPE_CHECKING:`` block declares names of the scope it sits in, so the + walk goes into compound statements; it stops at a function or class, which + opens a new scope. """ for stmt in body: @@ -54,9 +63,33 @@ def class_statements(body: 'Iterable[ast.stmt]') -> 'Iterator[ast.stmt]': continue yield stmt for field in ('body', 'orelse', 'finalbody'): - yield from class_statements(getattr(stmt, field, [])) + yield from scope_statements(getattr(stmt, field, [])) for handler in getattr(stmt, 'handlers', []): - yield from class_statements(handler.body) + yield from scope_statements(handler.body) + + +def bindings(body: 'Iterable[ast.stmt]', name: 'str') -> 'list[Binding]': + """Return every assignment or annotation of ``name`` in a scope, in order.""" + found = [] # type: list[Binding] + for stmt in scope_statements(body): + if isinstance(stmt, ast.AnnAssign): + targets = [stmt.target] # type: list[ast.expr] + elif isinstance(stmt, ast.Assign): + targets = stmt.targets + else: + continue + if any(isinstance(target, ast.Name) and target.id == name for target in targets): + found.append(stmt) + return found + + +def class_body(tree: 'ast.Module', cls: 'str') -> 'list[ast.stmt]': + """Return the body of the class ``cls`` defined anywhere in ``tree``.""" + classes = [node for node in ast.walk(tree) + if isinstance(node, ast.ClassDef) and node.name == cls] + if not classes: + raise AssertionError(f'no class {cls!r} in {SOURCE}') + return classes[0].body def doc_comment(source: 'str', cls: 'str', attr: 'str') -> 'Optional[list[str]]': @@ -67,30 +100,17 @@ def doc_comment(source: 'str', cls: 'str', attr: 'str') -> 'Optional[list[str]]' when the class body neither assigns nor annotates ``attr``. """ - lines = source.splitlines() - classes = [node for node in ast.walk(ast.parse(source)) - if isinstance(node, ast.ClassDef) and node.name == cls] - if not classes: - raise AssertionError(f'no class {cls!r} in {SOURCE}') - - for stmt in class_statements(classes[0].body): - if isinstance(stmt, ast.AnnAssign): - targets = [stmt.target] # type: list[ast.expr] - elif isinstance(stmt, ast.Assign): - targets = stmt.targets - else: - continue - if not any(isinstance(target, ast.Name) and target.id == attr for target in targets): - continue + found = bindings(class_body(ast.parse(source), cls), attr) + if not found: + return None - found = [] # type: list[str] - for line in reversed(lines[:stmt.lineno - 1]): - match = COMMENT.match(line) - if match is None: - break - found.append(match.group(1)) - return found[::-1] - return None + lines = [] # type: list[str] + for line in reversed(source.splitlines()[:found[0].lineno - 1]): + match = COMMENT.match(line) + if match is None: + break + lines.append(match.group(1)) + return lines[::-1] class VerboseCallbackDocCommentTests(unittest.TestCase): @@ -120,5 +140,34 @@ def test_sphinx_attaches_the_comment(self) -> 'None': self.assertEqual('\n'.join(doc_comment(self.source, CLASS, ATTR) or []).strip(), found) +class VerboseHandlerSignatureTests(unittest.TestCase): + """:attr:`Extractor._vfunc` is typed by the precise ``VerboseHandler`` alias.""" + + source = '' + tree = ast.parse('') + + @classmethod + def setUpClass(cls) -> 'None': + cls.source = SOURCE.read_text(encoding='utf-8') + cls.tree = ast.parse(cls.source) + + def test_attribute_is_annotated_with_the_alias(self) -> 'None': + annotations = [ast.literal_eval(stmt.annotation) + for stmt in bindings(class_body(self.tree, CLASS), ATTR) + if isinstance(stmt, ast.AnnAssign)] + self.assertEqual(annotations, [ALIAS]) + + def test_alias_spells_the_precise_signature(self) -> 'None': + found = bindings(self.tree.body, ALIAS) + self.assertEqual(len(found), 1, f'{ALIAS} is not bound exactly once in {SOURCE}') + value = found[0].value + if value is None: + self.fail(f'{ALIAS} is annotated but never assigned') + # Compared as trees, so spacing and quoting do not matter. + expected = ast.parse(SIGNATURE, mode='eval').body + self.assertEqual(ast.dump(value), ast.dump(expected), + f'{ALIAS} = {ast.get_source_segment(self.source, value)}') + + if __name__ == '__main__': unittest.main() From 817208c8a380cfb6c88aa6c23d3ca5a7d52e5abd Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Fri, 16 Oct 2026 20:24:57 -0400 Subject: [PATCH 3/6] docs(foundation): correct the VerboseHandler note's example (#1839) - Replace `sys.stdout.write`, which takes one argument and so is no verbose callback at all, with `lambda e, f: sys.stdout.write(str(f))`: it takes the extractor and the frame and returns a value, so `object` accepts it where `None` would not. Refs #1839 --- pcapkit/foundation/extraction.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/pcapkit/foundation/extraction.py b/pcapkit/foundation/extraction.py index c4d4a820c..32963961f 100644 --- a/pcapkit/foundation/extraction.py +++ b/pcapkit/foundation/extraction.py @@ -86,7 +86,8 @@ Protocols = Union[str, ProtocolBase, Type[ProtocolBase]] # NOTE: every caller discards the callback's result. ``object`` rather than - # ``None`` still accepts one that returns something, e.g. ``sys.stdout.write``. + # ``None`` still accepts one that returns something, e.g. + # ``lambda e, f: sys.stdout.write(str(f))``. VerboseHandler = Callable[['Extractor', Packet], object] __all__ = ['Extractor'] From 5b0e9fdbe0da48b99bbc87e0ff11c433aaca6d0a Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Fri, 16 Oct 2026 22:05:13 -0400 Subject: [PATCH 4/6] docs(foundation): type the verbose callback's result as Any (#1839) - Type `VerboseHandler`'s return as `Any` rather than `object`, per the owner's preference on #1867: every caller discards the result, and `Any` accepts any two-argument callback, whatever it returns. - Rewrite the NOTE above the alias to give that reason. - Pin `Any` in the signature test; the exact comparison still fails on a return of `None` or `object`. Refs #1839 --- pcapkit/foundation/extraction.py | 6 +++--- tests/project/test_extractor_vfunc_doc_comment_1839_unit.py | 5 +++-- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/pcapkit/foundation/extraction.py b/pcapkit/foundation/extraction.py index 32963961f..b7feaf080 100644 --- a/pcapkit/foundation/extraction.py +++ b/pcapkit/foundation/extraction.py @@ -85,10 +85,10 @@ PCAPFilePacket, tuple[float, bytes]] Protocols = Union[str, ProtocolBase, Type[ProtocolBase]] - # NOTE: every caller discards the callback's result. ``object`` rather than - # ``None`` still accepts one that returns something, e.g. + # NOTE: every caller discards the callback's result, so it is typed ``Any``: + # any two-argument callback is accepted, whatever it returns, e.g. # ``lambda e, f: sys.stdout.write(str(f))``. - VerboseHandler = Callable[['Extractor', Packet], object] + VerboseHandler = Callable[['Extractor', Packet], Any] __all__ = ['Extractor'] diff --git a/tests/project/test_extractor_vfunc_doc_comment_1839_unit.py b/tests/project/test_extractor_vfunc_doc_comment_1839_unit.py index 4c7e14fdf..a3c598d3c 100644 --- a/tests/project/test_extractor_vfunc_doc_comment_1839_unit.py +++ b/tests/project/test_extractor_vfunc_doc_comment_1839_unit.py @@ -9,7 +9,8 @@ The annotation is the ``VerboseHandler`` alias, whose signature is pinned too: the frame parameter is the whole ``Packet`` union, and the return is -:class:`object`, because every caller discards it. +:data:`~typing.Any`, because every caller discards it. The comparison is exact, +so it fails on a return of ``None`` or :class:`object` as well. Every test reads source text and never imports the package. The one that asks Sphinx itself skips without it; the others run everywhere. @@ -44,7 +45,7 @@ #: The alias that types the attribute, and the signature it should spell. ALIAS = 'VerboseHandler' -SIGNATURE = "Callable[['Extractor', Packet], object]" +SIGNATURE = "Callable[['Extractor', Packet], Any]" #: A documentation comment line, as :mod:`sphinx.pycode.parser` matches it. COMMENT = re.compile(r'^\s*#: ?(.*)$') From 06cafcb72bbb4730ce62aa43572d427104c89475 Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Fri, 16 Oct 2026 20:35:59 -0400 Subject: [PATCH 5/6] fix(foundation): keep a user's verbose callback on every engine (#1874) - Extractor records in a private `_vfunc_default` flag whether `verbose=True` asked for the default printer; a callable leaves it unset. - The scapy, dpkt, pyshark, pypcap, pcap-ct and pypcapfile engines install their own printer only when that flag is set, so a caller's callback is no longer replaced. The `verbose=True` output is unchanged. - Note the fix in Extractor's 1.5.0 versionchanged block, and correct the `_flag_v` comment, which said it gated the per-frame call. - Test each installed engine end to end on in.pcap, with a recording callback and with `verbose=True`, and add fake-extractor cases for the engines that cannot run here. Closes #1874 --- pcapkit/foundation/engines/dpkt.py | 4 +- pcapkit/foundation/engines/pcap_ct.py | 4 +- pcapkit/foundation/engines/pypcap.py | 4 +- pcapkit/foundation/engines/pypcapfile.py | 4 +- pcapkit/foundation/engines/pyshark.py | 7 +- pcapkit/foundation/engines/scapy.py | 4 +- pcapkit/foundation/extraction.py | 19 +- .../test_engine_verbose_callback_1874_unit.py | 216 ++++++++++++++++++ .../foundation/engines/test_pcap_ct_engine.py | 13 +- .../foundation/engines/test_pypcap_engine.py | 13 +- .../engines/test_pypcapfile_engine.py | 12 +- .../engines/test_pyshark_non_packet_unit.py | 23 +- .../engines/test_runtime_engines.py | 7 +- tests/foundation/test_extraction.py | 5 + 14 files changed, 312 insertions(+), 23 deletions(-) create mode 100644 tests/foundation/engines/test_engine_verbose_callback_1874_unit.py diff --git a/pcapkit/foundation/engines/dpkt.py b/pcapkit/foundation/engines/dpkt.py index 37edce47e..9aa6c0437 100644 --- a/pcapkit/foundation/engines/dpkt.py +++ b/pcapkit/foundation/engines/dpkt.py @@ -454,8 +454,8 @@ def run(self) -> 'None': "'context=' is ignored", AttributeWarning, stacklevel=stacklevel()) - # setup verbose handler - if ext._flag_v: + # setup verbose handler, unless the caller gave one of their own (#1874) + if ext._vfunc_default: from pcapkit.toolkit.dpkt import packet2chain # isort:skip ext._vfunc = lambda e, f: print( f'Frame {e._frnum:>3d}: {packet2chain(cast("DPKTPacket", f))}' # pylint: disable=protected-access diff --git a/pcapkit/foundation/engines/pcap_ct.py b/pcapkit/foundation/engines/pcap_ct.py index 5e1f81df3..880935ca0 100644 --- a/pcapkit/foundation/engines/pcap_ct.py +++ b/pcapkit/foundation/engines/pcap_ct.py @@ -399,8 +399,8 @@ def run(self) -> 'None': self._handle = cast('Handle', self._expkg.pcap(name=ext._ifnm, promisc=False)) self._dlink = Enum_LinkType.get(self._handle.datalink()) - # setup verbose handler - if ext._flag_v: + # setup verbose handler, unless the caller gave one of their own (#1874) + if ext._vfunc_default: from pcapkit.toolkit.pcap_ct import packet2chain # isort:skip ext._vfunc = lambda e, f: print( f'Frame {e._frnum:>3d}: {packet2chain(cast("RawFrame", f)[1], data_link=self._dlink)}' # pylint: disable=protected-access diff --git a/pcapkit/foundation/engines/pypcap.py b/pcapkit/foundation/engines/pypcap.py index 6eebc29fa..9b7172406 100644 --- a/pcapkit/foundation/engines/pypcap.py +++ b/pcapkit/foundation/engines/pypcap.py @@ -312,8 +312,8 @@ def run(self) -> 'None': handle = cast('Handle', self._expkg.pcap(name=ext._ifnm, promisc=False)) self._dlink = Enum_LinkType.get(handle.datalink()) - # setup verbose handler - if ext._flag_v: + # setup verbose handler, unless the caller gave one of their own (#1874) + if ext._vfunc_default: from pcapkit.toolkit.pypcap import packet2chain # isort:skip ext._vfunc = lambda e, f: print( f'Frame {e._frnum:>3d}: {packet2chain(cast("RawFrame", f)[1], data_link=self._dlink)}' # pylint: disable=protected-access diff --git a/pcapkit/foundation/engines/pypcapfile.py b/pcapkit/foundation/engines/pypcapfile.py index 75c5a118e..fa34c8bae 100644 --- a/pcapkit/foundation/engines/pypcapfile.py +++ b/pcapkit/foundation/engines/pypcapfile.py @@ -220,8 +220,8 @@ def run(self) -> 'None': self._dlink = Enum_LinkType.get(sfile.header.ll_type) self._declf = self._get_decoder(sfile.header.ll_type) - # setup verbose handler - if ext._flag_v: + # setup verbose handler, unless the caller gave one of their own (#1874) + if ext._vfunc_default: from pcapkit.toolkit.pypcapfile import packet2chain # isort:skip ext._vfunc = lambda e, f: print( f'Frame {e._frnum:>3d}: {packet2chain(cast("PCAPFilePacket", f), data_link=self._dlink)}' # pylint: disable=protected-access diff --git a/pcapkit/foundation/engines/pyshark.py b/pcapkit/foundation/engines/pyshark.py index 7a3dbbb31..dab850ce5 100644 --- a/pcapkit/foundation/engines/pyshark.py +++ b/pcapkit/foundation/engines/pyshark.py @@ -254,9 +254,10 @@ def run(self) -> 'None': f"so 'ipv4={ext._ipv4}', 'ipv6={ext._ipv6}' and 'tcp={ext._tcp}' will be ignored", AttributeWarning, stacklevel=stacklevel()) - # setup verbose handler; a packet always has a frame layer, since - # read_frame skips every record that does not (#1515) - if ext._flag_v: + # setup verbose handler, unless the caller gave one of their own + # (#1874); a packet always has a frame layer, since read_frame skips + # every record that does not (#1515) + if ext._vfunc_default: ext._vfunc = lambda e, f: print( f'Frame {e._frnum:>3d}: ' # pylint: disable=protected-access f'{cast("BaseLayer", _frame_layer(cast("PySharkPacket", f))).protocols}' diff --git a/pcapkit/foundation/engines/scapy.py b/pcapkit/foundation/engines/scapy.py index 0f406757c..f0819960a 100644 --- a/pcapkit/foundation/engines/scapy.py +++ b/pcapkit/foundation/engines/scapy.py @@ -383,8 +383,8 @@ def run(self) -> 'None': "'context=' is ignored", AttributeWarning, stacklevel=stacklevel()) - # setup verbose handler - if ext._flag_v: + # setup verbose handler, unless the caller gave one of their own (#1874) + if ext._vfunc_default: from pcapkit.toolkit.scapy import packet2chain # isort:skip ext._vfunc = lambda e, f: print( f'Frame {e._frnum:>3d}: {packet2chain(cast("ScapyPacket", f))}' # pylint: disable=protected-access diff --git a/pcapkit/foundation/extraction.py b/pcapkit/foundation/extraction.py index b7feaf080..714945406 100644 --- a/pcapkit/foundation/extraction.py +++ b/pcapkit/foundation/extraction.py @@ -127,7 +127,9 @@ class Extractor(Generic[_P]): so a positional argument after ``reasm_store`` has to move to a keyword. Flows are traced bidirectionally by default. :attr:`engine` is typed :class:`~pcapkit.foundation.engines.engine.EngineBase`, the - base every engine derives from (:issue:`1022`). + base every engine derives from (:issue:`1022`). A callback given as + ``verbose`` is called on every engine, where the third-party engines + replaced it with their own printer (:issue:`1874`). """ if TYPE_CHECKING: @@ -164,8 +166,10 @@ class Extractor(Generic[_P]): _flag_r: 'bool' #: Trace flag. It indicates if the flow tracing is enabled. _flag_t: 'bool' - #: Verbose flag. This is used to determine if the verbose callback - #: function should be called at each frame. + #: Verbose flag. It indicates if verbose output is enabled, whether by + #: ``verbose=True`` or by a callback, which + #: :attr:`~pcapkit.foundation.extraction.Extractor._vfunc_default` + #: tells apart. _flag_v: 'bool' #: No EOF flag. It is useful when the input is a live capture on a pipe or #: on standard input, where reaching the end of what has arrived so far @@ -182,6 +186,13 @@ class Extractor(Generic[_P]): #: Verbose callback function. _vfunc: 'VerboseHandler' + #: Default verbose printer flag. It is set only by ``verbose=True``, + #: which asks for the default output, and an engine whose frames are + #: not PyPCAPKit's own replaces + #: :attr:`~pcapkit.foundation.extraction.Extractor._vfunc` with its own + #: printer only when it is set. A callback given as ``verbose`` leaves + #: it unset, so every engine keeps that callback (:issue:`1874`). + _vfunc_default: 'bool' #: Frame number. _frnum: 'int' @@ -1282,6 +1293,7 @@ def __init__(self, # verbose callback function if isinstance(verbose, bool): self._flag_v = verbose + self._vfunc_default = verbose if verbose: # NOTE: ``verbose=True`` and the CLI's ``-v`` are a request for # user-facing output on stdout, not diagnostics -- the frame @@ -1297,6 +1309,7 @@ def __init__(self, self._vfunc = lambda e, f: None else: self._flag_v = True + self._vfunc_default = False self._vfunc = verbose self._frnum = 0 # frame number diff --git a/tests/foundation/engines/test_engine_verbose_callback_1874_unit.py b/tests/foundation/engines/test_engine_verbose_callback_1874_unit.py new file mode 100644 index 000000000..24333d447 --- /dev/null +++ b/tests/foundation/engines/test_engine_verbose_callback_1874_unit.py @@ -0,0 +1,216 @@ +# -*- coding: utf-8 -*- +"""A callback given as ``verbose`` reaches every frame on every engine (#1874). + +: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``. + +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 +extractor falls back to the default engine, with an +:class:`~pcapkit.utilities.warnings.EngineWarning`, when it cannot run the one +requested, and that would pass the callback case vacuously. An engine that +cannot run here is skipped, with the reason the extractor itself would give. + +""" +from __future__ import annotations + +import contextlib +import importlib.util +import io +import struct +import unittest +import warnings +from typing import TYPE_CHECKING + +from tests._support import close_extractor, reimport_once_per_class, sample_path + +if TYPE_CHECKING: + from typing import Any, Optional, Type + + from pcapkit.foundation.engines.engine import EngineBase + from pcapkit.foundation.extraction import Extractor + +RUNTIME_DEPS = ('tbtrim', 'aenum', 'chardet', 'dictdumper') +HAS_RUNTIME = all(importlib.util.find_spec(name) is not None for name in RUNTIME_DEPS) + +#: The capture every case reads, a little-endian PCAP. +CAPTURE = 'in.pcap' + + +def _record_count(fin: 'str') -> 'int': + """Count the records of a little-endian PCAP by walking its record headers.""" + with open(fin, 'rb') as file: + rest = file.read()[24:] + count = 0 + while rest: + incl_len, = struct.unpack(' None: + reimport_once_per_class(self) + reason = self.unavailable() + if reason is not None: + self.skipTest(f'the {self.ENGINE} engine cannot run here: {reason}') + + def engine_class(self) -> 'Type[EngineBase]': + """The class that runs :attr:`ENGINE`.""" + from pcapkit.corekit.module import ModuleDescriptor + from pcapkit.foundation.extraction import Extractor + + entry = Extractor.__engine__[self.ENGINE] + return entry.klass if isinstance(entry, ModuleDescriptor) else entry + + def unavailable(self) -> 'Optional[str]': + """Why the extractor would not run :attr:`ENGINE` here, if it would not. + + These are the checks ``Extractor._select_engine`` makes, in its order. + + """ + from pcapkit.foundation.extraction import Extractor + + engine = self.engine_class() + reason = engine.unsupported_reason() + if reason is not None: + return reason + with warnings.catch_warnings(): + warnings.simplefilter('ignore') + if Extractor.import_test(engine.module, name=engine.name) is None: # type: ignore[arg-type] + return f'{engine.module} is not installed' + return None + + def chain(self, extractor: 'Extractor', frame: 'Any') -> 'str': + """The protocol chain the engine's own printer reports for ``frame``.""" + raise NotImplementedError + + def extract(self: 'Any', verbose: 'Any') -> 'tuple[Extractor, str]': + """Extract :data:`CAPTURE` on :attr:`ENGINE`; return it and its stdout.""" + from pcapkit.foundation.extraction import Extractor + from pcapkit.utilities.warnings import EngineWarning + + out = io.StringIO() + with warnings.catch_warnings(record=True) as caught, contextlib.redirect_stdout(out): + warnings.simplefilter('always') + extractor = Extractor(fin=sample_path(CAPTURE), engine=self.ENGINE, # type: ignore[arg-type] + nofile=True, store=True, verbose=verbose) + self.addCleanup(close_extractor, extractor) + + # the requested engine ran, not the default one it would fall back to + self.assertEqual([str(item.message) for item in caught + if issubclass(item.category, EngineWarning)], []) + self.assertEqual(extractor._exnam, self.ENGINE) + self.assertIs(type(extractor._exeng), self.engine_class()) + self.assertEqual(len(extractor.frame), _record_count(sample_path(CAPTURE))) + return extractor, out.getvalue() + + def test_a_verbose_callback_is_called_with_every_frame(self: 'Any') -> None: + calls = [] # type: list[tuple[Extractor, Any]] + extractor, out = self.extract(lambda ext, frame: calls.append((ext, frame))) + + frames = extractor.frame + self.assertEqual(len(calls), len(frames)) + for (ext, frame), stored in zip(calls, frames): + self.assertIs(ext, extractor) + self.assertIs(frame, stored) + # and the engine's own printer did not run in its place + self.assertEqual(out, '') + + def test_verbose_true_prints_the_engines_own_chain(self: 'Any') -> None: + extractor, out = self.extract(True) + + self.assertEqual(out.splitlines(), [ + f'Frame {number:>3d}: {self.chain(extractor, frame)}' + for number, frame in enumerate(extractor.frame, 1) + ]) + + +@unittest.skipUnless(HAS_RUNTIME, 'runtime dependencies not installed') +class DefaultEngineVerboseTests(_VerboseCases, unittest.TestCase): + """The built-in PCAP engine, which always kept the callback.""" + + ENGINE = 'default' + + def engine_class(self) -> 'Type[EngineBase]': + from pcapkit.foundation.engines.pcap import PCAP + + return PCAP + + def unavailable(self) -> 'Optional[str]': + return None + + def chain(self, extractor: 'Extractor', frame: 'Any') -> 'str': + return str(frame.protochain) + + +@unittest.skipUnless(HAS_RUNTIME, 'runtime dependencies not installed') +class ScapyVerboseTests(_VerboseCases, unittest.TestCase): + ENGINE = 'scapy' + + def chain(self, extractor: 'Extractor', frame: 'Any') -> 'str': + from pcapkit.toolkit.scapy import packet2chain + + return packet2chain(frame) + + +@unittest.skipUnless(HAS_RUNTIME, 'runtime dependencies not installed') +class DPKTVerboseTests(_VerboseCases, unittest.TestCase): + ENGINE = 'dpkt' + + def chain(self, extractor: 'Extractor', frame: 'Any') -> 'str': + from pcapkit.toolkit.dpkt import packet2chain + + return packet2chain(frame) + + +@unittest.skipUnless(HAS_RUNTIME, 'runtime dependencies not installed') +class PySharkVerboseTests(_VerboseCases, unittest.TestCase): + ENGINE = 'pyshark' + + def chain(self, extractor: 'Extractor', frame: 'Any') -> 'str': + from pcapkit.foundation.engines.pyshark import _frame_layer + + return _frame_layer(frame).protocols # type: ignore[union-attr] + + +@unittest.skipUnless(HAS_RUNTIME, 'runtime dependencies not installed') +class PyPCAPVerboseTests(_VerboseCases, unittest.TestCase): + ENGINE = 'pypcap' + + def chain(self, extractor: 'Extractor', frame: 'Any') -> 'str': + from pcapkit.toolkit.pypcap import packet2chain + + return packet2chain(frame[1], data_link=extractor._exeng._dlink) # type: ignore[attr-defined] + + +@unittest.skipUnless(HAS_RUNTIME, 'runtime dependencies not installed') +class PCAP_CTVerboseTests(_VerboseCases, unittest.TestCase): + ENGINE = 'pcap_ct' + + def chain(self, extractor: 'Extractor', frame: 'Any') -> 'str': + from pcapkit.toolkit.pcap_ct import packet2chain + + return packet2chain(frame[1], data_link=extractor._exeng._dlink) # type: ignore[attr-defined] + + +@unittest.skipUnless(HAS_RUNTIME, 'runtime dependencies not installed') +class PyPCAPFileVerboseTests(_VerboseCases, unittest.TestCase): + ENGINE = 'pypcapfile' + + def chain(self, extractor: 'Extractor', frame: 'Any') -> 'str': + from pcapkit.toolkit.pypcapfile import packet2chain + + return packet2chain(frame, data_link=extractor._exeng._dlink) # type: ignore[attr-defined] + + +if __name__ == '__main__': + unittest.main() diff --git a/tests/foundation/engines/test_pcap_ct_engine.py b/tests/foundation/engines/test_pcap_ct_engine.py index 5ed95a605..f3effa2f2 100644 --- a/tests/foundation/engines/test_pcap_ct_engine.py +++ b/tests/foundation/engines/test_pcap_ct_engine.py @@ -152,6 +152,7 @@ def make_extractor(self, **overrides): '_flag_q': False, '_flag_f': False, '_flag_v': False, + '_vfunc_default': False, '_flag_r': False, '_flag_t': False, '_flag_d': True, @@ -443,7 +444,7 @@ def test_run_leaves_disabled_reassembly_and_tracing_untouched(self) -> None: self.assertIs(extractor._trace, original_trace) def test_run_installs_verbose_handler_reporting_the_raw_chain(self) -> None: - extractor, _ = self.make_extractor(_flag_v=True) + extractor, _ = self.make_extractor(_flag_v=True, _vfunc_default=True) engine = self.engine(extractor) with mock.patch.object(engine._expkg, 'pcap', return_value=FakeHandle()): engine.run() @@ -454,6 +455,16 @@ def test_run_installs_verbose_handler_reporting_the_raw_chain(self) -> None: printer.assert_called_once() self.assertIn('ETHERNET:Raw', printer.call_args.args[0]) + def test_run_keeps_a_callers_verbose_callback(self) -> None: + # #1874: a callable given as ``verbose`` turns verbose output on without + # asking for the default printer, so the engine must not replace it + callback = mock.Mock() + extractor, _ = self.make_extractor(_flag_v=True, _vfunc_default=False, _vfunc=callback) + engine = self.engine(extractor) + with mock.patch.object(engine._expkg, 'pcap', return_value=FakeHandle()): + engine.run() + self.assertIs(extractor._vfunc, callback) + ########################################################################## # read_frame() ########################################################################## diff --git a/tests/foundation/engines/test_pypcap_engine.py b/tests/foundation/engines/test_pypcap_engine.py index b98885882..a1bc94663 100644 --- a/tests/foundation/engines/test_pypcap_engine.py +++ b/tests/foundation/engines/test_pypcap_engine.py @@ -95,6 +95,7 @@ def make_extractor(self, **overrides): '_flag_q': False, '_flag_f': False, '_flag_v': False, + '_vfunc_default': False, '_flag_r': False, '_flag_t': False, '_flag_d': True, @@ -359,7 +360,7 @@ def test_run_leaves_disabled_reassembly_and_tracing_untouched(self) -> None: self.assertIs(extractor._trace, original_trace) def test_run_installs_verbose_handler_reporting_the_raw_chain(self) -> None: - extractor, _ = self.make_extractor(_flag_v=True) + extractor, _ = self.make_extractor(_flag_v=True, _vfunc_default=True) engine = self.engine(extractor) with mock.patch.object(engine._expkg, 'pcap', return_value=FakeHandle()): engine.run() @@ -370,6 +371,16 @@ def test_run_installs_verbose_handler_reporting_the_raw_chain(self) -> None: printer.assert_called_once() self.assertIn('ETHERNET:Raw', printer.call_args.args[0]) + def test_run_keeps_a_callers_verbose_callback(self) -> None: + # #1874: a callable given as ``verbose`` turns verbose output on without + # asking for the default printer, so the engine must not replace it + callback = mock.Mock() + extractor, _ = self.make_extractor(_flag_v=True, _vfunc_default=False, _vfunc=callback) + engine = self.engine(extractor) + with mock.patch.object(engine._expkg, 'pcap', return_value=FakeHandle()): + engine.run() + self.assertIs(extractor._vfunc, callback) + ########################################################################## # read_frame() ########################################################################## diff --git a/tests/foundation/engines/test_pypcapfile_engine.py b/tests/foundation/engines/test_pypcapfile_engine.py index 64a7da1dd..1f36588e0 100644 --- a/tests/foundation/engines/test_pypcapfile_engine.py +++ b/tests/foundation/engines/test_pypcapfile_engine.py @@ -179,6 +179,7 @@ def make_extractor(self, **overrides): '_flag_q': False, '_flag_f': False, '_flag_v': False, + '_vfunc_default': False, '_flag_r': False, '_flag_t': False, '_flag_d': True, @@ -312,7 +313,7 @@ def test_run_survives_the_malformed_upstream_link_layer_table(self) -> None: self.assertIsNone(engine._declf) def test_run_installs_verbose_handler_reporting_the_chain(self) -> None: - extractor, _ = self.make_extractor(_flag_v=True) + extractor, _ = self.make_extractor(_flag_v=True, _vfunc_default=True) engine = self.engine(extractor) engine.run() @@ -323,6 +324,15 @@ def test_run_installs_verbose_handler_reporting_the_chain(self) -> None: printer.assert_called_once() self.assertIn('ETHERNET:Raw', printer.call_args.args[0]) + def test_run_keeps_a_callers_verbose_callback(self) -> None: + # #1874: a callable given as ``verbose`` turns verbose output on without + # asking for the default printer, so the engine must not replace it + callback = mock.Mock() + extractor, _ = self.make_extractor(_flag_v=True, _vfunc_default=False, _vfunc=callback) + engine = self.engine(extractor) + engine.run() + self.assertIs(extractor._vfunc, callback) + ########################################################################## # Records (#1512, #1571). ########################################################################## diff --git a/tests/foundation/engines/test_pyshark_non_packet_unit.py b/tests/foundation/engines/test_pyshark_non_packet_unit.py index 25707b4e7..ebdd21f0b 100644 --- a/tests/foundation/engines/test_pyshark_non_packet_unit.py +++ b/tests/foundation/engines/test_pyshark_non_packet_unit.py @@ -125,7 +125,7 @@ def test_a_trailing_record_ends_the_capture(self) -> None: def test_verbose_mode_prints_a_commented_packet(self) -> None: engine, ext = self._engine([_packet(1, comment=True), _packet(2)]) vars(ext).update(_exlyr='none', _exptl='null', _exctx=None, _flag_r=False, _flag_v=True, - _ifnm='test.pcapng', record_header=lambda: None) + _vfunc_default=True, _ifnm='test.pcapng', record_header=lambda: None) stream = engine._extmp engine._expkg = types.SimpleNamespace(FileCapture=lambda *args, **kwargs: stream) engine.run() # installs the verbose handler @@ -136,6 +136,27 @@ def test_verbose_mode_prints_a_commented_packet(self) -> None: self.assertEqual(out.getvalue().splitlines(), [f'Frame 1: {PROTOCOLS}', f'Frame 2: {PROTOCOLS}']) + def test_a_verbose_callback_is_kept_and_called_per_packet(self) -> None: + # #1874: a callable given as ``verbose`` is not replaced by the printer + first, last = _packet(1), _packet(2) + engine, ext = self._engine([first, last]) + calls = [] # type: list[tuple[object, object]] + vars(ext).update(_exlyr='none', _exptl='null', _exctx=None, _flag_r=False, _flag_v=True, + _vfunc_default=False, _vfunc=lambda e, f: calls.append((e, f)), + _ifnm='test.pcapng', record_header=lambda: None) + stream = engine._extmp + engine._expkg = types.SimpleNamespace(FileCapture=lambda *args, **kwargs: stream) + engine.run() + + with contextlib.redirect_stdout(io.StringIO()) as out: + engine.read_frame() + engine.read_frame() + self.assertEqual(len(calls), 2) + for (called_ext, frame), packet in zip(calls, (first, last)): + self.assertIs(called_ext, ext) + self.assertIs(frame, packet) + self.assertEqual(out.getvalue(), '') + if __name__ == '__main__': unittest.main() diff --git a/tests/foundation/engines/test_runtime_engines.py b/tests/foundation/engines/test_runtime_engines.py index a989cd67e..03b6cd707 100644 --- a/tests/foundation/engines/test_runtime_engines.py +++ b/tests/foundation/engines/test_runtime_engines.py @@ -49,6 +49,7 @@ def make_extractor(**overrides): '_flag_q': False, '_flag_f': False, '_flag_v': False, + '_vfunc_default': False, '_flag_r': True, '_flag_t': True, '_flag_d': True, @@ -225,7 +226,7 @@ def __iter__(self): def __next__(self): return next(self.items) - extractor, sink = make_extractor(_exlyr='link', _flag_v=True) + extractor, sink = make_extractor(_exlyr='link', _flag_v=True, _vfunc_default=True) engine = DPKT(extractor) # A PCAP is read through the engine's :class:`dpkt.pcap.Reader` subclass, # which keeps each record's original length (#1507). @@ -366,7 +367,7 @@ def run(engine: 'Scapy') -> None: sniff.assert_called_once_with(opened_socket={reader: 'capture.pcap'}) reader.close.assert_called_once_with() - extractor, sink = make_extractor(_exptl='tcp', _flag_v=True) + extractor, sink = make_extractor(_exptl='tcp', _flag_v=True, _vfunc_default=True) run(Scapy(extractor)) self.assertTrue(callable(extractor._vfunc)) extractor.record_header.assert_called_once_with() @@ -444,7 +445,7 @@ def close(self) -> None: self.closed = True capture = FakeCapture() - extractor, sink = make_extractor(_exlyr='internet', _flag_v=True) + extractor, sink = make_extractor(_exlyr='internet', _flag_v=True, _vfunc_default=True) engine = PyShark(extractor) with mock.patch.object(engine._expkg, 'FileCapture', return_value=capture) as file_capture: engine.run() diff --git a/tests/foundation/test_extraction.py b/tests/foundation/test_extraction.py index 211aa8952..9e001284d 100644 --- a/tests/foundation/test_extraction.py +++ b/tests/foundation/test_extraction.py @@ -963,6 +963,8 @@ def test_constructor_initialises_without_running_when_patched(self) -> None: run.assert_called_once() self.assertEqual(extractor.input, str(capture)) self.assertTrue(extractor._flag_v) + # ``verbose=True`` asks for the default printer, which an engine may replace (#1874) + self.assertTrue(extractor._vfunc_default) self.assertTrue(extractor._ipv4) self.assertTrue(extractor._ipv6) self.assertTrue(extractor._tcp) @@ -1012,6 +1014,7 @@ def test_constructor_configuration_branches_with_run_patched(self) -> None: run.assert_called_once() self.assertEqual(defaulted._ifnm, str(capture)) self.assertFalse(defaulted._flag_v) + self.assertFalse(defaulted._vfunc_default) defaulted._ifile.close() verbose_handler = mock.Mock() @@ -1022,6 +1025,8 @@ def test_constructor_configuration_branches_with_run_patched(self) -> None: self.assertFalse(streamed._flag_s) self.assertTrue(streamed._flag_v) self.assertIs(streamed._vfunc, verbose_handler) + # a caller's callback is not the default printer, so no engine replaces it (#1874) + self.assertFalse(streamed._vfunc_default) self.assertFalse(hasattr(streamed, '_ofile')) streamed._ifile.close() From 657ef260fbeefb0fc4f5c88cb3ca69ccccc70e91 Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Fri, 16 Oct 2026 20:46:51 -0400 Subject: [PATCH 6/6] docs(foundation): teach custom engines to keep a user's verbose callback (#1874) - The custom-engine example in ext.rst tests `ext._vfunc_default`, not `ext._flag_v`, and says why: a `verbose=` callable must be kept. - Document `Extractor._vfunc_default` on the extraction page. - Run the verbose-callback module on every engine-tests leg, and update the tier guard test that pins the PyShark leg's TEST_PATHS line. Refs #1874 --- .github/workflows/unit-tests.yml | 10 +++++----- docs/source/ext.rst | 8 ++++++-- docs/source/pcapkit/foundation/extraction.rst | 1 + tests/test_tier_guard.py | 6 ++++-- 4 files changed, 16 insertions(+), 9 deletions(-) diff --git a/.github/workflows/unit-tests.yml b/.github/workflows/unit-tests.yml index 0cf7a2b0e..fbcfe482a 100644 --- a/.github/workflows/unit-tests.yml +++ b/.github/workflows/unit-tests.yml @@ -1030,11 +1030,11 @@ jobs: fi case "$ENGINE" in - Scapy) TEST_PATHS="tests/toolkit/test_scapy_unit.py tests/foundation/engines/test_scapy_engine.py tests/protocols/transport/test_sctp_unit.py" ;; - PyShark) TEST_PATHS="tests/toolkit/test_pyshark_unit.py tests/foundation/engines/test_pyshark_engine.py" ;; - PyPCAPFile) TEST_PATHS="tests/toolkit/test_pypcapfile_unit.py tests/foundation/engines/test_pypcapfile_engine.py" ;; - PyPCAP) TEST_PATHS="tests/toolkit/test_pypcap_unit.py tests/foundation/engines/test_pypcap_engine.py tests/foundation/engines/test_pcap_backend.py" ;; - PCAP_CT) TEST_PATHS="tests/toolkit/test_pcap_ct_unit.py tests/foundation/engines/test_pcap_ct_engine.py tests/foundation/engines/test_pcap_backend.py" ;; + Scapy) TEST_PATHS="tests/toolkit/test_scapy_unit.py tests/foundation/engines/test_scapy_engine.py tests/protocols/transport/test_sctp_unit.py tests/foundation/engines/test_engine_verbose_callback_1874_unit.py" ;; + PyShark) TEST_PATHS="tests/toolkit/test_pyshark_unit.py tests/foundation/engines/test_pyshark_engine.py tests/foundation/engines/test_engine_verbose_callback_1874_unit.py" ;; + PyPCAPFile) TEST_PATHS="tests/toolkit/test_pypcapfile_unit.py tests/foundation/engines/test_pypcapfile_engine.py tests/foundation/engines/test_engine_verbose_callback_1874_unit.py" ;; + PyPCAP) TEST_PATHS="tests/toolkit/test_pypcap_unit.py tests/foundation/engines/test_pypcap_engine.py tests/foundation/engines/test_pcap_backend.py tests/foundation/engines/test_engine_verbose_callback_1874_unit.py" ;; + PCAP_CT) TEST_PATHS="tests/toolkit/test_pcap_ct_unit.py tests/foundation/engines/test_pcap_ct_engine.py tests/foundation/engines/test_pcap_backend.py tests/foundation/engines/test_engine_verbose_callback_1874_unit.py" ;; esac echo "Running: $TEST_PATHS" # shellcheck disable=SC2086 diff --git a/docs/source/ext.rst b/docs/source/ext.rst index 919fcaee8..8d459e9ee 100644 --- a/docs/source/ext.rst +++ b/docs/source/ext.rst @@ -512,9 +512,13 @@ The following code snippet shows how to create a new engine class: # check if there's any extraction protocol and/or layer thresholds # set by the user (some engines may not support this feature); or # to set up the verbose handler for the underlying engine module, - # as in the following code snippet. + # as in the following code snippet. Install it only when the user + # asked for the default output, i.e., `verbose=True`, which sets + # `ext._vfunc_default`: a callable given as `verbose=` is the + # user's own handler, already stored as `ext._vfunc`, so it must + # be kept rather than replaced. - if ext._flag_v: # check if verbose mode is enabled + if ext._vfunc_default: # check if the default verbose output is requested # NOTE: PyPCAPKit has its own bundled supporting functions for # third-party libraries, e.g., the `packet2chain` function is # to convert a given Packet instance of the Scapy library to a diff --git a/docs/source/pcapkit/foundation/extraction.rst b/docs/source/pcapkit/foundation/extraction.rst index b589790cc..bd5fc4619 100644 --- a/docs/source/pcapkit/foundation/extraction.rst +++ b/docs/source/pcapkit/foundation/extraction.rst @@ -89,6 +89,7 @@ and extracts parameters from a PCAP file. .. autoattribute:: _eof_mark .. autoattribute:: _vfunc + .. autoattribute:: _vfunc_default .. autoattribute:: _exnam .. autoattribute:: _exeng diff --git a/tests/test_tier_guard.py b/tests/test_tier_guard.py index c30f3e729..07fd55cab 100644 --- a/tests/test_tier_guard.py +++ b/tests/test_tier_guard.py @@ -2410,7 +2410,8 @@ def test_a_commented_out_test_paths_arm_does_not_silently_win(self) -> None: """ live = ('PyShark) TEST_PATHS="tests/toolkit/test_pyshark_unit.py ' - 'tests/foundation/engines/test_pyshark_engine.py" ;;') + 'tests/foundation/engines/test_pyshark_engine.py ' + 'tests/foundation/engines/test_engine_verbose_callback_1874_unit.py" ;;') doctored = doctored_workflow( self, live, @@ -2422,7 +2423,8 @@ def test_a_commented_out_test_paths_arm_does_not_silently_win(self) -> None: pyshark_jobs = [job for job in jobs if job.name == 'engine-tests' and 'PyShark' in job.extras] self.assertEqual(len(pyshark_jobs), 1) self.assertEqual(pyshark_jobs[0].paths, ('tests/toolkit/test_pyshark_unit.py', - 'tests/foundation/engines/test_pyshark_engine.py')) + 'tests/foundation/engines/test_pyshark_engine.py', + 'tests/foundation/engines/test_engine_verbose_callback_1874_unit.py')) def test_the_undoctored_workflow_produces_no_unexplained_gap(self) -> None: """The control: the tests above fail for the doctoring, not by default."""