Merge pull request #2158 from modelcontextprotocol/v2/fix/2147-smoke-tui-pty
fix(scripts): make smoke:tui assert the TUI survives its first frame (#2147)
This commit is contained in:
Cliff Hall
GitHub
commit
833ad40a45
9 changed files with 1143 additions and 197 deletions
1
.github/copilot-instructions.md
vendored
1
.github/copilot-instructions.md
vendored
|
|
@ -105,6 +105,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` → `verify:bundle-externals` → `smoke` → `smoke:web:firefox` → Storybook; the Firefox step is local-only and not mirrored in GitHub CI). `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 — it derives its candidates from what each `tsc` program actually resolves (`tsc --listFilesOnly`, keeping packages that reach one program from two installs), so a package reached only through another package's `.d.ts` counts too (#1965) — so a PR bumping a package the shared sources pull in 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`; `pack:verify`'s `--web` child sets its own `MCP_CATALOG_PATH` for the same reason (#2003 — its App deep link persists a server row). Anything that boots the web backend and then *navigates* it needs that isolation, not just the scripts named `smoke:*`. 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.
|
||||
- **A smoke must assert the thing survives, not that it started.** `smoke:tui` runs the Ink TUI under a pseudoterminal (`scripts/lib/pty.mjs`) and requires it to still be running `SMOKE_TUI_SURVIVE_MS` after its first frame (`scripts/lib/render-smoke.mjs`) — because it previously settled OK on first paint while the TUI was exiting 1 ~40ms later with `Raw mode is not supported`, and won that race on every machine (#2147). Flag anything that collapses the survival wait back into "resolve on the marker", and any new smoke whose only assertion is that a process printed something once. A smoke walks only its own happy path, so the branch that matters — started, then died — belongs in a `scripts/lib/*.test.mjs` driven against a stub (as with `announced-child.mjs` #2000 and `ensure-test-servers.mjs` #2111), not in the smoke.
|
||||
- **The headless web smokes are engine-parameterized, not Chromium-only.** `smoke:web:browser` / `smoke:web:app` / `smoke:web:elicit` take their engine from `SMOKE_BROWSER` (chromium — the default — firefox, webkit). **GitHub CI runs Chromium only; Firefox runs in the local pre-push gate** via `smoke:web:firefox` (#2086 — a CI job was trialled and removed for never disagreeing with Chromium across a dozen runs, and moved into `npm run ci` instead). They exist because the MCP Apps sandbox is built out of the primitives that diverge between engines: `srcdoc` CSP inheritance, nested sandboxed iframes, `Permissions-Policy`, cross-frame `postMessage`. Nothing else covers that — `sandbox-csp.test.ts` asserts a policy *string*, and no Storybook story reaches the sandbox at all. Flag a new browser-driven script that launches Playwright itself instead of going through `scripts/lib/headless-browser.mjs`, an unrecognized-engine path that falls back to Chromium rather than failing (it would claim coverage that never ran), or a launch-failure message naming the wrong engine. `pack:verify` is pinned to Chromium on purpose — it is a packaging check.
|
||||
|
||||
- **Build output is never a gate target.** Lint, format, and typecheck read first-party source only; everything a build writes (`clients/*/build`, `clients/web/dist`, `storybook-static`, `coverage`, `test-servers/build`, `core/**/{build,dist}`, `*.tsbuildinfo`) stays out via each scope's `globalIgnores`, `format` globs, and tsconfig `include`. Gating generated code reports defects in vendored third-party source that nobody can fix, and a rule promotion turns that warning into a `validate` failure (#2043). Flag a PR that adds a build location without ignoring it in the same change, that widens an ignore to silence a finding in first-party code, or that adds a build directory to a tsconfig `include` to make a generated `.d.ts` resolve. Note the coverage guards don't catch this — they assert source is _covered_, not that output is _excluded_.
|
||||
|
|
|
|||
|
|
@ -973,7 +973,13 @@ The ⚠️ option-deletion hazard, the snapshot rule, and the recovery recipe ab
|
|||
- `smoke:web:elicit` (`scripts/smoke-web-elicitation.mjs`, #1854) is the app-rendered **elicitation** counterpart of `smoke:web:app`: same prod `--web` server and the same deep-link connect, but it then calls `app_choose_option` from the Tools tab, waits for `[data-testid="app-elicitation"][data-app-elicitation-status="ready"]`, clicks a choice **inside the sandboxed app** (two `frameLocator` hops — the trusted sandbox proxy, then the untrusted app), and asserts the app's standard `ElicitResult` comes back in the *tool result*, i.e. that it reached the server rather than merely the host. It then repeats against `app-elicitation-native-http.json` — the same tool and app on a server that never advertised the nested MCP Apps `elicitation` capability — and asserts the **native** elicitation dialog takes it and no app modal is rendered. That second half is the more valuable one: the failure this feature can produce is not "the app doesn't render" but "an app renders when it should not have been offered one", which strands every user of a server that never opted in. Set `SMOKE_SCREENSHOT_DIR` to capture PNGs of the three states (used for PR proof); unset, it asserts only. Two mechanics worth knowing: the main-view tabs are a Mantine `SegmentedControl`, so there is no `role="tab"` — the clickable element is the sibling `label[for$="-Tools"]`; and the prompt string also appears in the (hidden) Protocol-tab payload, so the fallback assertion is scoped to the dialog rather than a bare text lookup.
|
||||
|
||||
- **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.
|
||||
- `smoke:cli` (`scripts/smoke-cli.mjs`) drives `mcp-inspector --cli` through the built launcher against the bundled stdio test server via a temp `--catalog`: it asserts `tools/list` returns the server's tools (real connect over stdio), the default writable catalog is seeded empty on first run, a missing read-only `--config` errors without seeding, and `--catalog` + `--config` is rejected. `smoke:tui` (`scripts/smoke-tui.mjs`) launches `mcp-inspector --tui --catalog <temp>` and asserts the Ink app renders its first frame (the "MCP Servers" panel) within a timeout, then SIGTERMs it — a shallow boot/render check, not full interaction. **`smoke:tui` is local-only: it self-skips when `process.env.CI` is set**, because the Ink TUI needs a real TTY (raw mode) that headless CI lacks — so run it (via `npm run smoke`) on your own machine before pushing. Both build `test-servers/build` on demand if it's missing.
|
||||
- `smoke:cli` (`scripts/smoke-cli.mjs`) drives `mcp-inspector --cli` through the built launcher against the bundled stdio test server via a temp `--catalog`: it asserts `tools/list` returns the server's tools (real connect over stdio), the default writable catalog is seeded empty on first run, a missing read-only `--config` errors without seeding, and `--catalog` + `--config` is rejected. `smoke:tui` (`scripts/smoke-tui.mjs`) launches `mcp-inspector --tui --catalog <temp>` **under a pseudoterminal**, asserts the Ink app renders its first frame (the "MCP Servers" panel) within a timeout, then waits `SMOKE_TUI_SURVIVE_MS` (default 2s) and asserts it is **still running** before SIGTERMing it — a shallow boot/render/survival check, not full interaction. Both rebuild `test-servers/build` on **every run** — once per process, whether or not it already exists (#2111): presence is not freshness, and a smoke driving a stale fixture reports a product failure rather than a staleness one.
|
||||
|
||||
**The survival half is the assertion, and it is the whole point (#2147).** This smoke used to settle OK the instant the marker appeared and never look at the child again. It was spawning the TUI with `stdio: ["ignore", …]`, i.e. stdin on `/dev/null`; Ink mounts `useInput`, `useInput` needs raw mode, and raw mode is a property of the **file descriptor** — so the TUI painted one frame and died ~40ms later with `Raw mode is not supported on the current process.stdin`. The smoke won that ~40ms race and printed OK. What it asserted was *first paint*, while its own header claimed "boots and renders without crashing", and it had never once verified a running TUI **on any machine** — `stdio: ["ignore", …]` makes the child's stdin `/dev/null` whether or not the developer's own terminal is a TTY. The race also cut both ways: an `exit` processed first failed the run, which reads as flake rather than as the standing defect it was.
|
||||
|
||||
Two pieces, and the order matters. **`scripts/lib/pty.mjs`** gives the child a real terminal via `script(1)` — no dependency, and the flavors are not interchangeable (BSD takes argv, util-linux takes one shell string plus `-e`, busybox that string without `-e`), so the invocation is built and unit-tested rather than written inline; a platform with no `script(1)` (Windows) **skips** rather than running without one, since that is a guaranteed failure and not a weaker check. **`scripts/lib/render-smoke.mjs`** is the fix: it requires the child to outlive its first frame. Without that, a future harness change reintroduces the same false green and nothing notices — which is why it lives in a module `test:scripts` drives against a stub that paints the marker and immediately exits (the old harness passes that stub; this one must not), rather than in a smoke that only ever walks its own happy path. Do **not** collapse the survival wait back into "resolve on the marker" to save two seconds.
|
||||
|
||||
**`smoke:tui` is still local-only: it self-skips when `process.env.CI` is set** — now by decision rather than by capability. The PTY removes the technical blocker the skip used to cite; whether it joins GitHub CI is a separate call, and #2146 deliberately keeps the other local-only smokes out. So run it (via `npm run smoke`) on your own machine before pushing: a local-only gate is exactly where a false green is least likely to be caught by anything else.
|
||||
- Storybook play-function tests (`clients/web` `test:storybook`) run in headless Chromium via `@vitest/browser-playwright` (~10s). They are part of `npm run ci` (which installs Playwright chromium first); kept out of `validate` because they need the browser binary and are slower than the unit suite.
|
||||
|
||||
### The web smokes are engine-parameterized; Firefox is in the pre-push gate, not CI (#2086)
|
||||
|
|
|
|||
|
|
@ -486,11 +486,11 @@ Each client self-validates from its own folder; the root scripts chain them. The
|
|||
| ----------------------------------- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
|
||||
| `npm run validate` | Runs the three durable guards first — `verify:format-coverage` (every tracked source file is format-gated), `verify:typecheck-coverage` (every one lands in a tsconfig project), `verify:dep-lockstep` (no dependency reaching one `tsc` program from two installs skews across them) — then `test:scripts` (the guards' own parser unit tests), then `validate:core` (the shared `core/` `format:check` + `lint` gate), then per client: `format:check` + `lint` + **`typecheck`** (cli/tui/launcher; web typechecks via `tsc -b` inside its `build`) + `build` + fast unit tests. The quick inner-loop check. |
|
||||
| `npm run coverage` | The **per-file ≥90% gate** (lines/statements/functions/branches) under v8 instrumentation, per client. CI-enforced. For web this also runs the integration project and covers the shared `core/` runtime (including `core/json` and `core/client`). |
|
||||
| `npm run smoke` | End-to-end smokes through the built launcher (`--help` dispatch + prod cli/tui/web), plus three headless-browser smokes: a boot smoke that runs the prod web bundle and asserts a clean first render (no uncaught error — sync exception or unhandled rejection, how a Node built-in reaching the browser bundle manifests), and an **MCP Apps** smoke (`smoke:web:app`) that drives connect → open app → `data-app-status="ready"` against a composable App server, covering the sandbox proxy and UI-protocol bridge, and an **app-rendered elicitation** smoke (`smoke:web:elicit`) that drives one end to end — call the tool, answer inside the sandboxed app, see the app's `ElicitResult` reach the server — and then the same tool against a server that never advertised the capability, which must fall back to the native elicitation form. All three take their engine from `SMOKE_BROWSER` — see [Supported browsers](#supported-browsers). |
|
||||
| `npm run smoke` | End-to-end smokes through the built launcher (`--help` dispatch + prod cli/tui/web — the TUI one runs under a **pseudoterminal** and asserts the Ink app is still running two seconds after its first frame, not merely that it painted one, [#2147](https://github.com/modelcontextprotocol/inspector/issues/2147)), plus three headless-browser smokes: a boot smoke that runs the prod web bundle and asserts a clean first render (no uncaught error — sync exception or unhandled rejection, how a Node built-in reaching the browser bundle manifests), and an **MCP Apps** smoke (`smoke:web:app`) that drives connect → open app → `data-app-status="ready"` against a composable App server, covering the sandbox proxy and UI-protocol bridge, and an **app-rendered elicitation** smoke (`smoke:web:elicit`) that drives one end to end — call the tool, answer inside the sandboxed app, see the app's `ElicitResult` reach the server — and then the same tool against a server that never advertised the capability, which must fall back to the native elicitation form. All three take their engine from `SMOKE_BROWSER` — see [Supported browsers](#supported-browsers). |
|
||||
| `npm run verify:build-gate` | Runs a real `vite build` with a Node built-in forced into the browser graph and asserts the build **fails** via the #1769 gate (which turns Vite's browser-externalization warning into a hard error). Guards against the warning phrasing drifting in a Vite bump and silently disabling the gate. Part of `npm run ci`. |
|
||||
| `npm run verify:bundle-externals` | Guards the must-not-bundle invariant (#2067): for each tsup-bundled client it reads the **built** `build/` output and fails if any package that must stay external was inlined anyway. Candidates are the union of the client's own `external` array **and the root manifest's `dependencies`** — the latter because #2067 was a *missing* `external` entry, which a self-referential check would have passed. Detection is via esbuild's `// <path>` module banners, so it covers both shapes — a separate `<pkg>-HASH.js` chunk (what a dynamically `import()`ed CommonJS package produces) and a statically-imported package folded straight into `index.js`, which emits no chunk at all. A build with no banners fails as such rather than passing clean, so enabling `minify` cannot silently retire the check. `undici` was declared only in the root and `clients/cli` manifests, and tsup auto-externalizes only what the _nearest_ manifest declares, so the web and TUI bundles inlined 1.05MB of it — and CommonJS inlined into an ESM bundle throws `Dynamic require of "assert" is not supported` on first use, from a specifier no user-side install can satisfy. Reads the output rather than the config because those two disagreed for four releases. Part of `npm run ci`. |
|
||||
| `npm run verify:format-coverage` | Parses the `format:check` globs out of every `package.json` (only those reachable from `validate`), enumerates all tracked source files, and **fails** listing any not covered by a glob — the durable guard for the "every first-party source file is format-gated" invariant (#1792). Runs first in `validate`. |
|
||||
| `npm run test:scripts` | Table-driven unit tests (`node --test`) for the guard's own pure parsers (`scripts/lib/npm-scripts.mjs`, `scripts/lib/tsc-program.mjs` + the exported helpers of `verify-typecheck-coverage.mjs` and `verify-dep-lockstep.mjs`), one case per rule they encode, plus two suites over shared `scripts/lib` helpers that no smoke can check itself: `resolve-node-bin.test.mjs` — the cross-platform bin resolver (#1939), pinned against the real `bin`/`exports` shapes of the packages the scripts actually spawn — and `announced-child.test.mjs` — the spawn/readiness ownership helper (#2000), which drives real `node -e` children to prove a child that never announces is still published to the caller before the timeout throws, and so is reachable by teardown rather than orphaned. Two more do the same: `mcp-app-flow.test.mjs` covers the shared MCP Apps flow (#2003) — the deep link's two CSRF gates and `appArgs` encoding, plus `driveAppFlow`'s failure branches against a stand-in page, all of which are dead code from the happy-path smokes' point of view and would otherwise surface only as opaque timeouts; and `ensure-test-servers.test.mjs` pins the [#2111](https://github.com/modelcontextprotocol/inspector/issues/2111) invariant — that `test-servers/build` is rebuilt **even when it already exists** — which no smoke can assert about itself, since one driving a stale fixture reports a product failure rather than a staleness one. Runs in `validate` — and `verify:typecheck-coverage` guards *this* gate in turn (reachable from `validate`, non-empty test set, every test file matched by the `test:scripts` glob), since `node --test` silently skips a file its glob misses and still exits 0. |
|
||||
| `npm run test:scripts` | Table-driven unit tests (`node --test`) for the guard's own pure parsers (`scripts/lib/npm-scripts.mjs`, `scripts/lib/tsc-program.mjs` + the exported helpers of `verify-typecheck-coverage.mjs` and `verify-dep-lockstep.mjs`), one case per rule they encode, plus two suites over shared `scripts/lib` helpers that no smoke can check itself: `resolve-node-bin.test.mjs` — the cross-platform bin resolver (#1939), pinned against the real `bin`/`exports` shapes of the packages the scripts actually spawn — and `announced-child.test.mjs` — the spawn/readiness ownership helper (#2000), which drives real `node -e` children to prove a child that never announces is still published to the caller before the timeout throws, and so is reachable by teardown rather than orphaned. Four more do the same: `mcp-app-flow.test.mjs` covers the shared MCP Apps flow (#2003) — the deep link's two CSRF gates and `appArgs` encoding, plus `driveAppFlow`'s failure branches against a stand-in page, all of which are dead code from the happy-path smokes' point of view and would otherwise surface only as opaque timeouts; `render-smoke.test.mjs` and `pty.test.mjs` cover the TUI boot harness ([#2147](https://github.com/modelcontextprotocol/inspector/issues/2147)) — the former driving real `node -e` stubs to prove that a child which paints the marker and *then* dies is a **failure**, which the old harness reported as OK and which no fixed TUI can reproduce; the latter pinning the three mutually-incompatible `script(1)` invocations, where a wrong guess stops the pseudoterminal from being allocated at all; and `ensure-test-servers.test.mjs` pins the [#2111](https://github.com/modelcontextprotocol/inspector/issues/2111) invariant — that `test-servers/build` is rebuilt **even when it already exists** — which no smoke can assert about itself, since one driving a stale fixture reports a product failure rather than a staleness one. Runs in `validate` — and `verify:typecheck-coverage` guards *this* gate in turn (reachable from `validate`, non-empty test set, every test file matched by the `test:scripts` glob), since `node --test` silently skips a file its glob misses and still exits 0. |
|
||||
| `npm run verify:typecheck-coverage` | The typecheck-coverage analog of the above (#1791): for each Node client (auto-discovered from disk — enrolled via its `typecheck` script's projects, or for a `tsc -b` client like `clients/web` via its `tsconfig.json` `references`) it runs those projects with `tsc --listFilesOnly`, unions them, and **fails** listing any tracked `.ts`/`.tsx`/`.mts`/`.cts` under the client that lands in no project (so a new top-level config/helper can't silently go untypechecked). It also requires, deny-by-default, the first-party TS no client owns (`test-servers/src`, the root `vitest.shared.mts`, all of `core/`, and any new top-level location) to land in some client project's tsc pass — so a `core` `*.tsx` web's projects don't reach is caught too. Also asserts the gate is wired (each client's typecheck pass — its `typecheck` script, or web's `tsc -b` — is reachable from its `validate`, and the root chain runs each client's `validate`). Runs in `validate`. |
|
||||
| `npm run verify:dep-lockstep` | Guards the "one version per install-crossing dependency" invariant (#1896). v2 is not a workspace, so a client's test project compiles the shared first-party TypeScript — `core/`, `test-servers/src`, and the root-owned `vitest.shared.mts`, all of which resolve their dependencies from the **root** install — alongside the client's own sources, putting the same package in one `tsc` program twice. At the same version that's harmless; skewed, TypeScript must relate two structurally-distinct copies of every type, which for a recursive-generic surface is exponential (zod `4.3.6` vs `4.4.3` exhausted the 4GB tsc heap in `clients/web`). Derives its candidate set from **what actually enters each program** (#1965) — every client tsconfig project listed with `tsc --listFilesOnly` via the shared `scripts/lib/tsc-program.mjs`, each resolved `node_modules` file mapped to its owning install, keeping the packages that reach one program from two installs (a package whose declarations arrive only through another package's `.d.ts`, as `@modelcontextprotocol/sdk`'s do, is invisible to a scan of first-party imports). Prices each copy from the lockfile entry for the exact install path the program resolved, compares only the installs that met in one program, and **fails deny-by-default** on any disagreement not in the annotated `TOLERATED_SKEW` allowlist — empty today — with an allowlisted package tolerated only *within a major version*. Runs in `validate`.
|
||||
| `npm run ci` | **Mandatory pre-push command.** `validate` → `coverage` → `verify:build-gate` → `verify:bundle-externals` → `smoke` → `smoke:web:firefox` → Storybook. A strict superset of GitHub CI — `smoke:web:firefox` (and `smoke:tui`) run only here. |
|
||||
|
|
|
|||
|
|
@ -87,11 +87,21 @@ through the built launcher artifact (beyond the `--help` checks in
|
|||
resolution paths (default-catalog seed-on-missing, read-only `--config`
|
||||
error-without-seed, `--catalog`/`--config` conflict).
|
||||
- `npm run smoke:tui` (`scripts/smoke-tui.mjs`) — launches
|
||||
`mcp-inspector --tui --catalog <temp>` and asserts the Ink app renders its
|
||||
first frame within a timeout, then shuts it down (a shallow boot/render
|
||||
`mcp-inspector --tui --catalog <temp>` **under a pseudoterminal**, asserts the
|
||||
Ink app renders its first frame within a timeout, then waits and asserts it is
|
||||
**still running** before shutting it down (a shallow boot/render/survival
|
||||
check, not full interaction).
|
||||
|
||||
Both build `test-servers/build` on demand if it is missing.
|
||||
The second half is the assertion (#2147). Spawned with its stdin on
|
||||
`/dev/null`, Ink cannot enter raw mode for `useInput`, so the TUI painted one
|
||||
frame and exited 1 about 40ms later — and this smoke, which settled OK on the
|
||||
first frame, won that race and reported success on every machine. It is
|
||||
local-only (self-skips under `CI`), which is precisely where a false green
|
||||
goes unnoticed.
|
||||
|
||||
Both rebuild `test-servers/build` on **every run** — once per process, whether
|
||||
or not it already exists (#2111). Presence is not freshness: a smoke driving a
|
||||
stale fixture reports a product failure rather than a staleness one.
|
||||
|
||||
## Development
|
||||
|
||||
|
|
|
|||
180
scripts/lib/pty.mjs
Normal file
180
scripts/lib/pty.mjs
Normal file
|
|
@ -0,0 +1,180 @@
|
|||
/**
|
||||
* Wrap a command so it runs attached to a pseudoterminal (#2147).
|
||||
*
|
||||
* `smoke:tui` spawns the Ink TUI with `stdio: ["ignore", …]`, which makes the
|
||||
* child's stdin `/dev/null`. Ink mounts `useInput`, `useInput` needs raw mode,
|
||||
* and raw mode is a property of the *file descriptor* — so the child threw
|
||||
* "Raw mode is not supported on the current process.stdin" and exited 1 about
|
||||
* 40ms after painting its first frame, on every machine, TTY or not. A parent
|
||||
* cannot assert raw mode on a child's behalf; the fd has to be a terminal.
|
||||
*
|
||||
* `script(1)` allocates one, with no dependency to install. Measured here on
|
||||
* darwin: without it the TUI paints at ~400ms and is dead at ~440ms; with it
|
||||
* the same TUI is still running at 6s and never logs the raw-mode error.
|
||||
*
|
||||
* The invocation differs by flavor, which is why this is a module rather than
|
||||
* a string literal at the call site:
|
||||
*
|
||||
* BSD (macOS) script -q /dev/null <cmd> <args...> argv, no shell
|
||||
* util-linux script -qec "<cmd args...>" /dev/null ONE shell string
|
||||
* busybox script -qc "<cmd args...>" /dev/null as above, no -e
|
||||
*
|
||||
* The `-c` flavors take a single command *string* that the child shell parses,
|
||||
* so every word has to be quoted on the way in — an unquoted temp path with a
|
||||
* space would otherwise split into two arguments.
|
||||
*
|
||||
* `-e` (util-linux) makes `script` exit with the child's status rather than its
|
||||
* own. busybox has no such flag, so under it a crashed child reports 0. That is
|
||||
* survivable and deliberately not worked around: the smoke's assertion is the
|
||||
* child's `exit` *event*, not its code — the code only sharpens the diagnostic.
|
||||
*/
|
||||
|
||||
import { spawnSync } from "node:child_process";
|
||||
|
||||
/**
|
||||
* Platforms whose `script` takes the BSD argv form (`script -q FILE cmd args…`).
|
||||
*
|
||||
* Deliberately just these two. darwin is verified empirically here; freebsd is
|
||||
* where darwin's implementation comes from and shares its usage. **netbsd and
|
||||
* openbsd are excluded on purpose** — the BSDs do not agree on this, NetBSD's
|
||||
* `script` takes the command through `-c` — and nobody has run either here. A
|
||||
* guessed invocation is worse than none: it fails at `script` startup, which
|
||||
* this smoke would then report as a TUI crash. They fall through to `null` and
|
||||
* the smoke skips, saying so. Adding a `-c` flavor for them is a small change
|
||||
* for whoever can actually verify it on the box.
|
||||
*/
|
||||
const BSD_PLATFORMS = new Set(["darwin", "freebsd"]);
|
||||
|
||||
/**
|
||||
* Quote one argument for a POSIX shell command string.
|
||||
*
|
||||
* Single quotes are literal for everything except a single quote itself, which
|
||||
* has to leave the quoted run, emit an escaped quote, and re-enter it.
|
||||
*
|
||||
* @param {string} arg
|
||||
* @returns {string}
|
||||
*/
|
||||
export function shellQuote(arg) {
|
||||
return `'${String(arg).replaceAll("'", `'\\''`)}'`;
|
||||
}
|
||||
|
||||
/**
|
||||
* Decide which `script` flavor to build for.
|
||||
*
|
||||
* The version output is consulted first because it is the only *evidence*:
|
||||
* "linux" does not imply util-linux (Alpine ships busybox, whose `script`
|
||||
* rejects `-e`). Platform is the fallback for the case where `script --version`
|
||||
* tells us nothing, which is itself informative — BSD `script` has no
|
||||
* `--version` and fails the probe.
|
||||
*
|
||||
* @param {object} opts
|
||||
* @param {string} opts.platform `process.platform`.
|
||||
* @param {string} [opts.versionOutput] Combined stdout+stderr of `script --version`.
|
||||
* @returns {"bsd" | "util-linux" | "busybox" | null} null when unsupported.
|
||||
*/
|
||||
export function scriptFlavorFor({ platform, versionOutput = "" }) {
|
||||
if (/util-linux/i.test(versionOutput)) return "util-linux";
|
||||
if (/busybox/i.test(versionOutput)) return "busybox";
|
||||
if (BSD_PLATFORMS.has(platform)) return "bsd";
|
||||
// Linux with an unidentifiable `script` is overwhelmingly util-linux; guess
|
||||
// it rather than skip the smoke. A wrong guess fails loudly with `script`'s
|
||||
// own usage error in the captured output, which is diagnosable — silently
|
||||
// skipping is the outcome this issue exists to stop rewarding.
|
||||
if (platform === "linux") return "util-linux";
|
||||
// win32 (no `script(1)` at all), and any platform whose invocation nobody has
|
||||
// verified — aix, sunos, netbsd, openbsd. The caller skips loudly rather than
|
||||
// guessing. `node-pty` is the portable answer if this ever has to run there.
|
||||
return null;
|
||||
}
|
||||
|
||||
/**
|
||||
* Build the PTY-wrapped spawn arguments for a command.
|
||||
*
|
||||
* @param {object} opts
|
||||
* @param {string} opts.command
|
||||
* @param {string[]} [opts.args]
|
||||
* @param {"bsd" | "util-linux" | "busybox"} opts.flavor
|
||||
* @returns {{ command: string, args: string[] }}
|
||||
*/
|
||||
export function ptyCommand({ command, args = [], flavor }) {
|
||||
if (flavor === "bsd") {
|
||||
return { command: "script", args: ["-q", "/dev/null", command, ...args] };
|
||||
}
|
||||
if (flavor === "util-linux" || flavor === "busybox") {
|
||||
const line = [command, ...args].map(shellQuote).join(" ");
|
||||
const flags = flavor === "util-linux" ? "-qec" : "-qc";
|
||||
return { command: "script", args: [flags, line, "/dev/null"] };
|
||||
}
|
||||
throw new Error(`unknown script(1) flavor: ${flavor}`);
|
||||
}
|
||||
|
||||
/**
|
||||
* Probe the local `script(1)`: is it there, and what does it say about itself?
|
||||
*
|
||||
* **`spawnSync` does not throw on ENOENT** — it *returns* `{ error }` with
|
||||
* empty output. So a `try`/`catch` alone cannot tell "no `script` on this box"
|
||||
* from "a `script` that printed nothing", and collapsing both to `""` is
|
||||
* actively harmful: on linux the empty output falls through to the
|
||||
* `util-linux` guess, and `smoke:tui` then hard-fails spawning a binary that
|
||||
* does not exist, instead of taking the documented skip. Hence the `available`
|
||||
* flag rather than a bare string. The `catch` stays for a runner that really
|
||||
* does throw (an injected one, or a future spawn shape).
|
||||
*
|
||||
* A non-zero exit is NOT unavailability: BSD `script` has no `--version` and
|
||||
* answers `illegal option -- -` plus its usage on stderr — which is exactly the
|
||||
* evidence `scriptFlavorFor` reads. Only `error` means "could not run it".
|
||||
*
|
||||
* @param {(cmd: string, args: string[]) => { stdout?: string, stderr?: string, error?: Error }} [runner]
|
||||
* Injected for tests; defaults to a real `spawnSync`.
|
||||
* @returns {{ available: boolean, output: string }}
|
||||
*/
|
||||
export function probeScriptVersion(
|
||||
runner = (cmd, args) =>
|
||||
spawnSync(cmd, args, { encoding: "utf8", timeout: 5000 }),
|
||||
) {
|
||||
try {
|
||||
const r = runner("script", ["--version"]);
|
||||
// `error` covers ENOENT, EACCES and the timeout; a missing result at all
|
||||
// means the runner told us nothing, which is not evidence of a working one.
|
||||
if (!r || r.error) return { available: false, output: "" };
|
||||
return { available: true, output: `${r.stdout ?? ""}${r.stderr ?? ""}` };
|
||||
} catch {
|
||||
return { available: false, output: "" };
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Resolve a PTY wrapper for this machine, or say why there isn't one.
|
||||
*
|
||||
* The two ways it can be unavailable are worth telling apart in the skip
|
||||
* message — "you have no `script(1)`" and "nobody has verified this platform's
|
||||
* invocation" send the reader somewhere different — so the failure carries a
|
||||
* `reason` rather than being a bare `null` the caller has to narrate.
|
||||
*
|
||||
* @param {object} [opts]
|
||||
* @param {string} [opts.platform]
|
||||
* @param {() => { available: boolean, output: string }} [opts.probe]
|
||||
* @returns {{ ok: true, flavor: string, wrap: (spec: { command: string, args?: string[] }) => { command: string, args: string[] } }
|
||||
* | { ok: false, reason: string }}
|
||||
*/
|
||||
export function resolvePtyWrapper({
|
||||
platform = process.platform,
|
||||
probe = probeScriptVersion,
|
||||
} = {}) {
|
||||
const { available, output } = probe();
|
||||
if (!available) {
|
||||
return { ok: false, reason: "no `script(1)` on PATH" };
|
||||
}
|
||||
const flavor = scriptFlavorFor({ platform, versionOutput: output });
|
||||
if (!flavor) {
|
||||
return {
|
||||
ok: false,
|
||||
reason: `no verified \`script(1)\` invocation for ${platform}`,
|
||||
};
|
||||
}
|
||||
return {
|
||||
ok: true,
|
||||
flavor,
|
||||
wrap: ({ command, args = [] }) => ptyCommand({ command, args, flavor }),
|
||||
};
|
||||
}
|
||||
237
scripts/lib/pty.test.mjs
Normal file
237
scripts/lib/pty.test.mjs
Normal file
|
|
@ -0,0 +1,237 @@
|
|||
import { strict as assert } from "node:assert";
|
||||
import test from "node:test";
|
||||
|
||||
import {
|
||||
ptyCommand,
|
||||
probeScriptVersion,
|
||||
resolvePtyWrapper,
|
||||
scriptFlavorFor,
|
||||
shellQuote,
|
||||
} from "./pty.mjs";
|
||||
|
||||
// The flavors are not interchangeable: BSD `script` takes argv after the
|
||||
// typescript file, util-linux takes ONE shell string before it, and busybox
|
||||
// takes that string without `-e`. Getting it wrong is not a subtle failure —
|
||||
// `script` refuses to start, and the smoke reports it as a TUI crash.
|
||||
test("scriptFlavorFor prefers the version probe over the platform", () => {
|
||||
assert.equal(
|
||||
scriptFlavorFor({
|
||||
platform: "linux",
|
||||
versionOutput: "script from util-linux 2.39.3",
|
||||
}),
|
||||
"util-linux",
|
||||
);
|
||||
assert.equal(
|
||||
scriptFlavorFor({
|
||||
platform: "linux",
|
||||
versionOutput: "BusyBox v1.36.1 (2024-01-01) multi-call binary.",
|
||||
}),
|
||||
"busybox",
|
||||
);
|
||||
// Alpine is the reason the probe exists at all: "linux" does not imply
|
||||
// util-linux, and busybox's `script` rejects `-e`.
|
||||
assert.notEqual(
|
||||
scriptFlavorFor({ platform: "linux", versionOutput: "BusyBox v1.36.1" }),
|
||||
"util-linux",
|
||||
);
|
||||
});
|
||||
|
||||
test("scriptFlavorFor falls back to the platform when the probe says nothing", () => {
|
||||
// BSD `script` has no --version, so failing the probe is itself the signal.
|
||||
assert.equal(scriptFlavorFor({ platform: "darwin" }), "bsd");
|
||||
assert.equal(scriptFlavorFor({ platform: "freebsd" }), "bsd");
|
||||
// NOT every BSD: NetBSD takes the command through `-c`, and nobody has run
|
||||
// either it or OpenBSD here. Guessing builds an invocation that fails at
|
||||
// `script` startup, which this smoke would then report as a TUI crash.
|
||||
assert.equal(scriptFlavorFor({ platform: "netbsd" }), null);
|
||||
assert.equal(scriptFlavorFor({ platform: "openbsd" }), null);
|
||||
// An unidentifiable `script` on linux is overwhelmingly util-linux. Guess it:
|
||||
// a wrong guess fails loudly with `script`'s own usage error in the captured
|
||||
// output, whereas skipping is the silence this issue exists to stop.
|
||||
assert.equal(scriptFlavorFor({ platform: "linux" }), "util-linux");
|
||||
});
|
||||
|
||||
test("scriptFlavorFor returns null where there is no verified invocation", () => {
|
||||
assert.equal(scriptFlavorFor({ platform: "win32" }), null);
|
||||
assert.equal(
|
||||
scriptFlavorFor({ platform: "win32", versionOutput: "not a thing" }),
|
||||
null,
|
||||
);
|
||||
// Unsupported is the default, not a win32 special case.
|
||||
assert.equal(scriptFlavorFor({ platform: "aix" }), null);
|
||||
assert.equal(scriptFlavorFor({ platform: "sunos" }), null);
|
||||
});
|
||||
|
||||
test("ptyCommand builds the BSD argv form", () => {
|
||||
assert.deepEqual(
|
||||
ptyCommand({
|
||||
command: "/bin/node",
|
||||
args: ["app.js", "--x"],
|
||||
flavor: "bsd",
|
||||
}),
|
||||
{
|
||||
command: "script",
|
||||
args: ["-q", "/dev/null", "/bin/node", "app.js", "--x"],
|
||||
},
|
||||
);
|
||||
});
|
||||
|
||||
test("ptyCommand builds the util-linux shell-string form with -e", () => {
|
||||
// -e is what makes `script` exit with the child's status instead of its own.
|
||||
assert.deepEqual(
|
||||
ptyCommand({
|
||||
command: "/bin/node",
|
||||
args: ["app.js"],
|
||||
flavor: "util-linux",
|
||||
}),
|
||||
{
|
||||
command: "script",
|
||||
args: ["-qec", "'/bin/node' 'app.js'", "/dev/null"],
|
||||
},
|
||||
);
|
||||
});
|
||||
|
||||
test("ptyCommand omits -e for busybox, which has no such flag", () => {
|
||||
const { args } = ptyCommand({
|
||||
command: "node",
|
||||
args: ["app.js"],
|
||||
flavor: "busybox",
|
||||
});
|
||||
assert.equal(args[0], "-qc");
|
||||
assert.deepEqual(args, ["-qc", "'node' 'app.js'", "/dev/null"]);
|
||||
});
|
||||
|
||||
test("ptyCommand quotes arguments for the shell-string flavors", () => {
|
||||
// The smoke passes an mkdtemp path; a space in it would otherwise split into
|
||||
// two arguments inside the shell `script -c` runs.
|
||||
const { args } = ptyCommand({
|
||||
command: "/usr/local/my node/bin/node",
|
||||
args: ["/tmp/a dir/catalog.json", "it's-fine"],
|
||||
flavor: "util-linux",
|
||||
});
|
||||
assert.equal(
|
||||
args[1],
|
||||
`'/usr/local/my node/bin/node' '/tmp/a dir/catalog.json' 'it'\\''s-fine'`,
|
||||
);
|
||||
});
|
||||
|
||||
test("shellQuote survives an embedded single quote", () => {
|
||||
assert.equal(shellQuote("it's"), `'it'\\''s'`);
|
||||
assert.equal(shellQuote("plain"), "'plain'");
|
||||
});
|
||||
|
||||
test("probeScriptVersion combines both streams and never throws", () => {
|
||||
assert.deepEqual(
|
||||
probeScriptVersion(() => ({ stdout: "out ", stderr: "err" })),
|
||||
{ available: true, output: "out err" },
|
||||
);
|
||||
// A non-zero exit is NOT unavailability. BSD `script` has no `--version` and
|
||||
// answers with `illegal option -- -` plus its usage on stderr — which is the
|
||||
// very evidence the flavor fallback reads, so it must survive the probe.
|
||||
assert.deepEqual(
|
||||
probeScriptVersion(() => ({
|
||||
status: 1,
|
||||
stdout: "",
|
||||
stderr: "script: illegal option -- -\nusage: script [-aeFkpqr] ...",
|
||||
})),
|
||||
{
|
||||
available: true,
|
||||
output: "script: illegal option -- -\nusage: script [-aeFkpqr] ...",
|
||||
},
|
||||
);
|
||||
// A runner that throws outright (injected, or a future spawn shape).
|
||||
assert.deepEqual(
|
||||
probeScriptVersion(() => {
|
||||
throw new Error("ENOENT");
|
||||
}),
|
||||
{ available: false, output: "" },
|
||||
);
|
||||
// Told nothing at all is not evidence of a working `script`.
|
||||
assert.deepEqual(
|
||||
probeScriptVersion(() => undefined),
|
||||
{
|
||||
available: false,
|
||||
output: "",
|
||||
},
|
||||
);
|
||||
});
|
||||
|
||||
// `spawnSync` REPORTS ENOENT in `result.error` — it does not throw — so a
|
||||
// try/catch alone reads a missing binary as "ran, printed nothing". On linux
|
||||
// that empty output falls through to the util-linux guess, and `smoke:tui`
|
||||
// hard-fails spawning a binary that does not exist instead of skipping.
|
||||
test("probeScriptVersion treats result.error as unavailable, not as empty output", () => {
|
||||
const enoent = Object.assign(new Error("spawnSync script ENOENT"), {
|
||||
code: "ENOENT",
|
||||
});
|
||||
assert.deepEqual(
|
||||
probeScriptVersion(() => ({ error: enoent, stdout: "", stderr: "" })),
|
||||
{ available: false, output: "" },
|
||||
);
|
||||
// The timeout shape reports the same way.
|
||||
assert.equal(
|
||||
probeScriptVersion(() => ({ error: new Error("ETIMEDOUT") })).available,
|
||||
false,
|
||||
);
|
||||
});
|
||||
|
||||
test("resolvePtyWrapper reports a missing script(1) rather than guessing", () => {
|
||||
// linux would otherwise be classified util-linux off the empty output.
|
||||
const r = resolvePtyWrapper({
|
||||
platform: "linux",
|
||||
probe: () => ({ available: false, output: "" }),
|
||||
});
|
||||
assert.equal(r.ok, false);
|
||||
assert.match(r.reason, /on PATH/);
|
||||
});
|
||||
|
||||
test("resolvePtyWrapper wraps with the resolved flavor, or reports why not", () => {
|
||||
const wrapper = resolvePtyWrapper({
|
||||
platform: "linux",
|
||||
probe: () => ({ available: true, output: "script from util-linux 2.39.3" }),
|
||||
});
|
||||
assert.equal(wrapper.ok, true);
|
||||
assert.equal(wrapper.flavor, "util-linux");
|
||||
assert.deepEqual(wrapper.wrap({ command: "node", args: ["a.js"] }), {
|
||||
command: "script",
|
||||
args: ["-qec", "'node' 'a.js'", "/dev/null"],
|
||||
});
|
||||
// `args` is optional at the call site.
|
||||
assert.deepEqual(wrapper.wrap({ command: "node" }).args[1], "'node'");
|
||||
|
||||
// A present `script` on a platform whose invocation nobody verified is a
|
||||
// DIFFERENT failure from a missing one, and the reason must say which.
|
||||
const unsupported = resolvePtyWrapper({
|
||||
platform: "win32",
|
||||
probe: () => ({ available: true, output: "" }),
|
||||
});
|
||||
assert.equal(unsupported.ok, false);
|
||||
assert.match(unsupported.reason, /no verified .* invocation for win32/);
|
||||
});
|
||||
|
||||
// The platforms this repo claims a verified `script(1)` invocation for. Any
|
||||
// other — win32, aix, sunos, netbsd, openbsd — resolves to null BY DESIGN and
|
||||
// makes `smoke:tui` skip, so asserting "supported unless Windows" would fail
|
||||
// `test:scripts` on a machine where the smoke is behaving exactly as intended.
|
||||
const SUPPORTED_PLATFORMS = new Set(["darwin", "freebsd", "linux"]);
|
||||
|
||||
test("resolvePtyWrapper resolves on the platforms we claim to support", () => {
|
||||
// Not a tautology: it runs the real probe against the real machine, so a
|
||||
// `script` that has vanished from PATH surfaces here rather than as an opaque
|
||||
// smoke failure.
|
||||
const wrapper = resolvePtyWrapper();
|
||||
if (SUPPORTED_PLATFORMS.has(process.platform)) {
|
||||
assert.equal(
|
||||
wrapper.ok,
|
||||
true,
|
||||
`no PTY wrapper on ${process.platform}: ${wrapper.reason ?? ""}`,
|
||||
);
|
||||
assert.equal(wrapper.wrap({ command: "node" }).command, "script");
|
||||
} else {
|
||||
assert.equal(
|
||||
wrapper.ok,
|
||||
false,
|
||||
`${process.platform} is not in SUPPORTED_PLATFORMS but resolved a wrapper`,
|
||||
);
|
||||
}
|
||||
});
|
||||
332
scripts/lib/render-smoke.mjs
Normal file
332
scripts/lib/render-smoke.mjs
Normal file
|
|
@ -0,0 +1,332 @@
|
|||
/**
|
||||
* Spawn a terminal app, wait for it to paint a marker, and then wait again to
|
||||
* see whether it is still running (#2147).
|
||||
*
|
||||
* Extracted from `scripts/smoke-tui.mjs` for the same reason
|
||||
* `announced-child.mjs` was extracted from `smoke-web-app.mjs` (#2000): a smoke
|
||||
* script only ever exercises its own happy path, so the branch that matters
|
||||
* most here — a child that paints the marker and *then* dies — is unreachable
|
||||
* from the real TUI once it is fixed. Driving this from a test against a stub
|
||||
* that paints and immediately exits 1 is what keeps the assertion honest.
|
||||
*
|
||||
* The defect it encodes against: `smoke:tui` used to resolve OK the instant the
|
||||
* marker appeared and never looked at the child again. The TUI was exiting 1
|
||||
* about 40ms later, every run, on every machine, and the smoke won that race
|
||||
* and printed OK. So it asserted "painted once", while its own header claimed
|
||||
* "boots and renders without crashing". Worse, the race cut both ways: had the
|
||||
* `exit` event been processed first, the same run would have failed, which
|
||||
* reads as flake rather than as the standing defect it was.
|
||||
*
|
||||
* Hence `surviveMs`. First paint is not survival, and nothing shorter than an
|
||||
* explicit "still alive N ms later" check can tell the two apart.
|
||||
*/
|
||||
|
||||
import { spawn } from "node:child_process";
|
||||
|
||||
/**
|
||||
* Fallback durations, in ms. These are defaults, NOT minimums: `normalizeMs`
|
||||
* accepts any finite positive value, and the tests deliberately pass shorter
|
||||
* ones. What it rejects is the shape a bad env var produces, not smallness.
|
||||
*/
|
||||
export const DEFAULTS = Object.freeze({
|
||||
timeoutMs: 15_000,
|
||||
surviveMs: 2_000,
|
||||
exitGraceMs: 5_000,
|
||||
drainMs: 500,
|
||||
});
|
||||
|
||||
/**
|
||||
* Coerce a caller-supplied duration to a usable one, mirroring
|
||||
* `child-cleanup.mjs`'s `normalizeGraceMs`.
|
||||
*
|
||||
* Call sites read these from the environment, and `Number()` there has two
|
||||
* holes that `??` does not cover: `Number("")` is `0` and `Number("typo")` is
|
||||
* `NaN`. `setTimeout` treats both as "fire immediately" — which for `surviveMs`
|
||||
* means the survival check resolves before the child could possibly have died,
|
||||
* silently restoring the exact first-paint false green this module exists to
|
||||
* prevent, on a run that still prints OK. Falling back is therefore not
|
||||
* defensive tidiness; it is the difference between a gate and a decoration.
|
||||
*
|
||||
* A consequence worth stating: `surviveMs: 0` is NOT honored as "skip the
|
||||
* survival wait". There is no legitimate caller for that here, and reading it
|
||||
* literally is indistinguishable from the typo.
|
||||
*
|
||||
* @param {unknown} value
|
||||
* @param {number} fallback
|
||||
* @returns {number}
|
||||
*/
|
||||
export function normalizeMs(value, fallback) {
|
||||
return typeof value === "number" && Number.isFinite(value) && value > 0
|
||||
? value
|
||||
: fallback;
|
||||
}
|
||||
|
||||
/**
|
||||
* Tail of a child's output, for quoting in a diagnostic. Slicing a terminal
|
||||
* stream can land mid-CSI-sequence, so drop through the first newline rather
|
||||
* than lead with an escape-code fragment that mangles the line after it.
|
||||
*
|
||||
* @param {string} output
|
||||
* @param {number} [limit]
|
||||
* @returns {string}
|
||||
*/
|
||||
export function outputTail(output, limit = 800) {
|
||||
const tail = output.slice(-limit);
|
||||
const nl = tail.indexOf("\n");
|
||||
return output.length > limit && nl !== -1 ? tail.slice(nl + 1) : tail;
|
||||
}
|
||||
|
||||
/**
|
||||
* @param {object} opts
|
||||
* @param {string} opts.command Executable to spawn.
|
||||
* @param {string[]} [opts.args] Arguments for it.
|
||||
* @param {object} [opts.spawnOptions] Merged into the `spawn` options
|
||||
* (`stdio` is fixed — see below).
|
||||
* @param {string} opts.marker Substring whose appearance in the
|
||||
* child's output counts as first paint.
|
||||
* @param {number} [opts.timeoutMs] Budget for first paint (default 15s;
|
||||
* non-finite/non-positive falls back).
|
||||
* @param {number} [opts.surviveMs] How long after first paint the child
|
||||
* must stay alive (default 2s;
|
||||
* non-finite/non-positive falls back —
|
||||
* 0 is a typo, never "skip the check").
|
||||
* @param {number} [opts.exitGraceMs] SIGTERM → SIGKILL grace (default 5s;
|
||||
* non-finite/non-positive falls back).
|
||||
* @param {number} [opts.drainMs] Post-exit pipe-drain cap (default 500ms;
|
||||
* non-finite/non-positive falls back).
|
||||
* @param {(child: import("node:child_process").ChildProcess) => void} [opts.onSpawn]
|
||||
* Called synchronously with the child, before any waiting — the caller's
|
||||
* teardown handle on every failure path.
|
||||
* @param {{ pattern: RegExp, reason: string }} [opts.forbidOutput]
|
||||
* Text that must NOT appear in an otherwise-passing run. `smoke:tui` uses it
|
||||
* for Ink's raw-mode error: survival already implies its absence *today*,
|
||||
* because that throw is fatal — but if Ink ever degraded it to a warning, a
|
||||
* TUI with no keyboard input would survive happily and pass. Checked here
|
||||
* rather than at the call site so it runs against the fully drained output
|
||||
* and is reachable from a test. Use a non-global RegExp: `test()` on a `/g`
|
||||
* pattern advances `lastIndex` and alternates.
|
||||
* @param {(message: string) => void} [opts.warn]
|
||||
* @returns {Promise<{ code: 0 | 1, message: string, output: string }>}
|
||||
* On the normal path, resolves only after the child's streams have `close`d —
|
||||
* so `output` is complete and the caller may remove a directory the child was
|
||||
* using without re-entering the #1801 ENOTEMPTY race.
|
||||
*
|
||||
* That is **bounded, not guaranteed**. Both drain deadlines below resolve
|
||||
* *without* having observed `close`, after warning: the child may still hold
|
||||
* the dir, and `output` may be truncated mid-line. Cleanup at the call site
|
||||
* must therefore be best-effort (`removeSafe`, not `rmSync`) — the worst case
|
||||
* is a warning and a leaked temp dir, never a red smoke. Do not write a
|
||||
* caller that depends on the close having happened.
|
||||
*/
|
||||
export function runRenderSmoke({
|
||||
command,
|
||||
args = [],
|
||||
spawnOptions = {},
|
||||
marker,
|
||||
timeoutMs: requestedTimeoutMs,
|
||||
surviveMs: requestedSurviveMs,
|
||||
exitGraceMs: requestedExitGraceMs,
|
||||
drainMs: requestedDrainMs,
|
||||
forbidOutput,
|
||||
onSpawn = () => {},
|
||||
warn = (m) => console.warn(m),
|
||||
}) {
|
||||
// Normalized rather than defaulted, so an env-derived 0/NaN cannot reach a
|
||||
// `setTimeout` — see `normalizeMs`. Done once, before anything reads them,
|
||||
// so the interpolated durations in the messages below can't read `NaNms`.
|
||||
const timeoutMs = normalizeMs(requestedTimeoutMs, DEFAULTS.timeoutMs);
|
||||
const surviveMs = normalizeMs(requestedSurviveMs, DEFAULTS.surviveMs);
|
||||
const exitGraceMs = normalizeMs(requestedExitGraceMs, DEFAULTS.exitGraceMs);
|
||||
const drainMs = normalizeMs(requestedDrainMs, DEFAULTS.drainMs);
|
||||
|
||||
return new Promise((resolve) => {
|
||||
const child = spawn(command, args, {
|
||||
...spawnOptions,
|
||||
// stdin is `ignore` (i.e. /dev/null) deliberately, even under a PTY: BSD
|
||||
// `script` refuses to start when its own stdin is a pipe
|
||||
// ("tcgetattr/ioctl: Operation not supported on socket"). It reads the
|
||||
// immediate EOF and forwards one ^D into the pty before the app mounts,
|
||||
// which is harmless — verified against the real TUI, which survives it.
|
||||
stdio: ["ignore", "pipe", "pipe"],
|
||||
});
|
||||
// Publish before waiting, so every path below leaves the caller holding a
|
||||
// stoppable handle rather than an orphan (the #2000 rule).
|
||||
onSpawn(child);
|
||||
|
||||
let output = "";
|
||||
let settled = false;
|
||||
let finished = false;
|
||||
let childExited = false;
|
||||
let childClosed = false;
|
||||
let markerAt = null;
|
||||
/**
|
||||
* Both deadlines. Declared up front because `onData` and `done` each clear
|
||||
* them from closures defined above the `setTimeout` calls that assign them.
|
||||
*
|
||||
* @type {ReturnType<typeof setTimeout> | undefined}
|
||||
*/
|
||||
let surviveTimer;
|
||||
/** @type {ReturnType<typeof setTimeout> | undefined} */
|
||||
let renderTimer;
|
||||
|
||||
const t0 = Date.now();
|
||||
const since = () => Date.now() - t0;
|
||||
|
||||
const finish = (code, message) => {
|
||||
if (finished) return;
|
||||
finished = true;
|
||||
let verdict = code;
|
||||
let text = typeof message === "function" ? message() : message;
|
||||
// Only ever turns a pass into a failure. A run that already failed keeps
|
||||
// its own diagnostic: the crash reason is the more useful of the two, and
|
||||
// forbidden output is usually a *symptom* of it rather than a separate
|
||||
// finding.
|
||||
if (verdict === 0 && forbidOutput?.pattern.test(output)) {
|
||||
verdict = 1;
|
||||
text = `${forbidOutput.reason}\n${outputTail(output)}`;
|
||||
}
|
||||
resolve({ code: verdict, message: text, output });
|
||||
};
|
||||
|
||||
// Settle the verdict, then wait for the child to actually close before
|
||||
// resolving (#1801). The caller's work dir doubles as the app's HOME, so it
|
||||
// is still being written to when we signal; removing it while those writes
|
||||
// land fails with ENOTEMPTY. Waiting on `close` rather than `exit` also
|
||||
// covers a *spawn failure*, which emits `error` + `close` and never `exit`
|
||||
// — and guarantees the pipes are drained, which is what makes the thunked
|
||||
// messages below complete rather than truncated at the interesting line.
|
||||
//
|
||||
// Deliberately not `lib/child-cleanup.mjs`'s `stopChild()`: that resolves
|
||||
// on the FIRST of exit/close, whereas this wait must keep going for `close`
|
||||
// after `exit`, on a shorter re-armed deadline, for the drain.
|
||||
const done = (code, message) => {
|
||||
if (settled) return;
|
||||
settled = true;
|
||||
clearTimeout(renderTimer);
|
||||
clearTimeout(surviveTimer);
|
||||
if (childClosed) {
|
||||
finish(code, message);
|
||||
return;
|
||||
}
|
||||
const forceKill = setTimeout(() => child.kill("SIGKILL"), exitGraceMs);
|
||||
|
||||
let deadline;
|
||||
const armDeadline = (ms, warning) => {
|
||||
clearTimeout(deadline);
|
||||
deadline = setTimeout(() => {
|
||||
warn(`render-smoke — ${warning}`);
|
||||
finish(code, message);
|
||||
}, ms);
|
||||
};
|
||||
armDeadline(
|
||||
exitGraceMs * 2,
|
||||
`child did not close its output streams within ${exitGraceMs * 2}ms; giving up on the drain`,
|
||||
);
|
||||
|
||||
// Once the child itself is gone only the pipe drain remains, so the wait
|
||||
// drops to `drainMs`. `close` is bounded by whoever holds the stdio
|
||||
// pipes, which can outlive the direct child: any descendant that
|
||||
// inherited them keeps it pending.
|
||||
const drainOnly = () => {
|
||||
clearTimeout(forceKill);
|
||||
armDeadline(
|
||||
drainMs,
|
||||
`child exited but held its output streams open for ${drainMs}ms (a descendant may have inherited them); giving up on the drain`,
|
||||
);
|
||||
};
|
||||
if (childExited) drainOnly();
|
||||
else child.once("exit", drainOnly);
|
||||
|
||||
child.once("close", () => {
|
||||
clearTimeout(forceKill);
|
||||
clearTimeout(deadline);
|
||||
finish(code, message);
|
||||
});
|
||||
|
||||
// Only signal a child that is still running: on the crash and
|
||||
// spawn-failure paths there is nothing left to signal, and we are here
|
||||
// purely to wait out the remaining `close`.
|
||||
if (
|
||||
child.exitCode === null &&
|
||||
child.signalCode === null &&
|
||||
!child.killed
|
||||
) {
|
||||
child.kill("SIGTERM");
|
||||
}
|
||||
};
|
||||
|
||||
const onData = (chunk) => {
|
||||
output += chunk.toString();
|
||||
if (markerAt !== null || !output.includes(marker)) return;
|
||||
markerAt = since();
|
||||
// Disarm the first-paint deadline. It is first-caller-wins with `done`,
|
||||
// so leaving it armed means a paint landing near `timeoutMs` lets it fire
|
||||
// *during* the survival window and settle the run as "did not render
|
||||
// <marker>" — quoting an output tail that visibly contains the marker.
|
||||
// First paint has happened; its deadline no longer has a question to ask.
|
||||
clearTimeout(renderTimer);
|
||||
// `exit` can beat the last of the child's buffered output, and `done`
|
||||
// then keeps reading until `close` — so this can run *after* the verdict
|
||||
// is settled. Recording `markerAt` is still right (the deferred message
|
||||
// below reads it, and a marker that only surfaced in the drain was still
|
||||
// painted). Arming a timer is not: `done` is a no-op once settled, so it
|
||||
// could not change the result, but it holds the event loop open for the
|
||||
// whole survival window — which under `node --test` is a suite that
|
||||
// hangs on for two seconds per case for no reason.
|
||||
if (settled) return;
|
||||
// First paint is not the verdict — it only starts the clock on the
|
||||
// question this smoke actually asks.
|
||||
surviveTimer = setTimeout(() => {
|
||||
done(
|
||||
0,
|
||||
`rendered "${marker}" at ${markerAt}ms and was still running ${surviveMs}ms later`,
|
||||
);
|
||||
}, surviveMs);
|
||||
};
|
||||
|
||||
child.stdout.on("data", onData);
|
||||
child.stderr.on("data", onData);
|
||||
|
||||
// Registered before done()'s own one-shot listener, so this always runs
|
||||
// first — done() can trust `childClosed` even when called from a handler.
|
||||
child.on("close", () => {
|
||||
childClosed = true;
|
||||
});
|
||||
|
||||
child.on("exit", (code, signal) => {
|
||||
childExited = true;
|
||||
const exitAt = since();
|
||||
if (settled) return;
|
||||
const how = signal ? `signal ${signal}` : `code ${code}`;
|
||||
// Thunked: rendered at `finish()`, i.e. after `close`, so `markerAt` is
|
||||
// whatever the FULL output showed — a marker still sitting in the pipe
|
||||
// when `exit` fired is accounted for by the time this is read.
|
||||
//
|
||||
// Deliberately makes no claim about the order in which this process
|
||||
// *observed* the two. `exit` and the last `data` event race, so a
|
||||
// "N ms later" phrased off observation times would flip wording (and
|
||||
// sign) run to run on the same child. The times are reported as the
|
||||
// observations they are and the verdict does not rest on them.
|
||||
done(1, () =>
|
||||
markerAt === null
|
||||
? `child exited (${how}) before rendering "${marker}"\n${outputTail(output)}`
|
||||
: `child rendered "${marker}" and then exited (${how}) inside the ` +
|
||||
`${surviveMs}ms survival window — it painted one frame, it did not run ` +
|
||||
`(first paint seen at ${markerAt}ms, exit at ${exitAt}ms)` +
|
||||
`\n${outputTail(output)}`,
|
||||
);
|
||||
});
|
||||
|
||||
child.on("error", (err) => {
|
||||
done(1, `failed to spawn ${command}: ${err.message}`);
|
||||
});
|
||||
|
||||
renderTimer = setTimeout(() => {
|
||||
done(
|
||||
1,
|
||||
() =>
|
||||
`child did not render "${marker}" within ${timeoutMs}ms\n${outputTail(output)}`,
|
||||
);
|
||||
}, timeoutMs);
|
||||
});
|
||||
}
|
||||
258
scripts/lib/render-smoke.test.mjs
Normal file
258
scripts/lib/render-smoke.test.mjs
Normal file
|
|
@ -0,0 +1,258 @@
|
|||
import { strict as assert } from "node:assert";
|
||||
import test from "node:test";
|
||||
|
||||
import {
|
||||
DEFAULTS,
|
||||
normalizeMs,
|
||||
outputTail,
|
||||
runRenderSmoke,
|
||||
} from "./render-smoke.mjs";
|
||||
|
||||
const MARKER = "MCP Servers";
|
||||
|
||||
/** A stub terminal app, driven as a real child process rather than a spy. */
|
||||
function stub(body) {
|
||||
return { command: process.execPath, args: ["-e", body] };
|
||||
}
|
||||
|
||||
// `process.exitCode`, never `process.exit()`. Writing to a *pipe* is
|
||||
// asynchronous, and `process.exit()` tears the process down without draining —
|
||||
// so `console.log(MARKER); process.exit(1)` can lose the marker outright. That
|
||||
// is not hypothetical: probing the drain race for this PR, a 4MB write followed
|
||||
// by `process.exit()` arrived at the parent with the marker missing entirely.
|
||||
// A stub that intermittently never paints would fail the very assertion meant
|
||||
// to pin the paint-then-die path, and would fail it as "exited before
|
||||
// rendering" — the neighbouring branch. Setting `exitCode` and letting the loop
|
||||
// drain still exits immediately after the write; nothing holds it open.
|
||||
const paintThenExit = (code) =>
|
||||
stub(`console.log(${JSON.stringify(MARKER)}); process.exitCode = ${code};`);
|
||||
const paintThenLive = stub(
|
||||
`console.log(${JSON.stringify(MARKER)}); setInterval(() => {}, 1000);`,
|
||||
);
|
||||
|
||||
const opts = {
|
||||
marker: MARKER,
|
||||
timeoutMs: 5000,
|
||||
surviveMs: 400,
|
||||
exitGraceMs: 500,
|
||||
drainMs: 200,
|
||||
warn: () => {},
|
||||
};
|
||||
|
||||
// The regression this module exists for. The old harness resolved OK on the
|
||||
// marker and never looked again, so this exact stub — paint, then die — passed.
|
||||
test("fails when the child paints the marker and then exits non-zero", async () => {
|
||||
const r = await runRenderSmoke({ ...opts, ...paintThenExit(1) });
|
||||
assert.equal(r.code, 1, `expected failure, got: ${r.message}`);
|
||||
assert.match(r.message, /code 1/);
|
||||
assert.match(r.message, /it painted one frame, it did not run/);
|
||||
// The diagnostic must not read as a crash *before* first paint — that was a
|
||||
// different defect, and conflating them sends the reader to the wrong place.
|
||||
assert.doesNotMatch(r.message, /before rendering/);
|
||||
});
|
||||
|
||||
// `exit` races the child's last `data` event, so this stub can be observed
|
||||
// either way round on the same machine. Both orders must produce the same
|
||||
// verdict AND the same wording — a message phrased off observation order would
|
||||
// flip run to run, which is how a stable failure gets written off as flake.
|
||||
// Repeated because one pass proves nothing about a race.
|
||||
test("the verdict does not depend on whether exit beats the final output", async () => {
|
||||
for (let i = 0; i < 12; i++) {
|
||||
const r = await runRenderSmoke({ ...opts, ...paintThenExit(1) });
|
||||
assert.equal(r.code, 1, `run ${i}: expected failure, got: ${r.message}`);
|
||||
assert.match(r.message, /it painted one frame, it did not run/, `run ${i}`);
|
||||
assert.doesNotMatch(r.message, /before rendering/, `run ${i}`);
|
||||
}
|
||||
});
|
||||
|
||||
// Exiting 0 right after painting is the same failure: the app did not run. It
|
||||
// is called out separately because `script(1)` under busybox reports 0 for a
|
||||
// crashed child, so the verdict cannot rest on the exit code.
|
||||
test("fails when the child paints the marker and then exits zero", async () => {
|
||||
const r = await runRenderSmoke({ ...opts, ...paintThenExit(0) });
|
||||
assert.equal(r.code, 1, `expected failure, got: ${r.message}`);
|
||||
assert.match(r.message, /it painted one frame, it did not run/);
|
||||
});
|
||||
|
||||
test("passes when the child paints the marker and keeps running", async () => {
|
||||
const r = await runRenderSmoke({ ...opts, ...paintThenLive });
|
||||
assert.equal(r.code, 0, `expected pass, got: ${r.message}`);
|
||||
assert.match(r.message, /still running 400ms later/);
|
||||
});
|
||||
|
||||
// The first-paint deadline and the survival window overlap: `done` is
|
||||
// first-caller-wins, so an armed render timer firing during the survival window
|
||||
// settles the run as "did not render" — while quoting an output tail that
|
||||
// visibly contains the marker. A slow-but-fine TUI would fail, intermittently,
|
||||
// with a diagnostic pointing at the wrong thing entirely.
|
||||
test("a paint landing just under the deadline is not failed by the render timer", async () => {
|
||||
const paintAt = 250;
|
||||
const r = await runRenderSmoke({
|
||||
...opts,
|
||||
...stub(
|
||||
`setTimeout(() => { console.log(${JSON.stringify(MARKER)}); ` +
|
||||
`setInterval(() => {}, 1000); }, ${paintAt});`,
|
||||
),
|
||||
// Deliberately tight: the render deadline expires while the survival window
|
||||
// is still open, which is the whole shape of the bug.
|
||||
timeoutMs: paintAt + 150,
|
||||
surviveMs: 400,
|
||||
});
|
||||
assert.equal(r.code, 0, `expected pass, got: ${r.message}`);
|
||||
assert.doesNotMatch(r.message, /did not render/);
|
||||
});
|
||||
|
||||
test("fails when the child exits before painting", async () => {
|
||||
const r = await runRenderSmoke({
|
||||
...opts,
|
||||
// `exitCode`, not `exit()` — see `paintThenExit`. A forced exit can
|
||||
// truncate the stderr write, making the `boom` assertion below flaky.
|
||||
...stub(`console.error("boom"); process.exitCode = 3;`),
|
||||
});
|
||||
assert.equal(r.code, 1);
|
||||
assert.match(r.message, /before rendering/);
|
||||
assert.match(r.message, /code 3/);
|
||||
// The tail is quoted after the drain, so the crash reason survives.
|
||||
assert.match(r.message, /boom/);
|
||||
});
|
||||
|
||||
test("fails when the child never paints, and kills it", async () => {
|
||||
let spawned;
|
||||
const r = await runRenderSmoke({
|
||||
...opts,
|
||||
...stub(`setInterval(() => {}, 1000);`),
|
||||
timeoutMs: 300,
|
||||
onSpawn: (c) => (spawned = c),
|
||||
});
|
||||
assert.equal(r.code, 1);
|
||||
assert.match(r.message, /did not render/);
|
||||
// The #2000 ownership rule: the child is published before the wait, so the
|
||||
// timeout path leaves a stoppable handle rather than an orphan.
|
||||
assert.ok(spawned, "onSpawn was never called");
|
||||
assert.notEqual(
|
||||
spawned.exitCode === null && spawned.signalCode === null,
|
||||
true,
|
||||
);
|
||||
});
|
||||
|
||||
test("reports a spawn failure rather than throwing it uncaught", async () => {
|
||||
const r = await runRenderSmoke({
|
||||
...opts,
|
||||
command: "/definitely/not/a/binary-2147",
|
||||
args: [],
|
||||
});
|
||||
assert.equal(r.code, 1);
|
||||
assert.match(r.message, /failed to spawn/);
|
||||
});
|
||||
|
||||
test("scans stderr as well as stdout for the marker", async () => {
|
||||
// An Ink app writing to stderr is missed entirely by a stdout-only scan,
|
||||
// which then times out with an empty diagnostic.
|
||||
const r = await runRenderSmoke({
|
||||
...opts,
|
||||
...stub(
|
||||
`console.error(${JSON.stringify(MARKER)}); setInterval(() => {}, 1000);`,
|
||||
),
|
||||
});
|
||||
assert.equal(r.code, 0, `expected pass, got: ${r.message}`);
|
||||
});
|
||||
|
||||
// `Number("")` is 0 and `Number("typo")` is NaN, and `??` catches neither
|
||||
// because both are non-nullish. `setTimeout` fires immediately on either, so an
|
||||
// un-normalized `surviveMs` resolves the survival check before the child could
|
||||
// possibly have died — restoring the exact first-paint false green, on a run
|
||||
// that still prints OK. This is the guard on the guard.
|
||||
test("normalizeMs rejects the values a bad env var actually produces", () => {
|
||||
assert.equal(normalizeMs(Number(""), 2000), 2000);
|
||||
assert.equal(normalizeMs(Number("typo"), 2000), 2000);
|
||||
assert.equal(normalizeMs(undefined, 2000), 2000);
|
||||
assert.equal(normalizeMs(-1, 2000), 2000);
|
||||
assert.equal(normalizeMs(Infinity, 2000), 2000);
|
||||
assert.equal(normalizeMs("3000", 2000), 2000, "a string is not a duration");
|
||||
// 0 is a typo, never "skip the check" — there is no caller who wants that.
|
||||
assert.equal(normalizeMs(0, 2000), 2000);
|
||||
// A real value passes through untouched.
|
||||
assert.equal(normalizeMs(1234, 2000), 1234);
|
||||
});
|
||||
|
||||
test("a zero survival window falls back rather than passing instantly", async () => {
|
||||
// End to end through the real state machine: with surviveMs honored as 0 the
|
||||
// paint-then-exit stub would settle OK before the child's exit was seen.
|
||||
const r = await runRenderSmoke({
|
||||
...opts,
|
||||
...paintThenExit(1),
|
||||
surviveMs: Number(""),
|
||||
});
|
||||
assert.equal(r.code, 1, `expected failure, got: ${r.message}`);
|
||||
// The window named in the message is the normalized one, not the 0 passed in.
|
||||
assert.match(
|
||||
r.message,
|
||||
new RegExp(`${DEFAULTS.surviveMs}ms survival window`),
|
||||
);
|
||||
assert.doesNotMatch(r.message, /\b0ms survival window/);
|
||||
});
|
||||
|
||||
// `forbidOutput` is `smoke:tui`'s guard for the acceptance criterion that the
|
||||
// raw-mode error must not appear in a passing run. It lives here rather than at
|
||||
// the call site precisely so it can be driven both ways: the real PTY happy
|
||||
// path only ever exercises the pattern NOT matching, so a regression that broke
|
||||
// this check would stay green forever.
|
||||
test("forbidOutput fails a run that would otherwise have passed", async () => {
|
||||
const r = await runRenderSmoke({
|
||||
...opts,
|
||||
...stub(
|
||||
`console.log(${JSON.stringify(MARKER)}); ` +
|
||||
`console.error("Raw mode is not supported on the current process.stdin"); ` +
|
||||
`setInterval(() => {}, 1000);`,
|
||||
),
|
||||
forbidOutput: {
|
||||
pattern: /Raw mode is not supported/,
|
||||
reason: "survived but logged the raw-mode error",
|
||||
},
|
||||
});
|
||||
assert.equal(r.code, 1, `expected failure, got: ${r.message}`);
|
||||
assert.match(r.message, /survived but logged the raw-mode error/);
|
||||
// The offending output is quoted, so the reader sees the evidence.
|
||||
assert.match(r.message, /Raw mode is not supported/);
|
||||
});
|
||||
|
||||
test("forbidOutput leaves a passing run alone when it does not match", async () => {
|
||||
const r = await runRenderSmoke({
|
||||
...opts,
|
||||
...paintThenLive,
|
||||
forbidOutput: {
|
||||
pattern: /Raw mode is not supported/,
|
||||
reason: "should not be reached",
|
||||
},
|
||||
});
|
||||
assert.equal(r.code, 0, `expected pass, got: ${r.message}`);
|
||||
assert.doesNotMatch(r.message, /should not be reached/);
|
||||
});
|
||||
|
||||
// A crash reason is more useful than "and it also printed X", which is usually
|
||||
// a symptom of the same crash rather than a separate finding.
|
||||
test("forbidOutput does not overwrite an existing failure's diagnostic", async () => {
|
||||
const r = await runRenderSmoke({
|
||||
...opts,
|
||||
...stub(
|
||||
`console.error("Raw mode is not supported"); process.exitCode = 1;`,
|
||||
),
|
||||
forbidOutput: {
|
||||
pattern: /Raw mode is not supported/,
|
||||
reason: "should not replace the crash reason",
|
||||
},
|
||||
});
|
||||
assert.equal(r.code, 1);
|
||||
assert.match(r.message, /before rendering/);
|
||||
assert.doesNotMatch(r.message, /should not replace the crash reason/);
|
||||
});
|
||||
|
||||
test("outputTail drops a leading partial line but keeps short output whole", () => {
|
||||
assert.equal(outputTail("short", 800), "short");
|
||||
// Over the limit: the slice can land mid-escape-sequence, so it starts after
|
||||
// the first newline instead of leading with a fragment.
|
||||
const long = "x".repeat(50) + "\nkeep me";
|
||||
assert.equal(outputTail(long, 20), "keep me");
|
||||
// No newline in the tail at all: nothing to trim to, so return it as sliced.
|
||||
assert.equal(outputTail("y".repeat(50), 10), "y".repeat(10));
|
||||
});
|
||||
|
|
@ -6,13 +6,28 @@
|
|||
* the Ink app, so the launcher → TUI → core path and `--catalog` loading went
|
||||
* unverified by the launcher smokes. This script launches the built launcher in
|
||||
* `--tui` mode against a temp `--catalog`, waits for the app to render its first
|
||||
* frame (the "MCP Servers" panel) within a timeout, then sends SIGTERM and
|
||||
* exits.
|
||||
* frame (the "MCP Servers" panel), waits again to confirm it is *still running*,
|
||||
* then sends SIGTERM and exits.
|
||||
*
|
||||
* It asserts the TUI *boots and renders* without crashing — not full
|
||||
* interaction (driving an Ink UI deterministically in CI is flaky, so this is
|
||||
* intentionally a shallow render check). Exits non-zero on a crash-before-render
|
||||
* or a render timeout.
|
||||
* It asserts the TUI boots, renders, and keeps running — not full interaction
|
||||
* (driving an Ink UI deterministically is flaky, so this is intentionally a
|
||||
* shallow check). Exits non-zero on a crash before or after first paint, or on
|
||||
* a render timeout.
|
||||
*
|
||||
* ## Why it runs under a pseudoterminal (#2147)
|
||||
*
|
||||
* It used to spawn the TUI with its stdin on `/dev/null` and settle OK the
|
||||
* moment the marker appeared. Ink mounts `useInput`, `useInput` needs raw mode,
|
||||
* and raw mode is a property of the file descriptor — so the TUI painted one
|
||||
* frame and died ~40ms later with "Raw mode is not supported on the current
|
||||
* process.stdin". The smoke won that race and reported OK, on every machine,
|
||||
* TTY or not: what it actually asserted was *first paint*, not survival.
|
||||
*
|
||||
* Two changes, and the second is the fix: `scripts/lib/pty.mjs` gives the child
|
||||
* a real terminal via `script(1)`, and `scripts/lib/render-smoke.mjs` requires
|
||||
* it to outlive its first frame. Without the survival assertion a future
|
||||
* harness change reintroduces exactly this false green and nothing notices —
|
||||
* which is why that assertion lives in a tested module rather than here.
|
||||
*
|
||||
* Expects `clients/launcher/build` and `clients/tui/build` to be built first
|
||||
* (the validate / CI ordering guarantees this). The bundled stdio test server
|
||||
|
|
@ -20,39 +35,72 @@
|
|||
* `scripts/lib/ensure-test-servers.mjs` for why presence isn't freshness.
|
||||
*/
|
||||
|
||||
import { spawn } from "node:child_process";
|
||||
import { mkdtempSync, existsSync, writeFileSync } from "node:fs";
|
||||
import { tmpdir } from "node:os";
|
||||
import { join, resolve } from "node:path";
|
||||
import { removeSafe } from "./lib/child-cleanup.mjs";
|
||||
import { ensureTestServers } from "./lib/ensure-test-servers.mjs";
|
||||
import { resolvePtyWrapper } from "./lib/pty.mjs";
|
||||
import { DEFAULTS, normalizeMs, runRenderSmoke } from "./lib/render-smoke.mjs";
|
||||
|
||||
const repoRoot = resolve(import.meta.dirname, "..");
|
||||
const launcher = join(repoRoot, "clients", "launcher", "build", "index.js");
|
||||
const RENDER_MARKER = "MCP Servers";
|
||||
const TIMEOUT_MS = Number(process.env.SMOKE_TUI_TIMEOUT_MS ?? 15000);
|
||||
// `??` does not cover the two holes that matter here: an env var set to the
|
||||
// empty string parses as 0, and a typo as NaN, and `setTimeout` fires
|
||||
// immediately on both. For SURVIVE_MS that would silently restore the
|
||||
// first-paint false green on a run that still prints OK, so every duration goes
|
||||
// through `normalizeMs` — which is also where the defaults live, so this file
|
||||
// and the module cannot drift on what they are.
|
||||
const envMs = (name, fallback) =>
|
||||
normalizeMs(Number(process.env[name]), fallback);
|
||||
|
||||
const TIMEOUT_MS = envMs("SMOKE_TUI_TIMEOUT_MS", DEFAULTS.timeoutMs);
|
||||
// How long past first paint the TUI must still be running. Generous relative to
|
||||
// the failure it catches (the raw-mode crash landed ~40ms after the marker) —
|
||||
// the cost of a longer window is only wall-clock on a passing run.
|
||||
const SURVIVE_MS = envMs("SMOKE_TUI_SURVIVE_MS", DEFAULTS.surviveMs);
|
||||
const EXIT_GRACE_MS = envMs("SMOKE_TUI_EXIT_GRACE_MS", DEFAULTS.exitGraceMs);
|
||||
// Ink's own error text when it is asked for raw mode on a non-terminal fd.
|
||||
const RAW_MODE_ERROR = /Raw mode is not supported/;
|
||||
|
||||
function fail(message) {
|
||||
console.error(`smoke:tui FAILED — ${message}`);
|
||||
process.exit(1);
|
||||
}
|
||||
|
||||
// The Ink TUI requires a real TTY for raw-mode keyboard input. Headless CI has
|
||||
// none, so the app throws "Raw mode is not supported" on mount and exits before
|
||||
// it can render its first frame — making this boot/render check inherently a
|
||||
// LOCAL-only smoke (its own design notes call driving Ink in CI flaky). Skip it
|
||||
// under CI rather than fail spuriously: the TUI is still built and unit-tested
|
||||
// there; only this terminal-dependent render check is local-only.
|
||||
if (process.env.CI) {
|
||||
console.log(
|
||||
"smoke:tui SKIPPED — Ink needs a real TTY (raw mode), unavailable in CI; run it locally",
|
||||
);
|
||||
function skip(message) {
|
||||
console.log(`smoke:tui SKIPPED — ${message}`);
|
||||
process.exit(0);
|
||||
}
|
||||
|
||||
// Kept out of GitHub CI by decision, not by capability. The PTY above removes
|
||||
// the technical blocker this skip used to cite (a headless runner has no TTY),
|
||||
// but whether this smoke joins CI is a separate call for the maintainers, and
|
||||
// #2146 is deliberately keeping the other local-only smokes out. Making it
|
||||
// *valid* stands on its own: it is a local-only gate, which is exactly where a
|
||||
// false green is least likely to be caught by anything else.
|
||||
if (process.env.CI) {
|
||||
skip(
|
||||
"local-only by decision (see the header); the TUI is built and unit-tested in CI",
|
||||
);
|
||||
}
|
||||
|
||||
if (!existsSync(launcher)) {
|
||||
fail(`launcher build not found at ${launcher} — run \`npm run build\` first`);
|
||||
}
|
||||
|
||||
// No `script(1)` (Windows). Running without a PTY is not a weaker check, it is
|
||||
// a guaranteed failure — so say why rather than report a crash as a defect.
|
||||
// `node-pty` is the portable answer if this ever needs to run there.
|
||||
const pty = resolvePtyWrapper();
|
||||
if (!pty.ok) {
|
||||
skip(
|
||||
`${pty.reason} — the Ink TUI needs raw mode, which requires a real ` +
|
||||
`terminal fd (see scripts/lib/pty.mjs)`,
|
||||
);
|
||||
}
|
||||
|
||||
// Rebuilt on every run — presence is not freshness (#2111).
|
||||
let testServer;
|
||||
try {
|
||||
|
|
@ -76,183 +124,57 @@ writeFileSync(
|
|||
}),
|
||||
);
|
||||
|
||||
const child = spawn(
|
||||
process.execPath,
|
||||
[launcher, "--tui", "--catalog", catalogPath],
|
||||
{
|
||||
cwd: repoRoot,
|
||||
// Redirect HOME so the TUI's storage never touches the real ~/.mcp-inspector.
|
||||
// Pin MCP_OAUTH_CALLBACK_URL="" (empty reads as unset) so an ambient
|
||||
// non-loopback value can't crash the TUI before render via the loopback
|
||||
// callback guard — same class smoke-cli.mjs's SMOKE_BASE_ENV neutralizes.
|
||||
env: {
|
||||
...process.env,
|
||||
MCP_OAUTH_CALLBACK_URL: "",
|
||||
HOME: work,
|
||||
USERPROFILE: work,
|
||||
const { command, args } = pty.wrap({
|
||||
command: process.execPath,
|
||||
args: [launcher, "--tui", "--catalog", catalogPath],
|
||||
});
|
||||
|
||||
let result;
|
||||
try {
|
||||
result = await runRenderSmoke({
|
||||
command,
|
||||
args,
|
||||
marker: RENDER_MARKER,
|
||||
timeoutMs: TIMEOUT_MS,
|
||||
surviveMs: SURVIVE_MS,
|
||||
exitGraceMs: EXIT_GRACE_MS,
|
||||
spawnOptions: {
|
||||
cwd: repoRoot,
|
||||
// Redirect HOME so the TUI's storage never touches the real
|
||||
// ~/.mcp-inspector. Pin MCP_OAUTH_CALLBACK_URL="" (empty reads as unset)
|
||||
// so an ambient non-loopback value can't crash the TUI before render via
|
||||
// the loopback callback guard — same class smoke-cli.mjs's
|
||||
// SMOKE_BASE_ENV neutralizes.
|
||||
env: {
|
||||
...process.env,
|
||||
MCP_OAUTH_CALLBACK_URL: "",
|
||||
HOME: work,
|
||||
USERPROFILE: work,
|
||||
},
|
||||
},
|
||||
stdio: ["ignore", "pipe", "pipe"],
|
||||
},
|
||||
);
|
||||
|
||||
let output = "";
|
||||
let settled = false;
|
||||
let childExited = false;
|
||||
let childClosed = false;
|
||||
|
||||
// How long to let the TUI wind down after SIGTERM before escalating to SIGKILL,
|
||||
// and then before giving up on the close event entirely.
|
||||
const EXIT_GRACE_MS = Number(process.env.SMOKE_TUI_EXIT_GRACE_MS ?? 5000);
|
||||
// Once the child itself is gone, only the pipe drain remains, so the wait drops
|
||||
// to this. `close` is bounded by whoever holds the stdio pipes, which can
|
||||
// outlive the direct child: any descendant that inherited them keeps it pending.
|
||||
// The TUI spawns nothing at boot today (it does not auto-connect), so this
|
||||
// never fires — but that reason lives outside this file, so cap it here rather
|
||||
// than inherit an unrelated process's lifetime if that ever changes.
|
||||
//
|
||||
// The cost of capping: on that path we remove the work dir while a descendant
|
||||
// holding HOME=work may still be writing to it — the #1801 race, re-entered
|
||||
// deliberately. That is only acceptable because cleanup() warns instead of
|
||||
// throwing, so the worst case is a warning plus a leaked temp dir in tmpdir,
|
||||
// never a red smoke. The two are load-bearing for each other.
|
||||
const DRAIN_MS = 500;
|
||||
|
||||
function cleanup() {
|
||||
// Belt and braces for the acceptance criterion this fix is measured
|
||||
// against. Survival already implies it today, since the raw-mode throw is
|
||||
// fatal — but if Ink ever degrades it to a warning, a TUI with no keyboard
|
||||
// input would survive happily, and this smoke is the only thing watching.
|
||||
forbidOutput: {
|
||||
pattern: RAW_MODE_ERROR,
|
||||
reason:
|
||||
`TUI rendered and survived, but still logged the raw-mode error — ` +
|
||||
`the pseudoterminal did not take effect (flavor: ${pty.flavor})`,
|
||||
},
|
||||
warn: (m) => console.warn(m.replace(/^render-smoke/, "smoke:tui")),
|
||||
});
|
||||
} finally {
|
||||
// Never turn a passing smoke into a failure over a leftover temp dir; the OS
|
||||
// reclaims tmpdir anyway. On the normal path a warning should not fire, since
|
||||
// removal waits for the child's `close` (see done()) — but the two give-up
|
||||
// branches there call finish() without it, and a process may still hold the
|
||||
// dir, so it is expected rather than anomalous on those.
|
||||
// reclaims tmpdir anyway. runRenderSmoke resolves only after the child's
|
||||
// `close`, so on the normal path the #1801 ENOTEMPTY race is already closed
|
||||
// and a warning here should not fire — its give-up branches resolve without
|
||||
// that guarantee, so it is expected rather than anomalous on those.
|
||||
removeSafe(work, { label: "smoke:tui" });
|
||||
}
|
||||
|
||||
// Tail of the child's output, for quoting in a diagnostic. Slicing an Ink
|
||||
// stream can land mid-CSI-sequence, so drop through the first newline rather
|
||||
// than lead with an escape-code fragment that mangles the line after it.
|
||||
function outputTail(limit = 800) {
|
||||
const tail = output.slice(-limit);
|
||||
const nl = tail.indexOf("\n");
|
||||
return output.length > limit && nl !== -1 ? tail.slice(nl + 1) : tail;
|
||||
if (result.code === 0) {
|
||||
console.log(`smoke:tui OK — ${result.message}`);
|
||||
process.exit(0);
|
||||
}
|
||||
|
||||
// `message` may be a string or a thunk. Callers that quote the child's output
|
||||
// pass a thunk so it is rendered here — after the wait below, by which point
|
||||
// `close` guarantees stdout/stderr have been fully drained. Building the string
|
||||
// at call time instead can truncate the crash reason that matters most.
|
||||
let finished = false;
|
||||
function finish(code, message) {
|
||||
if (finished) return;
|
||||
finished = true;
|
||||
cleanup();
|
||||
const text = typeof message === "function" ? message() : message;
|
||||
if (code === 0) {
|
||||
console.log(`smoke:tui OK — ${text}`);
|
||||
} else {
|
||||
console.error(`smoke:tui FAILED — ${text}`);
|
||||
}
|
||||
process.exit(code);
|
||||
}
|
||||
|
||||
function done(code, message) {
|
||||
if (settled) return;
|
||||
settled = true;
|
||||
clearTimeout(timer);
|
||||
if (childClosed) {
|
||||
finish(code, message);
|
||||
return;
|
||||
}
|
||||
// Wait for the child to actually exit before removing the work dir (#1801).
|
||||
// The dir doubles as the TUI's HOME, so the Ink process is still writing into
|
||||
// it when we signal; an rmSync racing those writes fails with ENOTEMPTY when
|
||||
// a file lands after a directory has been read but before it is removed.
|
||||
//
|
||||
// Wait on `close`, not `exit`: a *spawn failure* emits `error` + `close` and
|
||||
// never `exit`, so an `exit` wait would hang here until the give-up timer and
|
||||
// then blame a SIGTERM the child never received. `close` also guarantees the
|
||||
// stdio pipes are drained, which is what makes the thunked messages complete.
|
||||
//
|
||||
// This deliberately does NOT use lib/child-cleanup.mjs's `stopChild()` (which
|
||||
// pack-and-verify.mjs uses against the same race): that helper resolves on the
|
||||
// FIRST of exit/close, whereas this wait is entangled with the drain phase —
|
||||
// it must keep waiting for `close` after `exit`, on a shorter re-armed
|
||||
// deadline, so the quoted output is complete. Only `removeSafe()` is shared.
|
||||
const forceKill = setTimeout(() => child.kill("SIGKILL"), EXIT_GRACE_MS);
|
||||
|
||||
// Deadline for the wait. Re-armed shorter once the child is gone, since from
|
||||
// that point we are only draining pipes. Each message describes what was
|
||||
// actually being waited on — the timer can fire for a child that already
|
||||
// exited and was never signalled, so it must not claim otherwise.
|
||||
let deadline;
|
||||
const armDeadline = (ms, warning) => {
|
||||
clearTimeout(deadline);
|
||||
deadline = setTimeout(() => {
|
||||
console.warn(`smoke:tui — ${warning}`);
|
||||
finish(code, message);
|
||||
}, ms);
|
||||
};
|
||||
armDeadline(
|
||||
EXIT_GRACE_MS * 2,
|
||||
`TUI did not close its output streams within ${EXIT_GRACE_MS * 2}ms; cleaning up anyway`,
|
||||
);
|
||||
|
||||
const drainOnly = () => {
|
||||
clearTimeout(forceKill);
|
||||
armDeadline(
|
||||
DRAIN_MS,
|
||||
`TUI exited but held its output streams open for ${DRAIN_MS}ms (a descendant may have inherited them); cleaning up anyway`,
|
||||
);
|
||||
};
|
||||
if (childExited) drainOnly();
|
||||
else child.once("exit", drainOnly);
|
||||
|
||||
child.once("close", () => {
|
||||
clearTimeout(forceKill);
|
||||
clearTimeout(deadline);
|
||||
finish(code, message);
|
||||
});
|
||||
// Only signal a child that is still running: on the crash-before-render and
|
||||
// spawn-failure paths there is nothing left to signal, and we are here purely
|
||||
// to wait out the remaining `close`.
|
||||
if (child.exitCode === null && child.signalCode === null && !child.killed) {
|
||||
child.kill("SIGTERM");
|
||||
}
|
||||
}
|
||||
|
||||
function onData(chunk) {
|
||||
output += chunk.toString();
|
||||
if (output.includes(RENDER_MARKER)) {
|
||||
done(0, `rendered "${RENDER_MARKER}" panel from --catalog within timeout`);
|
||||
}
|
||||
}
|
||||
|
||||
child.stdout.on("data", onData);
|
||||
child.stderr.on("data", onData);
|
||||
|
||||
// Registered before done()'s own one-shot listener, so this always runs first —
|
||||
// done() can trust `childClosed` even when called from inside a close handler.
|
||||
child.on("close", () => {
|
||||
childClosed = true;
|
||||
});
|
||||
|
||||
child.on("exit", (code) => {
|
||||
childExited = true;
|
||||
if (settled) return;
|
||||
// Exiting before the render marker appeared is a failure (crash on boot).
|
||||
done(
|
||||
1,
|
||||
() =>
|
||||
`TUI exited (code ${code}) before rendering "${RENDER_MARKER}"\n${outputTail()}`,
|
||||
);
|
||||
});
|
||||
|
||||
child.on("error", (err) => {
|
||||
done(1, `failed to spawn TUI: ${err.message}`);
|
||||
});
|
||||
|
||||
const timer = setTimeout(() => {
|
||||
done(
|
||||
1,
|
||||
() =>
|
||||
`TUI did not render "${RENDER_MARKER}" within ${TIMEOUT_MS}ms\n${outputTail()}`,
|
||||
);
|
||||
}, TIMEOUT_MS);
|
||||
fail(result.message);
|
||||
|
|
|
|||
Loading…
Reference in a new issue