Skip to content

ci: weekly CI timing review and per-test timings from the unit legs (#1538) - #1542

Merged
JarryShaw merged 1 commit into
mainfrom
ci/1538-ci-timing-review
Oct 10, 2026
Merged

JarryShaw merged 1 commit into
mainfrom
ci/1538-ci-timing-review

Conversation

@JarryShaw

@JarryShaw JarryShaw commented Oct 10, 2026 •

Copy link
Copy Markdown
Owner

Please follow the guide below

What is the purpose of your pull request?

Tick the commit type your subject line carries.

  • fix — corrects a defect
  • feat — adds a feature
  • perf — changes performance, not behaviour
  • refactor — changes neither behaviour nor performance
  • test — tests only
  • docs — documentation only
  • ci — workflows or build tooling
  • release — bumps the version or rolls up a distribution
  • chore — anything else

Description of your pull request and other information

Refs #1538. P3 plus the periodic review; P1, P2 and P4–P8 are left out.

  • P3: the test and integration legs add --durations=30 --durations-min=1.0 --junitxml and upload junit-unit-<ver> / junit-integration-<ver> (14 days), each if: !cancelled() and continue-on-error: true. No job renamed, no permission added, --ignore flags unchanged, no python -m pytest in the new steps. Parity, ordering, gate and engine legs are not instrumented.
  • Size: pytest writes one <testcase> per test however many subtests it has, so a unit leg's file is about 0.6 MB (estimate), not several MB. The suite's tests= count includes subtests, so the review reads each testcase's time.
  • Review: ci-timing.yml (Mon 07:23 UTC + dispatch; actions: read, contents: read; no workflow_run) runs stdlib util/ci_timing.py. It lists every completed run in the window a day per query (a filtered listing stops at 1,000; Pages had 1,154 that week) and reads jobs for an evenly spaced sample, printing the span read; about 625 calls (estimate) of the 1,000/h GITHUB_TOKEN budget. A job counts by its own outcome, so a timeout in a cancelled run (37323913186) is measured. H1, H2 and H4 are absolute; H3 needs a baseline and fires on a 25% p90 rise, and the absolute 30-min breach is W9. Exit 1 HARD, 2 API, 3 crash.
  • Docs: CI Timing Review in workflows.rst and process.rst. The unit-tests.yml:N citations in workflows.rst and releasing.rst are renumbered (281→297, 1250-1256→1277-1283, 1262-1283→1289-1310, 1402-1471→1429-1498, 1473-1483→1500-1510, 1474→1501, 1558-1612→1585-1639, 1559→1586, 1561→1588).
  • Local run (at 3077eba23, --days 2, 487 calls): H2 fired (Plain unittest ordering (foundation) step p90 20.2 min against its 20-min step cap, so cells are hitting the cap). PR time to Required checks passed p90 was 36.7 min, which is now W9, not HARD. Trend fits need 2 days of span.
  • Tests: tests/project 466 passed, 1 skipped (pre-existing); test_tier_guard 109; test_docstring_contract 7. Reverting each fix fails its tests; the P3 pin tests fail on main's workflow.

@JarryShaw JarryShaw added ci Pull requests that change CI or workflow configuration (ci: subject prefix) review: running A cross-review is in flight against the current head - no verdict yet labels Oct 10, 2026
@JarryShaw JarryShaw moved this to In review in PyPCAPKit Oct 10, 2026
@github-actions

github-actions Bot commented Oct 10, 2026 •

Copy link
Copy Markdown
Contributor

Coverage: 90.09% (unit tier, Python 3.14, accde1cb1, Unit Tests run success)

Package Statements Missed Branches Partial Cover
pcapkit (top level) 104 4 20 4 93.55%
pcapkit/const 18795 842 2356 756 92.09%
pcapkit/corekit 2202 67 710 34 96.26%
pcapkit/dumpkit 258 2 90 2 98.85%
pcapkit/foundation 2650 98 934 42 94.98%
pcapkit/interface 112 7 40 5 92.11%
pcapkit/protocols 17009 229 4574 189 98.04%
pcapkit/toolkit 539 74 168 3 84.58%
pcapkit/utilities 431 4 124 4 98.56%
pcapkit/vendor 4409 2342 1006 157 43.25%

Per-file detail: the coverage-html artifact of this run.

@JarryShaw

Copy link
Copy Markdown
Owner Author

Verdict: NEEDS CHANGES at 3077eba23. Cross-reviewed on Sonnet; the author ran Opus. The P3 workflow edits, ci-timing.yml, the required contexts and the contents: read contract all pass. Four fixes in util/ci_timing.py, the docs and a test docstring:

  1. H3 will fail its first scheduled run. On the newest 100 successful PR runs, wall time is p50 16.2 min and p90 38.9 min (I re-measured), above the 30-min line the doc anchors on 19.6. Monday's run would email for queue saturation. Make H3 WARN until a baseline report exists, or recalibrate it and say why.
  2. The "7-day" window covers about 2–4 days. Since 2026-10-03 there are 253 completed push and 442 PR runs; the default --runs 150 reads 59% and 34% of them, while the header prints 7 days. Select by date, or report the span actually read.
  3. A job timeout inside a cancelled run is dropped. analyse (util/ci_timing.py:590) skips any run that is not success/failure, so a 45-min job timeout in a cancelled run (e.g. run 37323913186) never reaches H1/H4. Count jobs by their own conclusion, or document the gap.
  4. The artifact fixture is not a real response. tests/project/test_ci_timing.py:13-17 says it is, but artifacts-37998639834.json lists junit-unit-3.13/3.14 for a push at ffca64637, whose unit-tests.yml has no JUnit upload. Fix the docstring or use a real listing.

H2's live firing is real: three foundation-ordering steps hit the 20-min step cap (runs 37710074044, 37710423353, 37710750303) in the 2026-10-08 01:00Z burst. The exact 20.2 p90 was not reproduced.

Non-blocking: no test covers an empty job conclusion; W7 will fire weekly (slowest test 21 s vs 10 s); non-OSError exceptions exit 1, same as HARD.

@JarryShaw JarryShaw added review: needs-changes Cross-review at the current head says changes are required; see the verdict comment and removed review: running A cross-review is in flight against the current head - no verdict yet labels Oct 10, 2026
…1538)

- unit-tests.yml (P3): the `test` and `integration` legs run pytest with
  --durations=30 --durations-min=1.0 --junitxml and upload the file as
  junit-unit-<version> / junit-integration-<version> (14 days); each upload
  is `if: !cancelled()` and `continue-on-error`, so it cannot fail a leg
  `Required checks passed` needs. No job renamed, no permission added.
- ci-timing.yml: Mondays 07:23 UTC plus workflow_dispatch, read-only token
  (actions/contents: read), runs util/ci_timing.py against the newest earlier
  report as baseline and fails on a HARD threshold.
- util/ci_timing.py: stdlib-only; lists every completed run in the window a
  day per query and reads jobs for an evenly spaced sample; a job counts by
  its own outcome, so a timeout in a cancelled run is measured; H1-H4 (H3
  only as a regression on the baseline) and W1-W9; exit 1 HARD, 2 API, 3 crash.
- docs: CI Timing Review in workflows.rst and process.rst; the
  unit-tests.yml line citations in workflows.rst and releasing.rst
  renumbered.
- tests/project/test_ci_timing.py: offline against recorded fixtures.

tests/project 466 passed, 1 skipped; test_tier_guard 109; docstring contract 7.

Refs #1538
@JarryShaw
JarryShaw force-pushed the ci/1538-ci-timing-review branch from 3077eba to accde1c Compare October 10, 2026 01:06
@JarryShaw JarryShaw added review: running A cross-review is in flight against the current head - no verdict yet and removed review: needs-changes Cross-review at the current head says changes are required; see the verdict comment labels Oct 10, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

Verdict: GOOD TO GO at accde1cb1. This was a Sonnet delta review; the author ran Opus. All four round-1 findings are fixed.

  1. H3: it now fires only with a baseline (n ≥ 5), when p90 is ≥ 30 min and has risen ≥ 25% (util/ci_timing.py:941-948). Without a baseline the run gets W9 and exits 0. The boundary is exact: p90 39.82 against a baseline of 31.8 fires H3, and against 31.9 it gives W9. The 25% is documented as a starting value.
  2. Window: it lists one UTC day per query. created=A..B is inclusive at both ends, and the 10-08 plus 10-09 slices sum to the two-day total (93 + 139 = 232). A property test over 300 random windows found 0 mismatches. A day over the API's 1,000-result cap (Pages had 1,154) is noted. The budget is about 630–643 calls, which fits --max-calls 900.
  3. Own outcome: jobs count by their own conclusion, and a cancel at ≥ 95% of the cap counts as a timeout. jobs-37323913186.json matches the real run; I compared the protocols-ordering job's timestamps myself.
  4. Fixture docstring: fixed.

Fail-before: reproduced for every revert. Contracts: permissions, triggers and required contexts are unchanged. tests/project gives 466 passed and 1 skipped, and nothing under .github, util, tests/project or docs/source/contributing has changed on main since the base.

CI: PyPCAP/PyPCAPFile parity Python 3.11 failed with the #1534 flake. All 9 failures are dhcp_little_endian.pcapng/pyshark with TSharkCrashException … retcode: 255, and this PR touches no pcapkit/ code. I'll re-run the job once its run finishes; #1536 is the fix for the flake.

Non-blocking:

  • A baseline whose JSON no longer parses or matches exits 3 and uploads nothing, so the next run picks the same artifact again and fails until someone intervenes. A future report-schema change would do this.
  • The fixture keeps 1 of the run's 3 real artifacts, so the docstring's "real listing" is slightly generous.

@JarryShaw JarryShaw added review: good-to-go Cross-review at the current head says ready; CI state is separate and removed review: running A cross-review is in flight against the current head - no verdict yet labels Oct 10, 2026
@JarryShaw
JarryShaw merged commit 686cc37 into main Oct 10, 2026
62 of 64 checks passed
@github-project-automation github-project-automation Bot moved this from In review to Done in PyPCAPKit Oct 10, 2026
@JarryShaw
JarryShaw deleted the ci/1538-ci-timing-review branch October 10, 2026 01:58
@JarryShaw JarryShaw removed the review: good-to-go Cross-review at the current head says ready; CI state is separate label Oct 10, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ci Pull requests that change CI or workflow configuration (ci: subject prefix)

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

1 participant