Merge pull request #1978 from modelcontextprotocol/v2/chore/1977-isolate-web-smoke-catalog
chore(smoke): run the web smokes against a throwaway catalog
This commit is contained in:
Cliff Hall
GitHub
commit
b737360088
7 changed files with 317 additions and 30 deletions
1
.github/copilot-instructions.md
vendored
1
.github/copilot-instructions.md
vendored
|
|
@ -100,6 +100,7 @@ Both exist and do different jobs. Theme files (`src/theme/<Component>.ts`) custo
|
|||
|
||||
- `npm run format` before committing; **`npm run ci` before pushing** (`validate` → `coverage` → `verify:build-gate` → `smoke` → Storybook). `npm run validate` is the fast inner-loop check and is **not** a substitute — it runs `test`, not `test:coverage`, so it does zero coverage gating.
|
||||
- **A dependency bump must land in every install that declares it.** v2 is not a workspace — the root and each `clients/*` have their own `node_modules`, and a client's test project compiles `core/` and `test-servers/src` (which resolve from the **root**) alongside the client's own sources. Bumping a shared dependency in one manifest only puts two versions of it in one `tsc` program; for a recursive-generic surface like zod that exhausts the tsc heap (#1896). `verify:dep-lockstep` fails the build on this, so a PR bumping a package that the shared sources import should update the root **and every client that already lists it** — not every client unconditionally, since a package absent from an install can't skew and adding it there would be a spurious dependency.
|
||||
- **A test or smoke must not touch real user state.** The web smokes run against a throwaway catalog via the shared `scripts/lib/prod-web-server.mjs` helper, never the developer's `~/.mcp-inspector/mcp.json` (#1977); the cli/tui smokes drive a temp `--catalog`. A new smoke spawning its own server, or teardown that removes a work dir without first awaiting `stopChild` (the #1801 race — `child-cleanup.mjs` exports both halves and both are required), should be flagged.
|
||||
- **Every PR references an issue**, first body line `Closes #<ISSUE_NUMBER>`.
|
||||
- **Every PR carries exactly one version label**, `v1` or `v2`, matching its base branch.
|
||||
- Update the relevant `README.md` / `AGENTS.md` when a change adds, removes, renames, or repurposes a file or folder, changes the structure or tech stack, or introduces a command, dependency, or architectural pattern.
|
||||
|
|
|
|||
|
|
@ -657,7 +657,9 @@ The ⚠️ option-deletion hazard, the snapshot rule, and the recovery recipe ab
|
|||
- **`npm run coverage`** is the per-file ≥90 gate and is now part of `npm run ci` — never treat it as optional before a push. It supersedes the old standalone `test:integration` step: web's `test:coverage` runs the `unit` **and** `integration` projects under v8 instrumentation, so `coverage` both enforces the ≥90 gate and exercises the same web integration paths CI covers.
|
||||
- **`smoke` is NOT part of `validate`** — it is included in `npm run ci`. It runs `smoke:launcher` (`--help` dispatch) plus the prod `smoke:cli` / `smoke:tui` / `smoke:web` / `smoke:web:browser` / `smoke:web:app`, and contains **no build commands** — it assumes the cli/tui/launcher bundles already exist (a full `validate` builds them; `smoke:web` builds `clients/web/dist` on demand). CI runs `validate`, then the `coverage` gate (which also covers the web integration project), then `verify:build-gate` (the #1769 build gate — see below), then `smoke` (with Playwright chromium installed just before it, since `smoke:web:browser` needs it). GitHub CI runs this same chain as separate workflow steps, with the Storybook play-function tests last (see below).
|
||||
- `smoke:launcher` (`scripts/smoke-launcher.mjs`) runs the built launcher with `--help`, `--cli --help`, and `--tui --help`, asserting each exits 0 and prints that mode's usage banner (which also proves the launcher resolved and loaded the right client build). It's the cheap dispatch check before the heavier prod smokes below.
|
||||
- `smoke:web` (`scripts/smoke-web.mjs`) starts `mcp-inspector --web` (prod, no `--dev`) against the built `clients/web/dist` and asserts `GET /` serves the SPA (HTTP 200) with the injected `__INSPECTOR_API_TOKEN__`. Prod `--web` serves from `clients/web/dist`, which ships in the published package but is absent in a fresh checkout — the runner builds it on demand (`build:client` = `vite build`) on first launch, or exits with an actionable error if that build can't run (see `clients/web/server/ensure-web-build.ts` and the launcher README). `--dev` runs Vite directly and never needs `dist`. It shares the spawn/readiness/teardown helper (`scripts/lib/prod-web-server.mjs`) with `smoke:web:browser`, so the two can't drift.
|
||||
- `smoke:web` (`scripts/smoke-web.mjs`) starts `mcp-inspector --web` (prod, no `--dev`) against the built `clients/web/dist` and asserts `GET /` serves the SPA (HTTP 200) with the injected `__INSPECTOR_API_TOKEN__`. Prod `--web` serves from `clients/web/dist`, which ships in the published package but is absent in a fresh checkout — the runner builds it on demand (`build:client` = `vite build`) on first launch, or exits with an actionable error if that build can't run (see `clients/web/server/ensure-web-build.ts` and the launcher README). `--dev` runs Vite directly and never needs `dist`. It shares the spawn/readiness/teardown helper (`scripts/lib/prod-web-server.mjs`) with **`smoke:web:browser` and `smoke:web:app`**, so the three can't drift.
|
||||
|
||||
**Every web smoke runs against a throwaway catalog (#1977).** The helper mints a temp dir per run and passes it as `MCP_CATALOG_PATH`; without it the web backend falls back to the developer's real `~/.mcp-inspector/mcp.json`, which made these smokes both destructive and non-deterministic — `smoke:web:app`'s deep link persists a `deep-link` server row, so a *second* run found it already on disk, raced hydration, and drew a spurious (swallowed, non-fatal) 409 that was really just residue from the previous run. CI never saw it: a fresh `HOME` per run made every CI run look like a first run. This matches `smoke:cli` / `smoke:tui`, which have always driven a temp `--catalog`. Only the **catalog** is redirected — other per-user state under `~/.mcp-inspector` (OAuth tokens, `storage/`) stays shared, because isolating it means redirecting `HOME` wholesale, which would also move the npx and Playwright caches these smokes depend on. Teardown uses **both** halves of `scripts/lib/child-cleanup.mjs` (`stopChild` to await the child's exit, then `removeSafe` to delete the dir) — a bare `kill()` only *delivers* the signal, so removing synchronously re-enters the #1801 ENOTEMPTY race. That makes `stop()` **async**, so every caller must `await` it (and a caller's own `fail()`/`shutdown()` becomes async in turn, or execution runs past the intended exit). The isolation contract is unit-tested in `scripts/lib/prod-web-server.test.mjs` via `test:scripts`, since the smokes exit immediately after teardown and so cannot detect a regression that silently reshared the catalog or stopped cleaning up: `createTempCatalog` and `buildWebServerEnv` cover *which* catalog the server gets, and `teardownWebServer` — extracted from `stop()` for exactly this reason — is driven against a stand-in child process so the teardown asserts on the real directory rather than a spy.
|
||||
- `smoke:web:browser` (`scripts/smoke-web-browser.mjs`, #1615) goes a step further than `smoke:web`: it boots the same prod `--web` server and then actually **runs** the bundle in headless Chromium (Playwright — already a `clients/web` devDependency for the Storybook tests), asserting the app renders its first meaningful frame (the "Add Servers" control) with **no uncaught error**. `smoke:web` only checks the served HTML, so a Node built-in reaching the browser bundle slipped through it; this smoke catches that regression as a _class_ (e.g. #1612). The mechanism is the uncaught error, not a magic string: under Vite the excluded module becomes an empty stub and the first _call_ into it (e.g. `fs.readFileSync(...)` during a transitive module's init) throws a `TypeError` that aborts app mount. A _synchronous_ such throw fires `pageerror`; its _async_ twin (the same `TypeError` via `await`/`.then()`, or a failed dynamic import) is logged on the console channel as `Uncaught (in promise) …` / `Failed to fetch dynamically imported module` — the smoke hard-fails on both. The literal `Module "…" has been externalized` text is, **in a prod build**, a build-time warning (`vite build` / `npm run build`), not a runtime message, so the browser never sees it (under `npm run dev` Vite's stub is instead a `Proxy` that `console.warn`s that string at runtime); and an externalized import that is never _called_ ships a harmless `{}` and is invisible here by design. Every _other_ console error is printed as a diagnostic, not a failure (so a benign font-CDN or React-warning `console.error` doesn't flake CI). Playwright is resolved via `createRequire` based at `clients/web/package.json` — a bare `import("playwright")` would resolve relative to `scripts/`, not the cwd, so it can't be reached that way (it only appears to work when an ancestor `node_modules` carries playwright, and fails in CI, which has none). The npm script's `cd clients/web` exists only so `npx playwright install chromium` finds the local playwright bin (a no-op when already installed).
|
||||
- `smoke:web:app` (`scripts/smoke-web-app.mjs`, #1859) goes one step further again: `smoke:web:browser` stops at first paint and never connects to a server, so the Apps tab, the sandbox controller, and the UI-protocol bridge were unexercised by any smoke. This one boots the same prod `--web` server, spawns the `mcp-app-http.json` composable test server (the `mcp_app_demo` tool + its `mcp_app_demo_widget` UI resource), and drives the whole **connect → open app → widget ready** chain through a single deep-link navigate (`?serverUrl=…&autoConnect=<token>&openApp=…&appArgs=…&autoOpen=<token>`). The assertion is the `data-app-status="ready"` contract from [clients/web/README.md](clients/web/README.md) — the renderer reports `ready` only once the widget has loaded inside the sandbox iframe _and_ fired `notifications/initialized` back through the bridge, so one attribute covers the sandbox proxy being served, the UI resource loading, and the handshake completing. Two mechanics are load-bearing and easy to get wrong: the test server announces readiness on **stderr** (`console.error` in `server-composable.ts`), so both child streams are piped and scanned — watching stdout alone times out with an empty diagnostic; and its bound port is **not** the config's, because `createTestServerHttp` resolves through `findAvailablePort()`, which walks upward when the configured port is taken — so the smoke parses the announced URL rather than assuming `3130`. **Scope note:** this runs against the repo build tree like every other smoke, so it would _not_ have caught #1859 itself (a packaging failure — the file is always present in-repo); `pack:verify` owns that dimension. It does carry a cheap structural pre-check that the proxy page exists at the path `sandbox-controller.ts` resolves, so a move/rename fails fast with a clear cause instead of an opaque render timeout.
|
||||
- **The build gate for the browser-externalized-builtin class (#1769)** is the earlier, more complete companion to `smoke:web:browser`. A Vite plugin in `clients/web/vite.config.ts` (logic in `clients/web/server/browser-externalized-builtin-gate.ts`, unit-tested) turns Vite 8's _browser-externalization warning_ (`Module "node:*" has been externalized for browser compatibility`) into a hard `vite build` error, so a Node built-in in the browser graph now **fails `npm run build` / `validate`** instead of shipping a `{}` stub. This catches **both** the _called-at-init_ case (which `smoke:web:browser` also catches, but later/at runtime) **and** the _imported-but-never-called_ case (the `{}` stub that is invisible to the runtime smoke "by design" — see above). Because rolldown **swallows a throw inside `onLog`** (the one hook where a thrown error doesn't abort — verified against vite@8.0.0), the plugin _records_ the warning in `onLog` and re-throws in `buildEnd`. There is **no stable log `code`**, so the gate keys off the documented message phrasing; `npm run verify:build-gate` (`scripts/verify-build-gate.mjs`, in `npm run ci` and the GitHub workflow) runs a real build with a `node:fs` probe forced into `src/main.tsx` and asserts the build fails via the gate — the only check that catches the message phrasing **drifting** in a future Vite bump and silently disabling the gate. The gate is scoped to `vite build` (`apply: 'build'`) — never `vite dev` or the vitest projects — **and** to the browser (`client`) environment (`applyToEnvironment`), so a future SSR/node environment built from this config isn't failed for a legitimate `node:*` import; the Node runner build (tsup, `build:runner`) is a separate config where built-ins are legitimate. `smoke:web:browser` stays as the runtime backstop for crashes the build can't reason about.
|
||||
|
|
|
|||
|
|
@ -1,10 +1,12 @@
|
|||
/**
|
||||
* Shared boot/readiness helper for the prod web smokes.
|
||||
*
|
||||
* Both `scripts/smoke-web.mjs` (serves-the-HTML check) and
|
||||
* `scripts/smoke-web-browser.mjs` (runs-the-bundle check, #1615) boot the *same*
|
||||
* prod `mcp-inspector --web` server, so the spawn + readiness-poll boilerplate
|
||||
* lives here once instead of being copy-pasted (and drifting) in each script.
|
||||
* `scripts/smoke-web.mjs` (serves-the-HTML check), `scripts/smoke-web-browser.mjs`
|
||||
* (runs-the-bundle check, #1615), and `scripts/smoke-web-app.mjs` (MCP Apps
|
||||
* end-to-end, #1859) all boot the *same* prod `mcp-inspector --web` server, so the
|
||||
* spawn + readiness-poll boilerplate lives here once instead of being copy-pasted
|
||||
* (and drifting) in each script. Catalog isolation (#1977) lives here for the same
|
||||
* reason — it is a property every web smoke needs, not one script's concern.
|
||||
*
|
||||
* Repo-root paths are derived from import.meta.url, so a caller's cwd (e.g.
|
||||
* `smoke:web:browser` does `cd clients/web` first so its `npx playwright
|
||||
|
|
@ -12,35 +14,136 @@
|
|||
*/
|
||||
|
||||
import { spawn } from "node:child_process";
|
||||
import { mkdtempSync } from "node:fs";
|
||||
import { tmpdir } from "node:os";
|
||||
import { setTimeout as delay } from "node:timers/promises";
|
||||
import { fileURLToPath } from "node:url";
|
||||
import { dirname, resolve } from "node:path";
|
||||
import { dirname, join, resolve } from "node:path";
|
||||
|
||||
import { removeSafe, stopChild } from "./child-cleanup.mjs";
|
||||
|
||||
const libDir = dirname(fileURLToPath(import.meta.url));
|
||||
const repoRoot = resolve(libDir, "..", "..");
|
||||
const launcherEntry = resolve(repoRoot, "clients/launcher/build/index.js");
|
||||
|
||||
/**
|
||||
* Mint a fresh throwaway catalog for one server run.
|
||||
*
|
||||
* `mkdtemp` (not a fixed name) is what makes concurrent runs safe: the three web
|
||||
* smokes run back-to-back today, but a fixed path would have them share — and
|
||||
* silently reintroduce the cross-run bleed this whole change removes.
|
||||
*
|
||||
* The catalog *file* is deliberately not created. The backend seeds an empty
|
||||
* catalog on first use, which is exactly the first-run state we want every run
|
||||
* to start from.
|
||||
*
|
||||
* @returns {{ dir: string, path: string }}
|
||||
*/
|
||||
export function createTempCatalog() {
|
||||
const dir = mkdtempSync(join(tmpdir(), "smoke-web-catalog-"));
|
||||
return { dir, path: join(dir, "catalog.json") };
|
||||
}
|
||||
|
||||
/**
|
||||
* Build the child env for the prod web server.
|
||||
*
|
||||
* Split out from the spawn so the isolation contract is unit-testable without
|
||||
* starting a server (#1977). The ordering is load-bearing: `MCP_CATALOG_PATH`
|
||||
* is assigned *after* the `process.env` spread, so an inherited value from the
|
||||
* developer's shell is overridden rather than silently winning — which would
|
||||
* put the smoke straight back on whatever catalog that variable names.
|
||||
*
|
||||
* @param {object} opts
|
||||
* @param {string} opts.host
|
||||
* @param {string} opts.port
|
||||
* @param {string} opts.token
|
||||
* @param {string} opts.catalogPath
|
||||
* @param {NodeJS.ProcessEnv} [opts.baseEnv]
|
||||
*/
|
||||
export function buildWebServerEnv({
|
||||
host,
|
||||
port,
|
||||
token,
|
||||
catalogPath,
|
||||
baseEnv = process.env,
|
||||
}) {
|
||||
return {
|
||||
...baseEnv,
|
||||
CLIENT_PORT: port,
|
||||
HOST: host,
|
||||
MCP_INSPECTOR_API_TOKEN: token,
|
||||
MCP_CATALOG_PATH: catalogPath,
|
||||
// Don't pop a browser in CI.
|
||||
MCP_AUTO_OPEN_ENABLED: "false",
|
||||
};
|
||||
}
|
||||
|
||||
/**
|
||||
* Terminate a spawned web server, then remove its catalog dir.
|
||||
*
|
||||
* The two halves are the documented pair in `child-cleanup.mjs`, and both are
|
||||
* needed. `stopChild` closes the #1801 race on the normal path (a bare `kill()`
|
||||
* only *delivers* the signal, so a synchronous remove can hit ENOTEMPTY when the
|
||||
* server writes the catalog on its way out); `removeSafe` then makes the
|
||||
* residual case harmless, warning instead of throwing so a leftover temp dir can
|
||||
* never turn a passing smoke red.
|
||||
*
|
||||
* Exported separately from `startProdWebServer` so the teardown *contract* is
|
||||
* testable against a stand-in child, without booting a real launcher. That split
|
||||
* is what makes the leak detectable at all: the smokes exit immediately after
|
||||
* teardown, so deleting the `removeSafe` call below would leave all three of them
|
||||
* green. The focused unit tests in `prod-web-server.test.mjs` are the only thing
|
||||
* that fails on it.
|
||||
*
|
||||
* @param {object} opts
|
||||
* @param {import("node:child_process").ChildProcess} opts.child
|
||||
* @param {string} opts.catalogDir
|
||||
* @param {string} [opts.label]
|
||||
*/
|
||||
export async function teardownWebServer({
|
||||
child,
|
||||
catalogDir,
|
||||
label = "smoke:web",
|
||||
}) {
|
||||
await stopChild(child, { label, what: "prod web server" });
|
||||
removeSafe(catalogDir, { label });
|
||||
}
|
||||
|
||||
/**
|
||||
* Spawn `mcp-inspector --web` (prod, no `--dev`) against the built
|
||||
* `clients/web/dist` and return handles for readiness + teardown.
|
||||
*
|
||||
* The server always runs against a **throwaway catalog**, never the developer's
|
||||
* real `~/.mcp-inspector/mcp.json` (#1977). Without `MCP_CATALOG_PATH` the web
|
||||
* backend falls back to that default writable catalog, which made these smokes
|
||||
* both destructive and non-deterministic: `smoke:web:app`'s deep link persists a
|
||||
* `deep-link` server row, so a second run finds it already on disk. The row is
|
||||
* then racing hydration — the deep-link effect misses it in `servers`, POSTs
|
||||
* `addServer` anyway, and the backend answers 409. The app swallows that 409 by
|
||||
* design so the smoke still passes, but it reports a spurious non-fatal console
|
||||
* error that is really just residue from the previous run. Isolating the catalog
|
||||
* makes every run look like the first one, and mirrors `smoke:cli` / `smoke:tui`,
|
||||
* which have always driven a temp `--catalog`.
|
||||
*
|
||||
* Only the catalog is redirected. Other per-user state under `~/.mcp-inspector`
|
||||
* (OAuth tokens, the `storage/` dir) is still shared — isolating that means
|
||||
* redirecting HOME wholesale, which these smokes deliberately do not do, since
|
||||
* HOME also resolves the npx and Playwright caches they depend on.
|
||||
*
|
||||
* @param {object} opts
|
||||
* @param {string} opts.host
|
||||
* @param {string} opts.port
|
||||
* @param {string} opts.token value injected as MCP_INSPECTOR_API_TOKEN
|
||||
* @param {string} [opts.label] prefix for teardown warnings (the temp dir's own
|
||||
* prefix is fixed, so every run's dir is greppable as `smoke-web-catalog-*`)
|
||||
*/
|
||||
export function startProdWebServer({ host, port, token }) {
|
||||
export function startProdWebServer({ host, port, token, label = "smoke:web" }) {
|
||||
const baseUrl = `http://${host}:${port}`;
|
||||
|
||||
const { dir: catalogDir, path: catalogPath } = createTempCatalog();
|
||||
|
||||
const child = spawn(process.execPath, [launcherEntry, "--web"], {
|
||||
env: {
|
||||
...process.env,
|
||||
CLIENT_PORT: port,
|
||||
HOST: host,
|
||||
MCP_INSPECTOR_API_TOKEN: token,
|
||||
// Don't pop a browser in CI.
|
||||
MCP_AUTO_OPEN_ENABLED: "false",
|
||||
},
|
||||
env: buildWebServerEnv({ host, port, token, catalogPath }),
|
||||
stdio: ["ignore", "inherit", "inherit"],
|
||||
});
|
||||
|
||||
|
|
@ -119,10 +222,10 @@ export function startProdWebServer({ host, port, token }) {
|
|||
|
||||
return {
|
||||
baseUrl,
|
||||
catalogPath,
|
||||
waitForReady,
|
||||
whenChildExits,
|
||||
stop: () => {
|
||||
if (!exited) child.kill("SIGTERM");
|
||||
},
|
||||
/** Terminate the server, then remove its catalog dir. **Await this.** */
|
||||
stop: () => teardownWebServer({ child, catalogDir, label }),
|
||||
};
|
||||
}
|
||||
|
|
|
|||
163
scripts/lib/prod-web-server.test.mjs
Normal file
163
scripts/lib/prod-web-server.test.mjs
Normal file
|
|
@ -0,0 +1,163 @@
|
|||
/**
|
||||
* Unit tests for the prod-web-server helper's catalog-isolation contract (#1977).
|
||||
*
|
||||
* The smokes themselves cannot guard this: they exercise startup and then exit
|
||||
* the process immediately after teardown, so a regression that reintroduced the
|
||||
* shared catalog — or that stopped cleaning up — would still leave every smoke
|
||||
* green. These lock down the three properties that make isolation real: each run
|
||||
* gets its own catalog, an inherited `MCP_CATALOG_PATH` cannot win, and the temp
|
||||
* dir is removable on stop.
|
||||
*
|
||||
* `startProdWebServer` itself is not unit-tested — it spawns a real launcher, so
|
||||
* its contract is the smokes' job. What is tested here is exactly the part that
|
||||
* decides *which catalog the server sees*, which is pure.
|
||||
*/
|
||||
|
||||
import { test } from "node:test";
|
||||
import assert from "node:assert/strict";
|
||||
import { spawn } from "node:child_process";
|
||||
import { once } from "node:events";
|
||||
import { existsSync, writeFileSync } from "node:fs";
|
||||
import { dirname } from "node:path";
|
||||
|
||||
import {
|
||||
buildWebServerEnv,
|
||||
createTempCatalog,
|
||||
teardownWebServer,
|
||||
} from "./prod-web-server.mjs";
|
||||
import { hasExited, removeSafe } from "./child-cleanup.mjs";
|
||||
|
||||
const BASE = {
|
||||
host: "127.0.0.1",
|
||||
port: "6299",
|
||||
token: "test-token",
|
||||
catalogPath: "/tmp/example/catalog.json",
|
||||
};
|
||||
|
||||
test("createTempCatalog: mints a unique dir per call", () => {
|
||||
const a = createTempCatalog();
|
||||
const b = createTempCatalog();
|
||||
try {
|
||||
assert.notEqual(a.dir, b.dir, "two runs must not share a catalog dir");
|
||||
assert.equal(dirname(a.path), a.dir);
|
||||
assert.ok(existsSync(a.dir));
|
||||
assert.ok(existsSync(b.dir));
|
||||
} finally {
|
||||
removeSafe(a.dir);
|
||||
removeSafe(b.dir);
|
||||
}
|
||||
});
|
||||
|
||||
test("createTempCatalog: does not create the catalog file itself", () => {
|
||||
// The backend seeds an empty catalog on first use; pre-creating the file would
|
||||
// hand it a zero-byte file to parse instead of the clean first-run state.
|
||||
const { dir, path } = createTempCatalog();
|
||||
try {
|
||||
assert.equal(existsSync(path), false);
|
||||
} finally {
|
||||
removeSafe(dir);
|
||||
}
|
||||
});
|
||||
|
||||
/**
|
||||
* A stand-in for the web server: a real child process that ignores nothing and
|
||||
* simply stays alive until signalled, so `teardownWebServer` exercises its true
|
||||
* SIGTERM→exit path rather than a mock's idea of one.
|
||||
*/
|
||||
function spawnIdleChild() {
|
||||
return spawn(process.execPath, ["-e", "setInterval(() => {}, 1000)"], {
|
||||
stdio: "ignore",
|
||||
});
|
||||
}
|
||||
|
||||
test("teardownWebServer: removes the catalog dir it was given", async () => {
|
||||
// The regression this guards: dropping the `removeSafe` call from the teardown
|
||||
// would leave every smoke green, since they exit right after teardown and so
|
||||
// never observe the leak. Assert on the dir itself, not on a spy.
|
||||
const child = spawnIdleChild();
|
||||
const { dir, path } = createTempCatalog();
|
||||
writeFileSync(path, "{}");
|
||||
assert.ok(existsSync(dir));
|
||||
|
||||
await teardownWebServer({ child, catalogDir: dir, label: "test" });
|
||||
|
||||
assert.equal(existsSync(dir), false, "teardown must remove the catalog dir");
|
||||
});
|
||||
|
||||
test("teardownWebServer: the catalog outlives the child", async () => {
|
||||
// The #1801 ordering, pinned at the one instant that distinguishes it. Simply
|
||||
// asserting `hasExited` after the await is too weak — that also passes if the
|
||||
// dir were removed *first*, or while an un-awaited stopChild was still pending.
|
||||
// So sample the dir from the child's own `exit` handler: registering it before
|
||||
// teardown puts it ahead of stopChild's listener, so it observes the world at
|
||||
// the moment the child dies. removeSafe cannot have run yet — it is sequenced
|
||||
// after the promise stopChild resolves from this very event.
|
||||
const child = spawnIdleChild();
|
||||
const { dir } = createTempCatalog();
|
||||
assert.equal(hasExited(child), false, "child should start alive");
|
||||
|
||||
let dirAtChildExit = null;
|
||||
child.once("exit", () => {
|
||||
dirAtChildExit = existsSync(dir);
|
||||
});
|
||||
|
||||
await teardownWebServer({ child, catalogDir: dir, label: "test" });
|
||||
|
||||
assert.equal(
|
||||
dirAtChildExit,
|
||||
true,
|
||||
"catalog must still exist when the child exits — removing earlier is the #1801 race",
|
||||
);
|
||||
assert.ok(hasExited(child), "teardown must await the child's exit");
|
||||
assert.equal(existsSync(dir), false, "and must remove it afterwards");
|
||||
});
|
||||
|
||||
test("teardownWebServer: an already-dead child is not an error", async () => {
|
||||
// The failure path: a smoke calls fail() after the launcher already crashed.
|
||||
// Teardown still has to clean up rather than hang on an exit that never comes.
|
||||
const child = spawnIdleChild();
|
||||
child.kill("SIGKILL");
|
||||
await once(child, "exit");
|
||||
const { dir } = createTempCatalog();
|
||||
|
||||
await teardownWebServer({ child, catalogDir: dir, label: "test" });
|
||||
|
||||
assert.equal(existsSync(dir), false);
|
||||
});
|
||||
|
||||
test("buildWebServerEnv: points the server at the given catalog", () => {
|
||||
const env = buildWebServerEnv({ ...BASE, baseEnv: {} });
|
||||
assert.equal(env.MCP_CATALOG_PATH, BASE.catalogPath);
|
||||
});
|
||||
|
||||
test("buildWebServerEnv: overrides an inherited MCP_CATALOG_PATH", () => {
|
||||
// The regression this guards: assigning before the spread would let a
|
||||
// developer's exported MCP_CATALOG_PATH win, putting the smoke back on a
|
||||
// real catalog while every test still passed.
|
||||
const env = buildWebServerEnv({
|
||||
...BASE,
|
||||
baseEnv: { MCP_CATALOG_PATH: "/home/dev/.mcp-inspector/mcp.json" },
|
||||
});
|
||||
assert.equal(env.MCP_CATALOG_PATH, BASE.catalogPath);
|
||||
});
|
||||
|
||||
test("buildWebServerEnv: passes through unrelated inherited vars", () => {
|
||||
const env = buildWebServerEnv({ ...BASE, baseEnv: { PATH: "/usr/bin" } });
|
||||
assert.equal(env.PATH, "/usr/bin");
|
||||
});
|
||||
|
||||
test("buildWebServerEnv: sets host, port, token, and disables auto-open", () => {
|
||||
const env = buildWebServerEnv({ ...BASE, baseEnv: {} });
|
||||
assert.equal(env.HOST, BASE.host);
|
||||
assert.equal(env.CLIENT_PORT, BASE.port);
|
||||
assert.equal(env.MCP_INSPECTOR_API_TOKEN, BASE.token);
|
||||
assert.equal(env.MCP_AUTO_OPEN_ENABLED, "false");
|
||||
});
|
||||
|
||||
test("buildWebServerEnv: an inherited auto-open setting cannot re-enable it", () => {
|
||||
const env = buildWebServerEnv({
|
||||
...BASE,
|
||||
baseEnv: { MCP_AUTO_OPEN_ENABLED: "true" },
|
||||
});
|
||||
assert.equal(env.MCP_AUTO_OPEN_ENABLED, "false");
|
||||
});
|
||||
|
|
@ -104,7 +104,12 @@ let mcpUrl = null;
|
|||
|
||||
let mcpServer = null;
|
||||
let browser = null;
|
||||
const server = startProdWebServer({ host: HOST, port: PORT, token: TOKEN });
|
||||
const server = startProdWebServer({
|
||||
host: HOST,
|
||||
port: PORT,
|
||||
token: TOKEN,
|
||||
label: "smoke:web:app",
|
||||
});
|
||||
|
||||
async function shutdown() {
|
||||
if (browser) {
|
||||
|
|
@ -115,7 +120,7 @@ async function shutdown() {
|
|||
}
|
||||
browser = null;
|
||||
}
|
||||
server.stop();
|
||||
await server.stop();
|
||||
if (mcpServer) {
|
||||
const child = mcpServer;
|
||||
mcpServer = null;
|
||||
|
|
|
|||
|
|
@ -78,7 +78,12 @@ const TOKEN = "smoke-web-browser-token";
|
|||
// the font/CDN flake.
|
||||
const FATAL_CONSOLE = /^Uncaught\b|Failed to fetch dynamically imported module/;
|
||||
|
||||
const server = startProdWebServer({ host: HOST, port: PORT, token: TOKEN });
|
||||
const server = startProdWebServer({
|
||||
host: HOST,
|
||||
port: PORT,
|
||||
token: TOKEN,
|
||||
label: "smoke:web:browser",
|
||||
});
|
||||
let browser = null;
|
||||
|
||||
async function shutdown() {
|
||||
|
|
@ -90,7 +95,7 @@ async function shutdown() {
|
|||
}
|
||||
browser = null;
|
||||
}
|
||||
server.stop();
|
||||
await server.stop();
|
||||
}
|
||||
|
||||
async function fail(message) {
|
||||
|
|
|
|||
|
|
@ -30,33 +30,41 @@ const TOKEN = "smoke-web-token";
|
|||
// literal because this plain .mjs script can't import the TS source.
|
||||
const TOKEN_GLOBAL = "__INSPECTOR_API_TOKEN__";
|
||||
|
||||
const server = startProdWebServer({ host: HOST, port: PORT, token: TOKEN });
|
||||
const server = startProdWebServer({
|
||||
host: HOST,
|
||||
port: PORT,
|
||||
token: TOKEN,
|
||||
label: "smoke:web",
|
||||
});
|
||||
|
||||
function fail(message) {
|
||||
// Async because `server.stop()` awaits the child's exit before removing its temp
|
||||
// catalog — so every call site must `await fail(...)`, or execution would run on
|
||||
// past it instead of exiting.
|
||||
async function fail(message) {
|
||||
console.error(`smoke:web FAILED — ${message}`);
|
||||
server.stop();
|
||||
await server.stop();
|
||||
process.exit(1);
|
||||
}
|
||||
|
||||
try {
|
||||
const res = await server.waitForReady();
|
||||
if (res.status !== 200) {
|
||||
fail(`GET / returned HTTP ${res.status}, expected 200`);
|
||||
await fail(`GET / returned HTTP ${res.status}, expected 200`);
|
||||
}
|
||||
const body = await res.text();
|
||||
if (!body.includes(TOKEN_GLOBAL)) {
|
||||
fail(
|
||||
await fail(
|
||||
`served HTML is missing the ${TOKEN_GLOBAL} global (token not injected)`,
|
||||
);
|
||||
}
|
||||
if (!body.includes(TOKEN)) {
|
||||
fail("served HTML is missing the injected auth-token value");
|
||||
await fail("served HTML is missing the injected auth-token value");
|
||||
}
|
||||
console.log(
|
||||
`smoke:web OK — GET / => 200 with injected ${TOKEN_GLOBAL} at ${server.baseUrl}`,
|
||||
);
|
||||
server.stop();
|
||||
await server.stop();
|
||||
process.exit(0);
|
||||
} catch (err) {
|
||||
fail(err instanceof Error ? err.message : String(err));
|
||||
await fail(err instanceof Error ? err.message : String(err));
|
||||
}
|
||||
|
|
|
|||
Loading…
Reference in a new issue