ci: queue the ordering cells after the unit legs, run Engines 3.15 on main only, drop Engines (DPKT) (#1538) (#1551)

Author: JarryShaw

.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

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 <https://github.com/JarryShaw/PyPCAPKit/issues/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
+  <https://github.com/JarryShaw/PyPCAPKit/issues/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 <https://github.com/JarryShaw/PyPCAPKit/issues/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
 ----------------------
 

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 --

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 []

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."""