Skip to content

test: switch integration project to folder-based manifest (#1314)#1318

Merged
cliffhall merged 2 commits into
v2/mainfrom
v2/issue-1314-integration-folder-manifest
May 18, 2026
Merged

test: switch integration project to folder-based manifest (#1314)#1318
cliffhall merged 2 commits into
v2/mainfrom
v2/issue-1314-integration-folder-manifest

Conversation

@cliffhall
Copy link
Copy Markdown
Member

Summary

  • Replaces the explicit integrationTests array in clients/web/vite.config.ts with a folder glob: clients/web/src/test/integration/**/*.test.ts. Files under that folder run in the integration vitest project (node env, 30s timeouts); everything else stays in unit (happy-dom).
  • Removes the silent misclassification trap from Run v1.5-ported integration tests under a node-env vitest project (#1302 follow-up) #1307 — a new integration-style test no longer falls into the unit project just because someone forgot to register it.
  • Moves the 13 ported v1.5 tests + the helpers/oauth-client-fixtures.ts fixture under src/test/integration/ as a strict mirror of core/:
    • mcp/ (5 inspectorClient* tests), mcp/node/transport.test.ts, mcp/remote/{transport,server-config}.test.ts
    • storage/adapters.test.ts
    • auth/{discovery,state-machine}.test.ts, auth/node/{storage,oauth-callback-server}.test.ts
  • Renames use git mv so history is preserved (98–100% similarity).
  • tsconfig.test.json already globs src/test/**/*.ts, so no change needed there.

Acceptance criteria

  • Integration tests live in a path that the integration project's include glob owns, and unit's include excludes — no explicit enumeration.
  • A new file under src/test/integration/ runs in node env automatically; outside, happy-dom.
  • npm run test:integration passes (13 files, 331 tests — count grew from 298 since Run v1.5-ported integration tests under a node-env vitest project (#1302 follow-up) #1307 landed).
  • npm run validate passes (1108 unit + 331 integration tests, lint/build/format/coverage gate clean).
  • npm run test:storybook passes (67 files, 298 tests).

Test plan

  • npm run test — unit suite (1108 passed)
  • npm run test:integration — integration suite (331 passed)
  • npm run validate — format/lint/build/coverage all green
  • npm run test:storybook — story plays pass

Closes #1314

🤖 Generated with Claude Code

Replace the `integrationTests` enumeration in vite.config.ts with a
folder glob (`src/test/integration/**/*.test.ts`). Files under that
folder run in the integration vitest project (node env, 30s timeouts);
everything else runs in the unit project (happy-dom).

This removes the silent misclassification trap from #1307: a new
integration-style test no longer falls into the unit project (and
happy-dom) just because someone forgot to add it to the array.

The 13 ported v1.5 tests + their helpers move under src/test/integration/
in a strict mirror of core/ source layout (mcp/, auth/, auth/node/,
storage/). Renames use git mv so history is preserved.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@cliffhall cliffhall added the v2 Issues and PRs for v2 label May 18, 2026
@cliffhall
Copy link
Copy Markdown
Member Author

@claude review

@claude
Copy link
Copy Markdown
Contributor

claude Bot commented May 18, 2026

Claude finished @cliffhall's task in 3m 2s —— View job


Claude PR Review

  • Gather PR context (diff, vite.config.ts, test layout)
  • Verify folder glob semantics (integration vs unit)
  • Check renames / import-path adjustments
  • Review for risks, readability, maintainability
  • Post review

Summary

The folder-manifest change is a clear win. Swapping the 13-entry integrationTests array for a single clients/web/src/test/integration/**/*.test.ts glob removes the silent misclassification trap from #1307 — a forgotten array entry used to silently demote an integration test to happy-dom; now placement in the folder is the declaration. Renames are rename-aware (git mv, 98–100% similarity per the stat), import-path touchups are confined to the four files that referenced ./helpers/oauth-client-fixtures.js (now ../helpers/...), and tsconfig.test.json already globs src/test/**/*.ts so no matching change was needed. The folder layout mirrors core/ source (mcp/, mcp/node/, mcp/remote/, auth/, auth/node/, storage/, helpers/), which will make adding new tests feel mechanical.

Findings

1. Doc drift in AGENTS.md (needs update before merge)

AGENTS.md:93 still reads:

Test files live alongside the source as <Name>.test.tsx (or .test.ts for non-React modules). v1.5-ported integration tests live under clients/web/src/test/core/ and are wired into the integration project via the integrationTests list in vite.config.ts.

After this PR, both halves of that second sentence are wrong: integration tests live under src/test/integration/, and the integrationTests list no longer exists (it's integrationGlob). The project's own "Maintenance Rules" in AGENTS.md:38-41 require this file to be updated when testing rules change. Suggested wording:

v1.5-ported integration tests live under clients/web/src/test/integration/. Any .test.ts under that folder is picked up by the integration vitest project (node env, 30s timeouts) via the folder glob in vite.config.ts; tests outside that folder run in the unit project (happy-dom). Mirror core/'s source layout when adding new files (e.g. a test for core/mcp/remote/foo.ts goes in src/test/integration/mcp/remote/foo.test.ts).

Fix this →

2. Glob extension mismatch partially reintroduces the trap this PR is closing

// clients/web/vite.config.ts:79
const integrationGlob = 'clients/web/src/test/integration/**/*.test.ts';

vs. the unit project:

// clients/web/vite.config.ts:170
include: ['clients/web/src/**/*.test.{ts,tsx}'],
exclude: [integrationGlob],

A hypothetical .test.tsx file placed under src/test/integration/ would:

  1. Match the unit include (matches {ts,tsx})
  2. Not be filtered by the unit exclude (only matches .ts)
  3. Not be picked up by the integration include either

Net effect: a .test.tsx file inside integration/ runs in happy-dom — silently misclassified, exactly the kind of trap this PR exists to eliminate.

Integration tests today are all .ts (no DOM), and arguably .tsx doesn't belong in a node-env project at all. But the PR's framing is "the folder is the manifest," and that promise holds more cleanly if the glob matches the unit project's extension set. Two low-cost options:

  • Mirror unit: const integrationGlob = 'clients/web/src/test/integration/**/*.test.{ts,tsx}'; — the folder fully owns its contents regardless of extension, and a stray .tsx either gets picked up or (if you want to forbid it) fails loudly on missing DOM globals.
  • Or keep .ts-only but add a one-line comment stating the intent ("JSX isn't meaningful under node env; keep this folder .ts-only") so the asymmetry is deliberate rather than accidental.

Fix this →

Nits (non-blocking)

  • src/test/core/ now coexists with src/test/integration/ — the former holds unit tests for core/ modules (happy-dom), the latter the v1.5-ported integration suite. That's fine structurally, but the overloaded "core" noun (source folder name vs test folder name) could confuse a newcomer deciding where a new test for, say, core/mcp/remote/foo.ts belongs. The new AGENTS.md wording I proposed above covers this explicitly.
  • The integration glob is expressed as a repo-root-relative path string — consistent with how the removed integrationTests array was written and how the unit include is written, so this is correct; just calling out that it implicitly relies on root: repoRoot on both projects (line 165 and 188). Not a bug, just a coupling worth being aware of.

Verdict

Approve with the two fixes above (AGENTS.md and the glob extension). The refactor is small, reversible, and genuinely closes a class of bug — net positive.
· v2/issue-1314-integration-folder-manifest

- vite.config.ts: widen integrationGlob from `*.test.ts` to
  `*.test.{ts,tsx}` so the integration folder fully owns its contents.
  Without this, a `.test.tsx` placed under src/test/integration/ would
  match the unit project's `{ts,tsx}` include, fail the unit `.ts`-only
  exclude, and silently land under happy-dom — the exact misclassification
  trap this PR set out to close.
- AGENTS.md: update the line describing where integration tests live;
  it still pointed at the old `src/test/core/` location and the removed
  `integrationTests` enumeration.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@cliffhall
Copy link
Copy Markdown
Member Author

Thanks for the review — both findings addressed in 2a87127.

1. AGENTS.md doc drift — Fixed. Replaced the stale sentence on line 93 with a description of the folder-as-manifest model, the strict-mirror layout (mcp/, auth/, storage/, etc.), and a concrete "where does my new test go?" example. Matches the wording you suggested with a few tweaks.

2. Glob extension mismatch (.ts vs unit's {ts,tsx}) — Good catch. Took option A: widened integrationGlob to *.test.{ts,tsx} so the folder fully owns its contents. A .test.tsx is still semantically odd under node env, but if one ever lands there it'll fail loudly on missing DOM globals rather than silently running in happy-dom — which is exactly the property this PR is supposed to guarantee. Added an inline comment on the asymmetry. Both integration (331) and unit (1108) suites still green.

Nits — kept as-is:

@cliffhall
Copy link
Copy Markdown
Member Author

@claude review

@claude
Copy link
Copy Markdown
Contributor

claude Bot commented May 18, 2026

Claude finished @cliffhall's task in 1m 1s —— View job


Claude PR Review (follow-up on 2a87127)

  • Re-read the updated AGENTS.md and vite.config.ts
  • Verify the two prior findings are genuinely resolved
  • Sanity-check unit/integration glob partition after the extension widening
  • Look for anything new introduced by the fixup commit

Verdict — looks good, ready to merge

Both prior findings are cleanly addressed by 2a87127. No new concerns.

Findings re-verification

1. AGENTS.md doc drift — fixed (AGENTS.md:93)

Old line's two incorrect claims (src/test/core/ and integrationTests enumeration) are both gone. New wording covers the folder-as-manifest model, the strict core/ mirror (mcp/, mcp/node/, mcp/remote/, auth/, auth/node/, storage/), the node-env / 30s-timeout behavior, and a concrete "where does my test go?" example. grep confirms no lingering src/test/core or integrationTests references anywhere in the tree.

2. Glob extension mismatch — fixed (clients/web/vite.config.ts:83)

const integrationGlob = 'clients/web/src/test/integration/**/*.test.{ts,tsx}';

Now symmetric with the unit project's *.test.{ts,tsx} include (vite.config.ts:174) and exclude (vite.config.ts:176). A hypothetical .test.tsx under integration/ is now picked up by the integration project and excluded from unit — no silent misclassification possible. The inline comment at vite.config.ts:80-82 documents why the {ts,tsx} match matters, which is the right place for that context. Good call taking option A over option B; keeping the two projects' extension sets in lockstep is the more robust invariant to maintain.

Extra verification I did

  • coverage.exclude at vite.config.ts:117 uses '**/*.test.{ts,tsx}', so integration test files won't pollute coverage metrics regardless of their folder. ✓
  • helpers/oauth-client-fixtures.ts doesn't match *.test.*, so it's correctly skipped by the integration glob — only the 13 .test.ts files run. ✓
  • Folder layout on disk matches AGENTS.md's description (auth/{node/,}, mcp/{node/,remote/,}, storage/, helpers/). ✓
  • src/test/core/ still exists (unit tests for core/ modules in happy-dom) alongside src/test/integration/, which is the intentional split called out in AGENTS.md. No ambiguity left about which folder a new test should land in.
    · v2/issue-1314-integration-folder-manifest

@cliffhall cliffhall merged commit 8bb389c into v2/main May 18, 2026
1 check passed
@cliffhall cliffhall deleted the v2/issue-1314-integration-folder-manifest branch May 18, 2026 15:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

v2 Issues and PRs for v2

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant