From c5756ed5f328ea7448173e7d54273fc6ba606147 Mon Sep 17 00:00:00 2001 From: Will Griffin Date: Wed, 5 Aug 2026 14:34:52 -0600 Subject: [PATCH] fix(ci): bound the mcp-conformance-fixture beforeAll with hookTimeout `packages/mcp-conformance-fixture/vitest.config.ts` set `testTimeout: 240_000` but no `hookTimeout`, so its `beforeAll` ran against vitest's 10s default. Hooks do not inherit `testTimeout`. Nearly all of that suite's work is in the hook, not the test bodies: it regenerates the MCP server with `MCPGenerator`, spawns a `tsx` child over stdio to negotiate the protocol and list tools, then spawns a second `tsx` child for the HTTP adapter. The two test bodies only issue requests against what the hook already built (25ms + 192ms locally, against 240s). On a loaded runner the hook measured 10,022ms against the 10,000ms default and ejected PR #2230 from the merge queue. The package sits in the shared `test-packages` shard, so the miss was never local to that PR: any PR in the queue could hit it, after a full merge-group validation cycle had already been spent. Verified rather than assumed. The hook takes no per-hook timeout argument; `vitest.workspace.ts` is a re-export shim, not a vitest workspace config; `smrtVitestPlugin` sets no timeouts; and CI passes no timeout flag or env. Nothing else was ever supplying a bound. Running the unmodified suite with `--hookTimeout=1` reproduces the CI signature exactly -- "Hook timed out in 1ms", 1 file failed, 2 tests skipped -- and a probe config carrying `hookTimeout: 1` fails the same way, confirming the config-level option is the operative one. Also documents the trap in TESTING_STANDARD.md and .claude/rules/testing.md. This is its third occurrence (smrt-svelte teardown in #1426, then #2220), and its signature is misleading: the file fails while every test body reports as skipped, so it does not read as a timeout in the test you budgeted for. Swept the other 32 `vitest.config.ts` files that set `testTimeout` without `hookTimeout`. Only three packages have process-spawning or generation code anywhere in their test files -- smrt-app-mcp, smrt-dev-mcp, tenancy -- and in all three that work sits in test bodies, not hooks. Their hooks are in-process HTTP listens or in-memory sqlite seeds. Measured: smrt-app-mcp's conformance test 189ms against 15s, smrt-dev-mcp's 622ms against 30s. No sibling needs the same fix. Closes #2238 --- .claude/rules/testing.md | 3 +++ TESTING_STANDARD.md | 25 +++++++++++++++++++ .../mcp-conformance-fixture/vitest.config.ts | 13 +++++++++- 3 files changed, 40 insertions(+), 1 deletion(-) diff --git a/.claude/rules/testing.md b/.claude/rules/testing.md index 7ff244aca8..4279d9f243 100644 --- a/.claude/rules/testing.md +++ b/.claude/rules/testing.md @@ -3,6 +3,9 @@ ### Required Setup `vitest.config.ts` MUST include `smrtVitestPlugin()` in plugins array. Without it: "No field metadata" errors. +### Timeouts +Never set `testTimeout` without also setting `hookTimeout` — hooks do NOT inherit it and stay on vitest's 10s default. Symptom: the file fails with "Hook timed out in 10000ms" while every test reports *skipped*. Budget for where the work happens: setup that spawns processes or generates code is bounded by `hookTimeout`, not `testTimeout`. + ### Database - Use real in-memory SQLite for SmrtObject/SmrtCollection tests - `createIsolatedTestDb()` for transaction-isolated tests (rolls back on cleanup) diff --git a/TESTING_STANDARD.md b/TESTING_STANDARD.md index 8479920397..d63d486314 100644 --- a/TESTING_STANDARD.md +++ b/TESTING_STANDARD.md @@ -138,6 +138,31 @@ No field metadata found for 'Document'. **Note on watch mode**: The manifest is generated once at vitest startup. If you add new classes or fields while vitest is running in watch mode, restart vitest to pick up the changes. +### ⚠️ Never raise `testTimeout` without raising `hookTimeout` + +`testTimeout` and `hookTimeout` are independent. Raising only `testTimeout` +leaves every `beforeAll`/`beforeEach`/`afterAll`/`afterEach` on vitest's **10s +default**, so a slow hook fails the whole file while the test bodies report as +*skipped* — a failure mode that looks nothing like a timeout in the test you +were budgeting for. + +```typescript +test: { + testTimeout: 30000, + hookTimeout: 30000, // match testTimeout; hooks do not inherit it +} +``` + +This has bitten the repo repeatedly: smrt-svelte's teardown hook (#1426), and +`mcp-conformance-fixture`, whose `beforeAll` generates a server and spawns two +`tsx` children — it measured 10,022ms against the 10,000ms default and ejected +a PR from the merge queue (#2238). Packages run in shared CI shards, so a hook +that overruns takes unrelated PRs down with it. + +Budget by **where the work happens**, not by where the assertions are: if setup +does the expensive work (spawning processes, generating code, building +bundles), `hookTimeout` is the limit that matters. + ### Watch Mode ```bash diff --git a/packages/mcp-conformance-fixture/vitest.config.ts b/packages/mcp-conformance-fixture/vitest.config.ts index d536043279..4a5371e8cb 100644 --- a/packages/mcp-conformance-fixture/vitest.config.ts +++ b/packages/mcp-conformance-fixture/vitest.config.ts @@ -3,5 +3,16 @@ import { smrtVitestPlugin } from '../vitest/src/index.ts'; export default defineConfig({ plugins: [smrtVitestPlugin()], - test: { environment: 'node', testTimeout: 240_000 }, + test: { + environment: 'node', + testTimeout: 240_000, + // Match testTimeout. Nearly all of this suite's work happens in `beforeAll`, + // not in the test bodies: it regenerates the MCP server with `MCPGenerator`, + // spawns a `tsx` child over stdio to negotiate the protocol and list tools, + // then spawns a second `tsx` child for the HTTP adapter. Left at vitest's + // 10s `hookTimeout` default that hook measured 10,022ms on a loaded runner + // and ejected PR #2230 from the merge queue (#2238). This package sits in + // the shared `test-packages` shard, so the miss was not local to that PR. + hookTimeout: 240_000, + }, });