diff --git a/.github/workflows/unit-tests.yml b/.github/workflows/unit-tests.yml index a354e184b2..8648214ec5 100644 --- a/.github/workflows/unit-tests.yml +++ b/.github/workflows/unit-tests.yml @@ -239,109 +239,137 @@ jobs: echo "Fixture-dependent selection: $selection" python -m pytest -q -n auto --dist load $selection - # Per-engine coverage for the third-party capture engines (Scapy, PyShark, - # PyPCAPFile, pcap-ct) -- ruled onto its own job in #751 rather than one more - # install line on `test`, `integration` or `gate`: "per-engine's tests - # covered by a separate test step and on all supported Python versions... - # so they dont intertwine with the other major tests". `test` had already - # declined Scapy on cost grounds (see its own install-step comment above); - # asking that question three more times, once per engine, would only repeat - # it instead of answering it. + # Per-engine coverage for the third-party capture engines -- rebuilt in #845 + # as a genuine two-dimension matrix (Python version x engine), replacing the + # single "install everything, run the unit tier once per Python version" job + # #751 landed. That shape had a real gap #845 measured on current `main`: + # PyPCAPFile's own "python_version < '3.12'" marker (pyproject.toml) meant + # editable-installing the old job's combined extras line silently resolved + # PyPCAPFile to nothing for that extra on 3.12/3.13/3.14 (deliberately not + # spelled out as the literal `pip install` form: tests/_dependency_gates.py's + # job_sections() slices on `^ ([\w-]+):`, so a comment sitting between two + # jobs is textually part of the one above it, and that guard's pytest_jobs() + # requires exactly one such literal per job section -- a second one here, + # even in a comment, makes it think this job has two install lines and + # cannot tell which extras the run would have. See #849's cross-review.), + # so "Engines Python 3.12/3.13/3.14" reported + # green while never exercising PyPCAPFile at all on three of five legs -- + # yet all three were promoted to ruleset 23497679's required checks anyway + # (finding 3), on the reasoning that the matrix rebuild would close finding + # 2 rather than leaving it hidden behind a passing gate. # - # Selection mirrors the `test` job's ignore flags exactly, so it reaches the - # same unit tier -- including - # tests/foundation/engines/test_runtime_engines.py, whose own HAS_RUNTIME - # reuses that name for the four core dependencies plus dpkt, scapy and - # pyshark (see tests/_dependency_gates.py's own exclusion for the history). - # Installing Scapy, PyShark and DPKT here closes that gap too, as a side - # effect of the engine extras rather than a separate install line. + # The maintainer's own ruling (issue #845) is what sets this job's shape: + # one leg per (Python version, engine) pair rather than one per Python + # version installing every engine together, so a failure -- or a correctly + # asserted decline -- names the one engine responsible instead of an + # ambiguous "Engines Python 3.12". Each cell resolves to one of three + # outcomes, tracked via the matrix's own `expect` field (defaulting to + # `supported` for any (python, engine) pair not listed in `include` below): # - # PyPCAPFile installs only on 3.10 and 3.11 here -- its own - # "python_version < '3.12'" marker -- so its 9 HAS_PYPCAPFILE-gated unit - # methods (tests/toolkit/test_pypcapfile_unit.py) run on two of these five - # legs and skip cleanly on the other three. tests/_dependency_gates.py's own - # guard cannot see that partial coverage -- it does not evaluate markers, by - # its own module docstring -- so this is recorded here instead: two legs of - # real coverage is the trade #751 asked to make, not a gap to hide. #747 and - # #748 are what make it worth taking at all -- they fixed the two bugs that - # used to turn those skips into 7 failures. + # * `supported` -- the engine installs via its normal pyproject.toml + # extra and its dedicated unit-tier tests must + # actually pass, proving this engine ran. + # * `unsupported` -- the engine is installable on this interpreter but + # its own PYTHON_CEILING means it declines at run + # time (PyShark on 3.14+; PyPCAPFile on 3.12+, whose + # pip marker would otherwise stop it ever installing + # here -- so this job installs the raw distribution + # past that marker, deliberately, rather than via the + # extra). The same test files run; their own + # version-aware assertions (e.g. + # PyPCAPFile's test_unsupported_reason_tracks_the_running_interpreter) + # are what prove the decline actually fired and named + # the interpreter, rather than merely skipping. + # * `not-installable` -- PyPCAP on 3.12+: a C-extension build failure, not + # a runtime refusal (no wheel; pcap.c does not + # compile against the 3.12+ C API -- PyPCAP carries + # no PYTHON_CEILING at all, so there is no decline to + # assert). This job still installs the raw + # distribution past pyproject.toml's marker to prove + # the failure is real rather than assumed, and treats + # that failure as the expected, passing outcome for + # the cell -- distinctly logged via `::notice`, never + # silently green and never red for the wrong reason. + # If it ever starts building, the job fails loudly + # (see the install step) rather than reporting a + # vacuous pass. # - # pcap-ct needs a system libpcap present at run time (no compiler, no - # headers -- see the PCAP_CT extra in pyproject.toml), which is what the - # apt-get step below installs. It must never share a venv with PyPCAP: both - # distributions install a top-level `pcap` module, and pcap-ct's package - # shadows upstream's extension whenever both are importable -- measured in - # 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. + # Scoped deliberately out of this rebuild: the "and integ, if any" half of + # the ruling. DPKT and PyShark also have integration-tier coverage + # (tests/integration/test_engine_parity.py, test_engine_runtime.py) and + # PyPCAP has tests/foundation/engines/test_new_engine_parity_runtime.py, but + # all three need example captures regenerated first (examples/generators/), + # which itself needs Scapy and DPKT together -- awkward to do per single- + # engine cell without installing engines this job is deliberately keeping + # apart. That tier stays covered in aggregate by the `integration` and + # `pypcap-parity` jobs above rather than being duplicated per engine here; + # worth the maintainer's separate call if per-engine integration coverage is + # wanted too. # - # `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. + # Also scoped out: the multi-engine HAS_RUNTIME gate + # (tests/foundation/engines/test_runtime_engines.py) that the old job + # exercised as a side effect of installing DPKT+Scapy+PyShark together, and + # the incidental crypto/NGAP/vendor coverage (#738) the old job carried on + # this leg because it happened to be the one non-required-check job touching + # every Python version. Splitting by engine removes the "everything + # installed at once" venv those relied on; both are pre-existing coverage + # this rebuild does not restore, and are unaffected by anything already + # required, but are worth the maintainer's attention -- not silently + # dropped, but not solved here either. # - # 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. + # `include` only needs to name the cells that are NOT `supported`; every + # other (python-version, engine) pair defaults to it, per the install step's + # `${EXPECT:-supported}`. engine-tests: - name: Engines Python ${{ matrix.python-version }} + name: Engines Python ${{ matrix.python-version }} (${{ matrix.engine }}) if: ${{ inputs.gate-only != true }} runs-on: ubuntu-latest - timeout-minutes: 45 + timeout-minutes: 30 + # 3.15 is experimental-prerelease per #845's ruling: present so a + # regression is visible, but never blocking -- a failure on this leg + # reports without failing the run or the required-check set. + continue-on-error: ${{ matrix.python-version == '3.15' }} strategy: fail-fast: false matrix: python-version: - # See the `test` job above for why 3.15 is excluded here. - "3.10" - "3.11" - "3.12" - "3.13" - "3.14" + - "3.15" + engine: + - DPKT + - Scapy + - PyShark + - PyPCAPFile + - PyPCAP + - PCAP_CT + include: + # PyShark's PYTHON_CEILING (pcapkit/foundation/engines/pyshark.py) + # is (3, 14): no pip marker stops it installing here, it just + # declines once constructed. + - { python-version: "3.14", engine: PyShark, expect: unsupported } + - { python-version: "3.15", engine: PyShark, expect: unsupported } + # PyPCAPFile's PYTHON_CEILING is (3, 12) -- see + # pcapkit/foundation/engines/pypcapfile.py. Its own pyproject.toml + # marker ("python_version < '3.12'") is exactly what #845 finding 2 + # is about: it stops the extra installing here at all, which is why + # the install step below reaches for the raw distribution on these + # four legs instead. + - { python-version: "3.12", engine: PyPCAPFile, expect: unsupported } + - { python-version: "3.13", engine: PyPCAPFile, expect: unsupported } + - { python-version: "3.14", engine: PyPCAPFile, expect: unsupported } + - { python-version: "3.15", engine: PyPCAPFile, expect: unsupported } + # PyPCAP carries no PYTHON_CEILING -- pyproject.toml's own + # "python_version < '3.12'" marker is the only thing that keeps pip + # from attempting the build here, because the build itself fails + # (no wheel; pcap.c does not compile against the 3.12+ C API). + - { python-version: "3.12", engine: PyPCAP, expect: not-installable } + - { python-version: "3.13", engine: PyPCAP, expect: not-installable } + - { python-version: "3.14", engine: PyPCAP, expect: not-installable } + - { python-version: "3.15", engine: PyPCAP, expect: not-installable } steps: - uses: actions/checkout@v7 @@ -349,56 +377,197 @@ jobs: - uses: actions/setup-python@v7 with: python-version: ${{ matrix.python-version }} + # setup-python does not ship a 3.15 interpreter as a release build + # yet; python-compatibility.yml's compatibility-nightly job already + # established that 3.15 needs this flag to resolve at all. + allow-prereleases: ${{ matrix.python-version == '3.15' }} cache: pip - # pcap-ct's ctypes loader calls find_library("pcap") at run time; without - # a system libpcap, PCAP_CT.unsupported_reason() degrades the engine to - # the default parser instead of running it, and the one HAS_PCAP_CT-gated - # test that exercises the real backend - # (test_the_real_backend_reads_a_committed_capture) would see that - # fallback and fail its own assertion that no EngineWarning was raised -- - # not merely skip. + # Only the engines that need a system package get one, rather than + # installing tshark/libpcap/a C toolchain on every cell regardless of + # which engine it is testing. + # + # tshark: PyShark is a wrapper around it and does no parsing of its + # own -- see the debconf pre-seed note below for why the two apt lines + # are shaped the way they are. + # + # libpcap0.8: PCAP_CT's ctypes loader calls find_library("pcap") at run + # time; without it, PCAP_CT.unsupported_reason() degrades the engine to + # the default parser instead of running it. # - # tshark joins it per #751's later ruling, which asked the same "try it, - # rip it if CI is not a good fit" of PyShark's binary that it asked of - # PyPCAP's toolchain -- not merely "install the distribution and leave - # the binary out", which an earlier reading of this thread had settled - # for. One HAS_PYSHARK-gated method - # (test_the_reason_tracks_the_running_interpreter) used to hard-assert - # tshark's *absence*, which would have flipped from a pass to a failure - # the moment tshark was installed; that assertion now probes the same - # way PyShark.unsupported_reason() itself does -- pyshark's own - # get_process_path(), not shutil.which(), which config.ini precedence - # can make disagree with it -- the same oracle its sibling - # test_this_host_really_has_no_tshark_so_the_check_is_not_vacuous already - # used, so the file no longer disagrees with itself about whether - # tshark is allowed to be present. `DEBIAN_FRONTEND=noninteractive` plus - # the debconf pre-seed below is what stops `tshark`'s postinst script - # from blocking on the "allow non-superusers to capture packets" prompt - # apt would otherwise show. - - name: Install system libpcap and tshark + # build-essential + libpcap-dev: PyPCAP is the one engine that compiles + # (pcap.c against libpcap's headers), on every leg -- including the + # `not-installable` ones, deliberately, so that failure is a genuine + # compile failure against the 3.12+ C API rather than merely "no + # compiler was present to try". + - name: Install this engine's system packages run: | - sudo apt-get update - echo "wireshark-common wireshark-common/install-setuid boolean false" | sudo debconf-set-selections - sudo DEBIAN_FRONTEND=noninteractive apt-get install -y --no-install-recommends libpcap0.8 tshark + set -eu + ENGINE="${{ matrix.engine }}" + if [ "$ENGINE" = "PyShark" ]; then + sudo apt-get update + echo "wireshark-common wireshark-common/install-setuid boolean false" | sudo debconf-set-selections + sudo DEBIAN_FRONTEND=noninteractive apt-get install -y --no-install-recommends tshark + elif [ "$ENGINE" = "PCAP_CT" ]; then + sudo apt-get update + sudo apt-get install -y --no-install-recommends libpcap0.8 + elif [ "$ENGINE" = "PyPCAP" ]; then + sudo apt-get update + sudo apt-get install -y --no-install-recommends build-essential libpcap-dev + fi - - name: Install package and per-engine test dependencies + # `expect` decides not just the assertion but *how* the engine gets + # installed: `supported` uses the ordinary pyproject.toml extra, whose + # own marker (if any) already resolves to "install" on this Python + # version; `unsupported` and `not-installable` install the raw + # distribution directly, deliberately bypassing whichever marker would + # otherwise have skipped it silently -- see this job's own comment + # above for why that bypass is the point of this rebuild. + # + # `set +e` is scoped to the one install command whose failure is + # sometimes expected, not to the baseline above it: upgrading pip + # itself and editable-installing this repo's own `test` extra failing + # is never expected on any cell (deliberately not spelled out as the + # literal `pip install` form below -- see this job's own comment + # earlier in this file, and tests/_dependency_gates.py's + # job_sections()/pytest_jobs(), for why a second such literal in a + # comment here would make that guard think this section holds two + # baseline install lines), and running them unchecked used to mean a + # broken base install on a `not-installable` cell read as "expected" + # while the job had installed and run nothing at all (#849's + # cross-review). They now run under this step's own explicit `set -eu` + # below -- not "the shell's default", since GitHub's default for a + # Linux `run:` step is plain `bash -e {0}` with no `pipefail`, and + # this step never sets `shell: bash` to change that -- so either + # failing aborts the step immediately and loudly. + - name: Install package and this engine's dependency + id: install run: | + set -eu python -m pip install -U pip setuptools wheel - python -m pip install -e '.[test,DPKT,crypto,NGAP,Scapy,PyShark,PyPCAPFile,PCAP_CT,vendor]' + python -m pip install -e '.[test]' + + ENGINE="${{ matrix.engine }}" + EXPECT="${{ matrix.expect }}" + EXPECT="${EXPECT:-supported}" + + # RAW_PKGS is an array, not a string, so PCAP_CT's two-package case + # ("pcap-ct libpcap") reaches pip as two argv entries rather than by + # relying on unquoted word-splitting (shellcheck SC2086) -- quoting + # the string form instead would hand pip one invalid "pcap-ct + # libpcap" argument and break exactly that case. + case "$ENGINE" in + DPKT) EXTRA=DPKT; RAW_PKGS=(dpkt); IMPORT_NAME=dpkt ;; + Scapy) EXTRA=Scapy; RAW_PKGS=(scapy); IMPORT_NAME=scapy ;; + PyShark) EXTRA=PyShark; RAW_PKGS=(pyshark); IMPORT_NAME=pyshark ;; + PyPCAPFile) EXTRA=PyPCAPFile; RAW_PKGS=(pypcapfile); IMPORT_NAME=pcapfile ;; + PyPCAP) EXTRA=PyPCAP; RAW_PKGS=(pypcap); IMPORT_NAME=pcap ;; + PCAP_CT) EXTRA=PCAP_CT; RAW_PKGS=(pcap-ct libpcap); IMPORT_NAME=pcap ;; + *) echo "::error::unknown engine $ENGINE"; exit 1 ;; + esac + + set +e + if [ "$EXPECT" = "supported" ]; then + output=$(python -m pip install -e ".[$EXTRA]" 2>&1) + else + output=$(python -m pip install "${RAW_PKGS[@]}" 2>&1) + fi + status=$? + set -e + echo "$output" + + echo "status=$status" >> "$GITHUB_OUTPUT" + + if [ "$status" -ne 0 ] && [ "$EXPECT" != "not-installable" ]; then + echo "::error title=$ENGINE failed to install on Python ${{ matrix.python-version }}::This cell expects '$EXPECT', which requires a successful install. pip exited $status." + exit 1 + fi + if [ "$status" -eq 0 ] && [ "$EXPECT" = "not-installable" ]; then + echo "::error title=$ENGINE now installs on Python ${{ matrix.python-version }}::This cell is recorded as not-installable in this workflow's include: list (and in pyproject.toml's marker for $ENGINE). It installed cleanly -- update both rather than leaving this green for the wrong reason." + exit 1 + fi + if [ "$status" -ne 0 ]; then + # A not-installable cell's whole point is a genuine C-API compile + # failure (see this job's own comment above), not a flaky index + # or a dropped connection -- treating *any* nonzero exit as that + # expected failure would let a PyPI 503 pass as though the + # engine had been proven unbuildable. Grep the captured output + # for the shape of a transient failure instead of a compile one. + # + # The transient list is pip's own retry/network vocabulary, not a + # bare "5xx or 429" number scan: pip prints a "(NNN kB)" download + # size on essentially every sdist fetch, and a Cython-generated + # .c file runs to thousands of lines, so an unanchored bare-number + # match (#849's cross-review) fires on ordinary download-size + # lines and on compiler diagnostics quoting a line number in that + # range -- e.g. "Downloading pypcap-1.3.0.tar.gz (500 kB)" and + # "pcap.c:501:24: error: ..." both matched, while the actual + # compile-failure markers matched nothing. A cell that hits that + # false positive would exit 1 on a transcript no re-run can ever + # change, so this is a loud, permanent false red, not a silent + # one -- see this job's own comment on "fails loudly in both + # directions" above for why that distinction still matters: a + # loud false red still blocks every PR just as surely as a + # silent false green would. Requiring the *absence* of a compile + # marker alongside a transient one is the second half of the + # fix: a message that happens to carry both is treated as the + # compile failure it also is, not the transient one it merely + # resembles. + if echo "$output" | grep -qiE \ + 'read timed out|connection (reset|refused|aborted)|temporary failure in name resolution|max retries exceeded|could not fetch url|newconnectionerror|readtimeouterror|httpsconnectionpool|retrying \(retry\(|http error (5[0-9][0-9]|429)|status code 429|too many 429 error responses|no matching distribution found' \ + && ! echo "$output" | grep -qiE \ + 'failed building wheel|error: command .* failed with exit code|subprocess-exited-with-error'; then + echo "::error title=$ENGINE's install failure looks transient, not a compile failure::pip exited $status, but the output matches a network/index failure pattern rather than the C-API compile failure this cell records as expected. Re-run the job rather than trusting this as a genuine 'not-installable' result." + exit 1 + fi + echo "::notice title=$ENGINE is not installable on Python ${{ matrix.python-version }} (expected)::pip exited $status, matching this cell's recorded expectation ('not-installable'), and the output does not match a transient-failure pattern. No tests run for this cell -- this is a deliberate skip, not a pass." + fi + + # #845 finding 2, reproduced structurally rather than left to be + # re-found by enumeration: a `supported` cell whose extra resolves + # to nothing (an unmet environment marker, a typo'd extra name) + # still exits 0, and every one of this cell's tests would then + # skip rather than fail, going green having proven nothing. This + # cell's own point is exercising $IMPORT_NAME, so require it. + if [ "$status" -eq 0 ]; then + if ! python -c "import $IMPORT_NAME" 2>/dev/null; then + echo "::error title=$ENGINE installed but $IMPORT_NAME does not import on Python ${{ matrix.python-version }}::pip exited 0 but the module this cell exists to exercise is not importable -- a green install that unlocked nothing." + exit 1 + fi + fi # See the `test` job above for why this step exists. - name: Report available parallelism + if: ${{ steps.install.outputs.status == '0' }} run: | nproc python -c "import os; print('cpu_count', os.cpu_count())" - - name: Run unit tests - run: >- - python -m pytest -q -n auto --dist load - --ignore=tests/integration - --ignore-glob='*_runtime.py' - --ignore-glob='*_regression.py' + # Runs the SAME test files whether `expect` is `supported` or + # `unsupported`: the difference is not in what runs here but in what + # those files themselves assert once they see this interpreter's + # version, e.g. PyPCAPFile's own + # test_unsupported_reason_tracks_the_running_interpreter and PyShark's + # test_the_reason_tracks_the_running_interpreter (added in #846) both + # branch on sys.version_info against the engine's own PYTHON_CEILING. + # That is what proves the decline actually fired and named the + # interpreter, rather than this job asserting it from the outside. + # Skipped entirely for `not-installable` cells -- see the install step. + - name: Run this engine's test suite + if: ${{ steps.install.outputs.status == '0' }} + run: | + set -eu + ENGINE="${{ matrix.engine }}" + case "$ENGINE" in + DPKT) TEST_PATHS="tests/toolkit/test_dpkt_unit.py" ;; + 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" ;; + esac + echo "Running: $TEST_PATHS" + python -m pytest -q -n auto --dist load $TEST_PATHS # Whether upstream PyPCAP is worth building in CI at all -- #751's own # instruction was "try to build and if the CI is not a good suit, then we diff --git a/tests/_dependency_gates.py b/tests/_dependency_gates.py index 45fe792438..30d75dde4a 100644 --- a/tests/_dependency_gates.py +++ b/tests/_dependency_gates.py @@ -313,8 +313,11 @@ class Exclusion(NamedTuple): 'the runner at run time. A green install would therefore not imply the ' 'engine can start.\n\n' "#751's dedicated engine-tests job now takes this: it installs PCAP_CT and a " - 'system libpcap (apt-get libpcap0.8) across the full 3.10-3.14 matrix, kept ' - 'in a venv of its own since pypcap and pcap-ct both install a top-level ' + 'system libpcap (apt-get libpcap0.8) on its own PCAP_CT matrix cell, across ' + 'the same 3.10-3.14 (plus non-blocking 3.15) matrix -- rebuilt per #849 as one ' + 'venv per (Python version, engine) pair rather than the one shared venv #751 ' + 'first landed, so PCAP_CT is kept apart from PyPCAP by construction now, not ' + 'merely by convention -- since pypcap and pcap-ct both install a top-level ' '``pcap`` module and cannot coexist -- see ' "pcapkit/foundation/engines/_pcap_backend.py's own docstring. test and gate " "stay dark on purpose, per #751's ruling that per-engine coverage gets its " @@ -334,17 +337,27 @@ class Exclusion(NamedTuple): 'model markers -- while 3.12, 3.13 and 3.14 went on skipping: two legs of ' 'real coverage bought with exactly the false confidence #745 exists to ' "remove.\n\n" - "#751's ruling was to take that trade: engine-tests now installs PyPCAPFile " - "across the full 3.10-3.14 matrix, covering the 9 HAS_PYPCAPFILE methods in " - 'test_pypcapfile_unit.py on the 3.10/3.11 legs where the marker lets it ' - 'resolve, and pypcap-parity does the same for the other 6 in ' - 'test_new_engine_parity_runtime.py. This guard still cannot see that only ' - 'two of five legs run for real -- it does not evaluate markers, by this ' - "module's own docstring -- so that partial coverage is recorded here in " - 'prose rather than modelled: the alternative, teaching this guard markers, ' - 'is out of scope for a workflow-only change. test, integration and gate stay ' - "dark on purpose, per #751's ruling that per-engine coverage gets its own " - 'job rather than one more install line on any of the three.' + "#751's ruling was to take that trade: engine-tests installs PyPCAPFile on its " + 'own PyPCAPFile cell, covering the 9 HAS_PYPCAPFILE methods in ' + 'test_pypcapfile_unit.py (plus test_pypcapfile_engine.py) genuinely on every ' + "one of the matrix's Python versions rather than only two of five -- #849's " + "rebuild is exactly what closes the partial-coverage problem this paragraph " + 'used to describe: on 3.10/3.11 the cell installs the ordinary extra, and on ' + "3.12-3.15, where pyproject.toml's marker would resolve it to nothing, the " + "install step installs the raw pypcapfile distribution instead (expect: " + "unsupported), deliberately bypassing the marker so the tests run for real and " + "assert the engine's own version-aware decline (see this workflow's own " + 'engine-tests comment). pypcap-parity does the same for the other 6 methods in ' + 'test_new_engine_parity_runtime.py, on the two legs its own marker allows. This ' + 'guard still does not evaluate markers by itself -- it takes selection and ' + 'extras from the YAML, not from what a given cell resolves to -- but for this ' + 'flag the workflow now does the marker-bypassing work directly, so the guard ' + "\"models markers\" caveat this paragraph used to carry no longer describes a " + 'real gap for PyPCAPFile specifically; it stays true in general (see this ' + "module's docstring) for any extra whose own marker is left unbypassed. test, " + "integration and gate stay dark on purpose, per #751's ruling that per-engine " + 'coverage gets its own job rather than one more install line on any of the ' + 'three.' ), ), 'HAS_VENDOR_DEPS': Exclusion( @@ -358,31 +371,35 @@ class Exclusion(NamedTuple): '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' + 'engine-tests (#751) used to be that leg: it mirrored test\'s ignore-shape ' + 'exactly (same --ignore flags) and carried vendor on its one shared install ' + 'line, so it reached and closed these unit-tier HAS_VENDOR_DEPS gates as a ' + "side effect of installing everything together. #849's rebuild removes that " + 'shared venv: each matrix.engine cell now installs only "test" plus its own ' + 'engine extra and runs only that engine\'s own explicit TEST_PATHS -- ' + 'tests/toolkit/test_dpkt_unit.py and five siblings, never tests/vendor/ -- so ' + 'no cell reaches these gates at all any more, let alone installs vendor for ' + 'them. This is not a *new* gap this guard reports, because ' + ':func:`dependency_gate_gaps` only counts a job that reaches a gate without ' + "installing what it needs; a job that no longer reaches the gate at all is " + 'invisible to it either way, which is exactly the "silence is the failure ' + 'mode" problem this module\'s own docstring opens with. It is a real, ' + 'disclosed regression rather than a silent one: #849\'s own PR description ' + "records vendor coverage as dropped, not restored, and worth its own issue. " + 'test and gate stay dark for their own, unrelated reasons below; engine-tests ' + 'is simply no longer part of the answer for this flag.\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' + 'on a pull request -- and now that engine-tests no longer carries vendor ' + 'either, no per-PR job does, so this flag\'s only per-PR-visible install ' + 'point is test\'s own decline above. gate could add vendor without ' + 'contradicting the "non-blocking" ruling, but doing so on the one job that ' + 'gates an actual release, for a network-and-parser extra whose own crawlers ' + "#518 already documents as flaky, is not worth the footprint on its own -- " + "and it would not restore the per-PR coverage #849 removed regardless, since " + 'gate never runs on a pull request at all.\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. ' @@ -427,14 +444,35 @@ class Exclusion(NamedTuple): "only where a caller passes gate-only: true -- the three Saturday schedules " '(deploy-pages, cron-vendor, cron-conda) and a v* release tag -- never on a ' 'pull request or a push to main. So no per-PR leg runs them at all.\n\n' - "#751's engine-tests job now installs Scapy across the full 3.10-3.14 " - 'matrix, closing the "no per-PR leg" problem this exclusion used to ' - 'describe -- it reaches 10 of this guard\'s 14 HAS_SCAPY methods, the same ' - 'unit-tier subset #738 counted; the other 4 live in tests/integration/ or ' - "match the *_runtime.py ignore-glob, already covered by Scapy on the " - "integration and gate jobs. test stays dark on purpose: the ruling on #751 " - 'was a dedicated job precisely so test would not have to reconsider the ' - 'cost question it already answered.' + "#751's engine-tests job used to install Scapy across the full 3.10-3.14 " + 'matrix and run it against the whole unit tier (same --ignore shape as ' + 'test), closing the "no per-PR leg" problem this exclusion used to describe ' + "for all 10 of this guard's 14 unit-tier HAS_SCAPY methods at once.\n\n" + "#849's rebuild only partially keeps that closure. engine-tests's Scapy cell " + 'now runs an explicit TEST_PATHS list -- tests/toolkit/test_scapy_unit.py, ' + 'tests/foundation/engines/test_scapy_engine.py and ' + "tests/protocols/transport/test_sctp_unit.py -- which covers 8 of the 10 (5 " + '+ 2 + 1), but not the other 2, one each in test_core.py and test_misc.py: ' + "those are simply not in that cell's path list, so no per-PR job reaches " + 'them with Scapy installed any more -- the exact "no per-PR leg runs them ' + 'at all" state from before #751, reopened for those two. This is not ' + 'visible as a wider Gap here: :func:`dependency_gate_gaps` only reports a job ' + 'that reaches a gate without installing for it, and a job that no longer ' + "reaches the gate at all is invisible to it either way, by construction -- " + "which is why this paragraph exists to say so in prose rather than leave it " + "to the tool. test_sctp_unit.py's one method was added to the Scapy cell's " + 'TEST_PATHS at zero new-dark-flag cost: unlike test_core.py and ' + 'test_misc.py, it carries no HAS_DPKT-gated tests of its own, so restoring ' + 'it traded nothing away. test_core.py and test_misc.py each still carry ' + "their own HAS_DPKT-gated tests (see the test job's own install-step " + "comment), and adding them here without DPKT would trade one dark flag for " + 'another on a cell that installs neither -- disclosed rather than ' + 'restored, a decision worth its own review rather than folding into a ' + 'workflow-only CR. The other 4 of the 14 live in ' + 'tests/integration/ or match the *_runtime.py ignore-glob, unaffected by any ' + 'of this, already covered by Scapy on the integration and gate jobs. test ' + 'stays dark on purpose: the ruling on #751 was a dedicated job precisely so ' + 'test would not have to reconsider the cost question it already answered.' ), ), 'HAS_PYSHARK': Exclusion( @@ -458,11 +496,21 @@ class Exclusion(NamedTuple): 'precedence can make disagree with -- matching what its sibling ' 'test_this_host_really_has_no_tshark_so_the_check_is_not_vacuous already ' 'used, so the file no longer disagrees with itself about whether tshark may ' - 'be present. engine-tests now installs both the distribution and tshark ' - '(apt-get, with a debconf pre-seed so the postinst prompt does not block) ' - 'across the full 3.10-3.14 matrix, closing the test-job gap this exclusion ' - "used to describe. integration and gate stay dark on purpose, per #751's " - 'ruling that per-engine coverage gets its own job.' + 'be present. engine-tests installs both the distribution and tshark (apt-get, ' + 'with a debconf pre-seed so the postinst prompt does not block) on its own ' + 'PyShark cell, and that cell\'s explicit TEST_PATHS (#849) -- ' + 'tests/toolkit/test_pyshark_unit.py (0 of the 4; it carries no HAS_PYSHARK ' + 'gate of its own, only its DPKT/PyPCAPFile/etc. siblings do) and ' + 'tests/foundation/engines/test_pyshark_engine.py (the other 3) -- reaches 3 ' + 'of the 4 HAS_PYSHARK methods. The fourth, ' + 'tests/integration/test_engine_runtime.py:70\'s ' + 'test_pyshark_engine_is_refused_before_asyncio_can_break, is integration-tier ' + "and outside any engine-tests cell's reach -- it stays dark on the integration " + 'job exactly as recorded above, unaffected by any of this. So the rebuild ' + "from one shared ignore-shape venv to one venv per engine leaves this flag's " + "closure unchanged, closing the test-job gap this exclusion used to describe " + "for those 3. integration and gate stay dark on purpose, per #751's ruling " + 'that per-engine coverage gets its own job.' ), ), 'HAS_RUNTIME': Exclusion( @@ -481,15 +529,25 @@ class Exclusion(NamedTuple): "module #751 did not otherwise need to change, and this guard does not " 'care what a flag is named, only whether the job that reaches it installs ' 'what it asks for.\n\n' - "#751's engine-tests job installs DPKT, Scapy and PyShark together, closing " - "this gap as a side effect of the per-engine extras rather than a separate " - 'install line: all 5 methods run wherever engine-tests does. test and gate ' - 'stay dark on purpose, for the same reason HAS_SCAPY and HAS_PYSHARK above ' - 'do.' + "#751's engine-tests job used to install DPKT, Scapy and PyShark together in " + "one shared venv, closing this gap as a side effect: all 5 methods ran " + "wherever engine-tests did. #849's rebuild removes that shared venv -- each " + 'matrix.engine cell now installs exactly one engine\'s own extra, so no cell ' + 'installs all three together any more, and none of the six explicit ' + 'TEST_PATHS lists names test_runtime_engines.py at all, so engine-tests no ' + 'longer reaches this gate either. Like the HAS_SCAPY and HAS_VENDOR_DEPS ' + 'entries above, this does not surface as a wider Gap -- a job that stops ' + "reaching a gate is invisible to :func:`dependency_gate_gaps` the same way a " + 'job that never reached it would be -- so it is recorded here in prose. ' + "Disclosed, not silent: #849's own PR description names this exact loss " + '("the multi-engine HAS_RUNTIME gate ... needs DPKT+Scapy+PyShark together") ' + 'and defers restoring it to a follow-up issue rather than reopening this ' + 'workflow-only change to add a seventh venv shape. test and gate stay dark on ' + 'purpose, for the same reason HAS_SCAPY and HAS_PYSHARK above do.' ), ), 'HAS_MYPY': Exclusion( - dark={'test': ('mypy',), 'engine-tests': ('mypy',), 'gate': ('mypy',)}, + dark={'test': ('mypy',), 'gate': ('mypy',)}, reason=( 'The one entry here that is not a missing *runtime* dependency, and the only ' 'one whose fix is not an install line. mypy is a type checker: the single ' @@ -526,13 +584,21 @@ class Exclusion(NamedTuple): 'This entry is therefore narrow: it is not a precedent that a gate on a lint ' 'tool gets excluded, only that a gate duplicating an existing lint.yml check ' 'does.\n\n' - 'test, engine-tests and gate are exactly the three jobs whose selection ' - 'reaches tests/vendor/test_vendor_reg_apptype_generator_unit.py -- the two ' - 'ignore-shape legs and the whole-suite one. integration and pypcap-parity ' - 'select by fixture tier and never collect that module, which is why they are ' - 'not listed, the same reason they are absent from HAS_VENDOR_DEPS above. If a ' - 'lint extra is ever declared in pyproject.toml, the right change is to delete ' - 'this entry and let the gap close on its own rather than to widen it.' + 'test and gate are exactly the two jobs whose selection reaches ' + 'tests/vendor/test_vendor_reg_apptype_generator_unit.py -- the ignore-shape ' + 'leg and the whole-suite one. engine-tests used to be a third, back when its ' + "one shared venv mirrored test's ignore-shape exactly; #849's rebuild to one " + "explicit TEST_PATHS list per matrix.engine cell means none of the six now " + 'names tests/vendor/ at all, so engine-tests no longer reaches this gate and ' + 'has dropped out of this entry\'s dark dict accordingly -- not widened or ' + 'narrowed against a gap that is still there, but genuinely gone, the case ' + ':meth:`~tests.test_tier_guard.DependencyGateCoverageTests\ +.test_each_exclusion_still_describes_a_gap_that_is_really_there` exists to catch if this ' + 'entry is not kept in step with it. integration and pypcap-parity select by ' + 'fixture tier and never collect that module, which is why they are not listed, ' + 'the same reason they are absent from HAS_VENDOR_DEPS above. If a lint extra ' + 'is ever declared in pyproject.toml, the right change is to delete this entry ' + 'and let the gap close on its own rather than to widen it.' ), ), } @@ -569,7 +635,20 @@ class Gate(NamedTuple): class Job(NamedTuple): - """A job of :data:`WORKFLOW` that runs :program:`pytest`.""" + """A job of :data:`WORKFLOW` that runs :program:`pytest`. + + Normally one :class:`Job` per YAML job -- but ``engine-tests`` (#849) is + not one static install line and one static selection: each + ``matrix.engine`` cell installs a different extra and runs a different, + explicit list of test files, so :func:`pytest_jobs` yields one + :class:`Job` per engine for it, all sharing the one YAML job ``name``. + 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 + and select the same thing. + + """ #: Job name as the workflow spells it. name: 'str' @@ -578,8 +657,14 @@ class Job(NamedTuple): #: How the job selects tests: ``'fixture-tier'`` when it asks #: :func:`~tests._tiers.fixture_tier_paths` for the selection, #: ``'ignore'`` when it subtracts ``--ignore`` flags from the whole suite, - #: ``'whole-suite'`` when it passes no selection at all. + #: ``'whole-suite'`` when it passes no selection at all, ``'explicit'`` + #: when it names literal test-file paths (see :attr:`paths`). selection: 'str' + #: The literal test-file paths named by an ``'explicit'`` selection, empty + #: for every other shape. Compared to :class:`Gate`.\ ``module`` by exact + #: string equality -- unlike ``'fixture-tier'``, there is no directory or + #: node-ID form here, because that is not the shape ``TEST_PATHS`` takes. + paths: 'tuple[str, ...]' = () class Gap(NamedTuple): @@ -1204,6 +1289,106 @@ def job_sections(text: 'str') -> 'dict[str, str]': return sections +def _engine_matrix_variants(name: 'str', section: 'str') -> 'Optional[tuple[Job, ...]]': + """One :class:`Job` per ``matrix.engine`` cell, for a job shaped like ``engine-tests``. + + Returns :data:`None` when ``section`` shows no such structure, so + :func:`pytest_jobs`'s caller falls back to its ordinary one-install-line + reading. Detected on ``matrix.engine`` appearing at all, rather than on the + job's name: this guard finds things by what a section *does*, the same + rule :func:`pytest_jobs` already applies to the step that runs + :program:`pytest`. + + ``engine-tests`` (#849) installs a shared baseline (``pip install -e + '.[test]'``) and then, per cell, one more extra chosen by a ``case + "$ENGINE" in ...`` block assigning ``EXTRA=``, and runs a *different*, + explicit ``TEST_PATHS=`` list chosen by an identically-shaped ``case`` + in its pytest step. Modelling that as one :class:`Job` with every engine's + extra and a ``'whole-suite'`` selection -- the reading a single static + install-line/selection pair would produce -- is exactly the false + confidence #745 exists to catch: it would credit the DPKT cell with + PyPCAP's install and credit every cell with reaching every other engine's + tests. One :class:`Job` per engine, sharing this job's ``name``, is what + :func:`dependency_gate_gaps`'s ``(flag, job.name)`` aggregation expects -- + see :class:`Job`'s own docstring. + + Every regex below runs against a comment-stripped copy of ``section`` + (bare ``#``-prefixed lines dropped), not ``section`` itself. Without that, + a *commented-out* ``ENGINE) TEST_PATHS="..."`` arm placed after the live + one would silently win a later cell's entry -- ``dict()`` over + ``re.findall()`` is last-wins -- which is the same comment-vs-code + confusion #849's own cross-review found in this file's ``pip install -e`` + scan, relocated into this function's case-arm scan instead. + + This also, separately, asserts that the workflow's own ``engine:`` matrix + list agrees with the ``case "$ENGINE" in ...`` arms it is supposed to + describe: earlier versions of this function built :class:`Job` entries + purely from the case arms and never looked at the ``engine:`` list at + all, so deleting a cell from that list (the ordinary way to retire one) + left this guard still crediting the retired engine with coverage it no + longer has, silently. + + """ + code = '\n'.join(line for line in section.splitlines() if not line.strip().startswith('#')) + + if 'matrix.engine' not in code: + return None + + declared_match = re.search(r'(?m)^[ \t]*engine:\n((?:[ \t]*-[ \t]*\w+[ \t]*\n)+)', code) + if declared_match is None: + raise AssertionError( + f'the {name!r} job varies by matrix.engine but this guard found no ' + f"'engine:' matrix list of bare '- NAME' entries, and cannot tell which " + f'engines the matrix itself declares' + ) + declared_engines = frozenset(re.findall(r'-[ \t]*(\w+)', declared_match.group(1))) + + base_installs = re.findall(r"pip install -e '\.\[([^]]*)\]'", code) + if len(base_installs) != 1: + raise AssertionError( + f'the {name!r} job varies by matrix.engine but has {len(base_installs)} ' + f"baseline \"pip install -e '.[...]'\" lines, and this guard cannot tell " + f'which extras every cell would always have' + ) + base_extras = tuple(extra.strip() for extra in base_installs[0].split(',')) + + engine_extra = dict(re.findall(r'(\w+)\)\s*EXTRA=(\w+);', code)) + engine_paths = { + engine: tuple(paths.split()) + for engine, paths in re.findall(r'(\w+)\)\s*TEST_PATHS="([^"]+)"', code) + } + if not engine_paths: + raise AssertionError( + f'the {name!r} job varies by matrix.engine but this guard found no ' + f'\'ENGINE) TEST_PATHS="..."\' case mapping engine names to test paths, and ' + f'cannot tell what any cell runs' + ) + + case_engines = frozenset(engine_paths) | frozenset(engine_extra) + if declared_engines != case_engines: + only_declared = sorted(declared_engines - case_engines) + only_case = sorted(case_engines - declared_engines) + raise AssertionError( + f'the {name!r} job\'s matrix.engine list ({sorted(declared_engines)}) does not ' + f'match the engines its \'case "$ENGINE" in ...\' arms cover ' + f'({sorted(case_engines)}): {only_declared} have no case arm, ' + f'{only_case} have a case arm but are not in the matrix list -- this guard ' + f'cannot tell which of the two is stale' + ) + + variants = [] # type: list[Job] + for engine, paths in engine_paths.items(): + extra = engine_extra.get(engine) + if extra is None: + raise AssertionError( + f'the {name!r} job runs {engine!r}\'s tests ({" ".join(paths)}) but this ' + f"guard found no matching 'ENGINE) EXTRA=...' case for it, and cannot tell " + f'what that cell installs' + ) + variants.append(Job(name, base_extras + (extra,), 'explicit', paths)) + return tuple(variants) + + def pytest_jobs(workflow: 'Optional[pathlib.Path]' = None) -> 'tuple[Job, ...]': """The jobs of ``workflow`` that run :program:`pytest`. @@ -1225,6 +1410,11 @@ def pytest_jobs(workflow: 'Optional[pathlib.Path]' = None) -> 'tuple[Job, ...]': if 'python -m pytest' not in section: continue + variants = _engine_matrix_variants(name, section) + if variants is not None: + jobs.extend(variants) + continue + installs = re.findall(r"pip install -e '\.\[([^]]*)\]'", section) if len(installs) != 1: raise AssertionError( @@ -1264,7 +1454,7 @@ def pytest_jobs(workflow: 'Optional[pathlib.Path]' = None) -> 'tuple[Job, ...]': def job_reaches(job: 'Job', gate: 'Gate') -> 'bool': """Whether ``job``'s selection would collect the tests ``gate`` guards. - The three selection shapes are answered three ways, and none of them + The four selection shapes are answered four ways, and none of them reimplements the workflow's own list: * ``'whole-suite'`` reaches everything under :file:`tests/`. @@ -1273,6 +1463,9 @@ def job_reaches(job: 'Job', gate: 'Gate') -> 'bool': :meth:`~tests.test_tier_guard.WorkflowAgreementTests\ .test_ignore_flags_match_the_fixture_tier_constants` is what keeps that equivalence true. + * ``'explicit'`` is exact membership in :attr:`Job.paths` -- the literal + ``TEST_PATHS`` list a ``matrix.engine`` cell of ``engine-tests`` (#849) + runs, at file granularity (that job names whole files, never node IDs). * ``'fixture-tier'`` is whatever :func:`~tests._tiers.fixture_tier_paths` returns, which is what the job itself runs. Its node-ID entries are honoured at method granularity: a @@ -1294,6 +1487,8 @@ def job_reaches(job: 'Job', gate: 'Gate') -> 'bool': return True if job.selection == 'ignore': return _tiers.is_unit_tier(gate.module) + if job.selection == 'explicit': + return gate.module in job.paths for entry in _tiers.fixture_tier_paths(): module, _, scope = entry.partition('::') diff --git a/tests/test_tier_guard.py b/tests/test_tier_guard.py index aaa9f8e068..b5cf5a83ad 100644 --- a/tests/test_tier_guard.py +++ b/tests/test_tier_guard.py @@ -1300,24 +1300,30 @@ def test_job_sections_agrees_with_the_single_job_slice(self) -> None: self.assertEqual(section, job_section(text, name)) def test_each_job_selection_is_one_of_the_three_recognised_shapes(self) -> None: - """A fourth shape has to fail here rather than be guessed at. + """A fifth shape has to fail here rather than be guessed at. - The three are the three the workflow uses: subtract ``--ignore`` flags - from the whole suite (``test``), ask - :func:`~tests._tiers.fixture_tier_paths` (``integration``), or pass no - selection at all (``gate``). A new job selecting some fourth way would - otherwise be classified ``'whole-suite'`` by the fallback and credited - with reaching gates it does not run. + Four are recognised: subtract ``--ignore`` flags from the whole suite + (``test``), ask :func:`~tests._tiers.fixture_tier_paths` + (``integration``), pass no selection at all (``gate``), or name + literal test-file paths (``engine-tests``, one :class:`Job` per + ``matrix.engine`` cell, #849). A new job selecting some fifth way + would otherwise be classified ``'whole-suite'`` by the fallback and + credited with reaching gates it does not run. """ for job in _dependency_gates.pytest_jobs(): with self.subTest(job=job.name): - self.assertIn(job.selection, ('ignore', 'fixture-tier', 'whole-suite')) + self.assertIn(job.selection, ('ignore', 'fixture-tier', 'whole-suite', 'explicit')) - selections = {job.name: job.selection for job in _dependency_gates.pytest_jobs()} - self.assertEqual(selections, {'test': 'ignore', 'integration': 'fixture-tier', - 'gate': 'whole-suite', 'engine-tests': 'ignore', - 'pypcap-parity': 'fixture-tier'}) + # engine-tests yields several Job entries sharing that one name -- + # one per matrix.engine cell, all ``'explicit'`` -- so this collects + # the *set* of selections seen per name rather than assuming one. + selections = {} # type: dict[str, set[str]] + for job in _dependency_gates.pytest_jobs(): + selections.setdefault(job.name, set()).add(job.selection) + self.assertEqual(selections, {'test': {'ignore'}, 'integration': {'fixture-tier'}, + 'gate': {'whole-suite'}, 'engine-tests': {'explicit'}, + 'pypcap-parity': {'fixture-tier'}}) def test_the_selection_is_read_off_the_step_that_runs_pytest(self) -> None: """Not off the job, whose comments contradict it. @@ -1342,11 +1348,16 @@ def test_a_job_whose_pytest_step_is_renamed_is_still_classified(self) -> None: """The step is found by what it runs, not by what it is called.""" doctored = doctored_workflow(self, '- name: Run unit tests', '- name: Execute the unit tier') - selections = {job.name: job.selection - for job in _dependency_gates.pytest_jobs(doctored)} - self.assertEqual(selections, {'test': 'ignore', 'integration': 'fixture-tier', - 'gate': 'whole-suite', 'engine-tests': 'ignore', - 'pypcap-parity': 'fixture-tier'}) + # engine-tests yields one Job per matrix.engine cell (all + # ``'explicit'``, and unaffected by this doctoring since its own + # pytest step -- "Run this engine's test suite" -- is not the one + # renamed here), so de-duplicate to the set of selections per name. + selections = {} # type: dict[str, set[str]] + for job in _dependency_gates.pytest_jobs(doctored): + selections.setdefault(job.name, set()).add(job.selection) + self.assertEqual(selections, {'test': {'ignore'}, 'integration': {'fixture-tier'}, + 'gate': {'whole-suite'}, 'engine-tests': {'explicit'}, + 'pypcap-parity': {'fixture-tier'}}) def test_two_install_lines_in_one_pytest_job_is_refused(self) -> None: """Ambiguity fails loudly instead of the first line winning.""" @@ -1582,17 +1593,22 @@ def test_the_mypy_gate_is_visible_and_dark_on_every_job_that_reaches_it(self) -> self.assertEqual(_dependency_gates.extras_providing('mypy'), frozenset()) gaps = {(gap.flag, gap.job) for gap in _dependency_gates.dependency_gate_gaps()} - for job in ('test', 'engine-tests', 'gate'): + for job in ('test', 'gate'): with self.subTest(reaches=job): self.assertIn(('HAS_MYPY', job), gaps) - # Both select by fixture tier and never collect tests/vendor/ at all -- - # the same reason they are absent from HAS_VENDOR_DEPS above. - for job in ('integration', 'pypcap-parity'): + # 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 + # and none of the six names tests/vendor/ -- see the HAS_MYPY + # exclusion's own reason for why that is a real, disclosed change + # 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) exclusion = _dependency_gates.DEPENDENCY_GATE_EXCLUSIONS['HAS_MYPY'] - self.assertEqual(set(exclusion.dark), {'test', 'engine-tests', 'gate'}) + self.assertEqual(set(exclusion.dark), {'test', 'gate'}) def test_the_isort_gate_is_visible_and_closed_by_the_test_extra(self) -> None: """#766: the same invisible shape as ``mypy``, resolved the opposite way. @@ -1640,10 +1656,12 @@ def test_the_isort_gate_is_visible_and_closed_by_the_test_extra(self) -> None: reaching = {job.name for job in _dependency_gates.pytest_jobs() if _dependency_gates.job_reaches(job, gates[0])} self.assertEqual( - reaching, {'test', 'engine-tests', 'gate'}, - 'the two ignore-shape legs and the whole-suite one collect ' - 'tests/project/; integration and pypcap-parity select by fixture tier and ' - 'never reach it, the same reason they are absent from HAS_MYPY above') + reaching, {'test', 'gate'}, + 'the ignore-shape leg and the whole-suite one collect tests/project/; ' + 'integration and pypcap-parity select by fixture tier and never reach it, ' + 'the same reason they are absent from HAS_MYPY above. engine-tests (#849) ' + 'no longer reaches it either -- none of its six explicit TEST_PATHS lists ' + 'names tests/project/, the same change that dropped it out of HAS_MYPY above') gaps = {(gap.flag, gap.job) for gap in _dependency_gates.dependency_gate_gaps()} for job in sorted(reaching): @@ -1919,9 +1937,11 @@ 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 + # test_the_isort_gate_is_visible_and_closed_by_the_test_extra above. self.assertEqual( sorted(job for (flag, job) in gaps if flag == 'HAS_ISORT'), - ['engine-tests', 'gate', 'test']) + ['gate', 'test']) gap = gaps[('HAS_ISORT', 'test')] self.assertEqual(gap.missing, ('isort',)) @@ -1938,7 +1958,7 @@ def test_dropping_isort_from_the_test_extra_is_caught(self) -> None: 'the exclusion list would swallow the gap this test relies on') def test_removing_dpkt_is_caught_on_every_job_that_installs_it(self) -> None: - """#729's defect, restaged. Five jobs install ``DPKT``; all five go dark. + """#729's defect, restaged. Four jobs install ``DPKT`` via a comma list; all four go dark. #751 added ``engine-tests`` and ``pypcap-parity`` to the three this test used to name, each carrying its own ``DPKT`` for the same reason as the @@ -1947,14 +1967,22 @@ def test_removing_dpkt_is_caught_on_every_job_that_installs_it(self) -> None: ``HAS_RUNTIME``, ``pypcap-parity`` 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. Four jobs, not five. + """ text = _dependency_gates.WORKFLOW.read_text(encoding='utf-8') doctored = doctored_workflow(self, text, text.replace('DPKT,', '').replace(',DPKT', '')) gaps = {gap.job: gap for gap in _dependency_gates.dependency_gate_gaps(doctored) if gap.flag == 'HAS_DPKT'} - self.assertEqual(sorted(gaps), - ['engine-tests', 'gate', 'integration', 'pypcap-parity', 'test']) + self.assertEqual(sorted(gaps), ['gate', 'integration', 'pypcap-parity', 'test']) for job, gap in sorted(gaps.items()): with self.subTest(job=job): self.assertEqual(gap.missing, ('dpkt',)) @@ -2022,19 +2050,21 @@ def test_swapping_pypcap_parity_to_pcap_ct_is_an_ambiguous_satisfaction(self) -> def test_swapping_engine_tests_to_pypcap_is_an_ambiguous_satisfaction(self) -> None: """#762, direction two: the wrong half of the ``pcap._pcap`` ambiguity. - ``engine-tests`` swapped from ``PCAP_CT`` to ``PyPCAP`` still reaches the - 1 ``HAS_PCAP_CT`` gate and still reads as satisfied to - :func:`~tests._dependency_gates.dependency_gate_gaps`, for the mirrored - reason: ``extras_providing`` truncates ``pcap._pcap`` to ``pcap`` before - its lookup, per this module's own docstring. + ``engine-tests``'s ``PCAP_CT`` cell (#849: a ``case "$ENGINE" in ...`` + block chooses the extra, not a shared comma-joined install line) + swapped to install ``PyPCAP`` instead still reaches the 1 + ``HAS_PCAP_CT`` gate -- its ``TEST_PATHS`` mapping is keyed on the + engine name ``'PCAP_CT'``, untouched by this doctoring -- and still + reads as satisfied to + :func:`~tests._dependency_gates.dependency_gate_gaps`, for the + mirrored reason: ``extras_providing`` truncates ``pcap._pcap`` to + ``pcap`` before its lookup, per this module's own docstring. """ doctored = doctored_workflow( self, - "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]'", + 'PCAP_CT) EXTRA=PCAP_CT;', + 'PCAP_CT) EXTRA=PyPCAP;', ) gaps = {(gap.flag, gap.job) for gap in _dependency_gates.dependency_gate_gaps(doctored)} @@ -2102,10 +2132,8 @@ 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,vendor]'", - "python -m pip install -e '.[test,DPKT,crypto,NGAP,Scapy,PyShark,PyPCAPFile," - "PyPCAP,vendor]'", + 'PCAP_CT) EXTRA=PCAP_CT;', + 'PCAP_CT) EXTRA=PyPCAP;', ) with self.subTest(mutation='flag_exclusions stubbed to report nothing'): @@ -2183,6 +2211,125 @@ def test_a_non_distribution_flag_is_not_reported_even_when_doctored_ambiguous(se for f in _dependency_gates.ambiguous_satisfactions(doctored)} 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 + ``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. + + """ + doctored = doctored_workflow(self, ' - PyShark\n', '') + + with self.assertRaises(AssertionError) as ctx: + _dependency_gates.pytest_jobs(doctored) + + message = str(ctx.exception) + self.assertIn("engine-tests", message) + 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. + + 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 + second literal ``pip install -e '.[...]'`` baseline line makes the count the + function checks for (exactly one) come out to two. + + """ + doctored = doctored_workflow( + self, + "python -m pip install -e '.[test]'", + "python -m pip install -e '.[test]'\n python -m pip install -e '.[test]'", + ) + + with self.assertRaises(AssertionError) as ctx: + _dependency_gates.pytest_jobs(doctored) + + message = str(ctx.exception) + self.assertIn('engine-tests', message) + 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. + + 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, + silently-vacuous result. + + """ + text = _dependency_gates.WORKFLOW.read_text(encoding='utf-8') + # Two `case "$ENGINE" in ... esac` blocks exist in this job -- the install step's + # (`EXTRA=` arms) and the pytest step's (`TEST_PATHS=` arms) -- so finding the + # first match is not enough; pick the one that actually carries `TEST_PATHS=`. + matches = [m for m in re.finditer(r'(?m)^( +)case "\$ENGINE" in\n(?:.*\n)*?\1esac\n', text) + if 'TEST_PATHS=' in m.group(0)] + self.assertEqual(len(matches), 1, 'expected exactly one TEST_PATHS case block to doctor') + match = matches[0] + block = match.group(0) + indent = match.group(1) + doctored = doctored_workflow(self, block, f'{indent}case "$ENGINE" in\n{indent}esac\n') + + with self.assertRaises(AssertionError) as ctx: + _dependency_gates.pytest_jobs(doctored) + + message = str(ctx.exception) + self.assertIn('engine-tests', message) + 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. + + 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 + different reason -- leaves DPKT declared in the ``engine:`` list and still + present in ``engine_paths`` (so the equality check sees the same six engines on + both sides and stays quiet), but with no ``EXTRA=`` arm of its own: exactly the + shape the per-engine loop's own check exists for. + + """ + doctored = doctored_workflow(self, 'DPKT) EXTRA=DPKT;', '# DPKT) EXTRA=DPKT;') + + with self.assertRaises(AssertionError) as ctx: + _dependency_gates.pytest_jobs(doctored) + + message = str(ctx.exception) + self.assertIn('engine-tests', message) + self.assertIn('DPKT', message) + + def test_a_commented_out_test_paths_arm_does_not_silently_win(self) -> None: + """The relocated blocker the second cross-review flagged, not just commented on. + + ``dict()`` over ``re.findall()`` is last-wins, so a *commented-out* + ``ENGINE) TEST_PATHS="..."`` arm placed after the live one would silently + override it -- the exact comment-vs-code confusion #849's first cross-review + found in this module's ``pip install -e`` scan, relocated into this function's + case-arm scan instead. Stripping ``#``-prefixed lines before every regex in + :func:`~tests._dependency_gates._engine_matrix_variants` is what keeps a + commented-out arm from being read as a live one. + + """ + doctored = doctored_workflow( + self, + 'DPKT) TEST_PATHS="tests/toolkit/test_dpkt_unit.py" ;;', + 'DPKT) TEST_PATHS="tests/toolkit/test_dpkt_unit.py" ;;\n' + ' # DPKT) TEST_PATHS="tests/toolkit/test_pypcap_unit.py" ;;', + ) + + jobs = _dependency_gates.pytest_jobs(doctored) + dpkt_jobs = [job for job in jobs if job.name == 'engine-tests' and 'DPKT' in job.extras] + self.assertEqual(len(dpkt_jobs), 1) + self.assertEqual(dpkt_jobs[0].paths, ('tests/toolkit/test_dpkt_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.""" unexplained = [gap for gap in _dependency_gates.dependency_gate_gaps()