fix: strip ANSI from TUI test frames so FORCE_COLOR cannot break assertions (#2207)
Ink writes styling as escape sequences inside the styled run, so an
accelerator underline splits the word it decorates: `<Text underline>I</Text>nfo`
reaches the frame buffer with escapes between `I` and `nfo`, and a plain
`toContain("Info")` fails against a component that is rendering correctly.
Chalk only emits color when it detects a TTY, so CI stays green and the six
failures land only on a developer whose shell exports FORCE_COLOR.
Add `__tests__/helpers/renderTui.tsx` — ink-testing-library's `render` with
`lastFrame()` and `frames` ANSI-stripped — and point all 16 renderer test
files at it. `frames` is a getter reading through to the live array, since
Ink appends to it as the component re-renders; the raw bytes stay reachable
as `stdout.lastFrame()`.
Pinning FORCE_COLOR=0 would turn the six green while hiding the class: any
future assertion on styled text has the same problem, and the suite would
stop covering the styled path at all.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01MFp2LVH7UJST5i8bDBJghw
Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
parent
42babb7482
commit
a3bb679f1e
23 changed files with 141 additions and 21 deletions
|
|
@ -171,10 +171,20 @@ Scope notes:
|
|||
only exclusion. `commander` uses `.exitOverride()` so a parse error throws
|
||||
instead of tearing down the test worker.
|
||||
- **TUI** covers **all of `src/**`, React surface included**. Components mount
|
||||
through `ink-testing-library` with the passthrough doubles in
|
||||
`__tests__/helpers/`; keypresses are driven through stdin. The only exclusion
|
||||
is `src/tui-servers.ts` (a pure re-export, excluded so it doesn't surface as a
|
||||
through `__tests__/helpers/renderTui.tsx` — `ink-testing-library`'s `render`
|
||||
with every frame ANSI-stripped — alongside the passthrough doubles in the same
|
||||
directory; keypresses are driven through stdin. The only exclusion is
|
||||
`src/tui-servers.ts` (a pure re-export, excluded so it doesn't surface as a
|
||||
misleading 0/0 row).
|
||||
⚠️ **Import `render` from that helper, not from `ink-testing-library`.** Ink
|
||||
writes styling *inside* the styled run, so `<Text underline>I</Text>nfo`
|
||||
reaches the frame buffer with escapes between `I` and `nfo` and a plain
|
||||
`toContain("Info")` fails against a component that is rendering correctly. It
|
||||
only shows up where chalk emits color — a developer whose shell exports
|
||||
`FORCE_COLOR` — so CI, which has no TTY, stays green on a suite that is red
|
||||
for them (#2207). If a frame assertion fails on a string you can plainly see
|
||||
in the printed diff, that is the tell. Reach `stdout.lastFrame()` on the
|
||||
returned instance for the raw bytes.
|
||||
|
||||
### When a `v8 ignore` is justified
|
||||
|
||||
|
|
|
|||
|
|
@ -386,6 +386,7 @@ diagnose a failing gate — is the `testing` skill. These are the rules.
|
|||
- **`clients/web`**: `<Name>.test.tsx` **next to the source** — components, hooks, `lib/`, `utils/`. A web-owned test living under `src/test/` instead is a bug. `src/test/` is for the three things that cannot be co-located: tests of the repo-root **`core/`** package (`src/test/core/…`, mirroring the `core/` layout — it lives outside `clients/web/` and has no harness of its own); the **`integration`** project (`src/test/integration/…` — _placement is the manifest_, picked up by a folder glob, with no enumeration to keep in sync); and **shared test infrastructure** (`renderWithMantine.tsx`, `setup.ts`, `fixtures/`).
|
||||
- **`clients/cli`, `clients/tui`, `clients/launcher`**: **all** tests in a top-level **`__tests__/`**, not beside their source. Their `tsconfig.json` excludes `**/*.test.*`, so a co-located test lands in **no** tsconfig project and fails `npm run verify:typecheck-coverage`.
|
||||
- **Root tooling**: a `scripts/*.mjs` helper with pure logic gets a sibling `*.test.mjs`. Keep that exact filename — `node --test` silently _skips_ a file its glob misses and still exits 0.
|
||||
- **Render Ink components through the TUI's own `render`** (`clients/tui/__tests__/helpers/renderTui.tsx`), never `ink-testing-library`'s directly. It is the same function with every frame ANSI-stripped, which is what keeps an assertion on styled text from depending on the ambient environment: Ink writes styling *inside* the styled run, so `<Text underline>I</Text>nfo` reaches the frame buffer with escapes between `I` and `nfo` and `toContain("Info")` fails. It only bites where chalk emits color — a developer whose shell exports `FORCE_COLOR` — so CI is green on a suite that is broken for them (#2207). A test that genuinely needs the raw bytes reads `stdout.lastFrame()` off the returned instance.
|
||||
- **Render React components through `renderWithMantine`** (`src/test/renderWithMantine.tsx`); do not hand-roll a bare `MantineProvider`, which skips the project theme and the helper's options and drifts from every other test. Pass the `colorScheme` option to exercise a forced scheme rather than hand-rolling `defaultColorScheme`. Use `renderWithMantineTransitions` **only** when a test must assert mid-flight transition state, and read the long comment on the helper before changing anything about it.
|
||||
- **The web coverage `include` is a whitelist.** It names `components`/`hooks`/`theme`/`lib`/`utils`/`server` plus the browser-consumed `core/*` runtime, so a module placed **outside** those directories falls out of the gate entirely, silently. Place new modules inside a gated directory. The documented exceptions — `src/App.tsx` and the `src/main.tsx` / `src/index.ts` bootstraps — are called out in a comment on the `include` array itself.
|
||||
|
||||
|
|
|
|||
|
|
@ -103,8 +103,10 @@ stricter react-hooks@7 rules are not enforced on the interim component surface
|
|||
(#1501).
|
||||
|
||||
Tests live in `__tests__/`. The coverage gate covers **all of `src/**`**, React
|
||||
surface included — the Ink components mount through `ink-testing-library` (with
|
||||
the `ink-scroll-view` / `ink-form` passthrough doubles in `__tests__/helpers/`),
|
||||
surface included — the Ink components mount through
|
||||
`__tests__/helpers/renderTui.tsx`, `ink-testing-library`'s `render` wrapped so
|
||||
every frame it hands back is ANSI-stripped (with the `ink-scroll-view` /
|
||||
`ink-form` passthrough doubles in the same directory),
|
||||
`App.tsx` mounts against a mock of the `@inspector/core` surface, and keypresses
|
||||
are driven through stdin. The former interim exclusion of the components and
|
||||
`App.tsx` was lifted in #1501; the only exclusion left in `vitest.config.ts` is
|
||||
|
|
@ -113,6 +115,16 @@ statements of its own (its logic is measured in `core/` via the web suite, and
|
|||
`tui-servers.test.ts` still exercises it behaviorally — it is excluded only so it
|
||||
doesn't surface as a misleading 0/0 row).
|
||||
|
||||
**Import `render` from `__tests__/helpers/renderTui.tsx`, never from
|
||||
`ink-testing-library` directly.** Ink writes styling as escape sequences *inside*
|
||||
the styled run, so an accelerator underline splits the word it decorates —
|
||||
`<Text underline>I</Text>nfo` reaches the frame buffer with escapes between `I`
|
||||
and `nfo`, and `expect(frame).toContain("Info")` fails against a component that
|
||||
is rendering correctly. Because chalk only emits color when it detects a TTY,
|
||||
this is invisible in CI and hits exactly the developer whose shell exports
|
||||
`FORCE_COLOR` (#2207). The wrapper strips styling from `lastFrame()` and
|
||||
`frames`; the untouched bytes stay available as `stdout.lastFrame()`.
|
||||
|
||||
### Bundling: React-rendering dependencies must be inlined (#1952)
|
||||
|
||||
`tsup.config.ts` splits the TUI's dependencies into bundled (`noExternal`) and
|
||||
|
|
|
|||
|
|
@ -1,6 +1,6 @@
|
|||
import React from "react";
|
||||
import { describe, it, expect, vi, beforeEach, afterEach } from "vitest";
|
||||
import { render } from "ink-testing-library";
|
||||
import { render } from "./helpers/renderTui";
|
||||
|
||||
type RenderResult = ReturnType<typeof render>;
|
||||
|
||||
|
|
|
|||
|
|
@ -1,6 +1,6 @@
|
|||
import React from "react";
|
||||
import { describe, it, expect, vi } from "vitest";
|
||||
import { render } from "ink-testing-library";
|
||||
import { render } from "./helpers/renderTui";
|
||||
import type { OAuthConnectionState } from "@inspector/core/auth/types.js";
|
||||
import type { InspectorClient } from "@inspector/core/mcp/index.js";
|
||||
|
||||
|
|
|
|||
|
|
@ -1,6 +1,6 @@
|
|||
import React from "react";
|
||||
import { describe, it, expect, vi } from "vitest";
|
||||
import { render } from "ink-testing-library";
|
||||
import { render } from "./helpers/renderTui";
|
||||
import { Text } from "ink";
|
||||
|
||||
// ScrollView: passthrough so `content` mounts and the imperative ref API
|
||||
|
|
|
|||
|
|
@ -1,6 +1,6 @@
|
|||
import React from "react";
|
||||
import { describe, it, expect, vi } from "vitest";
|
||||
import { render } from "ink-testing-library";
|
||||
import { render } from "./helpers/renderTui";
|
||||
import type { MessageEntry } from "@inspector/core/mcp/index.js";
|
||||
|
||||
// MUST mock ink-scroll-view: the real ScrollView renders a placeholder minimap
|
||||
|
|
|
|||
|
|
@ -1,6 +1,6 @@
|
|||
import React from "react";
|
||||
import { describe, it, expect, vi } from "vitest";
|
||||
import { render } from "ink-testing-library";
|
||||
import { render } from "./helpers/renderTui";
|
||||
|
||||
// MUST mock ink-scroll-view: the real ScrollView renders a placeholder minimap
|
||||
// in the non-TTY test env and never mounts its children. This passthrough
|
||||
|
|
|
|||
|
|
@ -1,6 +1,6 @@
|
|||
import React from "react";
|
||||
import { describe, it, expect, vi } from "vitest";
|
||||
import { render } from "ink-testing-library";
|
||||
import { render } from "./helpers/renderTui";
|
||||
|
||||
// MUST mock ink-scroll-view: the real ScrollView renders a placeholder minimap
|
||||
// in the non-TTY test env and never mounts its children. This passthrough
|
||||
|
|
|
|||
|
|
@ -1,6 +1,6 @@
|
|||
import React from "react";
|
||||
import { describe, it, expect, vi, afterEach } from "vitest";
|
||||
import { render } from "ink-testing-library";
|
||||
import { render } from "./helpers/renderTui";
|
||||
import type { InspectorClient } from "@inspector/core/mcp/index.js";
|
||||
import { AuthRecoveryRequiredError } from "@inspector/core/auth/challenge.js";
|
||||
import type { Prompt } from "@modelcontextprotocol/client";
|
||||
|
|
|
|||
|
|
@ -1,6 +1,6 @@
|
|||
import React from "react";
|
||||
import { describe, it, expect, vi } from "vitest";
|
||||
import { render } from "ink-testing-library";
|
||||
import { render } from "./helpers/renderTui";
|
||||
import type { InspectorClient } from "@inspector/core/mcp/index.js";
|
||||
import type { Prompt } from "@modelcontextprotocol/client";
|
||||
|
||||
|
|
|
|||
|
|
@ -1,6 +1,6 @@
|
|||
import React from "react";
|
||||
import { describe, it, expect, vi } from "vitest";
|
||||
import { render } from "ink-testing-library";
|
||||
import { render } from "./helpers/renderTui";
|
||||
import type { FetchRequestEntry } from "@inspector/core/mcp/index.js";
|
||||
|
||||
// MUST mock ink-scroll-view: the real ScrollView renders a placeholder minimap
|
||||
|
|
|
|||
|
|
@ -1,6 +1,6 @@
|
|||
import React from "react";
|
||||
import { describe, it, expect, vi, afterEach } from "vitest";
|
||||
import { render } from "ink-testing-library";
|
||||
import { render } from "./helpers/renderTui";
|
||||
import type { InspectorClient } from "@inspector/core/mcp/index.js";
|
||||
import { AuthRecoveryRequiredError } from "@inspector/core/auth/challenge.js";
|
||||
|
||||
|
|
|
|||
|
|
@ -1,6 +1,6 @@
|
|||
import React from "react";
|
||||
import { describe, it, expect, vi } from "vitest";
|
||||
import { render } from "ink-testing-library";
|
||||
import { render } from "./helpers/renderTui";
|
||||
import type { InspectorClient } from "@inspector/core/mcp/index.js";
|
||||
import type { Resource } from "@modelcontextprotocol/client";
|
||||
|
||||
|
|
|
|||
|
|
@ -1,6 +1,6 @@
|
|||
import React from "react";
|
||||
import { describe, it, expect } from "vitest";
|
||||
import { render } from "ink-testing-library";
|
||||
import { render } from "./helpers/renderTui";
|
||||
import { SelectableItem } from "../src/components/SelectableItem.js";
|
||||
|
||||
describe("SelectableItem", () => {
|
||||
|
|
|
|||
|
|
@ -1,6 +1,6 @@
|
|||
import React from "react";
|
||||
import { describe, it, expect } from "vitest";
|
||||
import { render } from "ink-testing-library";
|
||||
import { render } from "./helpers/renderTui";
|
||||
import { Tabs } from "../src/components/Tabs.js";
|
||||
|
||||
const noop = () => {};
|
||||
|
|
|
|||
|
|
@ -1,6 +1,6 @@
|
|||
import React from "react";
|
||||
import { describe, it, expect, vi, afterEach } from "vitest";
|
||||
import { render } from "ink-testing-library";
|
||||
import { render } from "./helpers/renderTui";
|
||||
import type { InspectorClient } from "@inspector/core/mcp/index.js";
|
||||
import type { Tool } from "@modelcontextprotocol/client";
|
||||
|
||||
|
|
|
|||
|
|
@ -1,6 +1,6 @@
|
|||
import React from "react";
|
||||
import { describe, it, expect, vi } from "vitest";
|
||||
import { render } from "ink-testing-library";
|
||||
import { render } from "./helpers/renderTui";
|
||||
import type { Tool } from "@modelcontextprotocol/client";
|
||||
|
||||
// MUST mock ink-scroll-view: the real ScrollView renders a placeholder minimap
|
||||
|
|
|
|||
38
clients/tui/__tests__/helpers/renderTui.tsx
Normal file
38
clients/tui/__tests__/helpers/renderTui.tsx
Normal file
|
|
@ -0,0 +1,38 @@
|
|||
// A drop-in replacement for ink-testing-library's `render` that strips ANSI
|
||||
// styling from every frame the tests read back.
|
||||
//
|
||||
// Ink writes styling as escape sequences *inside* the styled run, so an
|
||||
// accelerator underline splits the word it decorates: `<Text underline>I</Text>nfo`
|
||||
// reaches the frame buffer as `ESC[4mI ESC[24m nfo` (without the spaces). A plain
|
||||
// `expect(frame).toContain("Info")` then fails against a component that is
|
||||
// rendering perfectly — and only for a developer whose shell exports
|
||||
// FORCE_COLOR, since CI has no TTY and chalk emits nothing there (#2207).
|
||||
//
|
||||
// Making every assertion read a stripped frame fixes the whole class rather
|
||||
// than the six assertions that happened to trip it, and keeps the suite
|
||||
// covering the styled path instead of pinning FORCE_COLOR=0 to hide it. No test
|
||||
// asserts on escape sequences; one that needs the raw bytes can reach
|
||||
// `stdout.lastFrame()` on the returned instance, which is left untouched.
|
||||
import { render as inkRender } from "ink-testing-library";
|
||||
import stripAnsi from "strip-ansi";
|
||||
import type { ReactElement } from "react";
|
||||
|
||||
// ink-testing-library does not export its `Instance` type.
|
||||
type Instance = ReturnType<typeof inkRender>;
|
||||
|
||||
/** Strip ANSI styling from a frame, passing `undefined` through unchanged. */
|
||||
export const stripFrameStyling = (frame: string | undefined) =>
|
||||
frame === undefined ? undefined : stripAnsi(frame);
|
||||
|
||||
export const render = (tree: ReactElement): Instance => {
|
||||
const instance = inkRender(tree);
|
||||
return {
|
||||
...instance,
|
||||
lastFrame: () => stripFrameStyling(instance.lastFrame()),
|
||||
// Read through to the live array on each access — ink appends to it as the
|
||||
// component re-renders, so a copy taken here would go stale.
|
||||
get frames() {
|
||||
return instance.frames.map(stripAnsi);
|
||||
},
|
||||
};
|
||||
};
|
||||
57
clients/tui/__tests__/renderTui.test.tsx
Normal file
57
clients/tui/__tests__/renderTui.test.tsx
Normal file
|
|
@ -0,0 +1,57 @@
|
|||
import React from "react";
|
||||
import { Text } from "ink";
|
||||
import { describe, expect, it } from "vitest";
|
||||
import { render, stripFrameStyling } from "./helpers/renderTui";
|
||||
|
||||
// The escape sequences ink emits for `<Text underline>`, spelled out here so
|
||||
// the assertions hold regardless of whether the ambient environment has
|
||||
// FORCE_COLOR set — chalk emits nothing without a TTY, which is exactly why
|
||||
// #2207 never reproduced in CI.
|
||||
const ESC = "\u001B";
|
||||
const UNDERLINE_OPEN = `${ESC}[4m`;
|
||||
const UNDERLINE_CLOSE = `${ESC}[24m`;
|
||||
const SPLIT_WORD = `${UNDERLINE_OPEN}I${UNDERLINE_CLOSE}nfo`;
|
||||
|
||||
describe("stripFrameStyling", () => {
|
||||
it("removes styling that splits a word", () => {
|
||||
expect(stripFrameStyling(SPLIT_WORD)).toBe("Info");
|
||||
});
|
||||
|
||||
it("leaves an unstyled frame untouched", () => {
|
||||
expect(stripFrameStyling("Info")).toBe("Info");
|
||||
});
|
||||
|
||||
it("passes undefined through", () => {
|
||||
expect(stripFrameStyling(undefined)).toBeUndefined();
|
||||
});
|
||||
});
|
||||
|
||||
describe("render", () => {
|
||||
it("strips styling from lastFrame", () => {
|
||||
const { stdout, lastFrame } = render(<Text>placeholder</Text>);
|
||||
stdout.write(SPLIT_WORD);
|
||||
expect(lastFrame()).toBe("Info");
|
||||
});
|
||||
|
||||
it("strips styling from frames, and keeps reading the live array", () => {
|
||||
const instance = render(<Text>placeholder</Text>);
|
||||
const before = instance.frames.length;
|
||||
instance.stdout.write(SPLIT_WORD);
|
||||
expect(instance.frames).toHaveLength(before + 1);
|
||||
expect(instance.frames.at(-1)).toBe("Info");
|
||||
});
|
||||
|
||||
it("leaves the raw bytes reachable through stdout", () => {
|
||||
const instance = render(<Text>placeholder</Text>);
|
||||
instance.stdout.write(SPLIT_WORD);
|
||||
expect(instance.stdout.lastFrame()).toBe(SPLIT_WORD);
|
||||
});
|
||||
|
||||
it("still exposes the rest of the ink-testing-library instance", () => {
|
||||
const { rerender, lastFrame, unmount } = render(<Text>first</Text>);
|
||||
expect(lastFrame()).toContain("first");
|
||||
rerender(<Text>second</Text>);
|
||||
expect(lastFrame()).toContain("second");
|
||||
unmount();
|
||||
});
|
||||
});
|
||||
Some files were not shown because too many files have changed in this diff Show more
Loading…
Reference in a new issue