From e184be0ec223e3094ea2a41bf7a91984247f94ab Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Fri, 9 Oct 2026 22:22:59 -0400 Subject: [PATCH] ci: queue the ordering cells after the unit legs, run Engines 3.15 on main only, drop Engines (DPKT) (#1538) - unittest-ordering: needs: [test], with !cancelled() leading its if:, so its 11 cells queue behind the push run's own legs instead of ahead of them, still run when test fails, and still fail the push run (#1052 item H unchanged). - engine-tests: the 3.15 step skips on pull requests; same name, still continue-on-error. Narrows #845's ruling to main pushes. - engine-tests: no DPKT cell; its only path, test_dpkt_unit.py, already runs in test with DPKT on 3.10-3.14. - Tests pin all three; tier-guard doctoring moves off the retired DPKT arms; workflows.rst documents the cuts. Refs #1538 --- .github/workflows/unit-tests.yml | 40 +++++++++++++------------- docs/source/contributing/workflows.rst | 30 +++++++++++++++++-- tests/_dependency_gates.py | 11 +++---- tests/project/test_unit_tests_queue.py | 34 ++++++++++++++++++++-- tests/test_tier_guard.py | 30 ++++++++++--------- 5 files changed, 102 insertions(+), 43 deletions(-) diff --git a/.github/workflows/unit-tests.yml b/.github/workflows/unit-tests.yml index d9def589f5..2cb06fac1b 100644 --- a/.github/workflows/unit-tests.yml +++ b/.github/workflows/unit-tests.yml @@ -443,8 +443,8 @@ jobs: # ever starts building, the cell fails loudly. # # One job per engine loops over the six interpreters, each in a fresh venv, - # rather than one job per cell (6 Pythons x 6 engines): the queue, not the - # runners, is the bottleneck, and 36 jobs would each pay checkout, setup and + # rather than one job per cell (6 Pythons x 5 engines): the queue, not the + # runners, is the bottleneck, and 30 jobs would each pay checkout, setup and # apt for a minute of work. One job per *engine* rather than per Python # because the engine is what varies the environment: the apt packages # are per engine (installed once per job instead of six times), and pcap-ct @@ -459,17 +459,17 @@ jobs: # the job summary. # # 3.15 is experimental-prerelease per #845's ruling: present so a regression - # is visible, but never blocking. It runs as its own step with step-level - # `continue-on-error: true`, so a 3.15 failure shows as a failed step and an - # error annotation while the job -- and `required-checks`, which reads this - # job's result -- stays green. The 3.10-3.14 cells share one step, and any - # one of them failing fails the job. + # is visible, but never blocking; since #1538 it runs on main pushes only. + # It is its own step with step-level `continue-on-error: true`, so a 3.15 + # failure shows as a failed step and an error annotation while the job -- + # and `required-checks`, which reads its result -- stays green. The 3.10-3.14 + # cells share one step, and any one of them failing fails the job. # - # Scoped deliberately out (#845): per-engine integration - # coverage (needs regenerated captures, so it stays in aggregate in - # `integration` and `pypcap-parity`), and the multi-engine HAS_RUNTIME gate - # plus the incidental crypto/NGAP/vendor coverage the pre-#845 shared venv - # carried. + # Scoped deliberately out (#845): per-engine integration coverage (needs + # regenerated captures, so it stays in aggregate in `integration` and + # `pypcap-parity`), the multi-engine HAS_RUNTIME gate and the incidental + # crypto/NGAP/vendor coverage of the pre-#845 shared venv. #1538 dropped the + # DPKT cell: it ran only test_dpkt_unit.py, which `test` runs with DPKT. engine-tests: name: Engines (${{ matrix.engine }}) if: ${{ inputs.gate-only != true && needs.changes.outputs.code != 'false' }} @@ -486,7 +486,6 @@ jobs: fail-fast: false matrix: engine: - - DPKT - Scapy - PyShark - PyPCAPFile @@ -731,7 +730,6 @@ jobs: # RAW_PKGS is an array, not a string, so PCAP_CT's two-package case # ("pcap-ct libpcap") reaches pip as two argv entries. 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 ;; @@ -787,7 +785,6 @@ jobs: fi 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" ;; @@ -834,8 +831,9 @@ jobs: exit 1 fi - # Non-blocking by #845's ruling -- see this job's comment above. + # Non-blocking by #845, main pushes only by #1538 -- see this job's comment. - name: Run the 3.15 cell (experimental, non-blocking) + if: ${{ github.event_name != 'pull_request' }} shell: bash continue-on-error: true run: | @@ -1191,10 +1189,12 @@ jobs: # maintainer's call to make separately from landing it. unittest-ordering: name: Plain unittest ordering (${{ matrix.label || matrix.leg }}) - # Not on pull requests: it is not a required check, and its 11 cells - # would be a sixth of every PR push's jobs. It still runs on every push - # to main, which is the run that tests what actually lands. - if: ${{ inputs.gate-only != true && github.event_name != 'pull_request' }} + # Every push to main and no PR (#1052 item H): not required, and its 11 + # cells would be a sixth of each PR push's jobs. `needs: [test]` (#1538) + # queues them behind this push's own legs; `!cancelled()` still runs them + # if `test` fails, so a red cell still fails the run that gates publishing. + if: ${{ !cancelled() && inputs.gate-only != true && github.event_name != 'pull_request' }} + needs: [test] runs-on: ubuntu-latest # 45, not 20: `timeout-minutes` is job-level, so it has to cover # checkout/setup/install (~39s) as well as the leg itself, and this is diff --git a/docs/source/contributing/workflows.rst b/docs/source/contributing/workflows.rst index 33298d14ae..8b05d16ba4 100644 --- a/docs/source/contributing/workflows.rst +++ b/docs/source/contributing/workflows.rst @@ -245,8 +245,8 @@ repository target the same reusable workflow, with the same input: not one: ``gate`` (the full suite, one Python version) and ``changelog`` (checks ``CHANGELOG.md`` against its source entry). It skips the other **eight** -- the five matrix jobs, ``test`` (five Python legs), -``integration`` (five), ``engine-tests`` (one job per engine, each looping -over six Python versions), ``pypcap-parity`` (two) and ``unittest-ordering`` +``integration`` (five), ``engine-tests`` (one job for each of five engines, +each looping over six Python versions), ``pypcap-parity`` (two) and ``unittest-ordering`` (eleven matrix cells), each of which has already run once for this commit from Unit Tests' own ``push`` trigger (and, all but ``unittest-ordering``, which runs on ``main`` pushes only, from its ``pull_request`` trigger), so @@ -480,6 +480,32 @@ while the pool stays saturated, and the first runs with JUnit will show whether ``W7``'s 10-second test line needs raising. What to do when one fires is on :doc:`conventions/process`. +Work Kept Off the Pull-Request Queue (`#1538 `__) +----------------------------------------------------------------------------------------------------- + +Three cuts in :file:`unit-tests.yml` keep work that cannot gate a merge from competing +with pull-request jobs for the 20-runner pool. None touches a required check or adds a +permission to the reusable workflow. + +* ``unittest-ordering`` runs on ``main`` pushes only, per `#1052 + `__ item H, and has + ``needs: [test]``, so its eleven cells join the queue once ``test`` finishes rather + than when the push run is created, ahead of that run's own ``Python`` legs and of + pull-request jobs created later in a merge wave. Its ``if:`` opens with + ``!cancelled()``, so the cells still run when ``test`` fails, and a red cell still + fails the push run that GitHub Pages, Conda Update and Vendor Update (and so Create + Release) wait on. The cost is wall time: the push run, and those workflows, finish + about one slowest cell later, about 6 min before any queue wait (estimate). +* ``engine-tests``' ``Run the 3.15 cell (experimental, non-blocking)`` step runs on + ``main`` pushes only. This narrows `#845 `__'s + ruling -- present so a regression is visible, never blocking -- from every run to + every ``main`` push: a 3.15 regression shows within one merge. The step keeps + ``continue-on-error: true``. +* ``engine-tests`` has no ``DPKT`` cell. Its only path was + :file:`tests/toolkit/test_dpkt_unit.py`, which ``test`` runs with ``DPKT`` installed + on 3.10-3.14. Dropped with it: that file in a venv holding only ``test`` and ``DPKT``, + and on 3.15. + Required Status Checks ---------------------- diff --git a/tests/_dependency_gates.py b/tests/_dependency_gates.py index ae18ffc083..7e7392ebec 100644 --- a/tests/_dependency_gates.py +++ b/tests/_dependency_gates.py @@ -327,7 +327,8 @@ class Exclusion(NamedTuple): '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) 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 ' + 'the same 3.10-3.14 (plus non-blocking 3.15, main pushes only) 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 ' @@ -390,7 +391,7 @@ class Exclusion(NamedTuple): "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 ' + 'tests/toolkit/test_scapy_unit.py and four 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 ' @@ -546,7 +547,7 @@ class Exclusion(NamedTuple): "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 ' + 'installs all three together any more, and none of the five 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 ' @@ -601,7 +602,7 @@ class Exclusion(NamedTuple): '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 " + "explicit TEST_PATHS list per matrix.engine cell means none of the five 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 ' @@ -1319,7 +1320,7 @@ def _engine_matrix_variants(name: 'str', section: 'str') -> 'Optional[tuple[Job, 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 + confidence #745 exists to catch: it would credit the Scapy 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 -- diff --git a/tests/project/test_unit_tests_queue.py b/tests/project/test_unit_tests_queue.py index 5d7bf3f480..5807e36a22 100644 --- a/tests/project/test_unit_tests_queue.py +++ b/tests/project/test_unit_tests_queue.py @@ -10,8 +10,10 @@ * The ``changes`` classifier must call anything outside ``docs/`` and ``*.md`` code, and a Markdown file under a code directory code too. * ``unittest-ordering`` stays off pull requests and out of the required set, - and ``engine-tests``'s 3.15 cell stays non-blocking through a step-level - ``continue-on-error`` rather than a job-level one. + and queues behind ``test`` without needing it to pass (#1538); + ``engine-tests``'s 3.15 cell stays non-blocking through a step-level + ``continue-on-error`` rather than a job-level one, runs on ``main`` pushes + only, and the job has no ``DPKT`` cell (#1538). * The classifier, the engine cell loops and the apt retry are executed too: the classifier against a scratch merge commit, the others with their commands stubbed on ``PATH``. @@ -224,6 +226,32 @@ def test_engine_tests_is_one_job_per_engine_with_a_non_blocking_315_step(self) - self.assertNotIn('continue-on-error', gating) self.assertIn('for py in 3.10 3.11 3.12 3.13 3.14;', gating) + def test_unittest_ordering_queues_behind_test_and_survives_its_failure(self) -> None: + """#1538 P4(a): queued after ``test``, and ``!cancelled()`` so a red ``test`` still runs it. + + Without the status function the implicit ``success()`` would skip every cell + whenever ``test`` failed, and the push run would stop failing on ordering. + + """ + head = header(job('unittest-ordering')) + self.assertRegex(head, r'(?m)^ needs: \[test\]$') + self.assertIn(" if: ${{ !cancelled() && inputs.gate-only != true" + " && github.event_name != 'pull_request' }}\n", head) + + def test_the_315_cell_runs_on_main_pushes_only(self) -> None: + """#1538 P6: a step-level ``if:``, which narrows #845's ruling to ``main`` pushes.""" + experimental = step(job('engine-tests'), 'Run the 3.15 cell (experimental, non-blocking)') + self.assertIn(" if: ${{ github.event_name != 'pull_request' }}\n", experimental) + + def test_engine_tests_has_no_dpkt_cell(self) -> None: + """#1538 P8: its only path, test_dpkt_unit.py, already runs in ``test`` with DPKT.""" + section = job('engine-tests') + engines = re.search(r'(?m)^ engine:\n((?: - \w+\n)+)', section) + self.assertIsNotNone(engines) + declared = re.findall(r'- (\w+)', engines.group(1)) # type: ignore[union-attr] + self.assertNotIn('DPKT', declared) + self.assertNotRegex(section, r'(?m)^ +DPKT\)') + def step(section: 'str', name: 'str') -> 'str': """The text of the step called ``name`` in ``section``, up to the next step.""" @@ -368,7 +396,7 @@ def run_loop(self, name: 'str', fail: 'str') -> 'tuple[int, list[str]]': (self.tmp / 'engine-cell.sh').write_text( 'echo "$1" >> "$RUNNER_TEMP/calls"\n[ "$1" != "$FAIL_PY" ]\n', encoding='utf-8') status = bash(step_script(job('engine-tests'), name), - {'RUNNER_TEMP': str(self.tmp), 'ENGINE': 'DPKT', 'FAIL_PY': fail, + {'RUNNER_TEMP': str(self.tmp), 'ENGINE': 'Scapy', 'FAIL_PY': fail, 'GITHUB_STEP_SUMMARY': str(self.tmp / 'summary')}) calls = self.tmp / 'calls' return status, calls.read_text(encoding='utf-8').split() if calls.exists() else [] diff --git a/tests/test_tier_guard.py b/tests/test_tier_guard.py index 547a9a0575..fdf3ee4228 100644 --- a/tests/test_tier_guard.py +++ b/tests/test_tier_guard.py @@ -1646,7 +1646,7 @@ def test_the_mypy_gate_is_visible_and_dark_on_every_job_that_reaches_it(self) -> # 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 + # and none of the five 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'): @@ -1706,7 +1706,7 @@ def test_the_isort_gate_is_visible_and_closed_by_the_test_extra(self) -> None: '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 ' + 'no longer reaches it either -- none of its five 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()} @@ -2020,7 +2020,8 @@ def test_removing_dpkt_is_caught_on_every_job_that_installs_it(self) -> None: -- 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. + to reopen. Four jobs, not five. (#1538 later dropped that DPKT cell + outright: it ran only test_dpkt_unit.py, which ``test`` runs too.) """ text = _dependency_gates.WORKFLOW.read_text(encoding='utf-8') @@ -2337,20 +2338,20 @@ def test_a_test_paths_arm_with_no_matching_extra_arm_is_caught(self) -> None: 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 + different reason -- leaves Scapy declared in the ``engine:`` list and still + present in ``engine_paths`` (so the equality check sees the same five 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;') + doctored = doctored_workflow(self, 'Scapy) EXTRA=Scapy;', '# Scapy) EXTRA=Scapy;') with self.assertRaises(AssertionError) as ctx: _dependency_gates.pytest_jobs(doctored) message = str(ctx.exception) self.assertIn('engine-tests', message) - self.assertIn('DPKT', message) + self.assertIn('Scapy', 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. @@ -2364,17 +2365,20 @@ def test_a_commented_out_test_paths_arm_does_not_silently_win(self) -> None: commented-out arm from being read as a live one. """ + live = ('PyShark) TEST_PATHS="tests/toolkit/test_pyshark_unit.py ' + 'tests/foundation/engines/test_pyshark_engine.py" ;;') 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" ;;', + live, + live + '\n' + ' # PyShark) 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',)) + pyshark_jobs = [job for job in jobs if job.name == 'engine-tests' and 'PyShark' in job.extras] + self.assertEqual(len(pyshark_jobs), 1) + self.assertEqual(pyshark_jobs[0].paths, ('tests/toolkit/test_pyshark_unit.py', + 'tests/foundation/engines/test_pyshark_engine.py')) def test_the_undoctored_workflow_produces_no_unexplained_gap(self) -> None: """The control: the tests above fail for the doctoring, not by default."""