Skip to content

chore(test): land .test_durations so pytest-split shards balance by runtime - #7727

Closed
iamwhatever wants to merge 1 commit into
mainfrom
chore/update-test-durations
Closed

chore(test): land .test_durations so pytest-split shards balance by runtime#7727
iamwhatever wants to merge 1 commit into
mainfrom
chore/update-test-durations

Conversation

@iamwhatever

Copy link
Copy Markdown
Collaborator

Problem

ci.yml shards the backend suite with pytest-split and documents the intent at
line 649:

With a committed .test_durations file the split is balanced by recorded time
... without that file it falls back to an even split by test count.

That file has never existed on main. So every backend shard since sharding
landed has been balanced by test count, not runtime — the fallback path, not the
designed one.

The Update Test Durations workflow (.github/workflows/test-durations.yml) has
been running every Monday and doing its job correctly: it measures the full suite
and pushes this branch. It just cannot open the pull request, because this
repository has "Allow GitHub Actions to create and approve pull requests" turned
off (deliberately — the same switch also grants approving, and main's merge gate
is a required human review). The workflow already anticipates this and exits green
with a ::notice:: carrying a compare link, which is why it went unnoticed: the
scheduled run is green every week and the branch is pushed every week, but no PR
was ever opened and gh pr list --head chore/update-test-durations --state all
returns empty.

What this changes

Adds .test_durations — 77,781 recorded per-test durations, 60.5 minutes of
measured suite time, produced by the 2026-08-31 scheduled run. Nothing else.

pytest-split will now balance shards by recorded runtime, and brand-new tests are
auto-assigned the average duration so the split degrades gracefully between weekly
refreshes.

Honest scope note

This is a correctness fix for the sharding mechanism, not a large win on
today's numbers. Measured over the last five weeks, the four Backend Tests (3.12)
shards already land within 5% of each other (21.8 / 22.6 / 22.3 / 22.7 min at
week of 08-24), so count-based balancing happened to be close to time-based. The
value here is that the mechanism now works as designed and stays balanced as the
suite grows unevenly, rather than depending on that coincidence holding.

The actual driver of backend lane duration is separate and tracked elsewhere:
coverage instrumentation costs ~2x wall time on the 3.12 lane (measured 20.45s →
40.35s on a 1,517-test subset), which is why 3.12 takes 26.3 min and 3.10 takes
12.6 min on identical tests.

Tests

No code change; nothing to test. The file is consumed by pytest-split at shard
time, and the next CI run on this PR exercises it on all eight backend shards.

Manual verification

  • git ls-remote --heads origin refs/heads/chore/update-test-durations → branch
    present at ab0eb7c71, pushed 2026-08-31 07:52 UTC.
  • gh pr list --head chore/update-test-durations --state all → empty, confirming
    no PR was ever opened for any weekly run.
  • git log --all -- .test_durations → the single commit on this branch is the only
    one that has ever touched the file.
  • Parsed the file: 77,781 entries, 3,628s total, valid JSON.

Follow-up

The workflow will keep pushing to this branch every Monday and still cannot open
the PR itself. Once this merges, the next refresh can push to the same branch
while a PR is open (the push path works; only create is blocked), but after this
PR merges and the branch is deleted the create path is reached again and blocked
again. Two options worth a maintainer decision: mint a GitHub App token for this
workflow, or accept a standing manual step. Same constraint affects
cleanup-screenshots, memory-benchmark, and add-contributors.

@iamwhatever
iamwhatever requested a review from a team as a code owner September 1, 2026 21:00
@iamwhatever
iamwhatever requested a review from cixuuz September 1, 2026 21:00
@iamwhatever

Copy link
Copy Markdown
Collaborator Author

This file count is too crazy, cancel.

@iamwhatever iamwhatever closed this Sep 1, 2026
@iamwhatever
iamwhatever deleted the chore/update-test-durations branch September 1, 2026 21:17
@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Design Review (Fable 5) — ✅ PASS

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

Design-Verdict: PASS

Lands the exact artifact the documented sharding mechanism has been silently missing; scope, evidence, and the remaining delivery gap are all stated honestly.

[DESIGN-REVIEWED] ab0eb7c

@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Opus 4.8 Review — ✅ no blocking findings

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

Review details

The diff is exactly as the candidate file describes: a single new data file .test_durations containing autogenerated pytest duration mappings. No code, no logic, no security or governance surface. The discovery pass found no candidates, and there is nothing to falsify or extend — no code path executes anything from this file, and no AUTOSDE rule's file-patterns match a bare .test_durations data file.

No findings.

[OPUS-REVIEWED] ab0eb7c

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

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

@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

GPT 5.6 Review — ✅ no blocking findings

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

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] ab0eb7c

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

@JiaDe-Wu

JiaDe-Wu commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Your diagnosis here was right and is still true on main.test_durations has never existed there, chore/update-test-durations still carries it (c70e102a1, 2026-09-02, github-actions[bot]), and ci.yml:649 still describes the duration-balanced mode as though it were the active one.

What the thread never got was the number behind "is the fallback actually costing anything", and "77,783 lines is too crazy" is hard to weigh without it. So I measured it. Your instinct to cancel looks correct.

Controlled host, one machine, all four shards back to back, unpatched main (74eab0ca9), no .test_durations:

shard wall
1/4 360s
2/4 350s
3/4 298s
4/4 330s

Spread 1.21×; mean 334s; the critical-path shard sits 7.8% above the mean. Count-split is already close to balanced on this suite, so perfect duration balancing could recover about eight percent of one shard's wall time — for a file that rewrites on every test-timing change and conflicts with itself on any branch that adds tests.

And CI's own numbers say the imbalance you would be fixing is mostly not imbalance. Four recently merged PRs, Backend Tests (3.12, N):

PR 1 2 3 4
#8317 10 min 6 min 10 min 7 min
#8243 31 30 18 31
#8239 24 29 29 30
#8233 34 29 29 30

Two things fall out. The slow shard is not stable — it is 3 on #8243 and 2 on #8317 — which is not what a systematically mis-split suite looks like. And the same four-way split ranges from 6–10 min on one run to 29–34 min on another, so runner variance is 3–5×, against a split imbalance of 1.21×. Durations recorded on one runner cannot predict another when the host is that much noisier than the thing being balanced; that is the deeper reason this mechanism underdelivers here, independent of file size.

For what it is worth I arrived at this from the opposite direction and expected to argue the other way: a shard on one of my PRs (#8228) was cancelled after forty minutes of which twelve were silent, and I went looking for a mis-split as the cause. It was not that — the same shard runs in 5.5 minutes on a controlled host and the failure signature matches the memory-pressure class setup.cfg's own comment describes.

The one thing I would still change is a line, not a file: ci.yml:649 reads as if the recorded-time path is what runs today, so the next person reads a mechanism into the tree that is not there — which is exactly how you and I both ended up here. Saying plainly that no .test_durations is committed and the split is by test count would cost one sentence and settle it. Happy to send that if you would rather it came with the archaeology attached; it is your finding.

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.

2 participants