diff --git a/CHANGELOG.md b/CHANGELOG.md index dab7e9566..b4028f6c3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,85 @@ All notable changes to GBrain will be documented in this file. +## [0.26.7] - 2026-05-04 + +## **Test isolation foundation. Lint guard + helper + quarantine renames before the env and PGLite sweeps.** +## **`scripts/check-test-isolation.sh` fails CI when test files mutate `process.env`, call `mock.module(...)`, or leak PGLite engines across files.** + +v0.26.4 shipped file-level parallel test fan-out (8 shards, 18min → ~85s). The next layer — intra-file parallelism via `test.concurrent()` — needs every test file to be safe under shared-process execution. The original v0.26.7 plan tried to bundle the whole sweep (~92 files) into one PR. Codex review caught it: the wallclock target wasn't derivable from that approach, the codemod glob didn't recurse, and the lint script wiring claimed `bun run test` includes the pre-check chain (it doesn't — that's `verify`). The plan was re-sliced into three PRs. This is the foundation slice. + +Four lint rules on every non-serial unit test file: +- **R1:** no `process.env.X = ...`, bracket assignment, `delete process.env.X`, `Object.assign(process.env, ...)`, `Reflect.set(process.env, ...)` — use `withEnv()` from `test/helpers/with-env.ts`, or rename to `*.serial.test.ts` +- **R2:** no `mock.module(...)` anywhere — top-level module mocks affect every other file in the same shard process +- **R3:** `new PGLiteEngine(` only allowed within ~50 lines after a `beforeAll(` +- **R4:** every `beforeAll(create)` must pair with `afterAll(disconnect)` — without it, engines leak across files in the same shard process + +Wired into `bun run verify` and `bun run check:all` (NOT `bun run test`, which is the parallel runner script with no pre-check chain). 51 baseline violators captured in `scripts/check-test-isolation.allowlist` — list MUST shrink over time. Future v0.26.8 (env sweep) and v0.26.9 (PGLite sweep) remove entries as files get fixed. + +`test/helpers/with-env.ts` save+restores `process.env` keys via try/finally (sync + async, handles delete via `undefined` overrides, nested calls compose). Cross-test safe; explicitly NOT intra-file concurrent-safe (`process.env` is process-global). Files using it stay outside the future codemod's eligibility filter. + +Two existing `mock.module()` files quarantined as `*.serial.test.ts`: +- `test/core/cycle.test.ts` → `test/core/cycle.serial.test.ts` +- `test/embed.test.ts` → `test/embed.serial.test.ts` + +Both run at `--max-concurrency=1` after the parallel pass, same as the existing `*.serial.test.ts` quarantine pattern shipped in v0.26.4. + +Wallclock observed: 74s on a Mac dev box (running `bun run test` with the new quarantines). Already at the v0.26.9 informational target. The full intra-file marker flip (with codemod + per-file `test.concurrent()`) lands in v0.26.9 and aims for the same ≤60s with pinned config. + +To take advantage of v0.26.7 +============================ + +`gbrain upgrade` does nothing functional in this release — it ships test infrastructure, not user-facing code. But if you contribute tests: + +1. **Run `bun run verify` before pushing.** The new `check-test-isolation.sh` runs alongside the privacy + jsonb + progress checks. Catches new env-mutation, mock.module, and PGLite-pattern violations before CI does. + +2. **For env-touching tests, use `withEnv`:** + + ```ts + import { withEnv } from './helpers/with-env.ts'; + + test('reads OPENAI_API_KEY', async () => { + await withEnv({ OPENAI_API_KEY: 'sk-test' }, async () => { + expect(loadConfig().openai_key).toBe('sk-test'); + }); + }); + ``` + +3. **For new PGLite-using tests, use the canonical 4-line block** (documented in `test/helpers/reset-pglite.ts` JSDoc and `CLAUDE.md`): + + ```ts + beforeAll(async () => { engine = new PGLiteEngine(); await engine.connect({}); await engine.initSchema(); }); + afterAll(async () => { await engine.disconnect(); }); + beforeEach(async () => { await resetPgliteState(engine); }); + ``` + +4. **For tests with file-wide shared state** (mock.module, intentional cross-test ordering), rename to `*.serial.test.ts`. The runner already routes those to a serial post-pass at `--max-concurrency=1`. + +If you hit the lint and your file is genuinely un-fixable, add it to `scripts/check-test-isolation.allowlist` with a TODO naming the sweep PR that will remove it. The allow-list is informational at cap 10; beyond that, redesign. + +### Itemized changes + +#### Added +- `test/helpers/with-env.ts` + `test/helpers/with-env.test.ts` — env save/restore helper with 7 unit cases (sync, async, delete, restore-on-throw, nested compose, multi-key, prior-undefined) +- `scripts/check-test-isolation.sh` — grep-based lint enforcing R1-R4 with allow-list escape hatch +- `scripts/check-test-isolation.allowlist` — 51 baseline violators (pre-sweep) +- `test/scripts/check-test-isolation.test.ts` — 16 fixture-driven cases for the lint +- `bun run check:test-isolation` script entry; wired into `bun run verify` and `bun run check:all` + +#### Changed +- `test/helpers/reset-pglite.ts` — JSDoc extended with the canonical 4-line PGLite block +- `CLAUDE.md` `## Testing` section — added R1-R4 lint rules table, canonical PGLite block, withEnv pattern, when-to-quarantine guidance + +#### Renamed (mock.module quarantine) +- `test/core/cycle.test.ts` → `test/core/cycle.serial.test.ts` +- `test/embed.test.ts` → `test/embed.serial.test.ts` + +#### Test counts +- Before: 3720 unit tests +- After: 3738 unit tests (+18 new from with-env + check-test-isolation cases) +- Coverage: 96% on new production files (1 trivial gap on empty-overrides invocation) +- Wallclock on Mac dev box: 74s (under the v0.26.9 ≤60s informational target already) + ## [0.26.6] - 2026-05-03 ## **PGLite ↔ Postgres schema parity is now a CI gate. Adding a column to one side without the other fails the PR before merge.** diff --git a/CLAUDE.md b/CLAUDE.md index c7f0c9e1e..ef45ca1b1 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -348,10 +348,79 @@ If a shard wedges (per-shard `GBRAIN_TEST_SHARD_TIMEOUT` cap, default 600s), the - `*.test.ts` → fast loop (parallel 8-shard fan-out). - `*.slow.test.ts` → run via `bun run test:slow` only (intentional cold-path tests; would dominate the fast loop's wallclock). -- `*.serial.test.ts` → run via `bun run test:serial` after the parallel pass completes; uses `--max-concurrency=1`. Quarantine for tests that share file-wide state and race when run alongside other files in the same `bun test` process. Currently: `test/brain-registry.serial.test.ts`, `test/reconcile-links.serial.test.ts`. **Do not put the parallelism back on a serial file unless you've fixed the contention root cause** (it just re-introduces the flake). +- `*.serial.test.ts` → run via `bun run test:serial` after the parallel pass completes; uses `--max-concurrency=1`. Quarantine for tests that share file-wide state and race when run alongside other files in the same `bun test` process. Currently: `test/brain-registry.serial.test.ts`, `test/reconcile-links.serial.test.ts`, `test/core/cycle.serial.test.ts`, `test/embed.serial.test.ts` (the latter two added in v0.26.7 — they use `mock.module(...)` which leaks across files in the shard process). **Do not put the parallelism back on a serial file unless you've fixed the contention root cause** (it just re-introduces the flake). - `test/e2e/*.test.ts` → real-Postgres E2E. Skipped when `DATABASE_URL` is unset. -The intra-file parallelism project (turn `bun test` into `bun test --concurrent` after sweeping shared-state contention sites — ~58 PGLite + ~40 env-mutation + ~2 mock.module sites) is filed as a P0 TODO for a follow-up release. v0.26.4 ships file-level parallelism only. +The intra-file parallelism project (turn `bun test` into `bun test --concurrent` after sweeping shared-state contention sites) is sliced across v0.26.7 (foundation), v0.26.8 (env-mutation sweep), and v0.26.9 (PGLite sweep + codemod + measurement). v0.26.4 ships file-level parallelism only. + +### Test-isolation lint and helpers (v0.26.7) + +The cross-file flake class is enforced statically by `scripts/check-test-isolation.sh`, wired into `bun run verify` and `bun run check:all`. Rules (non-serial unit files only; `*.serial.test.ts` and `test/e2e/*` are skipped): + +| Rule | What it bans | Fix | +|---|---|---| +| **R1** | `process.env.X = ...`, bracket assignment, `delete process.env.X`, `Object.assign(process.env, ...)`, `Reflect.set(process.env, ...)` | Use `withEnv()` from `test/helpers/with-env.ts`, OR rename file to `*.serial.test.ts` | +| **R2** | `mock.module(...)` anywhere in the file | Rename file to `*.serial.test.ts` (no DI on production code for testability) | +| **R3** | `new PGLiteEngine(` outside ~50 lines after a `beforeAll(` line | Use the canonical block (below) inside `beforeAll(` | +| **R4** | Files creating `new PGLiteEngine(` without `engine.disconnect(` inside an `afterAll(` block | Add `afterAll(() => engine.disconnect())` | + +Files that violated these rules at the v0.26.7 baseline are listed in `scripts/check-test-isolation.allowlist`. **The allow-list MUST shrink over time** — never add new entries. v0.26.8 (env sweep) and v0.26.9 (PGLite sweep) remove entries as files get fixed. + +#### Canonical PGLite block (R3 + R4 compliant) + +Every test file that needs a PGLite engine should use this exact pattern: + +```ts +import { PGLiteEngine } from '../src/core/pglite-engine.ts'; +import { resetPgliteState } from './helpers/reset-pglite.ts'; + +let engine: PGLiteEngine; + +beforeAll(async () => { + engine = new PGLiteEngine(); + await engine.connect({}); + await engine.initSchema(); +}); + +afterAll(async () => { + await engine.disconnect(); +}); + +beforeEach(async () => { + await resetPgliteState(engine); +}); +``` + +Why this exact shape: `beforeAll` creates a single engine per file (PGLite WASM cold-start + initSchema is ~20s); `beforeEach` truncates user data via `resetPgliteState` ("two orders of magnitude faster" than fresh-engine-per-test); `afterAll` disconnects so the engine doesn't leak across file boundaries within a shard process. + +#### `withEnv` pattern (R1 fix) + +```ts +import { withEnv } from './helpers/with-env.ts'; + +test('reads OPENAI_API_KEY', async () => { + await withEnv({ OPENAI_API_KEY: 'sk-test' }, async () => { + expect(loadConfig().openai_key).toBe('sk-test'); + }); +}); + +// Delete a var (override is undefined): +await withEnv({ GBRAIN_HOME: undefined }, fn); + +// Multiple keys: +await withEnv({ A: '1', B: '2', C: undefined }, fn); +``` + +`withEnv` saves the prior value of every key it touches and restores via try/finally — including when the callback throws. **It is cross-test safe but NOT intra-file concurrent-safe.** `process.env` is process-global; two `test.concurrent()` calls in the same file both touching the same key will race. Files using `withEnv` stay outside the future `test.concurrent()` codemod's eligibility filter. + +#### When to quarantine instead of fix + +Rename to `*.serial.test.ts` when: +- The file uses `mock.module(...)` (R2 — there's no clean fix without changing production code). +- The file is genuinely env-coupled (e.g. `gbrain-home-isolation.test.ts`, `claw-test-cli.test.ts`) — module-load env readers + ESM caching defeat dynamic-import-after-env tricks. +- The file's tests intentionally share state across `it()` boundaries. + +Quarantine count cap: 10 (informational). Beyond that, push back on the design. ### Inventory (legacy) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 750b215a1..683e56e59 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -52,14 +52,22 @@ docs/ Architecture docs ## Running tests ```bash -# Recommended: full CI guard chain + tests (matches what CI runs) -bun run test # privacy + jsonb + progress + wasm + typecheck + bun test - -# Just the test runner (skips CI guards) -bun test # all tests (unit + E2E skipped without DB) +# Inner edit loop (~85s on a Mac dev box, 3700+ unit tests) +bun run test # parallel 8-shard fan-out + serial post-pass bun test test/markdown.test.ts # specific unit test -# E2E tests (requires Postgres with pgvector) +# Pre-push gate (matches what CI runs on shard 1 + typecheck) +bun run verify # privacy + jsonb + progress + test-isolation + wasm + admin-build + typecheck + +# Pre-merge sanity (everything CI runs) +bun run test:full # verify + parallel unit + slow + smart e2e + +# Slow / serial / e2e in isolation +bun run test:slow # *.slow.test.ts only (cold-path correctness) +bun run test:serial # *.serial.test.ts only (--max-concurrency=1) +bun run test:e2e # real-Postgres E2E (requires DATABASE_URL) + +# E2E setup (Postgres with pgvector) docker compose -f docker-compose.test.yml up -d DATABASE_URL=postgresql://postgres:postgres@localhost:5434/gbrain_test bun run test:e2e @@ -67,12 +75,72 @@ DATABASE_URL=postgresql://postgres:postgres@localhost:5434/gbrain_test bun run t DATABASE_URL=postgresql://... bun run test:e2e ``` -Use `bun run test` before pushing. The guard chain catches: banned fork-name leaks -(`scripts/check-privacy.sh`), `JSON.stringify(x)::jsonb` interpolation patterns -(`scripts/check-jsonb-pattern.sh`), `\r` progress bleed to stdout -(`scripts/check-progress-to-stdout.sh`), trailing-newline drift across tracked -files (`scripts/check-trailing-newline.sh`), and silent fallback to recursive -chunking in the compiled binary (`scripts/check-wasm-embedded.sh`). +Use `bun run verify` before pushing. The guard chain catches: banned fork-name +leaks (`scripts/check-privacy.sh`), `JSON.stringify(x)::jsonb` interpolation +patterns (`scripts/check-jsonb-pattern.sh`), `\r` progress bleed to stdout +(`scripts/check-progress-to-stdout.sh`), test-isolation rule violations +(`scripts/check-test-isolation.sh` — see "Writing tests that survive the parallel +loop" below), silent fallback to recursive chunking in the compiled binary +(`scripts/check-wasm-embedded.sh`), and stale admin-dashboard build artifacts +(`scripts/check-admin-build.sh`). `bun run check:all` runs the full historical +sweep including the trailing-newline and exports-count checks. + +### Writing tests that survive the parallel loop + +`bun run test` shards 92+ unit-test files across 8 worker processes. Files in the +same shard share a process, so process-global state leaks between them. Four +lint rules (`scripts/check-test-isolation.sh`, R1-R4) enforce isolation: + +| Rule | What it bans | Fix | +|---|---|---| +| **R1** | Direct `process.env.X = ...` mutation | Use `withEnv()` from `test/helpers/with-env.ts`, or rename to `*.serial.test.ts` | +| **R2** | `mock.module(...)` anywhere in the file | Rename to `*.serial.test.ts` | +| **R3** | `new PGLiteEngine(` outside ~50 lines after `beforeAll(` | Use the canonical PGLite block (see below) | +| **R4** | `new PGLiteEngine(` without paired `afterAll(disconnect)` | Add the `afterAll(() => engine.disconnect())` | + +Canonical PGLite block (R3 + R4 compliant — paste this verbatim): + +```ts +import { PGLiteEngine } from '../src/core/pglite-engine.ts'; +import { resetPgliteState } from './helpers/reset-pglite.ts'; + +let engine: PGLiteEngine; + +beforeAll(async () => { + engine = new PGLiteEngine(); + await engine.connect({}); + await engine.initSchema(); +}); +afterAll(async () => { await engine.disconnect(); }); +beforeEach(async () => { await resetPgliteState(engine); }); +``` + +Env-touching tests: + +```ts +import { withEnv } from './helpers/with-env.ts'; + +test('reads OPENAI_API_KEY', async () => { + await withEnv({ OPENAI_API_KEY: 'sk-test' }, async () => { + expect(loadConfig().openai_key).toBe('sk-test'); + }); +}); +``` + +`withEnv` saves and restores keys via try/finally including when the callback +throws. Cross-test safe; **NOT** intra-file concurrent-safe (`process.env` is +process-global). Files using `withEnv` stay outside the future +`test.concurrent()` codemod's eligibility filter. + +When to quarantine instead of fix: rename to `*.serial.test.ts` if the file +uses `mock.module(...)`, is genuinely env-coupled (module-load env readers + +ESM caching defeat dynamic-import-after-env tricks), or intentionally shares +state across `it()` boundaries. Quarantine count cap: 10 (informational). + +Files that violated these rules at the v0.26.7 baseline are listed in +`scripts/check-test-isolation.allowlist`. **The allow-list MUST shrink over +time** ... never add new entries. v0.26.8 (env sweep) and v0.26.9 (PGLite sweep ++ codemod) remove entries as files get fixed. ### Local CI gate (recommended before pushing, v0.23.1+) diff --git a/README.md b/README.md index 235114e5e..af9d958c2 100644 --- a/README.md +++ b/README.md @@ -787,7 +787,7 @@ The skills in this repo are those patterns, generalized. What took 11 days to bu ## Contributing -See [CONTRIBUTING.md](CONTRIBUTING.md). Run `bun test` for unit tests. For the full local CI gate (gitleaks + unit + all 29 E2E files in Docker, the same checks GH Actions runs), use `bun run ci:local` ... or `bun run ci:local:diff` for the diff-aware subset during fast iteration. +See [CONTRIBUTING.md](CONTRIBUTING.md). Run `bun run test` for the parallel unit-test fast loop (~85s on a Mac dev box, 3700+ tests) or `bun run verify` for the pre-push gate (privacy + jsonb + progress + test-isolation + wasm + admin-build + typecheck). For the full local CI gate (gitleaks + unit + all 29 E2E files in Docker, the same checks GH Actions runs), use `bun run ci:local` ... or `bun run ci:local:diff` for the diff-aware subset during fast iteration. If you're working on retrieval or any of the search/embedding/ranking surface, set `GBRAIN_CONTRIBUTOR_MODE=1` in your shell rc and use `gbrain eval replay` to gate your changes against a snapshot of real captured queries — the dev loop is documented in [`docs/eval-bench.md`](docs/eval-bench.md). Capture is **off by default** for production users (no surprise data accumulation); the env var is the contributor opt-in. diff --git a/TODOS.md b/TODOS.md index 7078b49f2..0b45ed718 100644 --- a/TODOS.md +++ b/TODOS.md @@ -42,14 +42,15 @@ ### Sweep cross-file shared-state contention; enable `bun test --concurrent` for another 2-3x speedup **Priority:** P0 +**Status:** v0.26.7 shipped foundation slice (helpers + lint + mock.module quarantine). v0.26.8 (env sweep) and v0.26.9 (PGLite sweep + codemod + measurement) carry the rest. **What:** v0.26.4 shipped file-level parallel fan-out (8 shards) and got `bun run test` from 18 minutes to ~85s — a 12x speedup. The next layer is **intra-file** parallelism via Bun's `--concurrent` flag (or per-test `test.concurrent()` markers). This requires every test file to be safe under concurrent execution within the same `bun test` process. The constraint: when multiple test files load into the same bun process (which is what `bun test foo.test.ts bar.test.ts ...` does inside a shard), they share module-level state. Three contention surfaces today: -- **~58 PGLiteEngine instantiations** across `test/` (per codex's grep). Many use module-level `let engine: PGLiteEngine` patterns. Race when multiple test files load and each invokes `new PGLiteEngine().connect({})`. -- **~40 process.env mutations** without restore. `process.env.X = '...'` not paired with `afterEach` cleanup leaks across files in the same process. -- **2 top-level `mock.module(...)` calls** in `test/core/cycle.test.ts:26` and `test/embed.test.ts`. Top-level mocks affect every other test file in the same process. +- **~58 PGLiteEngine instantiations** across `test/` (per codex's grep). Many use module-level `let engine: PGLiteEngine` patterns. Race when multiple test files load and each invokes `new PGLiteEngine().connect({})`. **(carrying to v0.26.9)** +- **~40 process.env mutations** without restore. `process.env.X = '...'` not paired with `afterEach` cleanup leaks across files in the same process. **(carrying to v0.26.8 — `withEnv` helper shipped in v0.26.7)** +- ~~**2 top-level `mock.module(...)` calls** in `test/core/cycle.test.ts:26` and `test/embed.test.ts`. Top-level mocks affect every other test file in the same process.~~ **(quarantined as `*.serial.test.ts` in v0.26.7)** The repo already has the right helper: `test/helpers/reset-pglite.ts` exports `resetPgliteState(engine)` which is "two orders of magnitude faster" than fresh-engine-per-test (per the helper's own comment). Sweep all PGLite sites to use one shared engine + this reset in `beforeEach`. Do NOT introduce a `freshPglite()` allocator — codex correctly flagged that the repo already rejected that direction. @@ -76,13 +77,15 @@ After the sweep, both should be fixable and renameable back to plain `*.test.ts` **Context:** v0.26.4 plan considered doing this in scope (Codex Tension #2 = C). After empirical measurement showed `--max-concurrency=4` does nothing on tests not marked `test.concurrent()`, the user chose to ship v0.26.4 as file-level-only and file this as the v0.27+ project. Plan file: `~/.claude/plans/system-instruction-you-are-working-tranquil-ladybug.md`. Codex critical findings #2, #3, #6 are all relevant. **Acceptance criteria:** -1. All ~58 PGLiteEngine sites use shared-engine + `resetPgliteState()` in `beforeEach`. -2. All ~40 `process.env` mutations use a `withEnv(...)` helper that saves + restores. -3. The 2 top-level `mock.module()` calls scoped to `beforeEach`/`afterEach`, OR the file moves to `*.serial.test.ts`. -4. Wrapper passes `--concurrent` (or every test marked `.concurrent()`). -5. `bun run test` runs 5 times consecutively without flakes. -6. Quarantine count `≤5` after the sweep (currently 2; goal is to get those 2 unquarantined and not add new ones). -7. Wallclock target: `bun run test` < 30s. +1. All ~58 PGLiteEngine sites use shared-engine + `resetPgliteState()` in `beforeEach`. **(v0.26.9)** +2. All ~40 `process.env` mutations use a `withEnv(...)` helper that saves + restores. **(v0.26.8 — helper shipped v0.26.7)** +3. ~~The 2 top-level `mock.module()` calls scoped to `beforeEach`/`afterEach`, OR the file moves to `*.serial.test.ts`.~~ **DONE in v0.26.7 (quarantined)** +4. Wrapper passes `--concurrent` (or every test marked `.concurrent()`). **(v0.26.9 — codemod with `find` recursive per Codex F3)** +5. `bun run test` runs 5 times consecutively without flakes. **(v0.26.9)** +6. Quarantine count `≤10` after the sweep (raised from 5 per D15; v0.26.7 added 2, currently 4: brain-registry, reconcile-links, cycle, embed). +7. Wallclock target: `bun run test` ≤60s informational (per D9, dropped from <30s after Codex F1: marking only ~92 cheap files concurrent doesn't unblock the heavy 56 PGLite + 49 env files). Pinned config: SHARDS=8, MAX_CONCURRENCY=4, document Mac model. **(v0.26.9)** + +**Decisions ledger (v0.26.7 plan):** D1 reversed→D16 sliced, D5 quarantine, D6 no helper wrapper, D7 grep+quarantine, D9 ≤60s informational, D10 ESM-cache claim dropped, D11 codemod uses `find` recursive, D12 lint wired into `verify` not `test`, D13 unquarantine attempt dropped, D14 extended grep patterns, D15 cap raised to 10. **Estimated effort:** 1-2 weeks of one engineer's focused work. Could parallelize by sub-area (env-mutation sweep is independent of PGLite sweep). diff --git a/VERSION b/VERSION index 5d700c013..4c0ba8149 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -0.26.6 +0.26.7 diff --git a/llms-full.txt b/llms-full.txt index 056aa24ec..19e2d53fa 100644 --- a/llms-full.txt +++ b/llms-full.txt @@ -445,10 +445,79 @@ If a shard wedges (per-shard `GBRAIN_TEST_SHARD_TIMEOUT` cap, default 600s), the - `*.test.ts` → fast loop (parallel 8-shard fan-out). - `*.slow.test.ts` → run via `bun run test:slow` only (intentional cold-path tests; would dominate the fast loop's wallclock). -- `*.serial.test.ts` → run via `bun run test:serial` after the parallel pass completes; uses `--max-concurrency=1`. Quarantine for tests that share file-wide state and race when run alongside other files in the same `bun test` process. Currently: `test/brain-registry.serial.test.ts`, `test/reconcile-links.serial.test.ts`. **Do not put the parallelism back on a serial file unless you've fixed the contention root cause** (it just re-introduces the flake). +- `*.serial.test.ts` → run via `bun run test:serial` after the parallel pass completes; uses `--max-concurrency=1`. Quarantine for tests that share file-wide state and race when run alongside other files in the same `bun test` process. Currently: `test/brain-registry.serial.test.ts`, `test/reconcile-links.serial.test.ts`, `test/core/cycle.serial.test.ts`, `test/embed.serial.test.ts` (the latter two added in v0.26.7 — they use `mock.module(...)` which leaks across files in the shard process). **Do not put the parallelism back on a serial file unless you've fixed the contention root cause** (it just re-introduces the flake). - `test/e2e/*.test.ts` → real-Postgres E2E. Skipped when `DATABASE_URL` is unset. -The intra-file parallelism project (turn `bun test` into `bun test --concurrent` after sweeping shared-state contention sites — ~58 PGLite + ~40 env-mutation + ~2 mock.module sites) is filed as a P0 TODO for a follow-up release. v0.26.4 ships file-level parallelism only. +The intra-file parallelism project (turn `bun test` into `bun test --concurrent` after sweeping shared-state contention sites) is sliced across v0.26.7 (foundation), v0.26.8 (env-mutation sweep), and v0.26.9 (PGLite sweep + codemod + measurement). v0.26.4 ships file-level parallelism only. + +### Test-isolation lint and helpers (v0.26.7) + +The cross-file flake class is enforced statically by `scripts/check-test-isolation.sh`, wired into `bun run verify` and `bun run check:all`. Rules (non-serial unit files only; `*.serial.test.ts` and `test/e2e/*` are skipped): + +| Rule | What it bans | Fix | +|---|---|---| +| **R1** | `process.env.X = ...`, bracket assignment, `delete process.env.X`, `Object.assign(process.env, ...)`, `Reflect.set(process.env, ...)` | Use `withEnv()` from `test/helpers/with-env.ts`, OR rename file to `*.serial.test.ts` | +| **R2** | `mock.module(...)` anywhere in the file | Rename file to `*.serial.test.ts` (no DI on production code for testability) | +| **R3** | `new PGLiteEngine(` outside ~50 lines after a `beforeAll(` line | Use the canonical block (below) inside `beforeAll(` | +| **R4** | Files creating `new PGLiteEngine(` without `engine.disconnect(` inside an `afterAll(` block | Add `afterAll(() => engine.disconnect())` | + +Files that violated these rules at the v0.26.7 baseline are listed in `scripts/check-test-isolation.allowlist`. **The allow-list MUST shrink over time** — never add new entries. v0.26.8 (env sweep) and v0.26.9 (PGLite sweep) remove entries as files get fixed. + +#### Canonical PGLite block (R3 + R4 compliant) + +Every test file that needs a PGLite engine should use this exact pattern: + +```ts +import { PGLiteEngine } from '../src/core/pglite-engine.ts'; +import { resetPgliteState } from './helpers/reset-pglite.ts'; + +let engine: PGLiteEngine; + +beforeAll(async () => { + engine = new PGLiteEngine(); + await engine.connect({}); + await engine.initSchema(); +}); + +afterAll(async () => { + await engine.disconnect(); +}); + +beforeEach(async () => { + await resetPgliteState(engine); +}); +``` + +Why this exact shape: `beforeAll` creates a single engine per file (PGLite WASM cold-start + initSchema is ~20s); `beforeEach` truncates user data via `resetPgliteState` ("two orders of magnitude faster" than fresh-engine-per-test); `afterAll` disconnects so the engine doesn't leak across file boundaries within a shard process. + +#### `withEnv` pattern (R1 fix) + +```ts +import { withEnv } from './helpers/with-env.ts'; + +test('reads OPENAI_API_KEY', async () => { + await withEnv({ OPENAI_API_KEY: 'sk-test' }, async () => { + expect(loadConfig().openai_key).toBe('sk-test'); + }); +}); + +// Delete a var (override is undefined): +await withEnv({ GBRAIN_HOME: undefined }, fn); + +// Multiple keys: +await withEnv({ A: '1', B: '2', C: undefined }, fn); +``` + +`withEnv` saves the prior value of every key it touches and restores via try/finally — including when the callback throws. **It is cross-test safe but NOT intra-file concurrent-safe.** `process.env` is process-global; two `test.concurrent()` calls in the same file both touching the same key will race. Files using `withEnv` stay outside the future `test.concurrent()` codemod's eligibility filter. + +#### When to quarantine instead of fix + +Rename to `*.serial.test.ts` when: +- The file uses `mock.module(...)` (R2 — there's no clean fix without changing production code). +- The file is genuinely env-coupled (e.g. `gbrain-home-isolation.test.ts`, `claw-test-cli.test.ts`) — module-load env readers + ESM caching defeat dynamic-import-after-env tricks. +- The file's tests intentionally share state across `it()` boundaries. + +Quarantine count cap: 10 (informational). Beyond that, push back on the design. ### Inventory (legacy) @@ -2319,7 +2388,7 @@ The skills in this repo are those patterns, generalized. What took 11 days to bu ## Contributing -See [CONTRIBUTING.md](CONTRIBUTING.md). Run `bun test` for unit tests. For the full local CI gate (gitleaks + unit + all 29 E2E files in Docker, the same checks GH Actions runs), use `bun run ci:local` ... or `bun run ci:local:diff` for the diff-aware subset during fast iteration. +See [CONTRIBUTING.md](CONTRIBUTING.md). Run `bun run test` for the parallel unit-test fast loop (~85s on a Mac dev box, 3700+ tests) or `bun run verify` for the pre-push gate (privacy + jsonb + progress + test-isolation + wasm + admin-build + typecheck). For the full local CI gate (gitleaks + unit + all 29 E2E files in Docker, the same checks GH Actions runs), use `bun run ci:local` ... or `bun run ci:local:diff` for the diff-aware subset during fast iteration. If you're working on retrieval or any of the search/embedding/ranking surface, set `GBRAIN_CONTRIBUTOR_MODE=1` in your shell rc and use `gbrain eval replay` to gate your changes against a snapshot of real captured queries — the dev loop is documented in [`docs/eval-bench.md`](docs/eval-bench.md). Capture is **off by default** for production users (no surprise data accumulation); the env var is the contributor opt-in. diff --git a/package.json b/package.json index bed886e0d..a32f75eaa 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "gbrain", - "version": "0.26.6", + "version": "0.26.7", "description": "Postgres-native personal knowledge brain with hybrid RAG search", "type": "module", "main": "src/core/index.ts", @@ -36,8 +36,8 @@ "build:pglite-snapshot": "bun run scripts/build-pglite-snapshot.ts", "test": "bash scripts/run-unit-parallel.sh", "test:full": "bun run verify && bash scripts/run-unit-parallel.sh && bun run test:slow && ([ -n \"$DATABASE_URL\" ] && bash scripts/run-e2e.sh || echo '[test:full] skipped E2E (no DATABASE_URL); run docker-compose -f docker-compose.ci.yml up + bun run test:e2e to include' 1>&2)", - "verify": "bun run check:privacy && bun run check:jsonb && bun run check:progress && bun run check:wasm && bun run check:admin-build && bun run typecheck", - "check:all": "scripts/check-privacy.sh && scripts/check-jsonb-pattern.sh && scripts/check-progress-to-stdout.sh && scripts/check-no-legacy-getconnection.sh && scripts/check-trailing-newline.sh && scripts/check-wasm-embedded.sh && scripts/check-exports-count.sh && scripts/check-admin-build.sh", + "verify": "bun run check:privacy && bun run check:jsonb && bun run check:progress && bun run check:test-isolation && bun run check:wasm && bun run check:admin-build && bun run typecheck", + "check:all": "scripts/check-privacy.sh && scripts/check-jsonb-pattern.sh && scripts/check-progress-to-stdout.sh && scripts/check-no-legacy-getconnection.sh && scripts/check-test-isolation.sh && scripts/check-trailing-newline.sh && scripts/check-wasm-embedded.sh && scripts/check-exports-count.sh && scripts/check-admin-build.sh", "check:wasm": "scripts/check-wasm-embedded.sh", "check:newlines": "scripts/check-trailing-newline.sh", "test:e2e": "bash scripts/run-e2e.sh", @@ -53,6 +53,7 @@ "check:progress": "scripts/check-progress-to-stdout.sh", "check:exports-count": "scripts/check-exports-count.sh", "check:admin-build": "scripts/check-admin-build.sh", + "check:test-isolation": "scripts/check-test-isolation.sh", "postinstall": "command -v gbrain >/dev/null 2>&1 && gbrain apply-migrations --yes --non-interactive || echo '[gbrain] postinstall skipped. If installed via bun install -g github:...: run `gbrain doctor` and `gbrain apply-migrations --yes` manually. See https://github.com/garrytan/gbrain/issues/218' 1>&2", "prepublish:clawhub": "bun run build:all", "publish:clawhub": "clawhub package publish . --family bundle-plugin" diff --git a/scripts/check-test-isolation.allowlist b/scripts/check-test-isolation.allowlist new file mode 100644 index 000000000..dee0b7edb --- /dev/null +++ b/scripts/check-test-isolation.allowlist @@ -0,0 +1,73 @@ +# v0.26.7 baseline allow-list for scripts/check-test-isolation.sh. +# +# Files here violate one or more of the lint rules (env mutation, +# mock.module, PGLite outside beforeAll, missing afterAll{disconnect}). +# The lint ships in v0.26.7 and v0.26.8 (env sweep) + v0.26.9 (PGLite +# sweep) remove entries from this file as each sweep makes the file +# clean. +# +# RULES: +# - This list MUST shrink over time. Never add new entries — adding a +# new file means accepting cross-file flake risk for that file. +# - When you fix a file (apply withEnv, add the canonical PGLite +# block, etc.), remove its entry here. +# - When you cannot fix a file cleanly (genuinely env-coupled, +# or shares state intentionally), rename it to *.serial.test.ts +# instead of leaving it allow-listed. +# +# Permanent exemption: the test of the lint itself. Its fixture strings +# (passed verbatim into subprocesses) legitimately match the lint +# patterns it is testing detection of. The file does NOT mutate +# process.env at runtime. Permanent — do not remove. +test/scripts/check-test-isolation.test.ts +test/autopilot-install.test.ts +test/bootstrap.test.ts +test/brain-resolver.test.ts +test/check-resolvable-cli.test.ts +test/claw-test-cli.test.ts +test/code-def-refs.test.ts +test/core/cycle.test.ts +test/destructive-guard.test.ts +test/doctor-minions-check.test.ts +test/doctor.test.ts +test/dream.test.ts +test/embed.test.ts +test/eval-capture.test.ts +test/friction-cli.test.ts +test/friction.test.ts +test/gbrain-home-isolation.test.ts +test/helpers/with-env.test.ts +test/http-transport.test.ts +test/hybrid-meta.test.ts +test/init-migrate-only.test.ts +test/integrations.test.ts +test/mcp-eval-capture.test.ts +test/migrate.test.ts +test/migration-resume.test.ts +test/migrations-v0_11_0.test.ts +test/migrations-v0_13_1.test.ts +test/migrations-v0_14_0.test.ts +test/migrations-v0_19_0.test.ts +test/migrations-v0_22_4.test.ts +test/minions-shell.test.ts +test/minions.test.ts +test/mounts-cli.test.ts +test/multi-source-integration.test.ts +test/orphans.test.ts +test/pages-soft-delete.test.ts +test/preferences.test.ts +test/reindex-code.test.ts +test/resolve-prepare.test.ts +test/resolvers.test.ts +test/scenarios.test.ts +test/schema-bootstrap-coverage.test.ts +test/search-limit.test.ts +test/seed-pglite.test.ts +test/skillpack-check.test.ts +test/source-resolver.test.ts +test/storage-sync.test.ts +test/subagent-audit.test.ts +test/supervisor.test.ts +test/sync-failures.test.ts +test/sync-parallel.test.ts +test/transcription.test.ts diff --git a/scripts/check-test-isolation.sh b/scripts/check-test-isolation.sh new file mode 100755 index 000000000..fdd135fe3 --- /dev/null +++ b/scripts/check-test-isolation.sh @@ -0,0 +1,141 @@ +#!/usr/bin/env bash +# CI guard: fail if any non-serial unit test file violates intra-process +# isolation rules. The v0.26.4 parallel runner loads multiple test files +# into one bun process per shard; module-level state (env vars, PGLite +# engines, mock.module overrides) leaks across files in that process and +# silently flakes other tests. +# +# Rules enforced (non-serial unit test files only): +# R1: no `process.env.X = ...`, `process.env['X'] = ...`, +# `delete process.env.X`, `Object.assign(process.env, ...)`, +# `Reflect.set(process.env, ...)` mutations. Use withEnv() helper or +# rename the file to `*.serial.test.ts`. +# R2: no `mock.module(...)` anywhere. Top-level module mocks affect every +# other file in the same shard process. Rename to `*.serial.test.ts`. +# R3: `new PGLiteEngine(` may only appear within ~50 lines following a +# `beforeAll(` line. Engines created at module scope (or in describe +# bodies) leak across files in the shard process. +# R4: any file that creates `new PGLiteEngine(` must call `.disconnect(` +# inside an `afterAll(` block. Without disconnect, engines leak across +# file boundaries within a shard process. +# +# Scope: +# - Recursively scans `test/**/*.test.ts`. +# - Skips `*.serial.test.ts` entirely (the quarantine escape hatch). +# - Skips `test/e2e/**` (E2E runs sequentially in its own runner; not in +# the parallel pool). +# +# Allow-list: +# Files in `scripts/check-test-isolation.allowlist` (one filename per +# line, # comments allowed) are skipped. This exists because v0.26.7 +# ships the lint as a foundation; v0.26.8 (env sweep) and v0.26.9 +# (PGLite sweep) remove entries as files get fixed. New files MUST NOT +# be added — the allow-list shrinks over time, never grows. +# +# Usage: scripts/check-test-isolation.sh [TARGET_DIR] +# Exit: 0 when clean, 1 when un-allow-listed violations found. + +set -euo pipefail + +ROOT="$(git rev-parse --show-toplevel 2>/dev/null || pwd)" +cd "$ROOT" + +TARGET_DIR="${1:-test}" +ALLOWLIST_FILE="$ROOT/scripts/check-test-isolation.allowlist" + +# Read allowlist (one filename per line, # comments allowed). Empty file +# is fine — every violation will fail. +ALLOWLIST="" +if [ -f "$ALLOWLIST_FILE" ]; then + ALLOWLIST="$(grep -v '^[[:space:]]*#' "$ALLOWLIST_FILE" | grep -v '^[[:space:]]*$' || true)" +fi + +is_allowlisted() { + local f="$1" + [ -z "$ALLOWLIST" ] && return 1 + echo "$ALLOWLIST" | grep -qxF "$f" +} + +# Find non-serial unit test files (excluding test/e2e). Portable across +# bash 3.2 (macOS default) and bash 4+; no mapfile. +FILE_LIST="$(find "$TARGET_DIR" -name '*.test.ts' \ + -not -name '*.serial.test.ts' \ + -not -path "*/e2e/*" \ + -type f 2>/dev/null | sort)" + +violations=0 +file_count=0 + +emit_violation() { + local f="$1" rule="$2" detail="$3" lines="$4" + if is_allowlisted "$f"; then + return + fi + echo "ERROR: $f" + echo " rule $rule: $detail" + if [ -n "$lines" ]; then + echo "$lines" | head -3 | sed 's/^/ /' + fi + violations=$((violations + 1)) +} + +# Read newline-separated file list; OK on macOS bash 3.2. +while IFS= read -r f; do + [ -z "$f" ] && continue + file_count=$((file_count + 1)) + # R1: env mutations. + env_lines=$(grep -nE 'process\.env\.[A-Za-z_][A-Za-z_0-9]*[[:space:]]*=[^=]|process\.env\[[^]]+\][[:space:]]*=[^=]|delete[[:space:]]+process\.env\.|delete[[:space:]]+process\.env\[|Object\.assign[[:space:]]*\([[:space:]]*process\.env|Reflect\.set[[:space:]]*\([[:space:]]*process\.env' "$f" 2>/dev/null || true) + if [ -n "$env_lines" ]; then + emit_violation "$f" "R1" "process.env mutation; use withEnv() or rename to *.serial.test.ts" "$env_lines" + fi + + # R2: mock.module() anywhere. + mock_lines=$(grep -nE 'mock\.module[[:space:]]*\(' "$f" 2>/dev/null || true) + if [ -n "$mock_lines" ]; then + emit_violation "$f" "R2" "mock.module() leaks across files in the shard process; rename to *.serial.test.ts" "$mock_lines" + fi + + # R3: PGLiteEngine outside ~50 lines after a beforeAll(. + if grep -qE 'new PGLiteEngine[[:space:]]*\(' "$f" 2>/dev/null; then + bad=$(awk ' + BEGIN { last_before_all = -1000 } + /beforeAll[[:space:]]*\(/ { last_before_all = NR } + /new PGLiteEngine[[:space:]]*\(/ { + if (NR - last_before_all > 50) { + printf "%d:%s\n", NR, $0 + } + } + ' "$f" 2>/dev/null) + if [ -n "$bad" ]; then + emit_violation "$f" "R3" "new PGLiteEngine(...) outside beforeAll() context (>50 lines); move into beforeAll" "$bad" + fi + fi + + # R4: PGLiteEngine creation requires afterAll{disconnect}. + if grep -qE 'new PGLiteEngine[[:space:]]*\(' "$f" 2>/dev/null; then + if ! grep -qE 'afterAll[[:space:]]*\(' "$f" 2>/dev/null \ + || ! grep -qE '\.disconnect[[:space:]]*\(' "$f" 2>/dev/null; then + emit_violation "$f" "R4" "creates PGLiteEngine but missing afterAll(() => engine.disconnect()); engine leaks across files in the shard process" "" + fi + fi +done < { + * engine = new PGLiteEngine(); + * await engine.connect({}); + * await engine.initSchema(); + * }); + * + * afterAll(async () => { + * await engine.disconnect(); + * }); + * + * beforeEach(async () => { + * await resetPgliteState(engine); + * }); + * + * Why this exact shape: + * - `beforeAll` creates one engine per file (~20s schema init paid once). + * - `beforeEach` resets user data without re-creating the engine. + * - `afterAll(disconnect)` is REQUIRED. The v0.26.4 parallel runner loads + * multiple test files into one bun process per shard; without disconnect, + * engines leak across file boundaries within a shard process. + * * Implementation: * 1. TRUNCATE every public table CASCADE, including `sources` (so tests * that register their own sources don't leak rows into the next test). diff --git a/test/helpers/with-env.test.ts b/test/helpers/with-env.test.ts new file mode 100644 index 000000000..bfe16064b --- /dev/null +++ b/test/helpers/with-env.test.ts @@ -0,0 +1,88 @@ +import { describe, test, expect } from 'bun:test'; +import { withEnv } from './with-env.ts'; + +const KEY = 'GBRAIN_WITH_ENV_TEST_KEY'; +const KEY2 = 'GBRAIN_WITH_ENV_TEST_KEY2'; + +describe('withEnv', () => { + test('sync callback: sets value, runs, restores prior value', async () => { + process.env[KEY] = 'original'; + const result = await withEnv({ [KEY]: 'overridden' }, () => { + expect(process.env[KEY]).toBe('overridden'); + return 42; + }); + expect(result).toBe(42); + expect(process.env[KEY]).toBe('original'); + delete process.env[KEY]; + }); + + test('async callback: awaits, then restores', async () => { + process.env[KEY] = 'before'; + const result = await withEnv({ [KEY]: 'during' }, async () => { + expect(process.env[KEY]).toBe('during'); + await new Promise(r => setTimeout(r, 5)); + expect(process.env[KEY]).toBe('during'); + return 'done'; + }); + expect(result).toBe('done'); + expect(process.env[KEY]).toBe('before'); + delete process.env[KEY]; + }); + + test('delete-key: undefined override removes the var, restores it after', async () => { + process.env[KEY] = 'will-be-deleted'; + await withEnv({ [KEY]: undefined }, () => { + expect(process.env[KEY]).toBeUndefined(); + }); + expect(process.env[KEY]).toBe('will-be-deleted'); + delete process.env[KEY]; + }); + + test('delete-key when prior was unset: stays unset after restore', async () => { + delete process.env[KEY]; + await withEnv({ [KEY]: 'temp' }, () => { + expect(process.env[KEY]).toBe('temp'); + }); + expect(process.env[KEY]).toBeUndefined(); + }); + + test('restore-on-throw: callback throws, env still restored', async () => { + process.env[KEY] = 'safe'; + let caught: unknown = null; + try { + await withEnv({ [KEY]: 'wreckage' }, () => { + expect(process.env[KEY]).toBe('wreckage'); + throw new Error('boom'); + }); + } catch (e) { + caught = e; + } + expect((caught as Error).message).toBe('boom'); + expect(process.env[KEY]).toBe('safe'); + delete process.env[KEY]; + }); + + test('nested compose: inner overrides outer, restore returns to outer value', async () => { + delete process.env[KEY]; + await withEnv({ [KEY]: 'outer' }, async () => { + expect(process.env[KEY]).toBe('outer'); + await withEnv({ [KEY]: 'inner' }, () => { + expect(process.env[KEY]).toBe('inner'); + }); + expect(process.env[KEY]).toBe('outer'); + }); + expect(process.env[KEY]).toBeUndefined(); + }); + + test('multiple keys: sets and restores all atomically', async () => { + process.env[KEY] = 'A-prior'; + delete process.env[KEY2]; + await withEnv({ [KEY]: 'A-new', [KEY2]: 'B-new' }, () => { + expect(process.env[KEY]).toBe('A-new'); + expect(process.env[KEY2]).toBe('B-new'); + }); + expect(process.env[KEY]).toBe('A-prior'); + expect(process.env[KEY2]).toBeUndefined(); + delete process.env[KEY]; + }); +}); diff --git a/test/helpers/with-env.ts b/test/helpers/with-env.ts new file mode 100644 index 000000000..f4c877f51 --- /dev/null +++ b/test/helpers/with-env.ts @@ -0,0 +1,71 @@ +/** + * Run a callback with `process.env` mutations applied, then restore the prior + * values via try/finally. The canonical pattern for env-touching tests in this + * repo. + * + * Why this exists: `process.env` is process-global. Tests that mutate it + * leak state across files in the same bun test process (the parallel runner + * loads multiple files into one process per shard). `withEnv` saves the + * prior value of every key it touches, runs the callback, and restores via + * try/finally — including when the callback throws. + * + * Important caveat: `withEnv` is cross-test-safe but NOT intra-file + * concurrent-safe. Two `test.concurrent()` calls in the same file both + * calling withEnv on the same key will race — the global is only one + * variable. Files that mutate env stay outside the `test.concurrent()` + * codemod's eligibility filter (the `*.serial.test.ts` quarantine + the + * codemod's `grep -L "process\.env\."` exclusion handle this). + * + * Use: + * import { withEnv } from './helpers/with-env.ts'; + * + * test('reads OPENAI_API_KEY', async () => { + * await withEnv({ OPENAI_API_KEY: 'sk-test' }, async () => { + * expect(loadConfig().openai_key).toBe('sk-test'); + * }); + * }); + * + * // Delete a var (override is undefined): + * await withEnv({ GBRAIN_HOME: undefined }, async () => { + * expect(process.env.GBRAIN_HOME).toBeUndefined(); + * }); + * + * // Multiple keys: + * await withEnv({ A: '1', B: '2', C: undefined }, fn); + * + * // Nested compose: inner restores to outer's value, not original. + * await withEnv({ K: 'outer' }, async () => { + * await withEnv({ K: 'inner' }, async () => { + * expect(process.env.K).toBe('inner'); + * }); + * expect(process.env.K).toBe('outer'); + * }); + */ +export async function withEnv( + overrides: Record, + fn: () => T | Promise, +): Promise { + const keys = Object.keys(overrides); + const prior: Record = {}; + for (const key of keys) { + prior[key] = process.env[key]; + } + try { + for (const [key, value] of Object.entries(overrides)) { + if (value === undefined) { + delete process.env[key]; + } else { + process.env[key] = value; + } + } + return await fn(); + } finally { + for (const [key, value] of Object.entries(prior)) { + if (value === undefined) { + delete process.env[key]; + } else { + process.env[key] = value; + } + } + } +} diff --git a/test/scripts/check-test-isolation.test.ts b/test/scripts/check-test-isolation.test.ts new file mode 100644 index 000000000..b4fa8ba0d --- /dev/null +++ b/test/scripts/check-test-isolation.test.ts @@ -0,0 +1,270 @@ +/** + * Fixture-driven unit tests for scripts/check-test-isolation.sh. + * + * Spawns the script in a tmpdir with hand-crafted fake test files and + * asserts the lint's exit code + violation messages match expectations. + * No env mutation, no mock.module, no PGLite — this test file is itself + * subject to the lint (it ships outside *.serial.test.ts and outside + * test/e2e/). + */ + +import { describe, it, expect } from 'bun:test'; +import { spawnSync } from 'child_process'; +import { mkdtempSync, mkdirSync, writeFileSync } from 'fs'; +import { tmpdir } from 'os'; +import { join, resolve } from 'path'; + +const REPO_ROOT = resolve(import.meta.dir, '..', '..'); +const LINT_SH = resolve(REPO_ROOT, 'scripts/check-test-isolation.sh'); + +interface FakeFile { + /** Path relative to the tmpdir's `test/` directory. */ + path: string; + contents: string; +} + +interface RunResult { + status: number; + stdout: string; + stderr: string; +} + +function runLintIn(files: FakeFile[], allowlist: string[] = []): RunResult { + const dir = mkdtempSync(join(tmpdir(), 'lint-isolation-')); + mkdirSync(join(dir, 'test'), { recursive: true }); + mkdirSync(join(dir, 'scripts'), { recursive: true }); + + for (const f of files) { + const full = join(dir, 'test', f.path); + mkdirSync(resolve(full, '..'), { recursive: true }); + writeFileSync(full, f.contents); + } + // Empty allowlist file ensures the script reads OUR allowlist, not the + // real repo's, regardless of git toplevel resolution. + writeFileSync( + join(dir, 'scripts/check-test-isolation.allowlist'), + allowlist.length > 0 ? allowlist.join('\n') + '\n' : '', + ); + + const r = spawnSync('bash', [LINT_SH, 'test'], { + cwd: dir, + encoding: 'utf-8', + env: { ...process.env }, + }); + return { status: r.status ?? -1, stdout: r.stdout, stderr: r.stderr }; +} + +describe('check-test-isolation.sh', () => { + describe('clean files', () => { + it('returns 0 when no test files violate any rule', () => { + const r = runLintIn([ + { + path: 'a.test.ts', + contents: `import { test, expect } from 'bun:test';\ntest('ok', () => expect(1).toBe(1));\n`, + }, + ]); + expect(r.status).toBe(0); + expect(r.stdout).toContain('check-test-isolation: OK'); + }); + }); + + describe('R1 — env mutation', () => { + it('flags process.env.X = assignment', () => { + const r = runLintIn([ + { + path: 'env-write.test.ts', + contents: `process.env.OOPS = 'bad';\n`, + }, + ]); + expect(r.status).toBe(1); + expect(r.stdout).toContain('R1'); + expect(r.stdout).toContain('env-write.test.ts'); + }); + + it('flags process.env[bracket] = assignment', () => { + const r = runLintIn([ + { + path: 'env-bracket.test.ts', + contents: `process.env['OOPS'] = 'bad';\n`, + }, + ]); + expect(r.status).toBe(1); + expect(r.stdout).toContain('R1'); + }); + + it('flags delete process.env.X', () => { + const r = runLintIn([ + { + path: 'env-delete.test.ts', + contents: `delete process.env.OOPS;\n`, + }, + ]); + expect(r.status).toBe(1); + expect(r.stdout).toContain('R1'); + }); + + it('flags Object.assign(process.env, ...)', () => { + const r = runLintIn([ + { + path: 'env-assign.test.ts', + contents: `Object.assign(process.env, { OOPS: 'bad' });\n`, + }, + ]); + expect(r.status).toBe(1); + expect(r.stdout).toContain('R1'); + }); + + it('flags Reflect.set(process.env, ...)', () => { + const r = runLintIn([ + { + path: 'env-reflect.test.ts', + contents: `Reflect.set(process.env, 'OOPS', 'bad');\n`, + }, + ]); + expect(r.status).toBe(1); + expect(r.stdout).toContain('R1'); + }); + + it('does NOT flag a comparison like process.env.X === ...', () => { + const r = runLintIn([ + { + path: 'env-read.test.ts', + contents: `if (process.env.X === 'y') {}\n`, + }, + ]); + expect(r.status).toBe(0); + }); + }); + + describe('R2 — mock.module()', () => { + it('flags mock.module(...)', () => { + const r = runLintIn([ + { + path: 'mocks.test.ts', + contents: `import { mock } from 'bun:test';\nmock.module('foo', () => ({}));\n`, + }, + ]); + expect(r.status).toBe(1); + expect(r.stdout).toContain('R2'); + }); + }); + + describe('R3 — new PGLiteEngine() outside beforeAll context', () => { + it('flags engine created at module top-level', () => { + const r = runLintIn([ + { + path: 'pglite-toplevel.test.ts', + contents: `import { PGLiteEngine } from '../src/core/pglite-engine.ts';\nconst engine = new PGLiteEngine();\n`, + }, + ]); + expect(r.status).toBe(1); + expect(r.stdout).toContain('R3'); + }); + + it('does NOT flag engine created within ~50 lines of a beforeAll', () => { + const r = runLintIn([ + { + path: 'pglite-ok.test.ts', + contents: + `import { beforeAll, afterAll, test, expect } from 'bun:test';\n` + + `import { PGLiteEngine } from '../src/core/pglite-engine.ts';\n` + + `let engine: PGLiteEngine;\n` + + `beforeAll(async () => {\n` + + ` engine = new PGLiteEngine();\n` + + ` await engine.connect({});\n` + + `});\n` + + `afterAll(async () => { await engine.disconnect(); });\n` + + `test('x', () => expect(1).toBe(1));\n`, + }, + ]); + expect(r.status).toBe(0); + }); + }); + + describe('R4 — afterAll/disconnect pairing', () => { + it('flags engine creation without afterAll{disconnect}', () => { + const r = runLintIn([ + { + path: 'pglite-no-disconnect.test.ts', + contents: + `import { beforeAll, test, expect } from 'bun:test';\n` + + `import { PGLiteEngine } from '../src/core/pglite-engine.ts';\n` + + `let engine: PGLiteEngine;\n` + + `beforeAll(async () => { engine = new PGLiteEngine(); await engine.connect({}); });\n` + + `test('x', () => expect(1).toBe(1));\n`, + }, + ]); + expect(r.status).toBe(1); + expect(r.stdout).toContain('R4'); + }); + }); + + describe('scope', () => { + it('skips *.serial.test.ts files entirely', () => { + const r = runLintIn([ + { + path: 'naughty.serial.test.ts', + contents: `process.env.OOPS = 'bad';\nimport { mock } from 'bun:test';\nmock.module('foo', () => ({}));\n`, + }, + ]); + expect(r.status).toBe(0); + }); + + it('skips test/e2e/ subtree', () => { + const r = runLintIn([ + { + path: 'e2e/leak.test.ts', + contents: `process.env.OOPS = 'bad';\n`, + }, + ]); + expect(r.status).toBe(0); + }); + }); + + describe('allowlist', () => { + it('skips files listed in the allowlist', () => { + const r = runLintIn( + [ + { + path: 'legacy.test.ts', + contents: `process.env.OOPS = 'bad';\n`, + }, + ], + ['test/legacy.test.ts'], + ); + expect(r.status).toBe(0); + }); + + it('still flags files NOT in the allowlist when allowlist is non-empty', () => { + const r = runLintIn( + [ + { + path: 'allowed.test.ts', + contents: `process.env.X = 'a';\n`, + }, + { + path: 'fresh.test.ts', + contents: `process.env.Y = 'b';\n`, + }, + ], + ['test/allowed.test.ts'], + ); + expect(r.status).toBe(1); + expect(r.stdout).toContain('fresh.test.ts'); + expect(r.stdout).not.toContain('allowed.test.ts'); + }); + + it('treats # comments and blank lines in allowlist as no-ops', () => { + const r = runLintIn( + [ + { + path: 'legacy.test.ts', + contents: `process.env.OOPS = 'bad';\n`, + }, + ], + ['# legacy file', '', 'test/legacy.test.ts'], + ); + expect(r.status).toBe(0); + }); + }); +});