Extract shared test helpers to remove cross-file duplication#3
Merged
Conversation
Sonar flagged 3.3% duplication on new code in PR #2 (limit 3%). The duplication is real and pre-existing: `startMock`, `textOf`, and HAPPY_FIXTURE were copy-pasted across cli.test.ts, server.test.ts, and e2e.test.ts. PR #2's renames happened to touch lines inside the duplicated blocks, which surfaced the finding. Extract the truly identical pieces to `tests/_helpers.ts`: - `startMock` — byte-for-byte identical in all three files (20 lines × 3 copies). - `textOf` — identical in server.test.ts and e2e.test.ts. - `HAPPY_FIXTURE` — server's 3-section version. cli.test.ts's assertions are a subset of what this fixture exposes, so they continue to pass. Keep per-file: - `spawnClient` / `spawnBuilt` / `runCli` — these differ meaningfully in transport and command. A polymorphic factory would muddy test-local clarity for ~10-line functions. - e2e.test.ts's own HAPPY_FIXTURE — uses distinctive sentinel strings ("E2E Fixture Heading") to prove the e2e tests are exercising their own pipeline, not accidentally hitting the server-test setup. The new file uses `_helpers.ts` (underscore prefix). The runner pattern `tsx --test tests/*.test.ts` (package.json) matches only `*.test.ts` and skips it by name. Net diff in test files: +3 import lines, -125 duplicated lines. The new helpers file is ~63 lines. No behavior change; all 50 tests pass.
Follow-up to the first commit on this branch. Three additional extractions
that the initial pass didn't cover; together they push the PR's net LOC
delta from -63 to -101 vs main.
New helpers in tests/_helpers.ts:
- `spawnClient({ name?, env? })` — moved from server.test.ts and
parameterized so snapshots.test.ts can share it. Existing callers using
env pass `{ env: {...} }` now.
- `assertSchemaRejection(client, args, msg)` — captures the F27 pattern
(try/catch around callTool to handle SDK-version-variant rejection
shapes, then assert no `[code]` prefix leaked). Used at 3 sites
(zod-syntax test, savePath relative, savePath tilde).
- `spawnAndCaptureExit(args, env)` — captures the env-var-validation
pattern (spawn → collect stderr → await exit). Used at 3 sites.
- `ERROR_CODE_PREFIX_RE` — single source for the seven `[code]` prefixes
the regex was previously inlined at 3 sites.
snapshots.test.ts: the inline `createServer` plumbing and
`StdioClientTransport` setup are replaced by `startMock` (with a closure
over `currentHtml`) and `spawnClient({ name: "snapshot-test" })`. The
`Mock` type and `setHtml` wrapper are gone — tests mutate `currentHtml`
directly, which the mock handler reads on each request.
Avoided one Sonar finding while at it: `opts.env ?? {}` was flagged by
S7744 ("empty object is useless"). Object spread already handles
`undefined`, so the `?? {}` fallback was strictly redundant — replaced
with `...opts?.env`.
No behavior change; all 50 tests pass.
|
There was a problem hiding this comment.
Pull request overview
This PR reduces test-code duplication by extracting shared utilities (mock HTTP server setup, MCP client spawning, common fixtures, and repeated assertion helpers) into a dedicated tests/_helpers.ts module, addressing the Sonar duplication finding without changing runtime behavior.
Changes:
- Added
tests/_helpers.tscontainingstartMock,spawnClient,textOf,HAPPY_FIXTURE, plus shared assertion/subprocess helpers. - Updated
cli.test.ts,server.test.ts,e2e.test.ts, andsnapshots.test.tsto import and use the shared helpers. - Simplified repeated schema-boundary rejection assertions and env-var startup-failure subprocess plumbing in
server.test.ts.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/_helpers.ts | New shared test helper module consolidating duplicated mock/client/fixture/assertion logic. |
| tests/snapshots.test.ts | Switched to shared startMock and spawnClient; uses a shared mutable HTML variable for responses. |
| tests/server.test.ts | Replaced duplicated helpers with imports; refactored schema-rejection and subprocess checks to shared helpers. |
| tests/e2e.test.ts | Uses shared startMock and textOf while keeping e2e-specific fixture and build-spawn logic. |
| tests/cli.test.ts | Uses shared startMock and shared HAPPY_FIXTURE while keeping CLI process harness local. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+114
to
+116
| const exitCode = await new Promise<number>((resolve) => | ||
| child.on("exit", (code) => resolve(code ?? -1)), | ||
| ); |
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
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.
What
Follow-up to #2 that addresses the Sonar quality gate finding: 3.3% duplication on new code, over the 3% threshold. The duplication is pre-existing —
startMock,textOf, andHAPPY_FIXTUREwere copy-pasted acrosstests/cli.test.ts,tests/server.test.ts, andtests/e2e.test.ts. PR #2's renames happened to touch lines inside those duplicated blocks, which is what surfaced the finding.Extract the truly identical pieces to a new
tests/_helpers.ts:startMock— byte-for-byte identical in all three files (~20 lines × 3 copies).textOf— identical inserver.test.tsande2e.test.ts.HAPPY_FIXTURE— server's 3-section version.cli.test.ts's assertions are a subset of what this fixture exposes, so they continue to pass against it.Intentionally kept per-file:
spawnClient/spawnBuilt/runCli— these differ meaningfully in transport and command. A polymorphic factory would muddy test-local clarity for ~10-line functions.e2e.test.ts's ownHAPPY_FIXTURE— uses distinctive sentinel strings ("E2E Fixture Heading") to prove the e2e tests are exercising their own pipeline, not accidentally hitting the server-test setup.The new file is named
_helpers.ts(underscore prefix). The runner patterntsx --test tests/*.test.tsinpackage.jsonmatches only*.test.tsand skips it by name — no runner change required.Net diff: +62 / -125 across 4 files. No behavior change.
Verification
npm run buildclean.npm test: 50/50 pass on macOS.