diff --git a/scripts/lib/headless-browser.mjs b/scripts/lib/headless-browser.mjs index d9282b09..50ec7d9b 100644 --- a/scripts/lib/headless-browser.mjs +++ b/scripts/lib/headless-browser.mjs @@ -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 `. 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)})`, ); diff --git a/scripts/lib/headless-browser.test.mjs b/scripts/lib/headless-browser.test.mjs index 187d02e7..c6b2d263 100644 --- a/scripts/lib/headless-browser.test.mjs +++ b/scripts/lib/headless-browser.test.mjs @@ -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; + }, + ); }); }); diff --git a/scripts/lib/prod-web-server.mjs b/scripts/lib/prod-web-server.mjs index 895d9509..1920267d 100644 --- a/scripts/lib/prod-web-server.mjs +++ b/scripts/lib/prod-web-server.mjs @@ -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";