Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 20 additions & 20 deletions .github/workflows/unit-tests.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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' }}
Expand All @@ -486,7 +486,6 @@ jobs:
fail-fast: false
matrix:
engine:
- DPKT
- Scapy
- PyShark
- PyPCAPFile
Expand Down Expand Up @@ -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 ;;
Expand Down Expand Up @@ -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" ;;
Expand Down Expand Up @@ -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: |
Expand Down Expand Up @@ -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
Expand Down
30 changes: 28 additions & 2 deletions docs/source/contributing/workflows.rst
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
----------------------

Expand Down
11 changes: 6 additions & 5 deletions tests/_dependency_gates.py
Original file line number Diff line number Diff line change
Expand Up @@ -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 '
Expand Down Expand Up @@ -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 '
Expand Down Expand Up @@ -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 '
Expand Down Expand Up @@ -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 '
Expand Down Expand Up @@ -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 --
Expand Down
34 changes: 31 additions & 3 deletions tests/project/test_unit_tests_queue.py
Original file line number Diff line number Diff line change
Expand Up @@ -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``.
Expand Down Expand Up @@ -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."""
Expand Down Expand Up @@ -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 []
Expand Down
30 changes: 17 additions & 13 deletions tests/test_tier_guard.py
Original file line number Diff line number Diff line change
Expand Up @@ -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'):
Expand Down Expand Up @@ -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()}
Expand Down Expand Up @@ -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')
Expand Down Expand Up @@ -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.
Expand All @@ -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."""
Expand Down
Loading