ci: rebuild engine-tests as a Python × engine matrix with three cell outcomes - #849
Conversation
|
NEEDS CHANGES on A prose comment breaks five required checks.
So the ruleset window this PR already warns about is not sufficient — five more required checks Required changes:
Claims 1, 2, 5 and 7 hold exactly as written — the 36-cell / 26-6-4 tally was independently Labelled |
c4b26ce to
e38e849
Compare
|
All three blockers cleared plus the six follow-ups. Head Verified by me on a scratch tree with current
B2 was completed rather than deferred — B3 was recorded, not restored — the HAS_SCAPY unit-tier drop is now in the description's gap list and in Two things still true and unchanged by this fix. Ruleset |
|
NEEDS CHANGES on 1. B3's justification is false for one of the three files. Both the description and the For 2. The transient-vs-compile grep is backwards on real pip output, and would make three required cells pip prints a 3. Everything else confirmed, including the merge being clean, 102 passed / 558 subtests with |
e38e849 to
102ad32
Compare
- Replace the single "install every engine, one leg per Python version" engine-tests job with a genuine two-dimension matrix: 6 Python versions (3.10-3.15) x 6 engines (DPKT, Scapy, PyShark, PyPCAPFile, PyPCAP, PCAP_CT), 36 cells, per the maintainer's ruling on #845. Each cell resolves to supported/unsupported/not-installable via a new `expect` field, closing the gap where PyPCAPFile's own pip marker silently installed nothing on 3.12-3.14 while the leg still reported green. - Reword the install-step comment that broke five required unit-tier checks: it held a literal `pip install -e '.[...]'` that tests/_dependency_gates.py's job_sections() attributed to the wrong job. - Teach tests/_dependency_gates.py the new per-(matrix.engine) shape: one Job per engine cell, an `explicit` selection over literal TEST_PATHS. `_engine_matrix_variants()` now parses the `engine:` matrix list itself and asserts set-equality against the `case "$ENGINE" in ...` arm keys, rather than trusting the arms alone -- deleting a cell from the matrix list used to leave it silently still modelled. Every regex in that function now runs against a comment-stripped copy of the section, so a commented-out arm placed after a live one cannot win the last-wins `dict()` merge. Update the DEPENDENCY_GATE_EXCLUSIONS entries whose justification the shape change falsified (HAS_PCAP_CT, HAS_PYPCAPFILE, HAS_VENDOR_DEPS, HAS_SCAPY, HAS_PYSHARK, HAS_RUNTIME, HAS_MYPY). - Restore the one HAS_SCAPY path that costs nothing to restore (tests/protocols/transport/test_sctp_unit.py, which carries no HAS_DPKT gate of its own) into the Scapy cell's TEST_PATHS; record rather than restore the other two (test_core.py, test_misc.py), which do each carry their own HAS_DPKT gate and would trade one dark flag for another. - Harden the install step: scope `set +e` to the engine install only (a broken baseline no longer reads as an expected not-installable cell); distinguish a transient pip failure from the genuine compile failure a not-installable cell expects by keying on pip's own retry/network vocabulary and requiring the absence of a compile marker, rather than an unanchored bare-number match that fired on ordinary "(NNN kB)" download-size lines and compiler diagnostics alike; add a post-install import assertion so a supported cell whose extra resolves to nothing fails loudly instead of skipping green; quote PCAP_CT's two-package raw install via an array instead of unquoted word-splitting. - Fix two prose errors: the HAS_PYSHARK exclusion claimed its cell's TEST_PATHS covered all 4 gated methods (it is 3 of 4 -- the fourth is integration-tier and outside any engine-tests cell's reach); the install step's own comment claimed the shell's default `-eo pipefail` (GitHub's Linux `run:` default is plain `bash -e {0}`, no pipefail -- harmless here since the step sets `set -eu` itself, but wrong). - Correct the PR description's stated gaps (crypto/NGAP were never lost; only vendor, HAS_RUNTIME and the HAS_SCAPY paths above are), and its claim that a silent false red is impossible (a heuristic transient-vs- compile match can still misfire loudly, which blocks merges just as effectively as a silent one would). - Add falsifiability tests for all four of _engine_matrix_variants's raise branches, including the exact matrix-list-vs-case-arm mismatch the second cross-review demonstrated, and a regression test for the commented-out-arm hazard above. Build: not run (workflow-only change, GHA cannot be exercised locally). YAML parses clean; 36 cells re-derived as 26 supported/6 unsupported/4 not-installable. tests/test_tier_guard.py: 107 passed, 558 subtests passed (pytest); Ran 107 tests ... OK (plain unittest, same tree).
102ad32 to
0cc59b7
Compare
|
All three required changes cleared. Head Item 1 — the grep is now correct in both directions. The unanchored bare-number branch is gone, and The false-positive pair no longer classifies as transient, real transient markers do, and a message carrying Item 2 — the parser now reads the Item 3 — Matrix unperturbed by the Both prose errors also fixed: Unchanged and still yours: ruleset |
|
GOOD TO GO on I re-derived the claim this PR was caught for twice — that a falsifiability test might pass on both sides. The two that pass pre-fix say so in their own docstrings — "the pre-existing What the review established beyond my floor, each measured rather than reasoned:
Two non-blocking residuals recorded rather than folded away, both worth a comment if anyone touches this This still cannot merge until ruleset |
|
CI all passed but russet gates not getting their expected results? Actually are those ruleset things necessary? We can already gate on CI passing. |
|
Recorded, verbatim: "CI all passed but russet gates not getting their expected results? Actually are those On the first half — yes, and here is exactly why. The five required contexts are name-specific and this PR #849 emits On the second half — measured, and the answer is no: there is no separate CI gate. Classic branch protection is off, so ruleset So removing the required-checks list would make a red PR mergeable — GitHub would still display the But your instinct is right in a different form, and the repo already has the mechanism. 22 name-specific Two ways forward, and this one is a judgement call rather than a measurement:
Say which and I will do it — (1) is yours to apply since the ruleset is yours; (2) I can file and implement. |
|
Take the better path (2) |
|
And am I good to merge this one for now? |
|
Recorded, verbatim: "Take the better path (2)" — the per-PR aggregate job. Mine to implement; a worker is on For the record, the measurement that makes (2) the better path. The ruleset ( Option (1) would have meant listing 30 cell names by hand and re-listing them on every matrix change, forever. Two traps the worker is briefed on, because getting them wrong makes the gate worse than no gate:
Your remaining step, once this lands: replace all 22 contexts in the ruleset with the single new one. I will |
Yes. One thing to expect when you click merge: the ruleset will likely still complain, and not because of this PR. That is the whole reason (2) is worth building rather than patching the list: after the aggregate PR lands you Ordering note, since you have asked about it before: #657 and #719 still merge last, after everything else is |
|
Correction to what I told you above, and it matters before you touch the ruleset. I said five of the 22 required contexts — And they pass on every PR — measured on #856 just now: So of the 22, only the five The consequence for the ruleset edit: do NOT replace all 22 contexts with the single new one. A I have held #856 at |
Ruleset 23497679's required_status_checks names 22 exact check contexts by
literal string, and GitHub rulesets support no wildcard. Of those 22, only
the five `Engines Python <version>` names are actually dead -- `engine-tests`
stopped emitting them once it became a Python x engine matrix. The five
`Compat Python <version>` names are live, emitted by the `compatibility` job
in python-compatibility.yml, and must stay required as-is; nothing in this
file can stand in for a job in a different workflow file.
- Add a `required-checks` job that `needs: [test, integration, engine-tests,
pypcap-parity]` and reports one stable check context, `Required checks
passed`, that the ruleset can require in place of the 17 `test`/
`integration`/`Engines`/`pypcap-parity` slots (not the five `Compat` ones).
- Guard it with `if: ${{ always() && inputs.gate-only != true }}`: without
`always()`, a plain `needs:` job is *skipped* -- not failed -- the moment
any dependency fails, and GitHub's own docs list a skip as a passing
status for required checks, which would make this gate worse than none.
`gate-only != true` keeps it from spuriously failing on the release-only
`gate-only: true` call path, where its four dependencies are themselves
skipped by design.
- The job's own step inspects each dependency's `needs.<job>.result`
explicitly, printing every value and failing loudly (naming the job) on
anything other than `success`.
- Comment documents one open, unverified risk: whether a `continue-on-error`
3.15 `engine-tests` cell's real failure is neutralised to `success` in
`needs.engine-tests.result` the way it is for the run's own conclusion is
not stated in GitHub's docs. Flagged rather than resolved by splitting the
3.15 legs into their own job, which was judged too much churn for a job
#849 only just rebuilt, to close a single undocumented edge.
YAML verified with `yaml.safe_load` via the repo venv; `git diff --numstat`
against `origin/main` shows one file, 140 insertions, 0 deletions.
engine-tests ran one Python-version leg per interpreter regardless of which extraction engines that interpreter could actually install, so pyproject.toml's own PyPCAPFile marker resolved to nothing on 3.12-3.14, yet those legs still passed and, once promoted to a required check, gated merges while exercising an engine that was never installed. Rebuilt as a genuine Python x engine matrix -- 36 cells after 10 include overrides -- with three honest outcomes: 26 supported cells that must genuinely pass, 6 unsupported cells that assert the engine's own decline, and 4 not-installable cells where a real C-extension build failure is logged and the test step skipped. Job names change shape, which needed the branch-protection ruleset's required-check list updated in the same window.
Adds util/pyshark_encap_map.py, a generator that regenerates ENCAP_TYPE_TO_LINKTYPE (152 entries) and FILTER_NAME_TO_LINKTYPE (58) in place inside pcapkit/toolkit/pyshark.py, so the two tables #850 hand-built stop being hand-maintained. Guards against its own worst failure mode: a naive LinkType(dlt) lookup cannot detect an unmapped DLT, because _missing_ mints a placeholder rather than raising, so the generator snapshots every known value before any lookup. The real-tshark sweep is gated on a new HAS_WIRESHARK flag and is version-pinned -- editcap -T accepts 226 encapsulations on Wireshark 4.6.9, 224 on 4.2.2 (CI's Ubuntu noble) -- so the count assertions run only under the measured version. One line of pyshark.py itself changes as a result: the IPMB_LINUX table entry becomes I2C_LINUX, the canonical name #848 gave value 209. util/changelog_md.py regenerated CHANGELOG.md for all seven entries added across this and the six preceding commits (#838, #846, #847, #848, #849, #850, #853); `--check` exit 0.
#846 and #850 - #838's "the 21 whose vendor crawler leaves Vendor.process() unmodified" measures 22, not 21 -- pcapkit.const.hip.transport.Transport also has an unmodified process(), but its FLAG-bound range has no unassigned gap, so its _missing_ carries no bounded-range extend_enum branch to fix. The real discriminator is that branch, not the process() override; reworded, and the "other 84" split into the 83 that do override process() and the one that doesn't but has nothing to fix either. Re-derived independently against 5e25db2^..5e25db2 (not main, which would fold in #847's own edit to ipx/socket.py): 22 registries have no process() override, 21 of them have the bounded-range branch, matching tests/const/test_const_enum_no_mint.py's own REGISTRIES_WITH_UNASSIGNED_RANGES (21 entries) vs ALL_REGISTRIES (22, +hip.transport, with a NOTE explaining the exclusion). - #846's "Only tests/integration/test_engine_runtime.py and test_engine_parity.py change" is false -- 30bca99's own numstat also touches .github/workflows/unit-tests.yml (17+/2-). Qualified to "only these two test files change". - #850 gets the breaking marker: its own prose already says the fallback is gone and an unrecognised encapsulation or filter name now raises MissingKeyError instead of substituting a plausible DLT -- the same shape as the two existing markers at AppType.get (1.5.0.rst:2456) and the four .get()-backed enum fields (1.5.0.rst:2720). Restructured to lead with the marker and the subject, matching their wording and placement; the later restatement of the same fact is dropped. None of these are code or test changes -- prose only, matching the cross-review's own framing. #848 stays unmarked (its own "Not breaking" paragraph holds); #838, #849 and #853 stay unmarked per explicit instruction not to add markers beyond what was asked.
…ck job Ruleset 23497679 names 22 exact check contexts with no wildcard support, so the required list needs hand-editing on every matrix change; five of them (the old single-cell Engines Python <version> names) are already dead since #849, and the five Compat Python 3.10-3.14 names stay live and untouched. Adds one job, required-checks (context "Required checks passed"), depending on test/integration/engine-tests/pypcap-parity with if: always() so it cannot itself be silently skipped -- the failure mode GitHub's own troubleshooting docs warn a bare needs: list falls into -- plus an explicit per-dependency check that names which job broke. Not breaking: workflow-only, no library or test code changes. The 17-of-22 ruleset replacement it enables is a separate repository setting this PR does not itself touch.
engine-tests ran one Python-version leg per interpreter regardless of which extraction engines that interpreter could actually install, so pyproject.toml's own PyPCAPFile marker resolved to nothing on 3.12-3.14, yet those legs still passed and, once promoted to a required check, gated merges while exercising an engine that was never installed. Rebuilt as a genuine Python x engine matrix -- 36 cells after 10 include overrides -- with three honest outcomes: 26 supported cells that must genuinely pass, 6 unsupported cells that assert the engine's own decline, and 4 not-installable cells where a real C-extension build failure is logged and the test step skipped. Job names change shape, which needed the branch-protection ruleset's required-check list updated in the same window.
Adds util/pyshark_encap_map.py, a generator that regenerates ENCAP_TYPE_TO_LINKTYPE (152 entries) and FILTER_NAME_TO_LINKTYPE (58) in place inside pcapkit/toolkit/pyshark.py, so the two tables #850 hand-built stop being hand-maintained. Guards against its own worst failure mode: a naive LinkType(dlt) lookup cannot detect an unmapped DLT, because _missing_ mints a placeholder rather than raising, so the generator snapshots every known value before any lookup. The real-tshark sweep is gated on a new HAS_WIRESHARK flag and is version-pinned -- editcap -T accepts 226 encapsulations on Wireshark 4.6.9, 224 on 4.2.2 (CI's Ubuntu noble) -- so the count assertions run only under the measured version. One line of pyshark.py itself changes as a result: the IPMB_LINUX table entry becomes I2C_LINUX, the canonical name #848 gave value 209. util/changelog_md.py regenerated CHANGELOG.md for all seven entries added across this and the six preceding commits (#838, #846, #847, #848, #849, #850, #853); `--check` exit 0.
#846 and #850 - #838's "the 21 whose vendor crawler leaves Vendor.process() unmodified" measures 22, not 21 -- pcapkit.const.hip.transport.Transport also has an unmodified process(), but its FLAG-bound range has no unassigned gap, so its _missing_ carries no bounded-range extend_enum branch to fix. The real discriminator is that branch, not the process() override; reworded, and the "other 84" split into the 83 that do override process() and the one that doesn't but has nothing to fix either. Re-derived independently against 5e25db2^..5e25db2 (not main, which would fold in #847's own edit to ipx/socket.py): 22 registries have no process() override, 21 of them have the bounded-range branch, matching tests/const/test_const_enum_no_mint.py's own REGISTRIES_WITH_UNASSIGNED_RANGES (21 entries) vs ALL_REGISTRIES (22, +hip.transport, with a NOTE explaining the exclusion). - #846's "Only tests/integration/test_engine_runtime.py and test_engine_parity.py change" is false -- 30bca99's own numstat also touches .github/workflows/unit-tests.yml (17+/2-). Qualified to "only these two test files change". - #850 gets the breaking marker: its own prose already says the fallback is gone and an unrecognised encapsulation or filter name now raises MissingKeyError instead of substituting a plausible DLT -- the same shape as the two existing markers at AppType.get (1.5.0.rst:2456) and the four .get()-backed enum fields (1.5.0.rst:2720). Restructured to lead with the marker and the subject, matching their wording and placement; the later restatement of the same fact is dropped. None of these are code or test changes -- prose only, matching the cross-review's own framing. #848 stays unmarked (its own "Not breaking" paragraph holds); #838, #849 and #853 stay unmarked per explicit instruction not to add markers beyond what was asked.
…ck job Ruleset 23497679 names 22 exact check contexts with no wildcard support, so the required list needs hand-editing on every matrix change; five of them (the old single-cell Engines Python <version> names) are already dead since #849, and the five Compat Python 3.10-3.14 names stay live and untouched. Adds one job, required-checks (context "Required checks passed"), depending on test/integration/engine-tests/pypcap-parity with if: always() so it cannot itself be silently skipped -- the failure mode GitHub's own troubleshooting docs warn a bare needs: list falls into -- plus an explicit per-dependency check that names which job broke. Not breaking: workflow-only, no library or test code changes. The 17-of-22 ruleset replacement it enables is a separate repository setting this PR does not itself touch.
Please follow the guide below
make pylint,make mypy,make isort)make testpasses, and a test case covers the change — N/A, workflow-only; no library code changesWhat is the purpose of your pull request?
ci— workflows or build toolingDescription of your pull request and other information
Closes #845's remaining part — the matrix rebuild. The other two parts already landed (#846, and
promoting the engine checks to required in ruleset
23497679).engine-tests's job name changes fromEngines Python ${{ matrix.python-version }}toEngines Python ${{ matrix.python-version }} (${{ matrix.engine }}). Ruleset23497679matches onname, and it currently requires these five, which this PR stops producing:
A required check that never reports blocks merges permanently. So the ruleset's required list must
be changed in the same window as this merge — 22 contexts today, five of which become dead. The
new subset is the maintainer's call; the obvious candidate is the 3.10–3.14 cells excluding 3.15,
which is 30 of the 36 names. I have not touched the ruleset — that is yours.
The defect this fixes
Still live on
main(30bca999c):pyproject.toml:201declaresPyPCAPFile = [ "pypcapfile; python_version < '3.12'" ], so installing via the extra resolves tonothing on 3.12/3.13/3.14 — yet
Engines Python 3.12/3.13/3.14passed and, since the earlierpromotion, gated merges while exercising an engine that was not installed.
The matrix, enumerated independently by me from the committed YAML
6 Python versions × 6 engines = 36 cells, 10
includeoverrides, tallying:supportedunsupportedPyPCAPFile3.12/3.13/3.14/3.15,PyShark3.14/3.15 — installed by rawpip install <dist>to bypass the marker; the engine's ownPYTHON_CEILINGdecline is what the tests assertnot-installablePyPCAP3.12–3.15 — real C-extension build failure, logged via::notice, test step skippedThe install step never passes silently — if a
not-installablecell ever installs, or asupported/unsupportedcell ever fails to, that is a loud failure, not a quiet one. Distinguishinga transient pip failure from the genuine compile failure a
not-installablecell expects is still aheuristic match against pip's captured output, so a false negative there is a loud false red
rather than a silent one — it still blocks every PR on that check, just honestly rather than
invisibly. (An earlier revision's transient-vs-compile grep was in fact backwards on real pip output —
an unanchored bare-number match fired on ordinary "(NNN kB)" download-size lines and on compiler
diagnostics quoting a line number, while matching none of the genuine compile-failure markers; fixed
to key on pip's own retry/network vocabulary instead, with the absence of a compile marker required
as a second check.) 3.15 is non-blocking via
continue-on-error: ${{ matrix.python-version == '3.15' }}plusallow-prereleasesonsetup-python.if: ${{ inputs.gate-only != true }}is retained, so the jobstill reports on every PR.
timeout-minutesdrops 45 → 30.test,integrationandpypcap-parityjob names are unchanged.Gaps stated rather than buried
HAS_RUNTIMEgate (needs DPKT+Scapy+PyShark together) and thevendorextra'sHAS_VENDOR_DEPScoverage as a side effect. Not restored here; worth its own issue. (Correctedfrom an earlier draft of this description:
cryptoandNGAPare not lost — thetestjobinstalls both and runs the whole unit tier regardless of this PR, and there is in fact no
HAS_NGAPgate anywhere intests/—grep -rl '\bHAS_NGAP\b' tests/returns nothing.)HAS_RUNTIME/vendorgap above:engine-tests's Scapy cell now runs an explicit test-path list rather than the old job'swhole-unit-tier selection, so it no longer reaches every
HAS_SCAPY-gated test that whole-tierselection used to.
tests/protocols/transport/test_sctp_unit.py's one method is restored — itspath is now in the Scapy cell's
TEST_PATHS, at zero cost, since that file carries noHAS_DPKT-gated test of its own. The other two, one method each intests/interface/test_core.pyand
tests/interface/test_misc.py, are not: no per-PR job now runs those with Scapy installed —recorded in
tests/_dependency_gates.py'sHAS_SCAPYexclusion rather than restored, since bothfiles also carry their own
HAS_DPKT-gated tests and adding them to the Scapy cell without DPKTwould trade one dark flag for another; worth its own review.
Scapy+DPKT together. Still covered in aggregate by
integrationandpypcap-parity.dpkt/scapy/pysharkhave installable 3.15prereleases at all; that raw
pip install pypcapfilesucceeds on 3.12+; that rawpip install pypcapgenuinely fails to build on 3.12+; the wall-clock cost of 36 legs vs 5; andthat the apt installs work on the 3.15 runner image. GitHub Actions cannot be exercised locally.
On the six 3.15 contexts
Left out of any required-check recommendation, deliberately.
python-compatibility.yml's owncomment records its 3.15 leg being moved to a schedule as non-gating queue contention, and that leg
was already
continue-on-error— a precedent against running 3.15 per-PR, not for it, and that legcost ~14s where each of these six pays apt + pip + pytest. The ruleset update above should stay
scoped to the 3.10–3.14 cells.