modelcontextprotocol/inspector
Watch
1
0
Fork
You've already forked inspector
 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions 2

test(smoke): cover loadBrowser's failure branches; fix a stale helper comment

Browse source 
Two findings from Copilot's suppressed block.

1. The launch-failure message is #2086's acceptance criterion — a missing
   browser must name THAT engine and its own install command — and it had
   no test. The smokes cannot supply one: install-smoke-browser fetches
   the binary first, so a passing smoke says nothing about what a failing
   one prints, and that message is the deliverable. loadBrowser now takes
   an injectable loadPlaywright purely to open that seam.

   The test asserts per engine that the message leads with that engine,
   names --with-deps <that engine> and no other, and preserves the
   underlying cause. Verified by mutation: hard-coding 'chromium' in the
   message fails it. It also pins the two branches apart — an unresolvable
   package must say
> @modelcontextprotocol/inspector@2.3.0 postinstall
> node scripts/install-clients.mjs

[install-clients] Installing dependencies for clients/web...

up to date, audited 583 packages in 614ms

198 packages are looking for funding
  run `npm fund` for details

found 0 vulnerabilities
[install-clients] Installing dependencies for clients/cli...

up to date, audited 237 packages in 496ms

69 packages are looking for funding
  run `npm fund` for details

found 0 vulnerabilities
[install-clients] Installing dependencies for clients/tui...

up to date, audited 314 packages in 518ms

102 packages are looking for funding
  run `npm fund` for details

found 0 vulnerabilities
[install-clients] Installing dependencies for clients/launcher...

up to date, audited 160 packages in 449ms

50 packages are looking for funding
  run `npm fund` for details

found 0 vulnerabilities

up to date, audited 281 packages in 3s

108 packages are looking for funding
  run `npm fund` for details

found 0 vulnerabilities and must NOT say ,
   which fetches binaries and cannot fix a missing package.

   The unsupported-engine test now also asserts loadPlaywright was never
   called, pinning the ORDER rather than only the message: validating
   first is what makes the error name the real mistake.

2. prod-web-server.mjs's header still described the  prefix this PR removed. Corrected, keeping why
   deriving paths from import.meta.url is still right, and adding the
   elicitation smoke to the list of callers it had also outgrown.

Addresses the suppressed comments on Copilot's review of #2133.

Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
cliffhall 2026-08-25 22:42:06 -04:00
commit 4ab0139de9
3 changed files with 134 additions and 26 deletions
Download patch file Download diff file Expand all files Collapse all files

47
scripts/lib/headless-browser.mjs
View file

@ -112,30 +112,55 @@ export function resolveBrowserName(env = process.env) {
return name;
}
/**
* Resolve the Playwright package from the web client's install.
*
* Separate from `loadBrowser` so it can be substituted in tests — see the
* `loadPlaywright` option there for why that seam has to exist.
*/
export function requirePlaywright(repoRoot) {
const requireFromWeb = createRequire(
join(repoRoot, "clients", "web", "package.json"),
);
return requireFromWeb("playwright");
}
/**
* Launch a headless browser of `browserName`, resolved from the web client's
* install.
*
* The launch-failure message names the engine that failed and the
* `playwright install` invocation that fixes it — naming "chromium" while
* WebKit was the thing missing sends the reader off to install a browser they
* already have.
* Both failure branches produce a message that names the actual remedy, and they
* are different remedies — which is the whole reason they are separate branches:
*
* - **not resolvable** means the Playwright *npm package* is missing, fixed by
* `npm install` at the repo root. `playwright install` (which fetches
* browser binaries) would not help and is the wrong thing to suggest.
* - **launch rejected** means the package is there but that engine's *binary*
* is not, fixed by `playwright install --with-deps <that engine>`. Naming
* "chromium" while WebKit was the missing one sends the reader off to
* install a browser they already have — the reason the engine is
* interpolated rather than hard-coded (#2086's acceptance criterion).
*
* `loadPlaywright` is injectable purely so those branches can be unit-tested.
* They are unreachable from the smokes by construction: `install-smoke-browser`
* runs first and fetches the binary, so a passing smoke proves nothing about the
* message a failing one would print — and that message IS the deliverable here,
* since its whole job is to be read by someone whose setup is broken.
*/
export async function loadBrowser(repoRoot, browserName = DEFAULT_BROWSER) {
export async function loadBrowser(
repoRoot,
browserName = DEFAULT_BROWSER,
{ loadPlaywright = requirePlaywright } = {},
) {
if (!SUPPORTED_BROWSERS.includes(browserName)) {
throw new Error(
`unsupported browser "${browserName}" — expected one of ${SUPPORTED_BROWSERS.join(", ")}`,
);
}
const requireFromWeb = createRequire(
join(repoRoot, "clients", "web", "package.json"),
);
let playwright;
try {
playwright = requireFromWeb("playwright");
playwright = loadPlaywright(repoRoot);
} catch (err) {
// Not resolvable means devDependencies are missing — fixed by `npm install`
// at the repo root, NOT by `playwright install` (which fetches binaries).
throw new Error(
`could not resolve the Playwright package from clients/web — run \`npm install\` at the repo root (${err instanceof Error ? err.message : String(err)})`,
);

91
scripts/lib/headless-browser.test.mjs
View file

@ -8,9 +8,15 @@
* a fallback to Chromium there would report a green run under a job labelled
* "webkit", claiming coverage that never ran.
*
* `loadBrowser`'s own launch path is deliberately not covered here — it takes a
* real browser binary, which is what the smokes are for. Only its argument
* validation, which fails before any of that, is.
* `loadBrowser`'s failure branches are covered here too, through its injectable
* `loadPlaywright`. The smokes cannot reach them by construction —
* `install-smoke-browser` fetches the binary before the smoke runs, so a passing
* smoke says nothing about what a failing one would print. And that message is
* the deliverable: #2086's acceptance criterion is that a missing browser fails
* naming *that engine* and its own `playwright install` command, which is
* exactly the kind of string that rots into naming the wrong one.
*
* Only the successful launch is left to the smokes, since it needs a real binary.
*/
import assert from "node:assert/strict";
@ -66,14 +72,85 @@ describe("resolveBrowserName", () => {
});
describe("loadBrowser", () => {
/** A Playwright stand-in whose every engine rejects on launch. */
const launchAlwaysFails = (message) => () => ({
chromium: { launch: () => Promise.reject(new Error(message)) },
firefox: { launch: () => Promise.reject(new Error(message)) },
webkit: { launch: () => Promise.reject(new Error(message)) },
});
it("rejects an unsupported engine before touching Playwright", async () => {
let loaded = false;
await assert.rejects(
// A repo root that does not exist: reaching the `createRequire` would
// throw a different (resolution) error, so this also pins the ORDER —
// validation first, so the message names the real mistake.
() => loadBrowser("/nonexistent-repo-root", "safari"),
() =>
loadBrowser("/nonexistent-repo-root", "safari", {
loadPlaywright: () => {
loaded = true;
return {};
},
}),
/unsupported browser "safari"/,
);
// Pins the ORDER, not just the message: validating first is what makes the
// error name the real mistake instead of a downstream resolution failure.
assert.equal(loaded, false);
});
it("names the engine that failed, and its own install command", async () => {
// The whole point of #2086's acceptance criterion: a reader whose WebKit
// binary is missing must not be sent to install chromium.
for (const name of SUPPORTED_BROWSERS) {
await assert.rejects(
() =>
loadBrowser("/repo", name, {
loadPlaywright: launchAlwaysFails("Executable doesn't exist"),
}),
(err) => {
assert.match(err.message, new RegExp(`^${name} failed to launch`));
assert.match(
err.message,
new RegExp(
`npx playwright install --with-deps ${name}\\\`(?![\\s\\S]*--with-deps (?!${name}))`,
),
);
// The underlying cause survives, so the reader can tell a missing
// binary from a sandbox/permissions problem.
assert.match(err.message, /Executable doesn't exist/);
// And no OTHER engine is named anywhere in the message.
for (const other of SUPPORTED_BROWSERS.filter((b) => b !== name)) {
assert.ok(
!err.message.includes(other),
`message for ${name} must not mention ${other}: ${err.message}`,
);
}
return true;
},
);
}
});
it("sends an unresolvable Playwright to `npm install`, not `playwright install`", async () => {
// Two different failures with two different remedies. Suggesting
// `playwright install` here would be actively wrong — it fetches browser
// binaries and cannot install the missing npm package.
await assert.rejects(
() =>
loadBrowser("/repo", "firefox", {
loadPlaywright: () => {
throw new Error("Cannot find module 'playwright'");
},
}),
(err) => {
assert.match(err.message, /could not resolve the Playwright package/);
assert.match(err.message, /npm install/);
assert.ok(
!/playwright install/.test(err.message),
`must not suggest \`playwright install\` for a missing package: ${err.message}`,
);
assert.match(err.message, /Cannot find module 'playwright'/);
return true;
},
);
});
});

22
scripts/lib/prod-web-server.mjs
View file

@ -2,15 +2,21 @@
* Shared boot/readiness helper for the prod web smokes.
*
* `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.
* (runs-the-bundle check, #1615), `scripts/smoke-web-app.mjs` (MCP Apps
* end-to-end, #1859) and `scripts/smoke-web-elicitation.mjs` (app-rendered
* elicitations, #1854) 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
* install` finds the local bin) doesn't affect which launcher/build tree runs.
* Repo-root paths are derived from import.meta.url rather than the cwd, so it
* does not matter where a caller is run from. (Until #2086 the browser-driven
* smokes were invoked through a `cd clients/web && npx playwright install …`
* prefix, which is what originally made that independence load-bearing; they now
* run from the repo root via `scripts/install-smoke-browser.mjs`. Deriving from
* import.meta.url is still the right call — it is what lets a smoke be run by
* hand from any directory.)
*/
import { spawn } from "node:child_process";