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

fix(scripts): make the test stubs drain, and the raw-mode guard testable (#2147)

Browse source 
Copilot review round 5 — four findings, all in the suppressed block.

1+4. The regression stubs did `console.log(MARKER); process.exit(1)`.
     Writing to a PIPE is asynchronous and `process.exit()` tears the
     process down without draining, so the marker can be lost outright —
     and then the stub that pins the paint-then-die path fails as "exited
     before rendering", the neighbouring branch and the wrong diagnostic
     for the thing it guards.

     Worth recording that I met this mechanism last round and did not join
     the dots: probing the drain race, a 4MB write followed by
     `process.exit()` reached the parent with the marker missing
     entirely. That was noted as a reason the repro failed, while the same
     construct stayed in the stubs. Both now set `process.exitCode` and
     let the loop drain — still exiting immediately after the write — with
     a comment, since `process.exit()` is the more obvious spelling and
     will read as a simplification to the next person. Same fix for the
     `console.error("boom")` stub, whose assertion had the same exposure.

2.   DEFAULTS claimed "every one is a hard floor as well as a default",
     which contradicts `normalizeMs` (any finite positive value passes)
     and the suite two files over, which passes 400ms against a 2000ms
     default. It rejects the shape a bad env var produces, not smallness.

3.   The raw-mode forbidden-output check was unreachable from any test:
     the real PTY happy path only exercises the pattern NOT matching, so
     a regression breaking it would stay green forever — an
     acceptance-criterion guard with nothing guarding it, which is the
     recursive form of this PR's own complaint.

     Now a `runRenderSmoke` option (`forbidOutput`), so it runs inside the
     tested state machine and against the fully drained output.
     `smoke:tui` passes its pattern in and drops the post-hoc check. It
     can only turn a pass into a failure, never rewrite an existing
     failure's message: a crash reason is more useful than "and it also
     printed X", which is usually a symptom of that crash. Three tests —
     matching, not matching, and that asymmetry.

Verified: test:scripts 213/213; `smoke:tui OK — rendered "MCP Servers" at
360ms and was still running 2000ms later`; and the no-PTY shim still
fails through the refactored path.

Signed-off-by: Cliff Hall <cliff@futurescale.com>
Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
cliffhall 2026-08-26 22:13:35 -04:00
commit e15dd8abee
3 changed files with 103 additions and 19 deletions
Download patch file Download diff file Expand all files Collapse all files

31
scripts/lib/render-smoke.mjs
View file

@ -23,7 +23,11 @@
import { spawn } from "node:child_process";
/** Durations, in ms. Every one is a hard floor as well as a default. */
/**
* 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,
@ -93,6 +97,14 @@ export function outputTail(output, limit = 800) {
* @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 —
@ -115,6 +127,7 @@ export function runRenderSmoke({
surviveMs: requestedSurviveMs,
exitGraceMs: requestedExitGraceMs,
drainMs: requestedDrainMs,
forbidOutput,
onSpawn = () => {},
warn = (m) => console.warn(m),
}) {
@ -162,11 +175,17 @@ export function runRenderSmoke({
const finish = (code, message) => {
if (finished) return;
finished = true;
resolve({
code,
message: typeof message === "function" ? message() : message,
output,
});
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

70
scripts/lib/render-smoke.test.mjs
View file

@ -15,8 +15,17 @@ 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.exit(${code});`);
stub(`console.log(${JSON.stringify(MARKER)}); process.exitCode = ${code};`);
const paintThenLive = stub(
`console.log(${JSON.stringify(MARKER)}); setInterval(() => {}, 1000);`,
);
@ -96,7 +105,9 @@ test("a paint landing just under the deadline is not failed by the render timer"
test("fails when the child exits before painting", async () => {
const r = await runRenderSmoke({
...opts,
...stub(`console.error("boom"); process.exit(3);`),
// `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/);
@ -181,6 +192,61 @@ test("a zero survival window falls back rather than passing instantly", async ()
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

21
scripts/smoke-tui.mjs
View file

@ -152,6 +152,16 @@ try {
USERPROFILE: work,
},
},
// 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 {
@ -163,17 +173,6 @@ try {
removeSafe(work, { label: "smoke:tui" });
}
// 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 to a warning instead, a TUI with no keyboard input would
// otherwise pass silently, and this smoke is the only thing watching.
if (result.code === 0 && RAW_MODE_ERROR.test(result.output)) {
fail(
`TUI rendered and survived, but still logged the raw-mode error — the ` +
`pseudoterminal did not take effect (flavor: ${pty.flavor})`,
);
}
if (result.code === 0) {
console.log(`smoke:tui OK — ${result.message}`);
process.exit(0);