test: give middleware auth guard assertion a CI-safe timeout - #2720
Conversation
The unit project set no `testTimeout`, so it inherited vitest's 5s default while holding 47 files that boot a dev server or run a production build. 39 of those pass a per-test timeout (up to 180_000); the ones that did not sat on a ~20% margin — `middleware-matcher-auth.test.ts`'s auth-guard case measures 4.0s under normal parallel load — which ordinary CI variance erases. It and `typegen.test.ts` each failed a different CI run that way while passing locally. Adopts the 30000 the integration project already declares. A timeout costs wall-clock only when a test hangs, so nothing that passes slows down, and per-test arguments still take precedence. Closes cloudflare#2719
commit: |
Performance benchmarksCompared 0 improved · 0 regressed · 6 within ±1.5%
View detailed results and traces 🟢 improvement · 🔴 regression · ⚫ change below 1.5% · paired base/head |
james-elicx
changed the title
|
/bigbonk review for issues |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review
The change is minimal, correct, and well-reasoned. I verified the root-cause claim: tests/middleware-matcher-auth.test.ts is not listed in the integration project's include array in vite.config.ts (lines 247-288), so it runs under the unit project, which has no testTimeout override and therefore inherits Vitest's 5000ms default. Only the integration project sets testTimeout: 30000 (vite.config.ts:289).
The fix is scoped correctly:
- The development server block (line 150) does a lazy cold compile during the requests — its
beforeAllonly callsstartFixtureServerwith no pre-build (lines 136-143). This is the assertion that can exceed 5s, so giving it30_000is the right target. - The Node (line 194) and Cloudflare Worker (line 263) blocks pre-build in
beforeAll(with120_000/180_000hook timeouts), so their assertions are fast and correctly left at the default. 30_000matches the dev block's ownbeforeAllhook timeout (line 143) and the repo's integration-timeout convention.
The test uses createIsolatedFixture (mkdtemp-based isolated dirs), so it does not hit the shared-fixture deps-optimizer race that motivates the serial integration project — it legitimately belongs in the unit project.
LGTM. One optional observation below.
| it("blocks every path selected by group and constrained-repeat matchers", async () => { | ||
| await assertAuthGuard(baseUrl); | ||
| }); | ||
| }, 30_000); |
There was a problem hiding this comment.
Optional: consider hoisting the 30_000 into a shared const (e.g. const DEV_ASSERT_TIMEOUT = 30_000;) reused by both this assertion and its beforeAll hook on line 143, so the two stay coupled if either is tuned later. Not blocking — the inline literal is fine and matches the existing style in this file.
|
Review posted successfully. SummaryI reviewed PR #2720, which adds a Verdict: LGTM. The change is a correct, minimal, and well-scoped one-line fix. Key findings I verified:
I left one optional, non-blocking suggestion to hoist |
2 participants
Fixes #2719.
Root cause
The development-server auth-guard assertion in
middleware-matcher-auth.test.tsperforms a cold compile while issuing requests across every supported matcher shape. Its setup hook already had a 30-second timeout, but the assertion itself inherited Vitest's 5-second unit-test default.That assertion exceeded the default on CI in run 30242579875:
Under a full local
unit 3/3shard it completed in roughly 2.3 seconds, leaving too little margin for normal runner and cold-compile variance.Fix
Give only that integration-style assertion an explicit
30_000timeout, matching the existing timeout on its dev-server setup hook and the repository's convention for long-running tests.The unit project's 5-second default remains unchanged. This avoids extending the hang/performance-regression window for the thousands of ordinary unit tests that do not boot a server.
Verification
vp test run --project unit tests/middleware-matcher-auth.test.ts: 27 passedvp check tests/middleware-matcher-auth.test.ts vite.config.ts: cleanunit 3/3shard forced to the original 5-second default: 2,219 passed, 1 skipped locally; the affected assertion completed in roughly 2.3 seconds.