fix(tests): #556 landing fixes

Move test/sse-tile-grid-filter.test.ts to an ephemeral port. The release
added it with #561 on fixed port 3287, after #556 was cut, so it is not on
the guard's LEGACY_FIXED_PORT_FILES and test/test-ports-guard.test.ts failed
on the merged tree. It now builds new WebServer(0, ...) and its url() helper
reads server.boundPort (only ever called inside tests, after beforeAll).
Converting it is preferred over listing it, since the legacy list is
shrink-only.

Update the five docs that still told contributors to pick a unique fixed
port, which the new guard now rejects for any WebServer test: CLAUDE.md
(Adding Features and Testing), AGENTS.md, .github/CONTRIBUTING.md and the
wiki's Contributing page (mirrored to the public GitHub wiki). They now say
to bind port 0 and read boundPort (or address().port for a raw server), and
note that the mobile suite keeps its fixed ports for now, because
test/mobile/helpers/server.ts caches servers by port, so createTestServer(0)
from two callers would share one server.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
This commit is contained in:
Codeman maintainer
2026-10-09 04:48:43 +02:00
parent 0f5613ba46
commit c2340ae88a
5 changed files with 12 additions and 10 deletions
+1 -1
View File
@@ -55,7 +55,7 @@ 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.
If you add a test that binds a port, bind port 0 (`new WebServer(0, …)` + `server.boundPort`, or `listen({ port: 0 })` + `address().port`), or use `app.inject()` when no socket is needed; `test/test-ports-guard.test.ts` fails a `WebServer` built on any other port. Mobile tests (`test/mobile/**`, via `createTestServer(PORT)`) keep the fixed-port convention in `test/mobile/README.md` for now, because that helper caches servers by port. Never 3000.
Tests are tmux-safe by design: under vitest, the tmux layer becomes an in-memory mock, so tests cannot touch real sessions.
+1 -1
View File
@@ -12,6 +12,6 @@ Quick pointers:
- Type check: `tsc --noEmit` · Lint: `npm run lint` · Format: `npm run format:check`
- Tests: `npm test` (the CI gate, safe to run bare) or `npm test -- test/<file>.test.ts` for one file
- Route tests use `app.inject()`; new tests needing ports must pick a unique `const PORT =`
- Route tests use `app.inject()`; new tests needing a socket bind port 0 (`new WebServer(0, …)` + `boundPort`), except mobile tests, which keep `createTestServer(PORT)` for now
- Branch off `master` for all work; Conventional Commit-style messages (`fix(mobile): ...`)
- Never commit secrets or local state from `~/.codeman/`
+2 -2
View File
@@ -436,7 +436,7 @@ One module per domain in `src/web/routes/` (plus a barrel; `ls src/web/routes/`
- **App setting**: decide per-device vs synced first. Per-device keys go in the `displayKeys` set in settings-ui.js and must NOT be added to `SettingsUpdateSchema` (it is `.strict()`). ⚠️ Anything in `PUT /api/settings` that acts on a setting (the `toggleService` watcher calls) must resolve from **`merged`** (persisted + incoming), never from the raw request body: a partial PUT omits keys it doesn't intend to change, and `body.x ?? default` turns every omission into "apply the default" and silently resets live services. Pinned by `test/routes/system-routes-settings-partial-put.test.ts`.
- **Hook event**: Add to `HookEventType`, add hook in `hooks-config.ts:generateHooksConfig()`, update `HookEventSchema`
- **Mobile feature**: Add to relevant singleton, guard with `MobileDetection.isMobile()`. New header buttons must stay off phones (`test/mobile-header-buttons-policy.test.ts`).
- **New test**: Pick unique port (search `const PORT =`). Route tests use `app.inject()` (no port needed) — see `test/routes/_route-test-utils.ts`.
- **New test**: a test that constructs `WebServer` itself binds port 0 (`new WebServer(0, …)`, then `server.boundPort`; enforced by `test/test-ports-guard.test.ts`), and a raw Fastify/`ws` server listens on `port: 0` and reads `address().port`. Mobile tests (`test/mobile/**`, via `createTestServer(PORT)`) keep the fixed-port convention in `test/mobile/README.md` until the follow-up sweep, because that helper caches servers by port. Route tests use `app.inject()` (no port needed); see `test/routes/_route-test-utils.ts`.
**Validation**: Zod v4 (different API from v3). Define schemas in `schemas.ts`, use `.parse()`/`.safeParse()`.
@@ -475,7 +475,7 @@ Raw `npx vitest` skips the config (and with it `setup.ts`); always use `npm test
**Tmux safety**: under vitest (`VITEST`), `TmuxManager` no-ops ALL shell commands (`IS_TEST_MODE` in `src/tmux-manager.ts`), docker IO is no-op'd likewise, and `Session` spawns an echo PTY (`TEST_PTY_SCRIPT`) instead of attaching tmux. `test/setup.ts` gives each file a temp `HOME`/`USERPROFILE` and strips `CODEMAN_PASSWORD`/`CODEMAN_USERNAME`, `CODEMAN_GESTURE` and `CODEMAN_INSTANCE`/`CODEMAN_DATA_DIR`/`CODEMAN_TMUX_SOCKET` (pinned by `test/test-env-isolation.test.ts`). ⚠️ `CODEMAN_DATA_DIR` overrides the temp HOME, so never drop its strip; strip `CODEMAN_INSTANCE` in the setup file, never in a hook (captured at first import). ⚠️ Delete case trees only via `safeRmHomeTree()`. ⚠️ Raw `npx vitest` without `--config` skips `setup.ts` and its isolation. → [architecture-invariants#test-isolation-tmux-docker-and-home](docs/architecture-invariants.md#test-isolation-tmux-docker-and-home)
**Ports**: Pick unique ports manually, 3150+. Search `const PORT =` before adding new tests. Never 3000 (the live instance).
**Ports**: a test that constructs `WebServer` binds port 0 and reads `server.boundPort`; `test/test-ports-guard.test.ts` fails any other port outside its shrink-only legacy list. A raw Fastify/`ws` server listens on `port: 0` and reads `address().port`. The mobile suite keeps fixed ports (3150+, per `test/mobile/README.md`) until the follow-up sweep, since `createTestServer()` caches servers by port. Never 3000 (the live instance).
⚠️ **Browser tests can pass vacuously on mobile input paths.** Two traps, both hit on 2026-07-27 while fixing the phone Enter button: **(1)** driving input with `app.sendInput('…')` writes PAST the `LocalEchoOverlay`, so `pendingText` stays empty and any overlay bug is invisible — type with `page.keyboard.type()` instead; **(2)** headless Chromium reports `MobileDetection.isTouchDevice()` **false even with `hasTouch: true`**, so `_localEchoEnabled` is off and the local-echo branch never executes. Force it (`app._localEchoEnabled = true`) or the test proves nothing. Assert on real state (`app._localEchoOverlay.pendingText`, plus `tmux -L codeman capture-pane -p -t <pane>` for what actually reached the PTY), not on HTTP 200.
+5 -2
View File
@@ -61,8 +61,11 @@ benchmarks for an otherwise idle machine). Expect those to fail where the machin
provide what they need; that means "not runnable here", not a regression.
Tests are tmux-safe by design: under vitest the tmux layer becomes an in-memory mock, so
tests cannot touch real sessions. If you add a test that binds a port, pick a unique one at
3150 or above, and never 3000.
tests cannot touch real sessions. If you add a test that binds a port, bind port 0
(`new WebServer(0, …)` + `server.boundPort`, or `listen({ port: 0 })` + `address().port`),
or use `app.inject()` when no socket is needed. Never 3000. Mobile tests (`test/mobile/**`,
via `createTestServer(PORT)`) keep the fixed ports in `test/mobile/README.md` for now,
because that helper caches servers by port.
## Finding your way around
+3 -4
View File
@@ -1,6 +1,6 @@
/**
* @fileoverview The server's side of the tile grid's SSE filter (live server,
* multi-user mode, port 3287).
* multi-user mode, ephemeral port).
*
* While tiles own the terminal the page subscribes with TILE_GRID_SSE_FILTER
* (constants.js), an id that names no session, so the server sends it no
@@ -31,8 +31,7 @@ import { createUser, invalidateUsersCache } from '../src/user-store.js';
vi.spyOn(TmuxManager, 'isTmuxAvailable').mockReturnValue(true);
const PORT = 3287;
const url = (p: string) => `http://localhost:${PORT}${p}`;
const url = (p: string) => `http://localhost:${server.boundPort}${p}`;
const basic = (u: string, p: string) => 'Basic ' + Buffer.from(`${u}:${p}`).toString('base64');
const alice = { Authorization: basic('alice', 'alicepass1') };
@@ -124,7 +123,7 @@ beforeAll(async () => {
await createUser({ username: 'root', role: 'admin', password: 'rootpass123' });
await createUser({ username: 'alice', role: 'user', password: 'alicepass1' });
await createUser({ username: 'bob', role: 'user', password: 'bobpass1234' });
server = new WebServer(PORT, false, true);
server = new WebServer(0, false, true);
await server.start();
});