Skip to content

ci: stop scheduling the test-durations refresh - #9511

Merged
bolichen97 merged 1 commit into
mainfrom
chore/stop-test-durations-schedule
Sep 8, 2026
Merged

ci: stop scheduling the test-durations refresh#9511
bolichen97 merged 1 commit into
mainfrom
chore/stop-test-durations-schedule

Conversation

@iamwhatever

Copy link
Copy Markdown
Collaborator

Problem / Motivation

Update Test Durations runs the full unsharded suite every Monday to record
per-test runtime into .test_durations, so pytest-split can balance ci.yml's
four shards by recorded time instead of by test count.

Every value in that file changes on each refresh, so each run rewrote the whole
thing: the last one was +89,283 / -0 across a single 11.7 MB file. And it
could never land on its own — this org disallows GitHub Actions from opening
pull requests, so the job pushed its branch, downgraded the refusal to a
::notice::, and exited green with nothing to show. Both PRs it ever produced
(#7727, #8506) were closed unmerged, so .test_durations has never existed on
main and the balancing this job exists to enable has never actually been in
effect.

Why it matters

The cost is permanent: a weekly full-file rewrite of ~89k lines is git history
that cannot be reclaimed later, on the order of 90 MB/yr, plus a full unsharded
suite run and a PR that needs a human every week.

What it buys was never measured. So I measured it.

What changed (motivation → approach → change)

Measured on 2026-09-08 against the full 89,281-test recording from the branch
that last job produced:

split strategy shards (min) slowest
by recorded runtime (with the file) 18.1 / 18.1 / 18.1 / 18.1 18.1
by test count (no file — the fallback) 18.7 / 15.1 / 19.9 / 18.7 19.9

The file buys ~1.8 min on the critical path, against a shard cap of 40
minutes that already has 20 minutes of headroom. That does not justify the
recording cost, so the schedule goes.

Shrinking the file instead does not work, and this is recorded here so it is
not retried. pytest_split/algorithms.py charges any test missing from the file
sum(durations.values()) / len(durations) — an average over what survived the
trim, not over what was removed:

avg_duration_per_test = _get_avg_duration_per_test(durations)
(item, durations.get(item.nodeid, avg_duration_per_test)) for item in items

Keeping only tests >= 50 ms leaves 9.5% of entries and inflates that average
23x (15.7 ms real → 362.1 ms charged). The 80,794 dropped tests then
contribute 487 minutes of fake weight, pytest-split believes the suite takes
538.8 min rather than 72.4, the slow tests' real signal drowns, and the split
degrades to count-based anyway — while still carrying 8,487 entries.

The trigger becomes workflow_dispatch only rather than deleting the workflow,
so the measurement stays one click away if the suite ever grows enough to change
this arithmetic. The refresh job, its checkout config and its PR-opening step
are all untouched.

ci.yml is deliberately not modified: pytest-split already falls back to
an even count split when the file is absent, which is the state main has been
in all along, so --splits / --group keep working with no change. Four open
PRs currently touch ci.yml; staying out of that file keeps this diff
conflict-free.

Slow tests remain visible without any of this — setup.cfg's addopts already
carry --durations=5, so every pytest run, including each CI shard with
coverage on and under real runner contention, prints its slowest tests.

Tests

No new tests. Three existing suites assert on this workflow and all keep
passing unchanged, which is why the job was left intact rather than deleted:

  • test/test_ci_failure_annotations.py — asserts test-durations.yml:refresh removes the python problem matcher
  • test/test_coverage_omit_contract.py — scans every workflow, not just ci.yml
  • test/test_workflow_checkout_credentials.py — asserts this workflow's persist-credentials posture

Manual verification

  • yaml.safe_load on the edited file reports triggers ['workflow_dispatch'] — the schedule is gone and nothing else in the on: block changed.
  • Ran the three suites above: 36 passed.
  • Confirmed no other open PR touches .github/workflows/test-durations.yml (checked with --limit 500; the four that overlap touch only ci.yml).

Related Issues

no linked issue: CI cost cleanup found while investigating why this job's PRs
were never landing.

Pattern harvest

Rule candidate: review-prompt
Pattern: a scheduled job that swallows a permission refusal into a ::notice::
and exits 0 reports success forever while landing nothing — and the value of
what it produces goes unmeasured precisely because nobody sees it fail.

Checklist

  • At most two commits (one is the norm), with a Conventional Commits title
  • Existing tests pass and new tests added for new functionality
  • Self-review completed; code follows project style guidelines
  • Documentation updated (if applicable)
  • No secrets, credentials, or internal references in the diff

Balancing pytest-split's shards by recorded runtime is not worth what the
recording costs. Measured 2026-09-08 against a full 89,281-test file:

  * by recorded runtime, every shard lands at 18.1 min;
  * by test count -- what pytest-split falls back to with no file -- the
    shards are 18.7 / 15.1 / 19.9 / 18.7, slowest 19.9 min.

So ~1.8 min on the critical path, against a 40-minute shard cap that already
has 20 minutes of headroom. The price was an 11.7 MB JSON file in which every
value changes on each refresh, so each weekly run rewrote all 89k lines -- git
history that cannot be reclaimed -- plus a full unsharded suite run and a PR
that nobody could merge automatically anyway (the org disallows Actions
opening PRs, so the job pushed its branch and exited green with nothing to
show).

Trimming the file instead does not work, which is worth recording so it is not
retried: pytest-split charges any test missing from the file
`sum(kept)/len(kept)`, an average over what SURVIVED the trim, not over what
was removed. Keeping only tests >= 50 ms leaves 9.5% of entries and inflates
that average 23x (15.7 ms real -> 362.1 ms charged), so the 80,794 dropped
tests contribute 487 minutes of fake weight, the slow tests' real signal
drowns, and the split degrades to count-based regardless.

The trigger becomes workflow_dispatch only rather than being deleted, so the
measurement stays one click away if the suite grows enough to change the
arithmetic. The refresh job, its checkout config and its PR-opening step are
untouched, which also keeps the three tests that assert on this workflow
(test_ci_failure_annotations, test_coverage_omit_contract,
test_workflow_checkout_credentials) passing unchanged.

Slow tests remain visible without any of this: setup.cfg's addopts already
carry --durations=5, so every pytest run -- including each CI shard, with
coverage on and under real runner contention -- prints its slowest tests.
@iamwhatever
iamwhatever requested a review from a team as a code owner September 8, 2026 18:46
@github-actions github-actions Bot added the readiness: checking Automated validation is still running label Sep 8, 2026
@github-actions

github-actions Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

Design Review (Fable 5) — ✅ PASS

Design-level review of 5be0a05339b387d098bbd329803aa501db591eb0 — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

Verified: .test_durations is indeed absent on the base commit, so CI has been running the count-based fallback all along, and ci.yml needs no change. The diff is exactly what the description claims — schedule removed, workflow_dispatch kept, measurement recorded in the comment. Fully reversible (re-adding one cron line restores the old behavior).

Design-Verdict: PASS

Removes a measured-zero-value weekly cost while keeping the measurement one click away; reversible, evidence-backed, and correctly scoped away from ci.yml.

[DESIGN-REVIEWED] 5be0a05

@github-actions

github-actions Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

Opus 4.8 Review — ✅ no blocking findings

Reviewed 5be0a05339b387d098bbd329803aa501db591eb0 — this comment is updated in place on each push.

Review details

No findings.

[OPUS-REVIEWED] 5be0a05

Verdict parsed from the review's SHA-scoped output markers for commit 5be0a05339b387d098bbd329803aa501db591eb0.

False positive or not applicable? A repository writer can comment:
/ai-review override fable 5be0a05339b387d098bbd329803aa501db591eb0: <one-sentence reason>

@github-actions

github-actions Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

First Principles Review (Fable 5) — ✅ PASS

Premise-level review of 5be0a05339b387d098bbd329803aa501db591eb0 — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

The diff matches the description exactly: two hunks in one file — the cron trigger removed (workflow_dispatch kept) and the header comment rewritten to record the measurement. Every in-repo claim checks out: .test_durations has never existed on main, ci.yml's pytest-split flags work without it (that's the current state), setup.cfg carries --durations=5, and exactly three test files reference this workflow, all untouched by the change. The deleted comment pinned the old "re-measures periodically" decision, and the PR overturns it with evidence (a measurement plus the verified fact that the job's output never landed). Nothing rides along, nothing is undeclared.

First-Principles-Verdict: PASS

Only the author's local shard timings (18.1 vs 19.9 min) are unverifiable here — but the removal stands without them: .test_durations was never on main.

What this change ships

Intent: stop paying weekly CI cost for a test-balancing file that never landed and buys ~2 minutes. This is a FIX (a subtraction).

Inventory (2 items)
  1. The weekly Monday durations refresh no longer runs; the workflow is manual-dispatch only — justified
  2. The workflow's header comment now records the measurement and why trimming the file fails — justified

[FIRST-PRINCIPLES-REVIEWED] 5be0a05

@github-actions

github-actions Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

GPT 5.6 Review — ✅ no blocking findings

GPT 5.6 completed its review of 5be0a05339b387d098bbd329803aa501db591eb0 and found no blocking issues.

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] 5be0a05

False positive or not applicable? A repository writer can comment:
/ai-review override gpt 5be0a05339b387d098bbd329803aa501db591eb0: <one-sentence reason>

@github-actions github-actions Bot added readiness: passed Eligible automated validation passed for the current revision and removed readiness: checking Automated validation is still running labels Sep 8, 2026
@bolichen97
bolichen97 merged commit 8c6992f into main Sep 8, 2026
64 checks passed
@bolichen97
bolichen97 deleted the chore/stop-test-durations-schedule branch September 8, 2026 19:56
@github-actions github-actions Bot removed the readiness: passed Eligible automated validation passed for the current revision label Sep 8, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants