test(js): guard the hydration ceiling, and bound CI runtime - #174
Merged
Conversation
All 87 Bootstrap icons sit at a fixed #a7aaad while dashicons lighten on hover, go white on the current item, and follow the admin colour scheme. A mixed menu looks inconsistent exactly where the eye lands. Recorded as a decision to make rather than work to schedule, because the cheap options are approximations and the correct one is architectural. The constraint is core's: a data-URI background-image cannot resolve currentColor, and menu-header.php has no branch that would let a plugin emit inline SVG. 7.1's SVG Icon API does not change that (#162). Also records that there are two icon sets, not three — a third via the SVG Icon API would land with the same limitation. Detail and options in #172. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two things that both exist because a failure already happened. GUARD: FULLSCREEN_SETTLE_MS was silently reverted once. #164 raised it 3000 -> 10000 with measurements; #165 put it back, not by disagreeing but because a wholesale `git checkout <branch> -- assets/maestro.js` replaced the file from a branch predating the change. Every suite stayed green, because the ceiling only bounds a give-up path nothing exercises. It surfaced days later during an unrelated audit. The guard asserts a FLOOR of 5000ms, not the exact value: 10000 is a tuning number, and pinning it exactly would turn every legitimate re-tune into a failing test and teach people to edit the assertion. A floor catches what actually happened — a revert to a value too small to be safe — and leaves tuning free above it. Verified against the real regression: setting 3000 fails the guard with a message saying why. It reads the source text because maestro.js is a browser IIFE with no export surface. Same shape as the doc-links test. TIMEOUTS: PR #173's Integration job hung in `Install Playwright Chromium` — the same apt-get stall that burned six hours on Borges #79 — and no Maestro workflow had any timeout. Sized against 200 successful runs, not a recent window: Integration + E2E 1052s -> 35m Runtime + Plugin Check 158s -> 10m everything else <=90s -> 10m Plus 20m on the install step itself, which across 80 runs is 21-689s (median 25s, p95 97s) — ~1.7x its observed max. The tail is far longer than the typical run, so the median would have been the wrong basis. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two changes, both because the failure already happened once.
1. A guard for
FULLSCREEN_SETTLE_MSThis constant was silently reverted. #164 raised it 3000 → 10000 with measurements behind it. #165 put it back — not by disagreeing, but because a wholesale
git checkout <branch> -- assets/maestro.jsreplaced the file from a branch that predated the change.Every suite stayed green, because the ceiling only bounds a give-up path nothing exercises. It surfaced days later, by accident, during an unrelated audit.
The guard asserts a floor of 5000ms, not the exact value. 10000 is a tuning number, and pinning it exactly would turn every legitimate re-tune into a failing test — which teaches people to edit the assertion, and that is how a guard stops guarding. A floor catches what actually happened (a revert to a value too small to be safe) and leaves tuning free above it.
Verified against the real regression rather than assumed: setting the constant back to 3000 fails the guard with a message explaining the measurement and what to do if the change is deliberate.
It reads the source as text because
maestro.jsis a browser IIFE with no export surface — the same shape as the existing doc-links test.2. CI timeouts
PR #173's Integration job hung in
Install Playwright Chromium— the same apt-get stall that burned six hours on Borges #79 — and no Maestro workflow had any timeout. I cancelled and re-ran it manually; without a bound it would have run to GitHub's 6-hour default, on a required check.Sized against 200 successful runs, not a recent window (the mistake that produced four wrong numbers on the Borges equivalent):
Plus 20 minutes on the install step itself. Across 80 successful runs it is 21–689 s (median 25 s, p95 97 s), so the bound is ~1.7× its observed max. The tail is far longer than the typical run here — the median would have been the wrong basis, and Borges' equivalent step maxes at half this.
A step-level bound earns its place beyond the job bound: the log names the step that timed out, and the job ends Failed rather than Cancelled, which is a cleaner signal for a required check.
Still uncovered
Five other Maestro workflows have no timeout —
docs-links,playground-demo,release,review-gate,wp-deploy.wp-deployis the one I would do next: it pushes to WordPress.org over SVN, a third-party network call on the release path.Unit 167, JS 86 (up from 83), phpcs clean.
🤖 Generated with Claude Code