diff --git a/scripts/install-smoke-browser.mjs b/scripts/install-smoke-browser.mjs index d052528c..16502ec1 100644 --- a/scripts/install-smoke-browser.mjs +++ b/scripts/install-smoke-browser.mjs @@ -21,64 +21,90 @@ * engine comes from `SMOKE_BROWSER` (default `chromium`); pass one explicitly * where the consumer's engine is fixed regardless of that variable, as * `pack:verify` does. + * + * Execution lives behind `main()` so importing this module for tests does not + * run an install — the same shape `verify-typecheck-coverage.mjs` uses. */ import { spawnSync } from "node:child_process"; import { join, resolve } from "node:path"; +import { pathToFileURL } from "node:url"; import { SUPPORTED_BROWSERS, resolveBrowserName, } from "./lib/headless-browser.mjs"; import { resolveNodeBin } from "./lib/resolve-node-bin.mjs"; -const repoRoot = resolve(import.meta.dirname, ".."); -const webDir = join(repoRoot, "clients", "web"); - -const requested = process.argv[2]; -let browserName; -try { - if (requested === undefined) { - browserName = resolveBrowserName(); - } else if (SUPPORTED_BROWSERS.includes(requested)) { - browserName = requested; - } else { +/** + * Which engine to install: the explicit argument if given, else `SMOKE_BROWSER`. + * + * The precedence is the load-bearing part, and it is why this is a function + * rather than three lines inline. `pack:verify` passes `chromium` explicitly + * *because* it launches Chromium explicitly — if the argument ever lost to the + * environment, `SMOKE_BROWSER=webkit npm run pack:verify` would install WebKit + * and then launch Chromium, and the mismatch would surface as a confusing + * "Executable doesn't exist" rather than as the wiring error it is. + * + * An explicit argument is validated here rather than deferred, so a typo in an + * npm script fails naming itself instead of reaching the Playwright CLI. + */ +export function resolveRequestedBrowser(argv = [], env = process.env) { + const requested = argv[0]; + if (requested === undefined) return resolveBrowserName(env); + if (!SUPPORTED_BROWSERS.includes(requested)) { throw new Error( `unsupported browser "${requested}" — expected one of ${SUPPORTED_BROWSERS.join(", ")}`, ); } -} catch (err) { - console.error( - `install-smoke-browser: ${err instanceof Error ? err.message : String(err)}`, - ); - process.exit(1); + return requested; } -let cli; -try { - cli = resolveNodeBin("playwright", "playwright", webDir); -} catch (err) { - console.error( - "install-smoke-browser: could not resolve the Playwright CLI from clients/web — " + - `run \`npm install\` at the repo root (${err instanceof Error ? err.message : String(err)})`, - ); - process.exit(1); +function main() { + const repoRoot = resolve(import.meta.dirname, ".."); + const webDir = join(repoRoot, "clients", "web"); + + let browserName; + try { + browserName = resolveRequestedBrowser(process.argv.slice(2), process.env); + } catch (err) { + console.error( + `install-smoke-browser: ${err instanceof Error ? err.message : String(err)}`, + ); + process.exit(1); + } + + let cli; + try { + cli = resolveNodeBin("playwright", "playwright", webDir); + } catch (err) { + console.error( + "install-smoke-browser: could not resolve the Playwright CLI from clients/web — " + + `run \`npm install\` at the repo root (${err instanceof Error ? err.message : String(err)})`, + ); + process.exit(1); + } + + // `install` is a no-op when the binary is already present, so this is cheap on + // a warm machine and on a CI runner whose Playwright cache was restored. + const result = spawnSync(process.execPath, [cli, "install", browserName], { + cwd: webDir, + stdio: "inherit", + }); + if (result.error) { + console.error( + `install-smoke-browser: could not run the Playwright CLI: ${result.error.message}`, + ); + process.exit(1); + } + if (result.status !== 0) { + console.error( + `install-smoke-browser: \`playwright install ${browserName}\` exited ${result.status}`, + ); + process.exit(result.status ?? 1); + } } -// `install` is a no-op when the binary is already present, so this is cheap on -// a warm machine and on a CI runner whose Playwright cache was restored. -const result = spawnSync(process.execPath, [cli, "install", browserName], { - cwd: webDir, - stdio: "inherit", -}); -if (result.error) { - console.error( - `install-smoke-browser: could not run the Playwright CLI: ${result.error.message}`, - ); - process.exit(1); -} -if (result.status !== 0) { - console.error( - `install-smoke-browser: \`playwright install ${browserName}\` exited ${result.status}`, - ); - process.exit(result.status ?? 1); -} +// Only when run as a script, never on import (see the header). Same guard as +// verify-typecheck-coverage.mjs — `pathToFileURL` so it holds on Windows paths. +if (process.argv[1] && import.meta.url === pathToFileURL(process.argv[1]).href) + main(); diff --git a/scripts/install-smoke-browser.test.mjs b/scripts/install-smoke-browser.test.mjs new file mode 100644 index 00000000..0b7bd589 --- /dev/null +++ b/scripts/install-smoke-browser.test.mjs @@ -0,0 +1,88 @@ +/** + * Unit tests for the smoke browser installer's argument resolution (#2086). + * + * The installer's I/O half — resolving the Playwright CLI and spawning it — is + * exercised on every `npm run smoke:web:*`, so it fails loudly the moment it + * breaks. Its *argument* half is not: every caller in the repo passes either + * nothing or the literal `chromium`, so the precedence rule below is asserted by + * nothing at runtime, and getting it wrong produces a mismatch (install one + * engine, launch another) that surfaces as a misleading "Executable doesn't + * exist" rather than as the wiring error it is. + * + * Importing this module must not run an install; `main()` is behind an + * entrypoint guard for exactly that reason, and these tests are also what would + * catch that guard regressing — an unguarded module would try to install a + * browser the moment the suite imported it. + */ + +import assert from "node:assert/strict"; +import { describe, it } from "node:test"; +import { resolveRequestedBrowser } from "./install-smoke-browser.mjs"; +import { + BROWSER_ENV_VAR, + DEFAULT_BROWSER, + SUPPORTED_BROWSERS, +} from "./lib/headless-browser.mjs"; + +describe("resolveRequestedBrowser", () => { + it("falls back to the environment when no argument is given", () => { + assert.equal(resolveRequestedBrowser([], {}), DEFAULT_BROWSER); + assert.equal( + resolveRequestedBrowser([], { [BROWSER_ENV_VAR]: "webkit" }), + "webkit", + ); + }); + + it("lets an explicit argument beat the environment", () => { + // The `pack:verify` invariant. It passes `chromium` because it LAUNCHES + // chromium; if the environment won here, `SMOKE_BROWSER=webkit npm run + // pack:verify` would install WebKit and then launch Chromium. + assert.equal( + resolveRequestedBrowser(["chromium"], { [BROWSER_ENV_VAR]: "webkit" }), + "chromium", + ); + for (const name of SUPPORTED_BROWSERS) { + assert.equal( + resolveRequestedBrowser([name], { [BROWSER_ENV_VAR]: "firefox" }), + name, + ); + } + }); + + it("rejects an unsupported argument, naming it and the allowed set", () => { + assert.throws( + () => resolveRequestedBrowser(["safari"], {}), + (err) => { + assert.match(err.message, /unsupported browser "safari"/); + SUPPORTED_BROWSERS.forEach((name) => + assert.ok(err.message.includes(name)), + ); + return true; + }, + ); + }); + + it("does not let a bad argument fall through to the environment", () => { + // Silently using SMOKE_BROWSER after ignoring a typo'd argument would + // install an engine nobody asked for, and the npm script's typo would + // survive unnoticed. + assert.throws( + () => + resolveRequestedBrowser(["chrome"], { [BROWSER_ENV_VAR]: "firefox" }), + /unsupported browser "chrome"/, + ); + }); + + it("propagates the environment's own validation, empty included", () => { + // Delegation, not a second copy of the rules — so the deny-by-default + // behavior cannot drift between the two entry points. + assert.throws( + () => resolveRequestedBrowser([], { [BROWSER_ENV_VAR]: "safari" }), + /is not a supported browser/, + ); + assert.throws( + () => resolveRequestedBrowser([], { [BROWSER_ENV_VAR]: "" }), + /set but empty/, + ); + }); +}); diff --git a/scripts/lib/headless-browser.mjs b/scripts/lib/headless-browser.mjs index 50ec7d9b..84f148c8 100644 --- a/scripts/lib/headless-browser.mjs +++ b/scripts/lib/headless-browser.mjs @@ -99,11 +99,27 @@ export const BROWSER_ENV_VAR = "SMOKE_BROWSER"; * An unrecognized value FAILS rather than falling back to Chromium: a silent * fallback would report a green Chromium run under a job labelled "webkit", * which is worse than no coverage — it claims coverage that never ran. + * + * **Only an ABSENT variable selects the default. A present-but-empty one is an + * error**, because that is what an unresolved CI expression looks like: GitHub + * Actions renders `SMOKE_BROWSER: ${{ matrix.browsr }}` (or any undefined key) + * as the empty string rather than omitting the variable. Treating that as "not + * set" would run Chromium inside a job named for another engine — precisely the + * false-coverage failure this resolver exists to prevent, arriving through a + * typo instead of a bad value. Unsetting the variable is how you ask for the + * default; setting it to nothing is not. */ export function resolveBrowserName(env = process.env) { const raw = env[BROWSER_ENV_VAR]; - if (raw === undefined || raw.trim() === "") return DEFAULT_BROWSER; + if (raw === undefined) return DEFAULT_BROWSER; const name = raw.trim().toLowerCase(); + if (name === "") { + throw new Error( + `${BROWSER_ENV_VAR} is set but empty — an unresolved CI expression ` + + `(a misspelled \`matrix\` key renders as "") looks exactly like this. ` + + `Unset it to use ${DEFAULT_BROWSER}, or set one of ${SUPPORTED_BROWSERS.join(", ")}.`, + ); + } if (!SUPPORTED_BROWSERS.includes(name)) { throw new Error( `${BROWSER_ENV_VAR}="${raw}" is not a supported browser — expected one of ${SUPPORTED_BROWSERS.join(", ")}`, diff --git a/scripts/lib/headless-browser.test.mjs b/scripts/lib/headless-browser.test.mjs index c6b2d263..bc9c2645 100644 --- a/scripts/lib/headless-browser.test.mjs +++ b/scripts/lib/headless-browser.test.mjs @@ -35,13 +35,25 @@ describe("resolveBrowserName", () => { assert.equal(resolveBrowserName({}), DEFAULT_BROWSER); }); - it("treats an empty or whitespace-only value as unset", () => { - // A CI expression that resolves to nothing (`SMOKE_BROWSER: ${{ … }}` with - // an undefined matrix key) sets the variable to "" rather than removing it. - for (const raw of ["", " "]) { - assert.equal( - resolveBrowserName({ [BROWSER_ENV_VAR]: raw }), - DEFAULT_BROWSER, + it("rejects a present-but-empty value instead of defaulting", () => { + // This is the case worth being strict about. A CI expression that resolves + // to nothing (`SMOKE_BROWSER: ${{ matrix.browsr }}`, an undefined key) sets + // the variable to "" rather than removing it — so defaulting here would run + // Chromium inside a job named for another engine, which is the very + // false-coverage outcome the unrecognized-value branch exists to stop. Only + // an ABSENT variable may select the default. + for (const raw of ["", " ", "\t\n"]) { + assert.throws( + () => resolveBrowserName({ [BROWSER_ENV_VAR]: raw }), + (err) => { + assert.match(err.message, /set but empty/); + // Names the likely cause and the actual remedy — "unset it", which is + // different from "set it to nothing" and is the whole point here. + assert.match(err.message, /matrix/); + assert.match(err.message, /Unset it/); + return true; + }, + `expected ${JSON.stringify(raw)} to be rejected`, ); } });