test: rebuild the testing architecture — Vitest everywhere, self-provisioning integration DB, five-layer contract - #258
Open
leon0399 wants to merge 20 commits into
Open
test: rebuild the testing architecture — Vitest everywhere, self-provisioning integration DB, five-layer contract#258leon0399 wants to merge 20 commits into
leon0399 wants to merge 20 commits into
Conversation
One runner across the workspace, three explicit projects (unit / integration / e2e) instead of two Jest configs with pattern games: - unplugin-swc emits the decorator metadata NestJS DI needs (vitest's esbuild default silently resolves every provider as undefined) - integration and e2e projects FAIL LOUDLY without a database instead of green-skipping to zero tests - jest-e2e's forceExit is gone: suites run sequentially in one worker thread (the --runInBand equivalent) and exit cleanly - rls-test.sh drives the new pnpm scripts; the duplicate active-runs.integration invocation (already covered by the .integration glob since #132) is dropped - worker-harness now overrides POSTGRES_URL unconditionally from TEST_DATABASE_URL: ConfigModule leaks a developer's .env.local dev-DB URL into process.env, and the old !POSTGRES_URL guard silently pointed harness suites at the dev database - test:integration/test:e2e/test:evals turbo tasks are cache: false — their pass/fail depends on live DB and model state turbo cannot hash Verified: 507 unit, 192 integration (8 formerly-skipped worker tests now run), 62 e2e — all green against a provisioned worst-case-owner Postgres. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014BCPobQSCvqFqbgHk2aZzo
- src/**/*.spec.ts -> *.test.ts; *.integration.spec.ts -> *.integration.test.ts - worker.module.spec.ts reclassified as *.integration.test.ts — it needs a real Postgres and was silently self-skipping inside the unit suite - test/*.e2e-spec.ts -> e2e/*.test.ts (support.ts and the pg-boss schema-isolation setup move with it) - qa-evals extracted to evals/ as its own opt-in vitest project (test:evals), out of the e2e include Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014BCPobQSCvqFqbgHk2aZzo
Root e2e/ is product-level tests through a user-facing surface, one subdirectory per surface: e2e/web/ holds today's browser suites, a future surface (bot, CLI) adds its own e2e/<surface>/ instead of competing for the root. Shared boot infra (db/model servers, fixtures, auth helpers) moves to e2e/support/. App-scoped suites live with their app (apps/api/e2e) — they never belonged here. Verified: playwright test --list collects all 19 tests in 6 files. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014BCPobQSCvqFqbgHk2aZzo
scripts/check-test-naming.mjs scans git ls-files for the three violations the runners' include globs would silently ignore: ".spec" outside root e2e/, ".test" inside it, and the retired ".e2e-spec". Wired as a plain step in the checks job and a lefthook pre-commit job — deliberately not a turbo task, so per-package input hashing can never replay a stale pass for a repo-wide invariant. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014BCPobQSCvqFqbgHk2aZzo
…ce sweep The pyramid, the suffix-is-runner naming contract, placement rules (including what a new app or surface inherits), the no-silent-skip and no-cached-live-state rules, CI mapping, and the deliberate follow-up list. AGENTS.md files updated off the Jest wording; CHANGELOG records the rebuild. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014BCPobQSCvqFqbgHk2aZzo
…gration; drop the naming cop Three simplifications, one theme — replace bespoke governance with stock tools and fewer categories: - Testcontainers globalSetup provisions the throwaway worst-case-owner Postgres (non-superuser `app` owns + migrates, `app_rls` BYPASSRLS function owner) — `pnpm --filter api test:integration` is now the whole story, locally and in CI. rls-test.sh (150 lines of container bash) is deleted; TEST_DATABASE_URL still overrides for docker-less machines. - The api HTTP e2e layer is gone as a category: supertest suites are integration tests of the HTTP boundary, moved into src/ feature dirs as *.integration.test.ts (support helpers in src/testing/). One DB-backed project, one command; the per-suite PGBOSS_SCHEMA isolation now covers every integration file. - check-test-naming.mjs, its CI step, and its lefthook job are deleted: with one Vitest suffix and Playwright confined to root e2e/, directory does all the work — and nest-cli generateOptions.spec=false stops the scaffolder emitting .spec.ts files at the source. Also: maxWorkers=2 (a WSL2-local constraint) no longer hardcoded. Verified: merged integration project 30 files / 256 tests green via the TEST_DATABASE_URL path (container path exercises in CI); unit 507 green; typecheck/lint/build green; Playwright collects 19/6. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014BCPobQSCvqFqbgHk2aZzo
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014BCPobQSCvqFqbgHk2aZzo
Root conventions gain the one-line naming/placement rule; api commands surface test:integration as its own line and Structure lists src/testing/ and evals/; web gains the stories-vs-jsdom placement gotcha (its biggest guidance gap); storybook's test/ is named the sanctioned tooling-guard home; packages/ui states that play-function stories ARE the component-test layer. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014BCPobQSCvqFqbgHk2aZzo
…evel deeper Caught by a local full-suite run: the rls-function-owner.sql path resolved to e2e/docker/... after the e2e/support/ move, failing the db webServer at startup. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014BCPobQSCvqFqbgHk2aZzo
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
You do not have enough credits to review this pull request. Please purchase more credits to continue. |
Contributor
|
Important Review skippedToo many files! This PR contains 152 files, which is 52 over the limit of 100. To get a review, narrow the scope: Upgrade to a paid plan to raise the limit. This review couldn't start because sufficient usage credits or metered capacity aren't available. Add credits or update usage-based reviews in the billing tab, then retry. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (152)
You can disable this status message by setting the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
Mesa DescriptionTL;DRRebuilds the monorepo testing architecture, migrating What changed?Testing Infrastructure & Tooling Config
Core API & Service Tests (Jest to Vitest Port)
Browser E2E Tests (
|
…uires it) .node-version pins 22.23.1 — the version the nix flake's nodejs_22 already provides and this branch was verified against. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014BCPobQSCvqFqbgHk2aZzo
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3e45a6082a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "scripts": { | ||
| "build": "nest build && node dist/instance-config/prompt-built-runtime.contract.js && node dist/generate-openapi.js && prettier --write --log-level warn openapi.json", | ||
| "format": "prettier --write \"src/**/*.ts\" \"test/**/*.ts\"", | ||
| "format": "prettier --write \"src/**/*.ts\" \"e2e/**/*.ts\" \"evals/**/*.ts\"", |
Contributor
There was a problem hiding this comment.
When pnpm --filter api format runs, its working directory is apps/api, which has no e2e/ directory after the browser suites were moved to the repository-root e2e/. Prettier exits with status 2 and No files matching the pattern were found: "e2e/**/*.ts", so this workspace formatting command now always fails even when the matched files are correctly formatted; remove this glob or point it at an existing directory.
AGENTS.md reference: apps/api/AGENTS.md:L206-L206
Useful? React with 👍 / 👎.
Root cause of the 11-day-red browser-e2e job (measured, not guessed): the e2e api process fell back to the default worker profile — `runs` at concurrency 1 — while fullyParallel Playwright workers piled chat turns onto it. Turn-completion telemetry in the failing CI run shows the single slot's throughput degrading from ~8s/turn to ~24s/turn (for turns whose model latency is milliseconds), so streamed answers blew through 15–25s assertion timeouts while still queued. The failures grew over 11 days precisely because the gate was already red and nobody read the list. - llame.config.e2e.json now serves `runs` at concurrency 8 with poolSize 12 (>= concurrency, per docs/scaling.md's invariant) - the RUN_EXECUTION_MODE env in playwright.config.ts was dead — removed by the durable-run-workers refactor, silently doing nothing — deleted - docs/testing.md follow-ups gain the ai/test consolidation item: 16 sites forge streamText results via double-casts while two integration tests already show the official MockLanguageModelV3 pattern Remaining after this fix is expected to be real product regressions the red gate hid (org-units dialog from the #238 Base UI migration is the prime suspect) — tracked separately. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014BCPobQSCvqFqbgHk2aZzo
… gate red Two vocabulary changes shipped while the browser-e2e gate was already red, so their stale assertions were never noticed: - org-units: dialogs are scoped by `[data-state="open"]` — Radix vocabulary. Base UI (#238) marks open state with `data-open`, so the selector matched nothing and every dialog interaction timed out. Reproduced solo on an idle machine; the dialog itself opens fine. - tool-loop: the tool chip's resolved badge read "done" in the pre-#239 ToolCallPart; the AI Elements Tool chip says "Completed". The failure snapshot shows the whole flow working — chip rendered, follow-up answer streamed — with only the stale string unmatched. The remaining browser-e2e failures are real product bugs the red gate hid, tracked separately: #259 (duplicated partial+final answer paragraphs, strict-mode violations) and #260 (browser TypeError reading 'state' during tool runs). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014BCPobQSCvqFqbgHk2aZzo
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014BCPobQSCvqFqbgHk2aZzo
- fold the one-file evals vitest project into integration (qa-evals self-gates on RUN_MODEL_EVALS, so plain test:integration skips it — and it now inherits the self-provisioned database it previously lacked) - provision app_rls by executing docker/postgres/initdb/02-app-rls-role.sql instead of restating its CREATE ROLE inline (same pattern as the rls-function-owner.sql step beside it) - PGBOSS_SCHEMA: random per-suite name at module eval (setup files re-evaluate per file), replacing the expect.getState().testPath derivation and its hook-ordering caveat — same pattern as the worker harness - drop workers.all entries that restate BUILT_IN_DEFAULTS (config-loader merges per-group; asserted by its own test) - delete qa-evals' vi.setConfig (project testTimeout already 120s), collapse tsconfig.build's three vitest excludes into "vitest.*", trim over-explaining comments in playwright.config and the e2e config Verified: unit 507, integration 30 files/256 tests (+ qa-evals self-skip), typecheck, lint, build, playwright --list all green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014BCPobQSCvqFqbgHk2aZzo
…ema path - The docs told test:evals users to "bring POSTGRES_URL", but the integration globalSetup (deliberately) only honors TEST_DATABASE_URL as the provisioning opt-out and overwrites POSTGRES_URL — honoring ambient POSTGRES_URL instead would let an exported dev-DB URL silently receive integration-test writes, the accident class this branch eliminated. The docs now state the real contract: bring model credentials; the DB self-provisions, TEST_DATABASE_URL overrides. Stale project list in apps/api/AGENTS.md fixed alongside. - llame.config.e2e.json's $schema pointer gains the directory level the e2e/support/ move added, restoring editor validation. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014BCPobQSCvqFqbgHk2aZzo
Per docs/testing.md rule 5's rubric (leaf component, <=2 module mocks): compaction-boundary, effective-context-inspector, admin-section-nav, app-sidebar-nav, app-sidebar-admin-entry (desktop), message-fork-button, model-selector, chat-activity-indicator, and tool-cap-notice-part now carry stories with play functions; their jsdom tests are deleted or reduced to the part a story can't express (pure resolvers, a byte-for-byte live-vs-history HTML parity check, and the mobile useIsMobile branch — browser mode runs a fixed desktop viewport). Three new sb.mock seams (chat/runs, chat/fork, models/queries) follow the existing pins/active-runs pattern; the models mock re-implements its two pure helpers rather than re-exporting them, since sb.mock redirects that specifier back into the mock itself. Running these in a real browser with the a11y addon immediately caught three shipped defects jsdom could not see: - chat-activity-indicator: aria-label on a bare span (aria-prohibited-attr) — now a native <output>, which carries role=status - model-selector trigger: role=combobox is not a name-from-content role, so the picker was nameless to screen readers — now aria-labelled - model-selector popover: Base UI's role=dialog needs its own name (aria-dialog-name) — now aria-labelled Verified: web unit 46 files/291 tests, storybook 58 files/265 tests, web+storybook lint/typecheck green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014BCPobQSCvqFqbgHk2aZzo
Both bugs are one race. `useChat`'s built-in `resume` effect (@ai-sdk/react dist/index.mjs:221-225) has neither a cleanup function nor a re-entrancy guard, so React Strict Mode's double-invoked mount effect calls `resumeStream()` twice on the same Chat instance. The api replays the run for both requests, so two concurrent `makeRequest()` calls run against one shared `activeResponse`: - the first clears it (`this.activeResponse = void 0`) in its finally, and the second's finally dereferences it — `ai` reads `this.activeResponse.state.message` unguarded on the very line above the null-safe `this.activeResponse?.state.finishReason` (#260) - each call accumulates its own streaming message state, so the answer lands twice: a partial paragraph beside the complete one (#259) ChatPage now passes `resume: false` and drives `resumeStream()` from its own effect behind a ref set synchronously before the call, so the second Strict Mode invocation is a no-op. The component already remounts per chat id (key={chatId}), so each session gets a fresh ref and Chat. The e2e `page` fixture now fails any test that produced an uncaught client exception: #260 threw on every mid-run reload for weeks while CI only logged it, because nothing asserted on page errors. The underlying SDK fragility (no re-entrancy guard; the unguarded `activeResponse` read in the finally block) is worth reporting upstream — this guard is the app-layer fix. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014BCPobQSCvqFqbgHk2aZzo
The remaining browser-e2e failures were `GET :id/messages` returning non-OK late in the run, worsening as more tests passed — the signature of the global ThrottlerModule ceiling (300 req/min per IP), which every parallel Playwright worker shares from localhost: page loads, history fetches and 4s run polling all draw on one bucket. AUTH_RATE_LIMIT_PER_MINUTE only ever covered /auth/v1. The limit becomes env-tunable exactly like its auth counterpart (read once at boot; production keeps the strict default), and the harness raises it. The two assertions now report the status they got, so the next failure of this class names 429 instead of "expected true, received false". Refs #259, #260 (this is the last of the four stacked causes behind the 11-day-red gate — stale selectors, stale badge text, runs-queue starvation, and now the request ceiling). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014BCPobQSCvqFqbgHk2aZzo
The last browser-e2e failure was the first turn's "Reply ready" toast rendering over the composer and intercepting the send click until it auto-dismissed (Playwright named the interception outright). The toast is incidental to what this spec asserts, so it waits for it to clear — `toBeHidden` passes immediately when no toast is showing. That a toast blocks the Send button at all is a real (if minor) UX defect, filed as #262 rather than silently worked around here: toast placement is a design decision. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014BCPobQSCvqFqbgHk2aZzo
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
6 issues found and verified against the latest diff
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/web/lib/services/chat/__mocks__/runs.ts">
<violation number="1" location="apps/web/lib/services/chat/__mocks__/runs.ts:14">
P3: Add the missing `detail` property to the mock's `runQueryKeys` and align the `contextReceipt` computed key with the real module. The mock omits `runQueryKeys.detail` (a function on the real export), and the `contextReceipt` key uses `"context"` instead of `"context-receipt"`. Any story or component that accesses `runQueryKeys.detail` gets `undefined` at runtime.</violation>
</file>
<file name="apps/api/package.json">
<violation number="1" location="apps/api/package.json:23">
P2: The `test` script lost the `--maxWorkers=2` CPU constraint that limited Jest to 2 parallel workers. Without a Vitest equivalent (e.g. `--poolOptions.threads.singleThread` or `maxConcurrency` in the vitest config), the unit suite will use all CPU cores, which can cause resource contention on constrained CI runners or developer machines.</violation>
</file>
<file name="apps/web/app/(chat)/components/tool-cap-notice-part.stories.tsx">
<violation number="1" location="apps/web/app/(chat)/components/tool-cap-notice-part.stories.tsx:27">
P3: The play-function assertion `/8\/8/` uses a loose substring regex that would pass even if the component rendered `(18/8)` or `(8/80)` instead of `(8/8)`. Anchor the pattern, for example with `/\\(8\/8\\)/` or `getByText('8/8')` with `exact: true`, so a rendering regression on the displayed values is caught.</violation>
</file>
<file name="apps/api/vitest.integration.global-setup.mts">
<violation number="1" location="apps/api/vitest.integration.global-setup.mts:34">
P2: Integration runs use a mutable Postgres image tag while the repository pins its Postgres image elsewhere, so a tag refresh can change test behavior without a code change; using the pinned digest would keep the self-provisioned topology reproducible.</violation>
<violation number="2" location="apps/api/vitest.integration.global-setup.mts:34">
P2: A provisioning failure can leak the throwaway container and open postgres.js clients because cleanup is registered only after every setup step succeeds; wrapping provisioning in `try/finally` would close clients and stop the container on both success and failure.</violation>
</file>
<file name="apps/api/vitest.integration.setup.ts">
<violation number="1" location="apps/api/vitest.integration.setup.ts:9">
P3: Runs using `TEST_DATABASE_URL` permanently accumulate one pg-boss schema per integration file. Because the override path intentionally reuses an already-provisioned database, repeated integration runs leave queue tables and metadata behind and can eventually pollute or exhaust the shared test database. A per-file teardown or run-level cleanup for the generated schemas would keep the override path isolated.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| "test:debug": "node --inspect-brk -r tsconfig-paths/register -r ts-node/register node_modules/.bin/jest --runInBand", | ||
| "test:e2e": "jest --config ./test/jest-e2e.json", | ||
| "test:evals": "RUN_MODEL_EVALS=1 jest --config ./test/jest-e2e.json --testPathPattern qa-evals --silent=false", | ||
| "test": "vitest run --project unit", |
There was a problem hiding this comment.
P2: The test script lost the --maxWorkers=2 CPU constraint that limited Jest to 2 parallel workers. Without a Vitest equivalent (e.g. --poolOptions.threads.singleThread or maxConcurrency in the vitest config), the unit suite will use all CPU cores, which can cause resource contention on constrained CI runners or developer machines.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/api/package.json, line 23:
<comment>The `test` script lost the `--maxWorkers=2` CPU constraint that limited Jest to 2 parallel workers. Without a Vitest equivalent (e.g. `--poolOptions.threads.singleThread` or `maxConcurrency` in the vitest config), the unit suite will use all CPU cores, which can cause resource contention on constrained CI runners or developer machines.</comment>
<file context>
@@ -20,12 +20,10 @@
- "test:debug": "node --inspect-brk -r tsconfig-paths/register -r ts-node/register node_modules/.bin/jest --runInBand",
- "test:e2e": "jest --config ./test/jest-e2e.json",
- "test:evals": "RUN_MODEL_EVALS=1 jest --config ./test/jest-e2e.json --testPathPattern qa-evals --silent=false",
+ "test": "vitest run --project unit",
+ "test:watch": "vitest --project unit",
+ "test:integration": "vitest run --project integration",
</file context>
| return; | ||
| } | ||
|
|
||
| const container = await new PostgreSqlContainer('postgres:17-alpine').start(); |
There was a problem hiding this comment.
P2: Integration runs use a mutable Postgres image tag while the repository pins its Postgres image elsewhere, so a tag refresh can change test behavior without a code change; using the pinned digest would keep the self-provisioned topology reproducible.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/api/vitest.integration.global-setup.mts, line 34:
<comment>Integration runs use a mutable Postgres image tag while the repository pins its Postgres image elsewhere, so a tag refresh can change test behavior without a code change; using the pinned digest would keep the self-provisioned topology reproducible.</comment>
<file context>
@@ -0,0 +1,84 @@
+ return;
+ }
+
+ const container = await new PostgreSqlContainer('postgres:17-alpine').start();
+
+ const asSuperuser = postgres(container.getConnectionUri(), { max: 1 });
</file context>
Suggested change
| const container = await new PostgreSqlContainer('postgres:17-alpine').start(); | |
| const container = await new PostgreSqlContainer('postgres:17-alpine@sha256:dc17045ccfd343b49600570ea734b9c4991cf1c3f3302e67df51e3b402dd55c4').start(); |
| return; | ||
| } | ||
|
|
||
| const container = await new PostgreSqlContainer('postgres:17-alpine').start(); |
There was a problem hiding this comment.
P2: A provisioning failure can leak the throwaway container and open postgres.js clients because cleanup is registered only after every setup step succeeds; wrapping provisioning in try/finally would close clients and stop the container on both success and failure.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/api/vitest.integration.global-setup.mts, line 34:
<comment>A provisioning failure can leak the throwaway container and open postgres.js clients because cleanup is registered only after every setup step succeeds; wrapping provisioning in `try/finally` would close clients and stop the container on both success and failure.</comment>
<file context>
@@ -0,0 +1,84 @@
+ return;
+ }
+
+ const container = await new PostgreSqlContainer('postgres:17-alpine').start();
+
+ const asSuperuser = postgres(container.getConnectionUri(), { max: 1 });
</file context>
| // vi.mock seam); the remaining exports are inert stand-ins so any other | ||
| // storied component importing this module stays off the network. | ||
|
|
||
| export const runQueryKeys = { |
There was a problem hiding this comment.
P3: Add the missing detail property to the mock's runQueryKeys and align the contextReceipt computed key with the real module. The mock omits runQueryKeys.detail (a function on the real export), and the contextReceipt key uses "context" instead of "context-receipt". Any story or component that accesses runQueryKeys.detail gets undefined at runtime.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/web/lib/services/chat/__mocks__/runs.ts, line 14:
<comment>Add the missing `detail` property to the mock's `runQueryKeys` and align the `contextReceipt` computed key with the real module. The mock omits `runQueryKeys.detail` (a function on the real export), and the `contextReceipt` key uses `"context"` instead of `"context-receipt"`. Any story or component that accesses `runQueryKeys.detail` gets `undefined` at runtime.</comment>
<file context>
@@ -0,0 +1,38 @@
+// vi.mock seam); the remaining exports are inert stand-ins so any other
+// storied component importing this module stays off the network.
+
+export const runQueryKeys = {
+ all: ["runs"] as const,
+ contextReceipt: (runId: string) => ["runs", runId, "context"] as const,
</file context>
| args: { stepsUsed: 8, maxSteps: 8 }, | ||
| play: async ({ canvasElement }) => { | ||
| const canvas = within(canvasElement); | ||
| await expect(canvas.getByText(/8\/8/)).toBeVisible(); |
There was a problem hiding this comment.
P3: The play-function assertion /8\/8/ uses a loose substring regex that would pass even if the component rendered (18/8) or (8/80) instead of (8/8). Anchor the pattern, for example with /\\(8\/8\\)/ or getByText('8/8') with exact: true, so a rendering regression on the displayed values is caught.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/web/app/(chat)/components/tool-cap-notice-part.stories.tsx, line 27:
<comment>The play-function assertion `/8\/8/` uses a loose substring regex that would pass even if the component rendered `(18/8)` or `(8/80)` instead of `(8/8)`. Anchor the pattern, for example with `/\\(8\/8\\)/` or `getByText('8/8')` with `exact: true`, so a rendering regression on the displayed values is caught.</comment>
<file context>
@@ -0,0 +1,30 @@
+ args: { stepsUsed: 8, maxSteps: 8 },
+ play: async ({ canvasElement }) => {
+ const canvas = within(canvasElement);
+ await expect(canvas.getByText(/8\/8/)).toBeVisible();
+ await expect(canvas.getByText(/Tool step limit reached/)).toBeVisible();
+ },
</file context>
Suggested change
| await expect(canvas.getByText(/8\/8/)).toBeVisible(); | |
| await expect(canvas.getByText(/\(8\/8\)/)).toBeVisible(); |
| * name here is unique per suite — same pattern as worker-harness.ts. Suites | ||
| * that provision their own schema simply override it. | ||
| */ | ||
| process.env.PGBOSS_SCHEMA = `pgboss_${Date.now()}_${Math.random().toString(36).slice(2, 8)}`; |
There was a problem hiding this comment.
P3: Runs using TEST_DATABASE_URL permanently accumulate one pg-boss schema per integration file. Because the override path intentionally reuses an already-provisioned database, repeated integration runs leave queue tables and metadata behind and can eventually pollute or exhaust the shared test database. A per-file teardown or run-level cleanup for the generated schemas would keep the override path isolated.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/api/vitest.integration.setup.ts, line 9:
<comment>Runs using `TEST_DATABASE_URL` permanently accumulate one pg-boss schema per integration file. Because the override path intentionally reuses an already-provisioned database, repeated integration runs leave queue tables and metadata behind and can eventually pollute or exhaust the shared test database. A per-file teardown or run-level cleanup for the generated schemas would keep the override path isolated.</comment>
<file context>
@@ -0,0 +1,9 @@
+ * name here is unique per suite — same pattern as worker-harness.ts. Suites
+ * that provision their own schema simply override it.
+ */
+process.env.PGBOSS_SCHEMA = `pgboss_${Date.now()}_${Math.random().toString(36).slice(2, 8)}`;
</file context>
A retry always restarts its worker, and Playwright reuses the worker SLOT index (`parallelIndex`) when it does — but `workerAccount` registers a fresh user per worker process. The storageState cache was keyed by slot and short-circuits on existence, so a retry's new account inherited the previous account's cookies: the browser acted as user A while the spec's Bearer requests authenticated as user B, and the owner-scoped `GET /chats/:id/messages` correctly 404'd on a chat the page had just created. Keying by `workerAccount.id` ties the cached session to the account it belongs to. This is the "Bearer GET /messages returns 404" signature that had been written off as a local-environment quirk; it was a harness bug all along, only visible once retries were the common path. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014BCPobQSCvqFqbgHk2aZzo
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="e2e/support/fixtures.ts">
<violation number="1" location="e2e/support/fixtures.ts:100">
P3: Keying the cached-auth file by the per-worker account id fixes the retry cookie bug (good), but it also makes the `.auth/` file names non-deterministic per run, so the `.auth/` directory under `project.outputDir` grows by one file per worker on every CI/local run and is never reused or cleaned up — previously `parallelIndex` names overwrote the same bounded set of files between runs. Consider removing the per-worker auth files at worker teardown (or during global setup) to avoid unbounded accumulation.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| const fileName = path.resolve( | ||
| test.info().project.outputDir, | ||
| `.auth/${test.info().parallelIndex}.json`, | ||
| `.auth/${workerAccount.id}.json`, |
There was a problem hiding this comment.
P3: Keying the cached-auth file by the per-worker account id fixes the retry cookie bug (good), but it also makes the .auth/ file names non-deterministic per run, so the .auth/ directory under project.outputDir grows by one file per worker on every CI/local run and is never reused or cleaned up — previously parallelIndex names overwrote the same bounded set of files between runs. Consider removing the per-worker auth files at worker teardown (or during global setup) to avoid unbounded accumulation.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At e2e/support/fixtures.ts, line 100:
<comment>Keying the cached-auth file by the per-worker account id fixes the retry cookie bug (good), but it also makes the `.auth/` file names non-deterministic per run, so the `.auth/` directory under `project.outputDir` grows by one file per worker on every CI/local run and is never reused or cleaned up — previously `parallelIndex` names overwrote the same bounded set of files between runs. Consider removing the per-worker auth files at worker teardown (or during global setup) to avoid unbounded accumulation.</comment>
<file context>
@@ -88,9 +88,16 @@ export const test = baseTest.extend<Fixtures, WorkerFixtures>({
const fileName = path.resolve(
test.info().project.outputDir,
- `.auth/${test.info().parallelIndex}.json`,
+ `.auth/${workerAccount.id}.json`,
);
</file context>
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.
Summary
Rebuilds the testing architecture around one explicit contract (docs/testing.md), replacing four runners and three meanings of
.spec.tswith a five-layer pyramid built from stock tools. Design was validated by a three-model review panel against twelve placement scenarios before implementation, then simplified a second time toward standard solutions.What changed
apps/api(80 files):unplugin-swcsupplies the decorator metadata Nest DI silently loses under esbuild; ts-jest, the ESMtransformIgnorePatternshacks,jest-e2e.json,forceExit, and six Jest devDependencies are gone.*.test.ts(x)is Vitest everywhere (.integrationinfix = needs real Postgres); roote2e/is Playwright's island with its.spec.tsconvention.nest gno longer scaffolds spec files.approle owns the schema and runs migrations,app_rlsBYPASSRLS function owner) — sopnpm --filter api test:integrationis the whole story locally and in CI, andrls-test.sh(150 lines of container bash) is deleted.TEST_DATABASE_URLoverrides for docker-less machines. Nothing silently skips anymore: the old DB-backed suites green-skipped to zero tests without env.src/feature modules (src/testing/holds shared helpers). One DB-backed project, one command.e2e/per product surface:e2e/web/suites,e2e/support/boot infra; a future non-browser surface addse2e/<surface>/.test:integration/test:evalsarecache: false— their outcome depends on live state turbo's hash cannot see.Real bugs found by the migration
worker-harness.tslet.env.local's dev-database URL (leaked intoprocess.envbyConfigModule.forRoot) win overTEST_DATABASE_URL— worker suites would have run against the dev database. Now an unconditional override.rls-test.shranactive-runs.integrationtwice (redundant since feat(web): background run-completion notifications, surviving a page reload #132).worker.module.spec.tsrequired a real Postgres while silently self-skipping inside the unit suite — reclassified.Verification
turbo run test).TEST_DATABASE_URLoverride path).Deliberate follow-ups (docs/testing.md § Follow-ups)
Story-migration of eligible
apps/webcomponent tests per the ≤2-mocks rubric; replacing the source-regex hydration test; removing deaddescribeIfDbguards; re-enabling fourvitest/*oxlint rules.Summary by cubic
Rebuilds the testing architecture with
vitestacross the repo and a self‑provisioned Postgres viaTestcontainers, merging API HTTP e2e into the integration layer. Also migrates eligibleapps/webtests to Storybook play tests, fixes the double‑resume race in chat, and further stabilizes browser e2e by raising the API rate‑limit ceiling for the harness.Refactors
apps/api: Jest →vitestviaunplugin-swc; removed Jest configs/deps; tsconfig typesvitest/globals;.oxlintrcswitches env tovitestand adds thevitestplugin.*.test.ts(x)for unit;.integration.test.tsneeds a real DB; roote2e/*.test.tsstays@playwright/test;nest-cli.jsonstops scaffolding specs.Testcontainers(ownerapp+app_rlsinit SQL);TEST_DATABASE_URLoverride still works; removedapps/api/scripts/rls-test.sh.src/**/.integration.test.tswith shared helpers insrc/testing/; one DB-backed project, one command.qa-evalslives in the integration project, stays opt‑in behindRUN_MODEL_EVALS; docs state the self‑provisioned DB contract.PGBOSS_SCHEMArandomized at module eval to prevent cross‑file job stealing.turbomarkstest:integration/test:evalsascache:false; Node pinned to 22.23.1.apps/web: migrate low‑friction component tests to Storybook play tests; reduce jsdom tests where stories cover behavior; addsb.mockseams for chat/runs, chat/fork, and models/queries.Bug Fixes
worker-harness.ts: forceTEST_DATABASE_URLto override.env.local’sPOSTGRES_URL.active-runs.integrationinvocation; DB-backed suites now fail loudly when misconfigured.runsqueue at real concurrency (8) and remove deadRUN_EXECUTION_MODE; update selectors and chip text for Base UI/AI Elements.API_RATE_LIMIT_PER_MINUTE(env-tunable, default 300) and raise it for the harness to avoid shared-IP 429s; assertions now report received status codes.$schemapath after thee2e/support/move.<output>with role=status; model selector trigger and dialog are labelled.resume: falseand drivesresumeStream()behind a guard), fixing duplicated answers and aTypeErroron reload; e2epagefixture now fails tests on uncaught client exceptions.GET /chats/:id/messages.Written for commit 1d285c0. Summary will update on new commits.
Update: browser-e2e investigated and mostly revived in this PR
The job had been red on master for 11 consecutive days (since Jul 20). CI-history bisection + failure-artifact analysis found four independent causes stacked behind the already-red gate:
[data-state="open"]; Base UI (Migrate packages/ui from Radix to Base UI (base-nova) + canonical re-surface #238) usesdata-open, so the selector matched nothing. Fixed here."done"; AI Elements says"Completed". The failure snapshot shows the whole tool flow rendering correctly. Fixed here.RUN_EXECUTION_MODEenv was removed by the durable-workers refactor and silently did nothing); measured throughput decayed 8→24s/turn during runs. Now concurrency 8 viallame.config.e2e.json. (Necessary, but experimentally NOT the cause of the persistent failures.)TypeError: reading 'state'during tool runs).Result on this branch: browser-e2e went from 6–7 failures to 16 passed / 2 failed / 1 flaky in 2.5m — the remaining red is exactly the #259 family. All other jobs green (rls = the Testcontainers path, 2m).
Also queued in docs/testing.md follow-ups: consolidating model test doubles onto
ai/test's officialMockLanguageModelV3pattern (already used by two integration tests) instead of the 16 hand-forgedReturnType<typeof streamText>casts.