From b5f5fb1d88f322b4ff5f0a541c85620d51f9aabc Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Thu, 15 Oct 2026 12:57:28 -0400 Subject: [PATCH] fix(vendor): make the public-API sweep cover pcapkit.vendor (#1772) tests/project/test_public_api.py skipped pcapkit.vendor through EXCLUDED_ROOTS for want of requests and bs4, which the test extra has carried since #507. Dropping it failed the package converse check twice: - pcapkit.vendor.ipx leaked `path`, its sys.path temporary: now `_path`. - pcapkit.vendor's Vendor, ModuleNotFoundError, stacklevel, VendorWarning and warn go in DELIBERATE_NON_EXPORTS, not __all__, which pcapkit.vendor.__main__ runs as its default crawler list (now commented). EXCLUDED_ROOTS is empty and the walk pins pcapkit.vendor and its ipx package. The docstring drops the stale exclusion and "one gap remains" (#531) paragraphs; test_crawler_reachability_unit's no longer calls vendor excluded. tests/project: 930 passed, 1 skipped. Closes #1772 --- pcapkit/vendor/__init__.py | 4 + pcapkit/vendor/ipx/__init__.py | 7 +- tests/project/test_public_api.py | 102 +++++++++--------- .../vendor/test_crawler_reachability_unit.py | 17 +-- 4 files changed, 71 insertions(+), 59 deletions(-) diff --git a/pcapkit/vendor/__init__.py b/pcapkit/vendor/__init__.py index 957e90f98d..9e77892704 100644 --- a/pcapkit/vendor/__init__.py +++ b/pcapkit/vendor/__init__.py @@ -55,6 +55,10 @@ from pcapkit.vendor.tcp import * from pcapkit.vendor.vlan import * +# Crawlers only. This list is also the default target list of +# :mod:`pcapkit.vendor.__main__`, which runs every name in it, so the base +# class ``Vendor`` and the warning machinery imported above stay out of it -- +# see ``DELIBERATE_NON_EXPORTS`` in :file:`tests/project/test_public_api.py`. __all__ = [ # Protocol Registration 'EtherType', 'LinkType', 'TransType', 'AppType', diff --git a/pcapkit/vendor/ipx/__init__.py b/pcapkit/vendor/ipx/__init__.py index 15570e0b65..28fe01ee54 100644 --- a/pcapkit/vendor/ipx/__init__.py +++ b/pcapkit/vendor/ipx/__init__.py @@ -24,14 +24,17 @@ ############################################################################### import sys -path = sys.path.pop(0) +# Private: ``__all__`` below is the ``--target ipx`` crawler list that +# :mod:`pcapkit.vendor.__main__` builds, so this module-level temporary must not +# read as a public name of the package. +_path = sys.path.pop(0) ############################################################################### from pcapkit.vendor.ipx.packet import Packet as IPX_Packet from pcapkit.vendor.ipx.socket import Socket as IPX_Socket ############################################################################### -sys.path.insert(0, path) +sys.path.insert(0, _path) ############################################################################### __all__ = ['IPX_Packet', 'IPX_Socket'] diff --git a/tests/project/test_public_api.py b/tests/project/test_public_api.py index f2a4bf80a2..e1af71309f 100644 --- a/tests/project/test_public_api.py +++ b/tests/project/test_public_api.py @@ -35,31 +35,28 @@ Two scoping decisions, both measured rather than assumed. -**:mod:`pcapkit.vendor` is excluded.** Its modules ``import requests`` and -``import bs4`` at module scope. The exclusion was written on the premise that -``pip install -e '.[test]'`` -- the install the unit tier, see -:mod:`tests._tiers`, has to pass on -- lacks both, but the ``test`` extra in -:file:`pyproject.toml` has carried ``requests`` and ``beautifulsoup4`` since -#507, so the import is not what keeps it. With ``'vendor'`` dropped from -:data:`EXCLUDED_ROOTS`, every assertion passes over the vendor subtree bar one: -:meth:`~PublicAPISurfaceTests.test_every_public_package_exports_every_public_attribute` -fails for :mod:`pcapkit.vendor`, which holds ``ModuleNotFoundError``, -``Vendor``, ``VendorWarning``, ``stacklevel`` and ``warn`` without listing them, -and for :mod:`pcapkit.vendor.ipx`, which holds ``path``. #1772 tracks those -gaps. When this module was written the same sweep was run by hand over -:mod:`pcapkit.vendor` and its 134 public descendants, and the single defect it -found was the duplicate described at the end of this docstring, in -:mod:`pcapkit.vendor` and :mod:`pcapkit.vendor.mh`. - -Nothing else is excluded, and no import is wrapped in a skip. That is deliberate -and also measured: with ``dpkt``, ``scapy``, ``emoji``, ``cryptography`` and -``pycrate`` all absent, every one of the 325 non-vendor modules there were when -this module was written still imported, because each optional dependency is -guarded at its import site -- e.g. +**Nothing is excluded, :mod:`pcapkit.vendor` included.** The vendor crawlers +``import requests`` and ``import bs4`` at module scope, and the subtree used to be +skipped through :data:`EXCLUDED_ROOTS` on the grounds that those came only from +the ``vendor`` extra: this module is unit-tier -- see :mod:`tests._tiers` -- so it +has to pass on ``pip install -e '.[test]'``. Both were already in the ``test`` +extra, since #507 (:file:`pyproject.toml`), so the skip never bought anything. +Dropping it (#1772) brought in 144 modules and 19 packages, and found two +packages holding public attributes their :attr:`__all__` omitted: +:mod:`pcapkit.vendor.ipx` leaked ``path``, the temporary of its ``sys.path`` +save-and-restore, now named ``_path``; :mod:`pcapkit.vendor`'s five are recorded +in :data:`DELIBERATE_NON_EXPORTS`. + +No import is wrapped in a skip, either. That is deliberate and also measured: with +``dpkt``, ``scapy``, ``emoji``, ``cryptography`` and ``pycrate`` all absent, every +one of the 325 non-vendor modules there were when this module was written still +imported, because each optional dependency is guarded at its import site -- e.g. ``pcapkit/toolkit/scapy.py`` catches :exc:`ModuleNotFoundError` and degrades to -``scapy = None`` with a warning. So an import failure in this sweep is a real -defect and is allowed to fail the test, rather than being softened into a skip -that would let the next :attr:`__all__` bug through. +``scapy = None`` with a warning. The vendor modules need nothing past the ``test`` +extra at import time: the two NGAP crawlers import ``pycrate`` only inside the +method that reads it. So an import failure in this sweep is a real defect and is +allowed to fail the test, rather than being softened into a skip that would let +the next :attr:`__all__` bug through. **The converse -- every public attribute appears in :attr:`__all__` -- holds for every public *package*, against a written-down allowlist of exclusions.** That is @@ -86,7 +83,8 @@ :attr:`__all__` literal that forgot them and imported by name from six modules, and the three decorators :mod:`pcapkit.utilities` then advertised in its own module docstring (since moved to :mod:`pcapkit.corekit.decorators` and dropped from that -package by #1735). The 22 that remain are listed with their reasons, and +package by #1735). The 22 that remain are listed with their reasons, as are the +five :mod:`pcapkit.vendor` added once it joined the sweep (#1772), and :meth:`~PublicAPISurfaceTests.test_the_non_export_allowlist_is_tight` stops the list rotting into a silencer: every entry has to name a package that exists, an attribute that package really holds, and a name its :attr:`__all__` really omits, @@ -129,19 +127,13 @@ class exists at ``pcapkit/const/mh/handover_initiate_status.py`` and is used by :meth:`~PublicAPISurfaceTests.test_every_all_entry_resolves`, because the name it repeats does resolve -- only counting the entries finds it. -One gap remains, and it is recorded rather than closed because it lies outside -this module's reach. :mod:`pcapkit.vendor` is excluded from the sweep for the -dependency reason above, and it carried the *same* displaced-duplicate defect in -``pcapkit/vendor/__init__.py`` and ``pcapkit/vendor/mh/__init__.py``. That one was -worse than its :mod:`pcapkit.const` twin rather than merely parallel: -``pcapkit/vendor/__main__.py`` builds its crawler target list out of -``pcapkit.vendor.__all__``, so the missing ``MH_HandoverInitiateStatus`` kept the -``handover_initiate_status`` crawler out of the default target list *and* out of -``--target mh``, and it had never run. #531 fixed it, and -:mod:`tests.vendor.test_crawler_reachability_unit` guards it. Dropping -``'vendor'`` from :data:`EXCLUDED_ROOTS` would let this module cover it too, but -only once the two :attr:`__all__` gaps named above are closed or recorded in -:data:`DELIBERATE_NON_EXPORTS`; #1772 tracks that. +:mod:`pcapkit.vendor` carried the *same* displaced duplicate, in +``pcapkit/vendor/__init__.py`` and ``pcapkit/vendor/mh/__init__.py``, and there it +was worse: ``pcapkit/vendor/__main__.py`` builds its crawler target lists out of +those :attr:`__all__` lists, so the ``handover_initiate_status`` crawler had never +run. #531 fixed it while the subtree was still excluded, and +:mod:`tests.vendor.test_crawler_reachability_unit` guards it; since #1772 these +assertions cover that copy too. """ from __future__ import annotations @@ -152,11 +144,12 @@ class exists at ``pcapkit/const/mh/handover_initiate_status.py`` and is used by import types import unittest -#: First path component under ``pcapkit.`` whose subtree the sweep skips. -#: :mod:`pcapkit.vendor` imports, since the ``test`` extra carries ``requests`` -#: and ``beautifulsoup4``, but two of its packages leave public attributes out -#: of :attr:`__all__` (#1772); see the module docstring. -EXCLUDED_ROOTS = frozenset({'vendor'}) +#: First path component under ``pcapkit.`` whose subtree the sweep skips. Empty: +#: it held ``'vendor'`` until #1772, though ``requests`` and ``beautifulsoup4`` +#: were in the ``test`` extra before the exclusion was written, and +#: :meth:`~PublicAPISurfaceTests.test_public_modules_were_actually_found` pins +#: :mod:`pcapkit.vendor` in the walk so that restoring it fails. +EXCLUDED_ROOTS = frozenset() # type: frozenset[str] #: The packages whose whole job is to re-export their children, and which #: therefore have to list every public name they hold -- with no allowlist, unlike @@ -183,6 +176,9 @@ class exists at ``pcapkit/const/mh/handover_initiate_status.py`` and is used by #: nor :mod:`pcapkit.corekit` exports any of them either. Three independent export #: surfaces agreeing is a convention rather than nine separate oversights, so they #: stay reachable by their documented dotted path and out of ``import *``. +#: :mod:`pcapkit.vendor` imports its base ``Vendor`` the same way, and has a +#: sharper reason: its :attr:`__all__` is also the default target list +#: :mod:`pcapkit.vendor.__main__` runs, so exporting the base would run it. #: #: **Names another module's import dragged in** -- ``Info`` and ``info_final`` #: belong to :mod:`pcapkit.corekit.infoclass`, which is where they are documented @@ -197,10 +193,14 @@ class exists at ``pcapkit/const/mh/handover_initiate_status.py`` and is used by #: :mod:`pcapkit.utilities`. ``ROOT`` is bound only on the non-``DEVMODE`` branch, #: so exporting it would make ``from pcapkit import *`` raise #: :exc:`AttributeError` under ``PCAPKIT_DEVMODE=1`` -- a fresh #515 rather than a -#: fix for one. ``pcapkit.protocols.name`` is the loop variable of the -#: ``for name in __all__`` that builds ``__proto__``, left behind in the namespace; -#: renaming it ``_name`` would express that better than this entry does, but that -#: is a change to a module body rather than to an export list. +#: fix for one. :mod:`pcapkit.vendor` likewise imports ``ModuleNotFoundError``, +#: ``stacklevel``, ``VendorWarning`` and ``warn`` only to warn when ``requests`` +#: or ``bs4`` is missing; they are :mod:`pcapkit.utilities`' names, and listing +#: them would hand them to that same crawler loop. ``pcapkit.protocols.name`` is +#: the loop variable of the ``for name in __all__`` that builds ``__proto__``, +#: left behind in the namespace; renaming it ``_name`` would express that better +#: than this entry does, but that is a change to a module body rather than to an +#: export list. DELIBERATE_NON_EXPORTS = { 'pcapkit': ('BaseError', 'DEVMODE', 'DevModeWarning', 'ROOT'), 'pcapkit.corekit.fields': ('Field',), @@ -214,6 +214,8 @@ class exists at ``pcapkit/const/mh/handover_initiate_status.py`` and is used by 'pcapkit.protocols.internet': ('Internet',), 'pcapkit.protocols.link': ('Link',), 'pcapkit.protocols.transport': ('Transport',), + 'pcapkit.vendor': ('ModuleNotFoundError', 'Vendor', 'VendorWarning', + 'stacklevel', 'warn'), } @@ -284,7 +286,8 @@ def test_public_modules_were_actually_found(self) -> None: # a silent pass over an empty list, so pin the shape of what it returns. # The floor is deliberately far below the 325 modules / 52 packages found # when this was written: it catches "found nothing", not "the library - # gained a module". + # gained a module". The two vendor packages are pinned because the subtree + # was excluded until #1772, and both held __all__ gaps the exclusion hid. modules = public_modules() packages = [name for name, is_package in modules if is_package] @@ -293,7 +296,8 @@ def test_public_modules_were_actually_found(self) -> None: self.assertGreater(len(packages), 40, 'the module walk found implausibly few packages') for name in ('pcapkit', 'pcapkit.protocols.data', 'pcapkit.protocols.schema', - 'pcapkit.foundation.registry', 'pcapkit.const.mh'): + 'pcapkit.foundation.registry', 'pcapkit.const.mh', + 'pcapkit.vendor', 'pcapkit.vendor.ipx'): self.assertIn((name, True), modules, f'{name} missing from the walk') self.assertFalse([name for name, _ in modules if _is_excluded(name)], 'the walk yielded a module it was supposed to exclude') diff --git a/tests/vendor/test_crawler_reachability_unit.py b/tests/vendor/test_crawler_reachability_unit.py index 17f9556e3f..42d37cb4e0 100644 --- a/tests/vendor/test_crawler_reachability_unit.py +++ b/tests/vendor/test_crawler_reachability_unit.py @@ -42,14 +42,15 @@ Relationship to :file:`tests/project/test_public_api.py` (GitHub pull request #527). That module sweeps :attr:`__all__` across the tree for a related family of -defects and excludes :mod:`pcapkit.vendor` via its ``EXCLUDED_ROOTS``; its -docstring records why, and #1772 tracks dropping the exclusion. This module is a -separate file rather than that edit, for two reasons: #527 had not merged when -this module was written, so its file did not exist on that branch; and the -assertion here is not the one it makes. The property it checks -- defined versus -reachable -- is about the crawler *registry* rather than about :attr:`__all__` -hygiene. Dropping the exclusion is still worth doing on top of this, and would -give the duplicate a second, cheaper detector. +defects. This module is a separate file for two reasons: #527 had not merged +when #531 added this one, so there was no file to extend; and the assertion here +is not the one it makes. The property checked here -- defined versus reachable -- +is about the crawler *registry* rather than about :attr:`__all__` hygiene. #527 +also skipped :mod:`pcapkit.vendor` through ``EXCLUDED_ROOTS``, saying the unit +tier lacked the ``vendor`` extra, though ``requests`` and ``bs4`` were already in +the ``test`` extra (#507). #1772 dropped that exclusion, so its +``test_no_duplicate_all_entries`` now gives the duplicate a second, cheaper +detector. The suite is unit-tier (see :mod:`tests._tiers`): it reads no capture, and it instantiates nothing. That last part is load-bearing rather than incidental.