mirror of
https://github.com/garrytan/gbrain.git
synced 2026-07-27 22:15:33 +00:00
v0.26.7 test: isolation foundation (helpers + lint + quarantine) (#613)
* test: add withEnv helper + canonical PGLite block JSDoc withEnv(overrides, fn) saves prior values, runs the callback, restores via try/finally — including on throw. Handles delete via undefined override. Nested calls compose. Cross-test safe; explicitly NOT intra-file concurrent-safe (process.env is process-global). 7 unit cases covering sync, async, delete-key, delete-when-prior-unset, restore-on-throw, nested compose, multi-key atomic restore. reset-pglite.ts JSDoc extended with the canonical 4-line PGLite block (beforeAll create + afterAll disconnect + beforeEach reset). The lint script in the next commit enforces this exact shape. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test: add check-test-isolation lint script + wire into verify Grep-based lint enforcing 4 rules on non-serial unit test files: R1: no process.env mutations (use withEnv() or rename to *.serial.test.ts) R2: no mock.module() (rename to *.serial.test.ts) R3: new PGLiteEngine( only inside beforeAll() context R4: PGLiteEngine creators must pair with afterAll{disconnect} 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). Matches the existing scripts/check-*.sh family shape (jsonb, progress, etc). 51 baseline violators captured in scripts/check-test-isolation.allowlist. List MUST shrink over time — entries removed by v0.26.8 (env sweep) and v0.26.9 (PGLite sweep). New files cannot be added. CLAUDE.md ## Testing section extended with R1-R4 rules table, the canonical 4-line PGLite block, withEnv pattern, and when-to-quarantine guidance. 16 fixture-driven test cases for the lint: clean, R1 (5 patterns + 1 negative), R2, R3 (top-level vs in-beforeAll), R4 (missing disconnect), *.serial.test.ts skip, test/e2e/ skip, allowlist (3 cases). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test: quarantine cycle and embed mock.module test files Both files use mock.module(...) at top level — leaks across files in the same shard process. The check-test-isolation lint (R2) bans this pattern in non-serial files; quarantine is the escape hatch. Per v0.26.7 plan D5: prefer quarantine over DI on runCycle/runEmbed. Production signatures stay frozen; tests run at --max-concurrency=1 in the serial post-pass (the existing pattern shipped in v0.26.4 for brain-registry and reconcile-links). Quarantine count: 2 → 4. Cap raised to 10 informational per D15. Renames: test/core/cycle.test.ts → test/core/cycle.serial.test.ts test/embed.test.ts → test/embed.serial.test.ts Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * chore: bump version and changelog (v0.26.7) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * docs: post-ship documentation sync for v0.26.7 - README.md "Contributing" line: point to bun run test + bun run verify (parallel fast loop) - CONTRIBUTING.md "Running tests": rewrite for the v0.26.4/v0.26.7 test surface (parallel runner, verify, slow/serial/e2e tiers) - CONTRIBUTING.md adds "Writing tests that survive the parallel loop" section: R1-R4 lint, canonical PGLite block, withEnv pattern, when to quarantine - llms-full.txt regenerated to pick up the README + CONTRIBUTING changes Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
Garry Tan
co-authored by
Claude Opus 4.7
Claude Opus 4.7
parent
9e2093fc9b
commit
058fe69575
@@ -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.**
|
||||
|
||||
@@ -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)
|
||||
|
||||
|
||||
+80
-12
@@ -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+)
|
||||
|
||||
|
||||
@@ -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.
|
||||
|
||||
|
||||
@@ -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).
|
||||
|
||||
|
||||
+72
-3
@@ -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.
|
||||
|
||||
|
||||
+4
-3
@@ -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"
|
||||
|
||||
@@ -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
|
||||
Executable
+141
@@ -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 <<EOF
|
||||
$FILE_LIST
|
||||
EOF
|
||||
|
||||
if [ $violations -gt 0 ]; then
|
||||
echo
|
||||
echo "check-test-isolation: FAIL ($violations violation(s))"
|
||||
echo
|
||||
echo "Fix:"
|
||||
echo " - For env mutations, use withEnv() from test/helpers/with-env.ts"
|
||||
echo " - For mock.module(), rename to *.serial.test.ts (quarantine)"
|
||||
echo " - For PGLiteEngine, follow the canonical pattern in"
|
||||
echo " test/helpers/reset-pglite.ts JSDoc and CLAUDE.md."
|
||||
echo
|
||||
echo "Or, if this is a baseline file from before the lint shipped,"
|
||||
echo "add it to scripts/check-test-isolation.allowlist (with a TODO"
|
||||
echo "comment naming the sweep PR that will remove it)."
|
||||
exit 1
|
||||
fi
|
||||
|
||||
echo "check-test-isolation: OK ($file_count non-serial unit files scanned)"
|
||||
@@ -8,6 +8,35 @@
|
||||
* that across every test in every file. Sharing one engine and wiping data
|
||||
* is two orders of magnitude faster.
|
||||
*
|
||||
* Canonical block (copy verbatim into PGLite-using test files; enforced by
|
||||
* scripts/check-test-isolation.sh rules R3 + R4):
|
||||
*
|
||||
* 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 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).
|
||||
|
||||
@@ -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];
|
||||
});
|
||||
});
|
||||
@@ -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<T>(
|
||||
overrides: Record<string, string | undefined>,
|
||||
fn: () => T | Promise<T>,
|
||||
): Promise<T> {
|
||||
const keys = Object.keys(overrides);
|
||||
const prior: Record<string, string | undefined> = {};
|
||||
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;
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -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);
|
||||
});
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user