From 947ff6f6fa04574009ec27aa88fcbe33620591d2 Mon Sep 17 00:00:00 2001 From: Codeman maintainer Date: Tue, 18 Aug 2026 19:55:23 +0200 Subject: [PATCH] chore(test): make `npm test` the CI gate and give each excluded suite a runner MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `npm test` ran config/vitest.config.ts, which includes the browser, visual and perf suites. On any machine without chromium, a free port and per-machine PNG baselines that fails ~87 tests on a clean master, so the repo's most obvious command could not be used as a pass/fail signal. The workaround had spread into four docs as "never run bare `npm test`" warnings. `npm test` now runs config/vitest.ci.config.ts — byte-for-byte what CI runs — so local green means CI green. Verified: 264 files, 5248 tests, exit 0. The suites it leaves out are not abandoned; each has a command: test:browser 5 Playwright files (chromium + a live server; codex-predictive-echo also needs a real codex binary) test:mobile unchanged — the above plus per-machine PNG baselines test:perf 2 wall-clock benchmarks; need an otherwise idle machine test:all the old everything-behaviour, kept reachable test:ci is untouched (CI still calls it). test:watch and test:coverage follow test onto the gate's config. The more important half is the hole this closes. The exclusion list lived as literals in one config and pointed one way only: a file excluded from CI and added to no runner would be tested by NOTHING, silently, with every command still green — vitest counts "no files matched a filter" as success. That is the same shape as the #279/#280 blind spot already documented in CLAUDE.md. So the globs moved to config/test-suites.ts, one array per REASON a suite cannot run in CI, and all three configs derive from it. test/test-suite-partition.test.ts then checks the arithmetic against the files on disk: it fails if any test file is reachable by no runner, or by two. Confirmed it fires by orphaning a file and watching it name it. The partition is exact today: gate 264 + browser 5 + perf 2 + mobile 9 = 280 = every *.test.ts in the repo ⚠️ One sharp edge, deliberate and documented: a file filter must match its runner. `npm test -- test/mobile/keyboard.test.ts` now matches nothing and exits GREEN having run zero tests, because the gate's config excludes that path. CLAUDE.md recommended exactly that command in the on-screen-keyboard note; that line now says `npm run test:mobile -- `, and the Testing section calls out the trap, since a green run of zero tests is worse than a red one. Docs synced: CLAUDE.md, AGENTS.md, .github/CONTRIBUTING.md, README.md, README.zh-CN.md, and two ci.yml comments that claimed only test/mobile/** was excluded — it is three suites, and 5 Playwright files rather than 3. Co-Authored-By: Claude Opus 5 (1M context) --- .changeset/test-script-split.md | 13 ++++ .github/CONTRIBUTING.md | 15 ++++- .github/workflows/ci.yml | 15 +++-- AGENTS.md | 2 +- CLAUDE.md | 36 +++++++---- README.md | 2 +- README.zh-CN.md | 2 +- config/test-suites.ts | 45 ++++++++++++++ config/vitest.browser.config.ts | 34 ++++++++++ config/vitest.ci.config.ts | 23 +++---- config/vitest.config.ts | 11 ++++ config/vitest.perf.config.ts | 25 ++++++++ package.json | 9 ++- test/test-suite-partition.test.ts | 100 ++++++++++++++++++++++++++++++ 14 files changed, 293 insertions(+), 39 deletions(-) create mode 100644 .changeset/test-script-split.md create mode 100644 config/test-suites.ts create mode 100644 config/vitest.browser.config.ts create mode 100644 config/vitest.perf.config.ts create mode 100644 test/test-suite-partition.test.ts diff --git a/.changeset/test-script-split.md b/.changeset/test-script-split.md new file mode 100644 index 00000000..194d7d2e --- /dev/null +++ b/.changeset/test-script-split.md @@ -0,0 +1,13 @@ +--- +'aicodeman': patch +--- + +`npm test` is now the CI gate and is safe to run bare; the suites it cannot run each got their own command. + +`npm test` ran the everything-config, which fails ~87 tests on a clean master on any machine without chromium, a free port and per-machine PNG baselines. That made the repo's most obvious command useless as a pass/fail signal, and the docs had accumulated "never run bare `npm test`" warnings in four files to work around it. It now runs `config/vitest.ci.config.ts` — exactly what CI runs — so local green means CI green. + +- New: `test:browser` (5 Playwright files), `test:perf` (2 wall-clock benchmarks), `test:all` (the old everything-behaviour, kept reachable). `test:ci` and `test:mobile` are unchanged; `test:watch` and `test:coverage` follow `test` onto the gate's config. +- The exclusion list moved to `config/test-suites.ts`, with the reason each suite cannot run in CI. Every config derives from it, so the gate's excludes and the runners' includes cannot drift. +- That drift was a silent hole, not a tidiness problem: a file excluded from CI and added to no runner is tested by NOTHING, and every command stays green, because vitest counts "no files matched" as success. `test/test-suite-partition.test.ts` now fails if any test file is reachable by no runner or by two. +- ⚠️ A file filter must match its runner: `npm test -- test/mobile/keyboard.test.ts` matches nothing and exits green having run zero tests, because the gate excludes that path. Use `npm run test:mobile -- `. Documented in CLAUDE.md, and the one place that recommended the old form was corrected. +- Docs synced: CLAUDE.md, AGENTS.md, .github/CONTRIBUTING.md, both READMEs, and two ci.yml comments that claimed only `test/mobile/**` was excluded (it is three suites, and 5 Playwright files rather than 3). diff --git a/.github/CONTRIBUTING.md b/.github/CONTRIBUTING.md index ee4ade77..71221862 100644 --- a/.github/CONTRIBUTING.md +++ b/.github/CONTRIBUTING.md @@ -37,11 +37,20 @@ npm run check:frontend-syntax # syntax-checks the plain-JS frontend modules ### Tests ```bash -npm test -- test/.test.ts # one file (the normal way) -npm run test:ci # the full CI sweep +npm test # the gate — exactly what CI runs +npm test -- test/.test.ts # one file ``` -**Never run bare `npm test`.** The default config includes browser-driven Playwright suites that need a live server, Chromium, and environment-specific baselines; they will hang or fail on a normal machine. `test:ci` is the honest "run everything" command, it is exactly what CI runs. +`npm test` is the same suite CI runs, so a green run locally means a green run there. It leaves out three suites that cannot pass on an arbitrary machine, each with its own command: + +```bash +npm run test:browser # Playwright + chromium (+ a live server; codex-predictive-echo needs a real codex binary) +npm run test:mobile # the above plus environment-specific PNG baselines +npm run test:perf # wall-clock benchmarks — run on an otherwise idle machine +npm run test:all # literally everything, environmental failures included +``` + +Expect `test:browser`/`test:mobile`/`test:perf` to fail where the machine cannot provide what they need; read that as "not runnable here", not as a regression. `config/test-suites.ts` holds the globs, and both configs derive from it, so the exclusions and those runners cannot drift apart. If you add a test that binds a port, pick a unique one at 3150 or above (search the repo for `const PORT =` first). Never 3000. diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 4378d627..d5cac857 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -87,7 +87,9 @@ jobs: fi - name: Run unit & integration tests - # Excludes the browser-driven mobile suite (test/mobile/**); see config/vitest.ci.config.ts. + # Excludes the suites that need chromium, per-machine PNG baselines or a + # quiet machine — see config/test-suites.ts for the list and the reason + # behind each entry. Identical to what `npm test` runs locally. # Safe in CI: TmuxManager no-ops all shell commands under VITEST (test/setup.ts). run: npm run test:ci @@ -99,6 +101,11 @@ jobs: run: npx vitest run working-directory: packages/xterm-zerolag-input -# Note: The browser-driven mobile suite (test/mobile/**) is excluded from CI — -# it needs a live server + chromium + environment-specific PNG baselines. -# Run it locally/manually. All other tests run via the `test` job above. +# Note: three suites are excluded from CI, each with its own local runner: +# npm run test:browser Playwright + chromium (+ a live server, and a real +# codex binary for codex-predictive-echo) +# npm run test:mobile the above plus environment-specific PNG baselines +# npm run test:perf wall-clock benchmarks; need an otherwise idle machine +# config/test-suites.ts holds the globs; the configs derive from it so the +# exclusions here and those runners cannot drift apart. Everything else runs in +# the `test` job above, which is the same thing `npm test` runs. diff --git a/AGENTS.md b/AGENTS.md index 37a36292..ed6638df 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -10,7 +10,7 @@ sections here. Quick pointers: - Type check: `tsc --noEmit` · Lint: `npm run lint` · Format: `npm run format:check` -- Targeted tests only: `npm test -- test/.test.ts` (bare `npm test` is unsafe in managed sessions) +- Tests: `npm test` (the CI gate, safe to run bare) or `npm test -- test/.test.ts` for one file - Route tests use `app.inject()`; new tests needing ports must pick a unique `const PORT =` - Branch off `master` for all work; Conventional Commit-style messages (`fix(mobile): ...`) - Never commit secrets or local state from `~/.codeman/` diff --git a/CLAUDE.md b/CLAUDE.md index 4ad1bd23..3bfa69f4 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -16,7 +16,7 @@ This file provides guidance to Claude Code (claude.ai/code) when working with co | Type check | `npm run typecheck` (= `tsc --noEmit`) | | Lint | `npm run lint` (fix: `npm run lint:fix`) | | Format | `npm run format` (check: `npm run format:check`) | -| Single test | `npm test -- test/.test.ts` (or `npx vitest run --config config/vitest.config.ts test/.test.ts`) — ⚠ **never** run bare `npm test`, see Testing section | +| Tests | `npm test` (the CI gate — safe to run bare) · one file: `npm test -- test/.test.ts` · see Testing for the excluded suites | | Build | `npm run build` (esbuild via `scripts/build.mjs`, NOT tsc — `tsc --noEmit` is type-check only) | | Production | `npm run build && systemctl --user restart codeman-web` | @@ -98,7 +98,7 @@ Codeman is a Claude Code session manager with web interface and autonomous Ralph | Override window title hostname | `npx tsx src/index.ts web --title-hostname ` (default: `os.hostname()` — `codeman:` is used for tab title, title-flash, and OS desktop notification prefix) | | Bind a non-loopback host | `npx tsx src/index.ts web --host 0.0.0.0` (or `-H`; env `CODEMAN_HOST`; default `127.0.0.1`). Without `CODEMAN_PASSWORD` it **starts but warns loudly** — see Common Gotchas + `docs/security-architecture.md` | | Continuous typecheck | `tsc --noEmit --watch` | -| Watch-mode test | `npm run test:watch -- test/.test.ts` (always pass a file — bare watch includes the browser suites) | +| Watch-mode test | `npm run test:watch -- test/.test.ts` (runs the CI gate's config; pass a file to narrow it) | | Test coverage | `npm run test:coverage` | | Dead-code sweep | `npm run knip` (config in `config/knip.json`, passed via `--config`) | | Rebuild gesture overlay | `npm run build:gesture` (esbuild `packages/gesture-control/src/codeman/entry.ts` → `src/web/public/gesture/gesture-codeman.js`; commit the result) | @@ -106,13 +106,13 @@ Codeman is a Claude Code session manager with web interface and autonomous Ralph | Gesture playground | `npm run dev` **in** `packages/gesture-control/` (standalone vite demo, fake tabs) | | Check public-asset formatting | `npm run check:public-assets` (prettier-checks `src/web/public/**` text assets; `scripts/check-public-assets.mjs`) | | Frontend JS syntax check | `npm run check:frontend-syntax` (`scripts/check-frontend-syntax.mjs`; runs in CI) | -| CI-equivalent test sweep | `npm run test:ci` (full suite minus browser/perf — see Testing) | +| Excluded-suite runners | `npm run test:browser` · `npm run test:mobile` · `npm run test:perf` · `npm run test:all` (everything, environmental failures included) — see Testing | | Production start | `npm run start` | | Production logs | `journalctl --user -u codeman-web -f` | | Detached server | `codeman web -d` (`--status`, `--stop`; pidfile+log at `dataPath('web.pid'/'web.log')`). ⚠ Refuses to start a 2nd server on one data dir — see Instance isolation | | Install/remove the service | `codeman service install` / `status` / `uninstall` (systemd user unit on Linux, LaunchAgent on macOS; names from `config/service-names.ts`) | -**CI**: `.github/workflows/ci.yml` (push to master/main + PRs, Node 22) runs two jobs: **(1)** `check:lockfile`, `typecheck`, `lint`, `check:frontend-syntax`, `format:check`, then a **server boot smoke test** (`tsx src/index.ts web --port 3151` must answer `/api/status` within 30s); **(2)** the **unit/integration test suite** via `npm run test:ci` (`config/vitest.ci.config.ts` — excludes the browser-driven `test/mobile/**` suite, `perf-*` benchmarks, and 3 Playwright tests). Tests are tmux-safe in CI: `TmuxManager` no-ops all shell commands under `VITEST` (see Testing). +**CI**: `.github/workflows/ci.yml` (push to master/main + PRs, Node 22) runs two jobs: **(1)** `check:lockfile`, `typecheck`, `lint`, `check:frontend-syntax`, `format:check`, then a **server boot smoke test** (`tsx src/index.ts web --port 3151` must answer `/api/status` within 30s); **(2)** the **unit/integration test suite** via `npm run test:ci` (`config/vitest.ci.config.ts` — excludes the browser-driven `test/mobile/**` suite, `perf-*` benchmarks, and 5 Playwright tests; globs live in `config/test-suites.ts`). `npm test` runs this same config, so local green == CI green. Tests are tmux-safe in CI: `TmuxManager` no-ops all shell commands under `VITEST` (see Testing). **Code style**: Prettier (`singleQuote: true`, `printWidth: 120`, `trailingComma: "es5"`) — config lives in the **`"prettier"` key of `package.json`**, not a `.prettierrc` (keeps the repo root short; editors read it natively). `.prettierignore` stays at the root because Prettier resolves it relative to cwd. ESLint flat config (`config/eslint.config.js`) allows `no-console`, warns on `@typescript-eslint/no-explicit-any`. Ignores: `app.js`, `scripts/**/*.mjs`, `src/web/public/vendor/**`, `scripts/remotion/**`. @@ -288,7 +288,7 @@ Frontend JS modules have `@fileoverview` with `@dependency`/`@loadorder` tags. L **Shell keyboard accessory bar + one-shot Ctrl** (issue #262, `keyboard-accessory.js`): a **shell**-mode session automatically swaps the mobile accessory bar for terminal controls (Ctrl, Esc, Tab, four arrows, paste, dismiss); every other mode keeps the agent bar. `setMode()` now records the user's `extendedKeyboardBar` preference as the **base** layout and `refreshForActiveSession()` (called from `selectSession`) resolves base-vs-shell, so a settings save during a shell session cannot yank the bar away and switching back restores the user's choice. ⚠️ **Ctrl is a ONE-SHOT modifier applied in `terminal.onData`, not in a keydown handler**: a virtual keyboard emits no usable key events, so the character only exists as onData text. The hook sits AFTER `shouldSuppressTerminalQueryResponse` (xterm answers DA/CPR through onData too, and one of those would silently spend the modifier) and BEFORE every send path, so the control byte follows the normal control-char route. ⚠️ **Not every onData chunk is a keystroke**, and the query filter is not enough on its own: xterm ALSO emits mouse and focus reports on its own initiative, so the hook skips them via `isTerminalFocusOrMouseReport()` (they still reach the PTY, they just don't count as the next key). The mouse half is live — a shell session keeps the NARROW strip, so mouse DECSETs reach the browser and one tap while vim/htop runs spent the armed modifier silently (measured). The focus half is defense in depth: `FOCUS_ESCAPE_FILTER` in `session.ts` strips `\x1b[?1004h` from every PTY read, so `sendFocusMode` never turns on today; if it ever did, the bar's own post-key refocus would emit `\x1b[I` and eat the modifier before the user typed. ⚠️ It must disarm on ALL of: use, second tap, any other accessory key, session switch, keyboard dismissal, and a layout swap; a modifier left armed turns the next innocent keystroke into a control byte. ⚠️ **onData is not the only input path** — with `cjkInputEnabled` on, the CJK textarea owns the keyboard (onData returns early for everything it swallows, and the focus router sends `terminal.focus()` there, which is where the bar refocuses after every key), so `_handleCjkInput()` applies the modifier too. It is that module's single choke point to the PTY, so one call covers typed characters, IME flushes, Enter, backspace and arrows. Without it an armed modifier could neither fire NOR be spent, and survived to a later keystroke. Mapping is `ctrlByteFor()` (`code & 0x1f` over @A-Z[\]^_ and a-z, plus Ctrl+Space=NUL / Ctrl+?=DEL); characters with no control equivalent pass through unchanged, like a hardware keyboard. ⚠️ The armed style is `.accessory-btn.accessory-btn-ctrl.armed` (0,3,0) in BOTH stylesheets, and it cannot outrank mobile.css's light-skin repaint at **(0,3,1)** (`:is()` inherits its most specific argument, and that list holds `.btn-toolbar.btn-shell`) — so that rule excludes the state by hand as `.accessory-btn:not(.armed)`. Without the exclusion the armed button renders identically to a resting one on all four light skins, which is worse than no armed style at all. -**Dismissing the on-screen keyboard** (PRs #279/#280, `terminal-ui.js`): the terminal parks focus on a hidden textarea that nothing used to release, so TWO gestures now blur it, and they own different regions. **(1)** `_installMobileKeyboardDismiss()` — a document-level `touchend` that fires only while the terminal input actually holds focus, **never inside `#terminalContainer`** (tap classification owns that) and **never on a control** (`MOBILE_KEYBOARD_DISMISS_EXEMPT_SELECTOR`, matched with `closest()` so an icon inside a button counts). Session tabs are covered by the selector's `[tabindex]:not([tabindex="-1"])` arm, which is what stops a tab tap from blurring and then being re-focused by `selectSession()`. **(2)** In `_handleMobileTerminalTap`, a second tap on **inert `content`** (`startedWithTerminalFocus`) blurs instead of re-focusing. ⚠️ Scoped to `content` on purpose: the prompt row (`input`) keeps focus-then-position so a second tap still places the caret, and actionable rows blur earlier via `_isActionableMobileTerminalTap`. ⚠️ **A scroll ends in `touchend` too** — dismissing there closes the keyboard and drops the composer mid-read, so travel is tracked from `touchstart` and multi-touch is never a tap. Both classifiers MUST share one threshold: `initTerminal`'s `TAP_THRESHOLD` reads `MOBILE_KEYBOARD_DISMISS_TAP_SLOP`, since a gesture the terminal calls a scroll and the dismiss handler calls a tap is exactly that bug. ⚠️ **`test:ci` excludes `test/mobile/**`, so CI cannot see the only test covering (1)** — run `npm test -- test/mobile/keyboard.test.ts` by hand and diff the FAIL list against master. That blind spot is why merging the two PRs, which conflicted semantically but not textually, produced a red suite with two green CI checks. +**Dismissing the on-screen keyboard** (PRs #279/#280, `terminal-ui.js`): the terminal parks focus on a hidden textarea that nothing used to release, so TWO gestures now blur it, and they own different regions. **(1)** `_installMobileKeyboardDismiss()` — a document-level `touchend` that fires only while the terminal input actually holds focus, **never inside `#terminalContainer`** (tap classification owns that) and **never on a control** (`MOBILE_KEYBOARD_DISMISS_EXEMPT_SELECTOR`, matched with `closest()` so an icon inside a button counts). Session tabs are covered by the selector's `[tabindex]:not([tabindex="-1"])` arm, which is what stops a tab tap from blurring and then being re-focused by `selectSession()`. **(2)** In `_handleMobileTerminalTap`, a second tap on **inert `content`** (`startedWithTerminalFocus`) blurs instead of re-focusing. ⚠️ Scoped to `content` on purpose: the prompt row (`input`) keeps focus-then-position so a second tap still places the caret, and actionable rows blur earlier via `_isActionableMobileTerminalTap`. ⚠️ **A scroll ends in `touchend` too** — dismissing there closes the keyboard and drops the composer mid-read, so travel is tracked from `touchstart` and multi-touch is never a tap. Both classifiers MUST share one threshold: `initTerminal`'s `TAP_THRESHOLD` reads `MOBILE_KEYBOARD_DISMISS_TAP_SLOP`, since a gesture the terminal calls a scroll and the dismiss handler calls a tap is exactly that bug. ⚠️ **The gate excludes `test/mobile/**`, so CI cannot see the only test covering (1)** — run `npm run test:mobile -- test/mobile/keyboard.test.ts` by hand and diff the FAIL list against master. (Not `npm test --`: the gate's config excludes that path, so a file filter pointing into it matches nothing and exits green having run zero tests.) That blind spot is why merging the two PRs, which conflicted semantically but not textually, produced a red suite with two green CI checks. **Phone toolbar: Enter replaces Shell** (post-1.8.0): inside `@media (max-width: 430px)` `btn-shell` is `display:none` and `btn-enter` takes its slot (`order: 4`); starting a shell moved into the Run dropdown (`Terminal / Shell` → `setRunMode('shell')` → `run()` → `runShell()`, button label "Run SH"). `runMode` is `z.string().max(20)` server-side, so new modes need no schema change. Desktop and tablet keep the green Run Shell button unchanged. @@ -356,18 +356,30 @@ All in `~/.codeman/`: `state.json` (sessions, settings, respawn, orchestrator, c ## Testing -**Never run the bare full suite** (`npm test` with no file argument): the default config includes the browser-driven suites (`test/mobile/**` and 3 other Playwright tests), which need a live server + chromium + environment-specific PNG baselines and will fail/hang locally. Run individual files, or `test:ci` for a broad sweep: +**`npm test` is the gate and is safe to run bare** — it runs `config/vitest.ci.config.ts`, exactly what CI runs, so local green means CI green. ```bash -npm test -- test/.test.ts # Single file (SAFE, uses config/vitest.config.ts) -npm test -- -t "pattern" # By name (SAFE) -npm run test:ci # Everything except browser/perf suites — what CI runs -# npm test # DON'T — includes browser/visual suites +npm test # The gate — what CI runs +npm test -- test/.test.ts # Single file +npm test -- -t "pattern" # By name ``` -Raw `npx vitest` skips `config/vitest.config.ts`; always use `npm test --` or pass `--config config/vitest.config.ts`. +Three suites are deliberately left out, because they cannot pass on an arbitrary machine. Each has its own runner, and a failure there means "not runnable here", not a regression: -**Config**: Vitest with `globals: true`, `fileParallelism: false`. Timeout 30s, teardown 60s. `config/vitest.ci.config.ts` = same minus the browser/perf excludes — keep the two configs in sync when changing shared options. +```bash +npm run test:browser # Playwright + chromium, live server; codex-predictive-echo also needs a real codex binary +npm run test:mobile # the above plus environment-specific PNG baselines (own config, own pretest vendor step) +npm run test:perf # wall-clock benchmarks — need an otherwise idle machine +npm run test:all # literally everything; fails ~87 tests on a clean master here, which is why it is not the default +``` + +⚠️ **`npm test` cannot see those suites**, so a change touching mobile/gesture/terminal-render behaviour needs the matching runner by hand — diff its FAIL list against master rather than reading it as pass/fail. That blind spot is what let two semantically-conflicting PRs merge green (see the on-screen-keyboard note above). + +⚠️ **A file filter must match the runner.** `npm test -- test/mobile/keyboard.test.ts` matches nothing and exits GREEN having run zero tests, because the gate's config excludes that path — an excluded file needs its own runner (`npm run test:mobile -- `, `npm run test:browser -- `, `npm run test:perf -- `). Vitest treats "no files matched a filter" as success, so read the file count, not just the colour. + +Raw `npx vitest` skips the config (and with it `setup.ts`); always use `npm test --` or pass `--config`. + +**Config**: Vitest with `globals: true`, `fileParallelism: false`. Timeout 30s, teardown 60s. `config/vitest.config.ts` is the everything-config behind `test:all`; `config/vitest.ci.config.ts` is the gate and derives its excludes from `config/test-suites.ts`, which is also what `vitest.browser.config.ts` and `vitest.perf.config.ts` derive their includes from — so the exclusions and the runners cannot drift apart. Keep shared options in sync across them. **Tmux safety**: under vitest (`VITEST` env var, set automatically), `TmuxManager` no-ops ALL shell commands and becomes a pure in-memory mock — tests physically cannot create/kill/attach real tmux sessions (`IS_TEST_MODE` in `src/tmux-manager.ts`). Every docker IO path is no-op'd the same way. `Session` is test-gated too: instead of attaching a real tmux client, it spawns a raw-mode echo PTY (`TEST_PTY_SCRIPT` in `src/session.ts`), so integration tests get a live input/output loop that echoes each byte exactly once. `test/setup.ts` gives every test file a temporary `HOME`/`USERPROFILE` (all `homedir()`-derived state, `~/.codeman` and `~/codeman-cases` included, resolves into a per-file fixture; the Playwright browser cache path is preserved), and additionally strips `CODEMAN_PASSWORD`/`CODEMAN_USERNAME` (so auth state from the running instance can't leak into tests) and `CODEMAN_GESTURE` (a shell-exported gesture flag would flip render-injection assertions). ⚠️ Raw `npx vitest` without `--config` skips `setup.ts` and with it the temp-HOME isolation. diff --git a/README.md b/README.md index 32a1c3ea..9847d979 100644 --- a/README.md +++ b/README.md @@ -1037,7 +1037,7 @@ flowchart TB npm install npx tsx src/index.ts web # Dev mode npm run build # Production build -npm run test:ci # Run tests (the CI suite; browser suites need extra setup) +npm test # Run tests (same suite CI runs; browser/mobile/perf suites have their own commands) ``` See [CLAUDE.md](./CLAUDE.md) for full documentation. diff --git a/README.zh-CN.md b/README.zh-CN.md index 38d34620..051db98a 100644 --- a/README.zh-CN.md +++ b/README.zh-CN.md @@ -937,7 +937,7 @@ flowchart TB npm install npx tsx src/index.ts web # 开发模式 npm run build # 生产构建 -npm run test:ci # 运行测试（CI 套件；浏览器套件需要额外环境） +npm test # 运行测试（与 CI 相同；浏览器/移动端/性能套件另有独立命令） ``` 完整文档见 [CLAUDE.md](./CLAUDE.md)。 diff --git a/config/test-suites.ts b/config/test-suites.ts new file mode 100644 index 00000000..a1effdc7 --- /dev/null +++ b/config/test-suites.ts @@ -0,0 +1,45 @@ +/** + * The test suites that `npm test` deliberately does NOT run, in one place. + * + * Why this file exists: the exclusion list used to live only in + * config/vitest.ci.config.ts, as literals. Anything excluded there was + * therefore reachable only by running the everything-config by hand and reading + * past its failures — and a newly excluded file was reachable by nothing at + * all, silently, because nothing pointed at it. Both configs now derive their + * globs from the arrays below, so adding a suite here puts it in exactly one + * runner and takes it out of exactly one gate. + * + * Adding a new test that cannot run in CI: put its glob in the array that + * describes WHY it cannot, not in whichever one is shortest. + */ + +/** + * Playwright-driven: needs chromium and, in most cases, a live Codeman server + * on a real port. Deterministic where the environment provides both, which is + * why these are a runnable suite (`npm run test:browser`) rather than skipped. + */ +export const BROWSER_TEST_GLOBS = [ + 'test/inline-rename.test.ts', + 'test/opencode-resize.test.ts', + 'test/webgl-fallback.test.ts', + 'test/terminal-copy-shortcut.test.ts', + 'test/codex-predictive-echo.test.ts', // also needs a real codex binary +]; + +/** + * Wall-clock benchmarks. They assert on durations, so a loaded shared runner + * fails them for reasons that have nothing to do with the diff under test. + */ +export const PERF_TEST_GLOBS = ['test/perf-*.test.ts']; + +/** + * Browser + visual regression: chromium AND environment-specific PNG baselines + * that are generated per machine. Has its own config + * (test/mobile/vitest.config.ts) because it needs serial execution, a longer + * timeout and the `pretest:mobile` vendor step — run it with + * `npm run test:mobile`, not through the configs here. + */ +export const MOBILE_TEST_GLOBS = ['test/mobile/**']; + +/** Everything `npm test` skips. */ +export const NON_CI_TEST_GLOBS = [...MOBILE_TEST_GLOBS, ...PERF_TEST_GLOBS, ...BROWSER_TEST_GLOBS]; diff --git a/config/vitest.browser.config.ts b/config/vitest.browser.config.ts new file mode 100644 index 00000000..062a2ea5 --- /dev/null +++ b/config/vitest.browser.config.ts @@ -0,0 +1,34 @@ +import { resolve } from 'node:path'; +import { defineConfig } from 'vitest/config'; +import { BROWSER_TEST_GLOBS } from './test-suites'; + +const root = resolve(import.meta.dirname, '..'); + +/** + * The Playwright-driven suite `npm test` skips — `npm run test:browser`. + * + * Needs chromium and, for most of these, a live Codeman server on a real port; + * codex-predictive-echo also needs a real codex binary. Expect failures where + * the machine cannot provide those, and read them as "not runnable here", not + * as a regression. + * + * The mobile suite is NOT here: it needs per-machine PNG baselines, serial + * execution and the `pretest:mobile` vendor step, so it keeps its own config + * (test/mobile/vitest.config.ts) behind `npm run test:mobile`. + * + * fileParallelism stays off for the same reason as every other config in this + * directory: these bind real ports and drive real tmux sessions, and two files + * doing that at once fail each other rather than the code. + */ +export default defineConfig({ + test: { + root, + globals: true, + environment: 'node', + include: BROWSER_TEST_GLOBS, + setupFiles: ['./test/setup.ts'], + fileParallelism: false, + testTimeout: 60000, + teardownTimeout: 60000, + }, +}); diff --git a/config/vitest.ci.config.ts b/config/vitest.ci.config.ts index b5bc31b9..0305838c 100644 --- a/config/vitest.ci.config.ts +++ b/config/vitest.ci.config.ts @@ -1,13 +1,17 @@ import { resolve } from 'node:path'; import { defineConfig, configDefaults } from 'vitest/config'; +import { NON_CI_TEST_GLOBS } from './test-suites'; const root = resolve(import.meta.dirname, '..'); /** - * CI test config — same as vitest.config.ts but EXCLUDES the browser-driven - * mobile suite (test/mobile/**). Those are Playwright visual-regression tests - * that need a live server + chromium + environment-specific PNG baselines, so - * they are run/maintained separately and are not part of the CI gate. + * The default gate — what `npm test` and CI both run. + * + * Same as vitest.config.ts but EXCLUDES the suites that cannot pass on an + * arbitrary machine: browser-driven (Playwright + chromium), visual-regression + * (per-machine PNG baselines) and wall-clock perf. Those are not unmaintained; + * they have their own runners (`test:browser`, `test:mobile`, `test:perf`). + * See config/test-suites.ts for the list and the reason behind each entry. * * Keep the rest in sync with config/vitest.config.ts. */ @@ -17,16 +21,7 @@ export default defineConfig({ globals: true, environment: 'node', include: ['test/**/*.test.ts'], - exclude: [ - ...configDefaults.exclude, - 'test/mobile/**', // browser/visual (Playwright + chromium) - 'test/perf-*.test.ts', // timing-sensitive perf benchmarks (flaky in CI) - 'test/inline-rename.test.ts', // browser (Playwright) - 'test/opencode-resize.test.ts', // browser (Playwright) - 'test/webgl-fallback.test.ts', // browser (Playwright) - 'test/terminal-copy-shortcut.test.ts', // browser (Playwright) - 'test/codex-predictive-echo.test.ts', // browser (Playwright) + real codex binary - ], + exclude: [...configDefaults.exclude, ...NON_CI_TEST_GLOBS], setupFiles: ['./test/setup.ts'], fileParallelism: false, testTimeout: 30000, diff --git a/config/vitest.config.ts b/config/vitest.config.ts index 48ecb9c2..578e6c4a 100644 --- a/config/vitest.config.ts +++ b/config/vitest.config.ts @@ -3,6 +3,17 @@ import { defineConfig } from 'vitest/config'; const root = resolve(import.meta.dirname, '..'); +/** + * EVERY test in the repo, including the ones that cannot pass on an arbitrary + * machine — `npm run test:all`. Reach for it when you want the complete picture + * and are prepared to read past environmental failures. + * + * This is NOT what `npm test` runs. On a machine without chromium, a free port + * or per-machine PNG baselines this config fails ~87 tests on a clean master, + * which makes it useless as a pass/fail signal: the default gate is + * config/vitest.ci.config.ts, and the suites it leaves out each have their own + * runner (`test:browser`, `test:perf`, `test:mobile`). See config/test-suites.ts. + */ export default defineConfig({ test: { root, diff --git a/config/vitest.perf.config.ts b/config/vitest.perf.config.ts new file mode 100644 index 00000000..2e526898 --- /dev/null +++ b/config/vitest.perf.config.ts @@ -0,0 +1,25 @@ +import { resolve } from 'node:path'; +import { defineConfig } from 'vitest/config'; +import { PERF_TEST_GLOBS } from './test-suites'; + +const root = resolve(import.meta.dirname, '..'); + +/** + * The wall-clock benchmarks `npm test` skips — `npm run test:perf`. + * + * These assert on durations, so run them on an otherwise idle machine: a loaded + * runner fails them for reasons that have nothing to do with the diff under + * test, which is exactly why they are not part of the default gate. + */ +export default defineConfig({ + test: { + root, + globals: true, + environment: 'node', + include: PERF_TEST_GLOBS, + setupFiles: ['./test/setup.ts'], + fileParallelism: false, + testTimeout: 60000, + teardownTimeout: 60000, + }, +}); diff --git a/package.json b/package.json index 17162489..7397b149 100644 --- a/package.json +++ b/package.json @@ -17,10 +17,13 @@ "dev": "tsx src/index.ts web", "web": "node dist/index.js web", "clean": "rm -rf dist", - "test": "vitest run --config config/vitest.config.ts", - "test:watch": "vitest --config config/vitest.config.ts", - "test:coverage": "vitest run --config config/vitest.config.ts --coverage", + "test": "vitest run --config config/vitest.ci.config.ts", + "test:watch": "vitest --config config/vitest.ci.config.ts", + "test:coverage": "vitest run --config config/vitest.ci.config.ts --coverage", "test:ci": "vitest run --config config/vitest.ci.config.ts", + "test:browser": "vitest run --config config/vitest.browser.config.ts", + "test:perf": "vitest run --config config/vitest.perf.config.ts", + "test:all": "vitest run --config config/vitest.config.ts", "pretest:mobile": "node scripts/prepare-test-vendor.mjs", "test:mobile": "vitest run --config test/mobile/vitest.config.ts", "check:frontend-syntax": "node scripts/check-frontend-syntax.mjs", diff --git a/test/test-suite-partition.test.ts b/test/test-suite-partition.test.ts new file mode 100644 index 00000000..1c76a8ef --- /dev/null +++ b/test/test-suite-partition.test.ts @@ -0,0 +1,100 @@ +/** + * @fileoverview The test runners must PARTITION the repo: every test file + * reachable by exactly one command, no file reachable by none. + * + * `npm test` deliberately skips three suites (browser, mobile, perf) because + * they cannot pass on an arbitrary machine. The failure mode that creates is + * silent: exclude a file from the gate, forget to add it to a runner, and it is + * now tested by nothing — with every command still green, because vitest treats + * "no files matched" as success. That is not hypothetical; the exclusion list + * lived as literals in one config for its whole life, and nothing pointed the + * other way. + * + * So the globs live in config/test-suites.ts, every config derives from them, + * and this file checks the arithmetic actually works out on the files on disk. + */ + +import { readdirSync } from 'node:fs'; +import { resolve } from 'node:path'; +import { describe, expect, it } from 'vitest'; +import { BROWSER_TEST_GLOBS, MOBILE_TEST_GLOBS, NON_CI_TEST_GLOBS, PERF_TEST_GLOBS } from '../config/test-suites'; + +const ROOT = resolve(import.meta.dirname, '..'); + +/** Every `*.test.ts` under test/, repo-relative, POSIX separators. */ +function allTestFiles(dir = 'test'): string[] { + const out: string[] = []; + for (const entry of readdirSync(resolve(ROOT, dir), { withFileTypes: true })) { + const rel = `${dir}/${entry.name}`; + if (entry.isDirectory()) out.push(...allTestFiles(rel)); + else if (entry.name.endsWith('.test.ts')) out.push(rel); + } + return out.sort(); +} + +/** + * Matches the three glob shapes test-suites.ts actually uses, and THROWS on + * anything else rather than quietly returning false — a glob this cannot read + * would otherwise make the partition below pass by mis-classifying it. + */ +function matches(glob: string, file: string): boolean { + if (glob.endsWith('/**')) return file.startsWith(glob.slice(0, -2)); + if (!glob.includes('*')) return file === glob; + const star = glob.indexOf('*'); + if (glob.indexOf('*', star + 1) !== -1) throw new Error(`unsupported glob (2+ wildcards): ${glob}`); + const [head, tail] = [glob.slice(0, star), glob.slice(star + 1)]; + if (tail.includes('/')) throw new Error(`unsupported glob (wildcard before a slash): ${glob}`); + return file.startsWith(head) && file.endsWith(tail) && !file.slice(head.length).includes('/'); +} + +const claims = (globs: string[], file: string) => globs.some((g) => matches(g, file)); + +describe('test suite partition', () => { + it('routes every test file to exactly one runner', () => { + const runners = { + 'test:browser': BROWSER_TEST_GLOBS, + 'test:mobile': MOBILE_TEST_GLOBS, + 'test:perf': PERF_TEST_GLOBS, + }; + + const orphaned: string[] = []; + const contested: string[] = []; + for (const file of allTestFiles()) { + const owners = Object.entries(runners) + .filter(([, globs]) => claims(globs, file)) + .map(([name]) => name); + // Not in any excluded suite == owned by the gate, which is correct and + // the common case. Only >1 excluded owner is a bug. + if (owners.length > 1) contested.push(`${file} -> ${owners.join(' + ')}`); + // An excluded file with no runner is the silent hole this file exists for. + if (owners.length === 0 && claims(NON_CI_TEST_GLOBS, file)) orphaned.push(file); + } + + expect(contested, 'a file claimed by two runners runs twice, or not at all').toEqual([]); + expect(orphaned, 'excluded from `npm test` but no runner picks it up — this file is tested by NOTHING').toEqual([]); + }); + + it('keeps NON_CI_TEST_GLOBS the union of the three excluded suites', () => { + // The gate excludes NON_CI_TEST_GLOBS; the runners include the three arrays. + // If the union drifts, the gate skips something no runner covers. + expect([...NON_CI_TEST_GLOBS].sort()).toEqual( + [...MOBILE_TEST_GLOBS, ...PERF_TEST_GLOBS, ...BROWSER_TEST_GLOBS].sort() + ); + }); + + it('names only globs the matcher above can actually read', () => { + // matches() throws on shapes it would otherwise silently mis-classify. + for (const glob of NON_CI_TEST_GLOBS) expect(() => matches(glob, 'test/x.test.ts')).not.toThrow(); + }); + + it('points every excluded glob at files that exist', () => { + // A stale entry (file renamed or deleted) makes its runner silently empty. + const files = allTestFiles(); + for (const glob of NON_CI_TEST_GLOBS) { + expect( + files.some((f) => matches(glob, f)), + `${glob} matches no test file` + ).toBe(true); + } + }); +});