ci(unittest-ordering): shard the protocols leg into internet and the rest (#1029)

.github/workflows/unit-tests.yml

@@ -776,22 +776,66 @@ jobs:
   # this and the paragraph above.
   #
   # util/run_unittest_leg.py (its own docstring has the full reasoning) runs
-  # one tests/ subdirectory per matrix cell, paired with every root-level
+  # one tests/ subdirectory per matrix cell (for protocols, one shard of it,
+  # see #1029 below), paired with every root-level
   # tests/test_*.py module, directory first and root second -- the order
   # #981's own reproduction needs, since the defect is an earlier module
   # polluting a later one. Running the whole suite in one process is not an
   # option (it OOMs at 29 GB on the machine this was diagnosed on); a matrix
-  # of one leg per top-level directory is the affordable approximation, and it
+  # of one leg per top-level directory (protocols, since #1029, being two
+  # cells) is the affordable approximation, and it
   # is sized for wall time rather than for memory. Peak RSS was measured for
   # the three cheapest legs only -- cli 152 MiB, dumpkit 237 MiB, interface
   # 305 MiB (resource.getrusage on RUSAGE_CHILDREN, this venv) -- leaving the
   # largest of the three some 90x short of 29 GB; no figure was taken for
   # tests/protocols, the largest leg. Measured wall times (this venv, serial):
   # cli 18s, const 268s, dumpkit 39s, foundation 272s, interface 38s, project
-  # 70s, protocols 847s, toolkit 97s, utilities 42s, vendor 104s, one leg per
+  # 70s, protocols 847s (before the #1029 split), toolkit 97s, utilities 42s,
+  # vendor 104s, one leg per
   # matrix cell -- see `timeout-minutes` below for why the job-level budget is
   # no longer 20.
   #
+  # GitHub issue #1029: the protocols cell measured 30.4 minutes on a CI
+  # runner against this job's 45-minute cap (#1029 measured the next slowest
+  # leg at 6.2 minutes on CI), so it is now two cells: `protocols/internet` and the rest of
+  # tests/protocols (its other five subdirectories and the modules sitting
+  # directly in it), the latter expressed as `protocols` with
+  # `--exclude protocols/internet`. run_unittest_leg.py takes a sub-path and
+  # repeatable --exclude for this; every other cell is untouched.
+  #
+  # Measured on CI (GitHub Actions wall time per cell, one unit throughout):
+  #
+  #   leg                  run 37334173854     run 37341197811
+  #                        (#1031, unsharded)  (#1032, sharded)
+  #   protocols            27.8m               -
+  #   protocols/internet   -                   8.2m
+  #   protocols (rest)     -                   7.6m
+  #   foundation (control) 6.2m                6.4m
+  #   const (control)      6.0m                6.1m
+  #
+  # The baseline is 27.8m from a same-hour run, deliberately not #1029's
+  # headline 30.4m (its quiet-runner re-run, taken after a window of
+  # cancelled runs). The controls moved by 0.1-0.2m between the two runs, so
+  # the runners compare. Two cells rather than three because these already
+  # balance (8.2m against 7.6m) and the next slowest unsharded leg is
+  # foundation at 6.4m: a third cell could save under two minutes of wall time
+  # and would cost more co-running module pairs. No local per-module timings
+  # are recorded here; the figure above is the one that matters, and the
+  # pre-#1029 local serial table above is left as it was measured.
+  #
+  # What sharding costs: modules in different cells no longer share a process,
+  # so pollution from one to the other is invisible again. Counts below were
+  # derived with leg_modules() on main at 30714cd51 plus this change: 51
+  # modules in tests/protocols (12 under internet, 39 in the rest, of which 10
+  # sit directly in tests/protocols). "Pairs" are unordered, C(n,2), since a
+  # single serial run realises each pair in one order. Before: 1275 pairs
+  # co-ran in one process. After: 807 (internet x internet 66, rest x rest
+  # 741); 468 do not. The 255 pairs of a protocols module followed by a
+  # root-level module (51 x 5 roots) all survive, since every cell still runs
+  # the root modules after its own. The 10 modules sitting directly in
+  # tests/protocols stay in the rest cell: moving them into internet would keep
+  # 637 pairs instead of 807 and lengthen internet, the slower cell already.
+  #
   # tests/vendor was first left out of ``leg`` because running it this way
   # surfaced an instance of the very defect this job exists to catch (issue
   # #985): a stale VendorRuntimeWarning generation made assertWarnsRegex fail
@@ -827,7 +871,7 @@ jobs:
   # the required set once its own track record justifies it, which is the
   # maintainer's call to make separately from landing it.
   unittest-ordering:
-    name: Plain unittest ordering (${{ matrix.leg }})
+    name: Plain unittest ordering (${{ matrix.label || matrix.leg }})
     if: ${{ inputs.gate-only != true }}
     runs-on: ubuntu-latest
     # 45, not 20: `timeout-minutes` is job-level, so it has to cover
@@ -839,8 +883,17 @@ jobs:
     # contended run of it hit 1275s outright -- over the old 20-minute cap.
     # 45 matches the `test`/`integration`/`pypcap-parity` jobs' own cap
     # rather than inventing a new number (`engine-tests` is 30, but runs
-    # under xdist, which this job deliberately does not); splitting
-    # `protocols` into sub-legs is the alternative if 45 ever stops fitting.
+    # under xdist, which this job deliberately does not). 45 did stop fitting
+    # comfortably (#1029: protocols at 30.4 minutes on CI), so `protocols` is
+    # now split into sub-legs, as foreseen -- see the matrix comment above. The
+    # cap is left at 45: CI measured the slowest cell, protocols/internet, at
+    # 8.2m (run 37341197811), so even a slowdown like #1029's window leaves it
+    # far inside the cap: that window's runs were cancelled at 45.3m or more,
+    # so it ran at least 45.3/27.8, about 1.6x (a lower bound, the runs were
+    # cut off), which would put the 8.2m cell near 13m. Lowering the cap
+    # would only trade headroom for a tighter timeout. Raising it to 60
+    # instead was the one-line alternative; it would have kept every
+    # cross-module pair co-running but left the slowest cell at 28 minutes.
     timeout-minutes: 45
     strategy:
       fail-fast: false
@@ -852,10 +905,15 @@ jobs:
           - foundation
           - interface
           - project
-          - protocols
+          - protocols/internet
           - toolkit
           - utilities
           - vendor
+        include:
+          # The remainder of tests/protocols -- see the #1029 note above.
+          - leg: protocols
+            exclude: --exclude protocols/internet
+            label: protocols (rest)
 
     steps:
       - uses: actions/checkout@v7
@@ -880,7 +938,7 @@ jobs:
           python -c "import os; print('cpu_count', os.cpu_count())"
 
       - name: Run tests/${{ matrix.leg }} and the root-level modules under plain unittest
-        run: python util/run_unittest_leg.py ${{ matrix.leg }}
+        run: python util/run_unittest_leg.py ${{ matrix.leg }} ${{ matrix.exclude }}
 
   # ``CHANGELOG.md`` is generated from the newest entry under
   # ``docs/source/changelog/`` by ``util/changelog_md.py``, so it falls out of step

docs/source/contributing/releasing.rst

@@ -46,7 +46,7 @@ Editing ``__version__`` by hand without also moving :file:`CITATION.cff`'s
 (``:494-514``) asserts the two agree, and ``create-release.yml``'s
 ``unit-tests`` job (``:148-155``) calls ``unit-tests.yml`` with
 ``gate-only: true``, which runs the **full** suite rather than the tiered
-subset an ordinary push runs (its ``gate`` job, ``unit-tests.yml:931-1000``).
+subset an ordinary push runs (its ``gate`` job, ``unit-tests.yml:989-1058``).
 ``version_check`` depends on that job (``needs: [ unit-tests ]``, ``:160``), so
 a citation left behind fails the gate before ``version_check`` runs, and well
 before any approval is requested. Either run the script, or move both fields by
@@ -55,7 +55,7 @@ hand in the same commit.
 This is the **only** edit a person makes to get a release started -- the tag,
 the Release, and every upload are the workflow's job from here. Two other
 checks run unconditionally on the same path, though neither is a step in *this*
-process: the ``changelog`` job (``unit-tests.yml:904-925``) fails outright if
+process: the ``changelog`` job (``unit-tests.yml:962-983``) fails outright if
 ``CHANGELOG.md`` has drifted from its source entry under
 :file:`docs/source/changelog/`, and the release-body step warns, without
 failing, if that entry's heading still reads "unreleased"

docs/source/contributing/workflows.rst

@@ -207,14 +207,14 @@ not one: ``gate`` (the full suite, one Python version) and ``changelog``
 (checks ``CHANGELOG.md`` against its source entry). It skips the other
 **six** -- the five matrix jobs, ``test`` (five Python legs),
 ``integration`` (five), ``engine-tests`` (a Python x engine matrix),
-``pypcap-parity`` (two) and ``unittest-ordering`` (ten ``tests/`` legs),
+``pypcap-parity`` (two) and ``unittest-ordering`` (eleven matrix cells),
 each of which has already run once for this commit from Unit Tests' own
 ``push``/``pull_request`` triggers, so running any of them again per caller
 would test the same commit several times over -- and
 ``required-checks`` (see `Required Status Checks`_ below), gated out by its
 own ``if:`` rather than by having already run. ``changelog`` carries no
 ``if:`` at all and runs on every path regardless -- deliberately, per its own
-comment (``unit-tests.yml:892-898``): ``create-release.yml`` feeds
+comment (``unit-tests.yml:950-956``): ``create-release.yml`` feeds
 ``CHANGELOG.md`` to the GitHub Release body, so the release path is exactly
 where a drifted file must not go unchecked. Confirmed on run `36210743295
 <https://github.com/JarryShaw/PyPCAPKit/actions/runs/36210743295>`__ (a
@@ -310,7 +310,7 @@ grepping every workflow file for its name:
      - Where
    * - ``Required checks passed``
      - job ``required-checks``
-     - ``unit-tests.yml:1142``
+     - ``unit-tests.yml:1200``
    * - ``Compat Python 3.10``
      - job ``compatibility``, matrix leg ``3.10``
      - ``python-compatibility.yml:31,41``
@@ -328,16 +328,16 @@ grepping every workflow file for its name:
      - ``python-compatibility.yml:31,45``
 
 ``Required checks passed`` is defined by exactly one job
-(``unit-tests.yml:1142``). ``Compat Python`` is defined by two in
+(``unit-tests.yml:1200``). ``Compat Python`` is defined by two in
 ``python-compatibility.yml``: the required ``compatibility`` job (``:31``,
 ``Compat Python ${{ matrix.python-version }}``, expanding to the five required
 legs at ``:41-45``) and the non-required ``compatibility-nightly`` job (``:69``,
 ``Compat Python 3.15 (scheduled)``, see below). A comment in
-``unit-tests.yml`` (``:1003``) mentions the string without defining it.
+``unit-tests.yml`` (``:1061``) mentions the string without defining it.
 
 ``Required checks passed`` is itself an aggregate, not a single check run --
 but it stands in for **17** of the ruleset's originally-named 22 contexts,
-not all 22. ``unit-tests.yml``'s own comment (``:1002-1023``) accounts for the
+not all 22. ``unit-tests.yml``'s own comment (``:1060-1081``) accounts for the
 22: five each for ``test`` and ``integration``, five for the live ``Compat
 Python 3.10``-``3.14`` contexts (emitted by ``python-compatibility.yml``, a
 *different* workflow file -- nothing in ``unit-tests.yml`` could ever stand
@@ -348,11 +348,11 @@ stopped producing once it became a Python x engine matrix, and two for
 ``test``/``integration``/dead-``Engines``/``pypcap-parity`` slots -- 5 + 5 +
 5 + 2 = 17 -- via its own ``needs: [test, integration, engine-tests,
 pypcap-parity]`` plus an explicit per-dependency check
-(``unit-tests.yml:1141-1167``). The five ``Compat`` contexts stay required
+(``unit-tests.yml:1199-1225``). The five ``Compat`` contexts stay required
 exactly as they are and are listed separately in the table above. This job
 runs on Unit Tests' own ``push``/``pull_request`` triggers; the
 ``gate-only: true`` reusable calls documented above skip it via
-``if: ${{ always() && inputs.gate-only != true }}`` (``:1144``), so it is never
+``if: ${{ always() && inputs.gate-only != true }}`` (``:1202``), so it is never
 produced -- and never expected -- on those paths.
 
 ``Compat Python 3.10``-``3.14`` are five ordinary matrix legs, not an

tests/project/test_unittest_ordering_shards.py

@@ -0,0 +1,119 @@
+# -*- coding: utf-8 -*-
+"""Tests that the ``unittest-ordering`` matrix shards ``protocols`` without losing a module.
+
+#1029: the ``protocols`` cell of ``unittest-ordering`` measured 30.4 minutes
+against the job's ``timeout-minutes: 45`` while every sibling leg stays under
+about 6.2 minutes, so the cell was split into sub-legs. Splitting has one way
+to go quietly wrong, which is what this module pins: a module that falls between
+two shards is never run by that job again, and nothing fails to say so -- the
+job stays green while covering less. The assertions are therefore over the
+*union*, not over the current list of names:
+
+* the shards' module sets are disjoint and together equal the unsharded
+  ``protocols`` leg, so a new ``tests/protocols/<dir>/`` is either inside a
+  shard or fails here;
+* every other matrix cell is a bare direct child of ``tests/`` with no
+  ``--exclude``, i.e. runs exactly what it ran before the split.
+
+The workflow is read with a small hand-rolled scan rather than :mod:`yaml`,
+which is in no extra of :file:`pyproject.toml`; the same split
+:file:`tests/project/test_workflow_apt_timeouts.py` uses.
+
+"""
+
+from __future__ import annotations
+
+import pathlib
+import re
+import sys
+import unittest
+
+ROOT = pathlib.Path(__file__).resolve().parents[2]
+WORKFLOW = ROOT / '.github' / 'workflows' / 'unit-tests.yml'
+
+if str(ROOT / 'util') not in sys.path:
+    sys.path.insert(0, str(ROOT / 'util'))
+
+import run_unittest_leg as leg  # noqa: E402  pylint: disable=wrong-import-position
+
+
+def _matrix() -> 'list[tuple[str, list[str]]]':
+    """``(leg, exclude)`` for every cell of the ``unittest-ordering`` matrix."""
+    lines = []
+    inside = False
+    for raw in WORKFLOW.read_text(encoding='utf-8').splitlines():
+        code = raw.split('#', 1)[0].rstrip()
+        if re.match(r'  unittest-ordering:\s*$', code):
+            inside = True
+        elif inside and re.match(r'  \S', code):
+            break
+        elif inside:
+            lines.append(code)
+    text = '\n'.join(lines)
+
+    matrix = text.split('matrix:', 1)[1].split('steps:', 1)[0]
+    plain, _, extra = matrix.partition('include:')
+    cells = [(name, []) for name in re.findall(r'^\s+- (\S+)\s*$', plain.split('leg:', 1)[1], re.M)]
+    for block in re.split(r'^\s+- (?=leg:)', extra, flags=re.M)[1:]:
+        name = re.search(r'leg:\s*(\S+)', block).group(1)
+        flags = re.search(r'exclude:\s*"?([^"\n]*)"?', block)
+        cells.append((name, re.findall(r'--exclude\s+(\S+)', flags.group(1)) if flags else []))
+    return cells
+
+
+class TestShardedProtocolsLeg(unittest.TestCase):
+
+    def test_shards_partition_the_unsharded_leg(self):
+        shards = [leg.leg_modules(name, exclude) for name, exclude in _matrix()
+                  if name == 'protocols' or name.startswith('protocols/')]
+        self.assertGreater(len(shards), 1, 'protocols is no longer sharded (#1029)')
+
+        flat = [module for shard in shards for module in shard]
+        self.assertEqual(len(flat), len(set(flat)), 'a module runs in two shards')
+        self.assertEqual(set(flat), set(leg.leg_modules('protocols')),
+                         'a tests/protocols module falls between shards')
+
+    def test_every_other_cell_is_a_plain_direct_child(self):
+        others = [(name, exclude) for name, exclude in _matrix()
+                  if name != 'protocols' and not name.startswith('protocols/')]
+        self.assertEqual(sorted(name for name, _ in others),
+                         sorted(['cli', 'const', 'dumpkit', 'foundation', 'interface',
+                                 'project', 'toolkit', 'utilities', 'vendor']))
+        for name, exclude in others:
+            self.assertEqual(exclude, [], name)
+            self.assertEqual(leg.leg_modules(name),
+                             leg.leg_modules(name, ()), name)
+
+    def test_no_shard_is_empty(self):
+        for name, exclude in _matrix():
+            self.assertTrue(leg.leg_modules(name, exclude), name)
+
+
+class TestLegModulesSubpath(unittest.TestCase):
+
+    def test_subpath_is_a_subset_of_its_parent(self):
+        sub = leg.leg_modules('protocols/internet')
+        self.assertTrue(sub)
+        self.assertTrue(set(sub) <= set(leg.leg_modules('protocols')))
+        self.assertTrue(all(m.startswith('tests.protocols.internet.') for m in sub))
+
+    def test_exclude_removes_only_that_subtree(self):
+        full = leg.leg_modules('protocols')
+        rest = leg.leg_modules('protocols', ['protocols/internet'])
+        self.assertEqual(set(full) - set(rest), set(leg.leg_modules('protocols/internet')))
+        self.assertEqual([m for m in full if m in rest], list(rest), 'order changed')
+
+    def test_exclude_must_be_inside_the_leg(self):
+        with self.assertRaises(SystemExit):
+            leg.leg_modules('protocols', ['const'])
+        with self.assertRaises(SystemExit):
+            leg.leg_modules('protocols', ['protocols/no_such_dir'])
+
+    def test_leg_must_stay_inside_tests(self):
+        for bad in ('no_such_dir', '..', '../pcapkit', '.'):
+            with self.assertRaises(SystemExit, msg=bad):
+                leg.leg_modules(bad)
+
+
+if __name__ == '__main__':
+    unittest.main()