diff --git a/.github/workflows/unit-tests.yml b/.github/workflows/unit-tests.yml index ad97043b7c..b2e0217c7a 100644 --- a/.github/workflows/unit-tests.yml +++ b/.github/workflows/unit-tests.yml @@ -274,6 +274,59 @@ jobs: # pcapkit/foundation/engines/_pcap_backend.py's own module docstring. That is # why PyPCAP is not in this job's install line at all; it gets the # `pypcap-parity` job below, entirely to itself. + # + # `vendor` joins the install line for #738's remaining gap, HAS_VENDOR_DEPS + # (41 methods across tests/vendor/test_request_prompt_unit.py, + # test_user_agent_unit.py, test_ipx_packet_unit.py and + # test_ftp_return_code_unit.py) -- one requirement extra short of the `test` + # extra's own requests+bs4 (#507): html5lib, which only + # beautifulsoup4[html5lib] provides. #738's ruling was to land this on a + # non-blocking leg rather than add it to `test`, following the precedent of + # 3.15 in python-compatibility.yml (b7f51401b) -- a job that exists and + # reports without being one of ruleset 23497679's 15 required checks. This + # job already is exactly that leg: "Engines Python X" is not among the 15 + # (Python/Integration Python/Compat Python 3.10-3.14), so adding the extra + # here, rather than opening a new job, closes the gap without a sixth matrix + # to maintain. Its selection mirrors `test`'s ignore-shape, which is what + # already reaches these gates dark -- see this job's own comment above and + # tests/_dependency_gates.py's HAS_VENDOR_DEPS exclusion. It is also the + # only *per-pull-request* job with that property: `gate` reaches these + # gates too but never runs on a pull request at all (only via a release's + # `gate-only: true` call), and `integration`'s fixture-tier selection never + # reaches tests/vendor/ in the first place -- so this was the forced + # choice, not merely a convenient one. + # + # Worth recording since it bears on the ruling: none of the 41 methods this + # closes make a live network call, though not for one reason across the + # four files. + # + # test_user_agent_unit.py (11 methods) and test_request_prompt_unit.py (16) + # gate on HAS_VENDOR_DEPS only because pcapkit.vendor's package import + # chain needs requests/bs4/html5lib importable -- html5lib itself is never + # used by either file's test bodies, both of which replace requests.get + # with a recorder for the whole of every case (each file's own module + # docstring says so in its own words, not a shared one). + # + # test_ipx_packet_unit.py (9 methods) is a regression suite for a + # *retired* scrape: pcapkit.vendor.ipx.packet.Packet.LINK is None, so + # _request() short-circuits before ever calling requests.get, and + # test_request_makes_no_network_call asserts exactly that by making + # requests.get and requests.Session.request raise if reached at all. + # html5lib is an import precondition here too, not something the test + # bodies use. + # + # test_ftp_return_code_unit.py (5 methods) is not retired -- + # pcapkit.vendor.ftp.return_code.ReturnCode.LINK is a live Wikipedia URL -- + # and is the one file where html5lib is a *functional* dependency rather + # than an import precondition: its tests build the HTML fixture inline and + # hand it straight to ReturnCode.request(), which is + # bs4.BeautifulSoup(text, 'html5lib'). What keeps it off the network is the + # fixture being inline, not an absence of html5lib use. + # + # The #518 Wikipedia-403 flakiness the ruling was guarding against belongs + # to the crawlers HAS_CRAWLER_DEPS gates (live, unconditionally, since + # #507), not to any of the above -- but the leg placement follows the + # ruling as recorded rather than re-opening it here. engine-tests: name: Engines Python ${{ matrix.python-version }} if: ${{ inputs.gate-only != true }} @@ -332,7 +385,7 @@ jobs: - name: Install package and per-engine test dependencies run: | python -m pip install -U pip setuptools wheel - python -m pip install -e '.[test,DPKT,crypto,NGAP,Scapy,PyShark,PyPCAPFile,PCAP_CT]' + python -m pip install -e '.[test,DPKT,crypto,NGAP,Scapy,PyShark,PyPCAPFile,PCAP_CT,vendor]' # See the `test` job above for why this step exists. - name: Report available parallelism diff --git a/pyproject.toml b/pyproject.toml index f07d6352b3..a5bd2c3f15 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -244,8 +244,8 @@ test = [ # Without these, ``tests/vendor/test_ipx_socket_unit.py`` reports "9 skipped" # rather than failing, so the byte-for-byte regeneration check that is the # only guard on the hand-maintained IPX socket registry never runs on any CI - # path: the ``unit-tests.yml`` jobs install ``.[test]``, and - # ``cron-vendor.yml`` installs ``.[all]`` but never invokes pytest. + # path: every ``unit-tests.yml`` job's install line builds on ``.[test]``, + # and ``cron-vendor.yml`` installs ``.[all]`` but never invokes pytest. # # Both are needed, not just ``requests``. ``pcapkit.vendor.default`` imports # ``requests`` at module scope, and ``pcapkit.vendor.__init__`` then imports @@ -254,9 +254,11 @@ test = [ # ``ModuleNotFoundError`` that the crawler imports below it raise anyway. # # Plain, without the ``vendor`` extra's ``[socks]`` and ``[html5lib]``: - # nothing in the test suite goes through a proxy, and no unit-tier test - # parses HTML (none of them may -- the tier makes no network call), so - # neither parser nor proxy support is reachable from here. Their absence + # nothing in the test suite goes through a proxy, and the one unit-tier + # test that parses HTML (``tests/vendor/test_ftp_return_code_unit.py``) + # does so from an inline fixture with the ``vendor`` extra's own + # ``html5lib``, not through anything this extra provides, so neither + # parser nor proxy support is reachable from here. Their absence # makes :mod:`pcapkit.vendor` log two ``VendorWarning``s at import, which # pytest captures and shows only on failure; they change no result. See #507. "requests", diff --git a/tests/_dependency_gates.py b/tests/_dependency_gates.py index dd0c57632f..a6c750abcf 100644 --- a/tests/_dependency_gates.py +++ b/tests/_dependency_gates.py @@ -313,25 +313,69 @@ class Exclusion(NamedTuple): ), ), 'HAS_VENDOR_DEPS': Exclusion( - dark={'test': ('html5lib',), 'gate': ('html5lib',), 'engine-tests': ('html5lib',)}, + dark={'test': ('html5lib',), 'gate': ('html5lib',)}, reason=( - "Ruled onto a non-blocking leg by #738's option (b), because the crawlers " - 'fetch from IANA and Wikipedia and #518 records four Wikipedia 403s and a ' - 'dead IETF URL -- so a blocking leg would go red on upstream availability ' - 'rather than on this code. That leg is #738\'s own remaining scope and does ' - 'not exist yet. Note what is actually missing, which #738 gets wrong: ' - 'requests and bs4 have shipped in the test extra since #507 and are ' - 'installed on every job, so these five classes are one *requirement extra* ' - 'short -- html5lib, which only beautifulsoup4[html5lib] provides, i.e. the ' - 'vendor and all extras.\n\n' - "engine-tests (#751) joins test and gate here rather than closing the gap: " - "its selection mirrors test's ignore-shape exactly (same --ignore flags), so " - 'it reaches the same unit-tier HAS_VENDOR_DEPS gates and is dark on html5lib ' - 'for the identical reason test is. pypcap-parity does not appear here even ' - 'though it also declines html5lib, because its fixture-tier selection never ' - "reaches tests/vendor/test_ipx_socket_unit.py in the first place -- same " - 'shape as integration above it, which is why integration is not listed ' - 'either.' + "Ruled onto a non-blocking leg by #738's option (b), on the premise that the " + 'crawlers fetch from IANA and Wikipedia and #518 records four Wikipedia 403s ' + 'and a dead IETF URL -- so a blocking leg would go red on upstream ' + 'availability rather than on this code. Note what is actually missing, which ' + "#738's own inventory gets wrong: requests and bs4 have shipped in the test " + 'extra since #507 and are installed on every job, so these classes are one ' + '*requirement extra* short -- html5lib, which only beautifulsoup4[html5lib] ' + 'provides, i.e. the vendor and all extras.\n\n' + 'engine-tests (#751) is that leg. It already mirrors test\'s ignore-shape ' + 'exactly (same --ignore flags), so it already reached these unit-tier ' + 'HAS_VENDOR_DEPS gates; adding vendor to its install line is what closes ' + 'them, and #738\'s own precedent for a job that "exists and reports" without ' + "gating a merge -- 3.15 in python-compatibility.yml (b7f51401b) -- already " + 'describes this job: "Engines Python X" has never been one of ruleset ' + "23497679's 15 required checks, so it was already the non-blocking leg the " + 'ruling asked for, and no new job was needed to get one. It is also the only ' + '*per-pull-request* job with that property: gate reaches these gates too but ' + "never runs on a pull request at all (only via a release's " + "gate-only: true call), and integration's fixture-tier selection never " + 'reaches tests/vendor/ in the first place -- so engine-tests was the forced ' + 'choice, not merely a convenient one.\n\n' + 'test and gate stay dark for different reasons, not the same one. test ' + 'declines html5lib per the non-blocking-leg ruling itself -- that is the ' + 'whole reason it is dark. gate is not part of the blocking matrix at all -- ' + "it only runs on the release path (workflow_call's gate-only: true), never " + 'on a pull request -- so the ruling does not require it dark; nothing about ' + '"non-blocking" would stop vendor being added there too. It stays dark on a ' + 'separate, substantive ground instead: by the time a commit reaches the ' + 'release path it has already run test, integration and engine-tests, and ' + 'engine-tests already carries vendor, so gate would be re-verifying coverage ' + 'that already ran rather than adding any. Gaining a network-and-parser ' + 'extra on the one job that gates an actual release, for coverage the release ' + 'path already has by the time it runs, is not worth the footprint.\n\n' + 'Worth recording since it bears on the ruling itself, though this exclusion ' + 'is not the place to relitigate it: none of the 41 methods this closes make ' + 'a live network call, though not for one reason across all four files. ' + 'test_user_agent_unit.py (11 methods) and test_request_prompt_unit.py (16) ' + 'gate on HAS_VENDOR_DEPS only because the pcapkit.vendor package import ' + 'chain needs requests/bs4/html5lib importable -- html5lib itself is never ' + "used by either file's test bodies, both of which replace requests.get with " + "a recorder for the whole of every case (each file's own module docstring " + 'says so in its own words, not a shared one). test_ipx_packet_unit.py (9 ' + 'methods) is a regression suite for a *retired* scrape -- Packet.LINK is ' + 'None, so _request() short-circuits before ever calling requests.get, and ' + 'test_request_makes_no_network_call asserts exactly that by making ' + 'requests.get and requests.Session.request raise if reached at all; ' + 'html5lib is an import precondition there too, nothing in the file uses it. ' + 'test_ftp_return_code_unit.py (5 methods) is not retired -- ' + 'pcapkit.vendor.ftp.return_code.ReturnCode.LINK is a live Wikipedia URL -- ' + 'and is the one file where html5lib is a *functional* dependency rather ' + 'than an import precondition: its tests build the HTML fixture inline and ' + "hand it straight to ReturnCode.request(), which is " + "bs4.BeautifulSoup(text, 'html5lib'); what keeps it off the network is the " + 'fixture being inline, not an absence of html5lib use. ' + "#518's flakiness is real for HAS_CRAWLER_DEPS's live crawlers, which run " + 'unconditionally since #507; it does not describe any of the above. ' + 'pypcap-parity does not appear here even though it also declines html5lib, ' + 'because its fixture-tier selection never reaches ' + 'tests/vendor/test_ipx_packet_unit.py or its three siblings in the first ' + 'place -- same shape as integration above it, which is why integration is ' + 'not listed either.' ), ), 'HAS_SCAPY': Exclusion( diff --git a/tests/test_tier_guard.py b/tests/test_tier_guard.py index c283833ea4..25d36ba64a 100644 --- a/tests/test_tier_guard.py +++ b/tests/test_tier_guard.py @@ -1483,6 +1483,36 @@ def test_the_crawler_dependencies_are_satisfied_by_the_test_extra(self) -> None: self.assertNotIn('HAS_CRAWLER_DEPS', _dependency_gates.DEPENDENCY_GATE_EXCLUSIONS) self.assertIn('HAS_VENDOR_DEPS', _dependency_gates.DEPENDENCY_GATE_EXCLUSIONS) + def test_the_vendor_extra_closes_engine_tests_but_not_test_or_gate(self) -> None: + """#738's remaining scope: an existing non-blocking leg absorbs it. + + ``engine-tests`` (#751) already reaches these gates through the same + ignore-shape selection as ``test`` -- see this guard's own + ``HAS_VENDOR_DEPS`` exclusion -- and it has never been one of ruleset + 23497679's 15 required checks, so it already was the "job that exists + and reports without gating a merge" #738's ruling asked for. Installing + ``vendor`` there, rather than opening a sixth job, is what closes the + 41-method gap on it. It is also the only *per-pull-request* job with + that property: ``gate`` reaches these gates too but never runs on a + pull request at all (only via a release's ``gate-only: true`` call), + and ``integration``'s fixture-tier selection never reaches + :file:`tests/vendor/` in the first place -- so ``engine-tests`` was + the forced choice, not merely a convenient one. + + ``test`` and ``gate`` stay dark for different reasons, not the same + one -- see the exclusion's own reason for why ``gate`` is not merely + "also non-blocking". + + """ + gaps = {(gap.flag, gap.job) for gap in _dependency_gates.dependency_gate_gaps()} + self.assertNotIn(('HAS_VENDOR_DEPS', 'engine-tests'), gaps) + self.assertIn(('HAS_VENDOR_DEPS', 'test'), gaps) + self.assertIn(('HAS_VENDOR_DEPS', 'gate'), gaps) + + exclusion = _dependency_gates.DEPENDENCY_GATE_EXCLUSIONS['HAS_VENDOR_DEPS'] + self.assertNotIn('engine-tests', exclusion.dark) + self.assertEqual(set(exclusion.dark), {'test', 'gate'}) + def test_each_exclusion_still_describes_a_gap_that_is_really_there(self) -> None: """The anti-rot half, and the reason this is an allowlist and not a skip list. @@ -1804,8 +1834,10 @@ def test_swapping_engine_tests_to_pypcap_is_an_ambiguous_satisfaction(self) -> N """ doctored = doctored_workflow( self, - "python -m pip install -e '.[test,DPKT,crypto,NGAP,Scapy,PyShark,PyPCAPFile,PCAP_CT]'", - "python -m pip install -e '.[test,DPKT,crypto,NGAP,Scapy,PyShark,PyPCAPFile,PyPCAP]'", + "python -m pip install -e '.[test,DPKT,crypto,NGAP,Scapy,PyShark,PyPCAPFile," + "PCAP_CT,vendor]'", + "python -m pip install -e '.[test,DPKT,crypto,NGAP,Scapy,PyShark,PyPCAPFile," + "PyPCAP,vendor]'", ) gaps = {(gap.flag, gap.job) for gap in _dependency_gates.dependency_gate_gaps(doctored)} @@ -1873,8 +1905,10 @@ def test_removing_the_negation_or_the_exact_path_reopens_762(self) -> None: ) doctored_engine_tests = doctored_workflow( self, - "python -m pip install -e '.[test,DPKT,crypto,NGAP,Scapy,PyShark,PyPCAPFile,PCAP_CT]'", - "python -m pip install -e '.[test,DPKT,crypto,NGAP,Scapy,PyShark,PyPCAPFile,PyPCAP]'", + "python -m pip install -e '.[test,DPKT,crypto,NGAP,Scapy,PyShark,PyPCAPFile," + "PCAP_CT,vendor]'", + "python -m pip install -e '.[test,DPKT,crypto,NGAP,Scapy,PyShark,PyPCAPFile," + "PyPCAP,vendor]'", ) with self.subTest(mutation='flag_exclusions stubbed to report nothing'): diff --git a/tests/vendor/test_ftp_return_code_unit.py b/tests/vendor/test_ftp_return_code_unit.py index c38f0e6cf5..368394e5f1 100644 --- a/tests/vendor/test_ftp_return_code_unit.py +++ b/tests/vendor/test_ftp_return_code_unit.py @@ -50,7 +50,8 @@ #: Every distribution importing :mod:`pcapkit.vendor` needs -- see #: :mod:`tests.vendor.test_user_agent_unit` for why ``requests`` alone is not #: enough. They ship in the ``vendor`` extra (:file:`pyproject.toml`), not -#: ``test``, and CI installs ``.[test]``. +#: ``test`` -- so these run on ``engine-tests``, which installs ``vendor`` too +#: (#738), and skip on the ``test`` and ``gate`` jobs, which do not. VENDOR_DEPS = ('requests', 'bs4', 'html5lib') #: Whether the crawlers are importable at all. diff --git a/tests/vendor/test_ipx_packet_unit.py b/tests/vendor/test_ipx_packet_unit.py index 05ac0104f4..2db7e79497 100644 --- a/tests/vendor/test_ipx_packet_unit.py +++ b/tests/vendor/test_ipx_packet_unit.py @@ -53,8 +53,10 @@ VENDOR_DEPS = ('requests', 'bs4', 'html5lib') #: Whether the crawlers are importable at all. They ship in the ``vendor`` extra -#: (:file:`pyproject.toml`), not ``test``, and CI installs ``.[test]`` -- so these -#: tests skip in CI as things stand. Guarded the same way +#: (:file:`pyproject.toml`), not ``test`` -- so these tests skip on the ``test`` +#: and ``gate`` jobs of :file:`.github/workflows/unit-tests.yml`, which never +#: install ``vendor``, but run for real on ``engine-tests``, which does (#738). +#: Guarded the same way #: :file:`tests/protocols/test_dispatch_registry_unit.py` guards its own optional #: runtime dependencies, rather than making the whole unit tier depend on the #: crawlers' requirements. See #518. diff --git a/tests/vendor/test_request_prompt_unit.py b/tests/vendor/test_request_prompt_unit.py index 65e75668bb..ac46713f85 100644 --- a/tests/vendor/test_request_prompt_unit.py +++ b/tests/vendor/test_request_prompt_unit.py @@ -99,10 +99,12 @@ VENDOR_DEPS = ('requests', 'bs4', 'html5lib') #: Whether the crawlers are importable at all. They ship in the ``vendor`` extra -#: (:file:`pyproject.toml`), not ``test``, and CI installs ``.[test]``, so these -#: cases skip in CI as things stand. Guarded the way -#: :file:`tests/protocols/test_dispatch_registry_unit.py` guards its own optional -#: dependencies rather than making the whole unit tier depend on the crawlers'. +#: (:file:`pyproject.toml`), not ``test`` -- so these cases skip on the ``test`` +#: and ``gate`` jobs of :file:`.github/workflows/unit-tests.yml`, but run for +#: real on ``engine-tests``, which installs ``vendor`` too (#738). +#: Guarded the way :file:`tests/protocols/test_dispatch_registry_unit.py` guards +#: its own optional dependencies rather than making the whole unit tier depend on +#: the crawlers'. HAS_VENDOR_DEPS = all(importlib.util.find_spec(name) is not None for name in VENDOR_DEPS) #: How many prompts :func:`_fake_input` will answer before it decides the loop is diff --git a/tests/vendor/test_user_agent_unit.py b/tests/vendor/test_user_agent_unit.py index d3fa0f0d72..400a2392ef 100644 --- a/tests/vendor/test_user_agent_unit.py +++ b/tests/vendor/test_user_agent_unit.py @@ -63,8 +63,10 @@ VENDOR_DEPS = ('requests', 'bs4', 'html5lib') #: Whether the crawlers are importable at all. They ship in the ``vendor`` extra -#: (:file:`pyproject.toml`), not ``test``, and CI installs ``.[test]`` -- so these -#: tests skip in CI as things stand. Guarded the same way +#: (:file:`pyproject.toml`), not ``test`` -- so these tests skip on the ``test`` +#: and ``gate`` jobs of :file:`.github/workflows/unit-tests.yml`, which never +#: install ``vendor``, but run for real on ``engine-tests``, which does (#738). +#: Guarded the same way #: :file:`tests/protocols/test_dispatch_registry_unit.py` guards its own optional #: runtime dependencies, rather than making the whole unit tier depend on the #: crawlers' requirements. See #518. diff --git a/tests/vendor/test_vendor_dest_path_unit.py b/tests/vendor/test_vendor_dest_path_unit.py index 2eecc6e705..d8a9bedcdc 100644 --- a/tests/vendor/test_vendor_dest_path_unit.py +++ b/tests/vendor/test_vendor_dest_path_unit.py @@ -93,9 +93,14 @@ nothing here reaches -- no test in this file calls :meth:`Vendor.__init__` or :meth:`~Vendor.context`. ``requests`` and ``beautifulsoup4`` both ship in the ``test`` extra precisely so vendor tests can run (:file:`pyproject.toml`, -around line 243), so both classes below run in CI as things stand, unlike -:file:`tests/vendor/test_user_agent_unit.py`, whose own gate also lists -``html5lib`` and skips there because of it. +around line 243), so both classes below run on ``test``, ``engine-tests`` +and ``gate`` -- the only three pytest jobs of +:file:`.github/workflows/unit-tests.yml` that collect anything under +:file:`tests/vendor/` at all, since ``integration`` and ``pypcap-parity`` +never reach it -- unlike :file:`tests/vendor/test_user_agent_unit.py`, whose +own gate also lists ``html5lib`` and so still skips on ``test`` and ``gate``, +though it now runs for real on ``engine-tests`` too, which installs the +``vendor`` extra (#738). """ from __future__ import annotations