diff --git a/.archgate/adrs/ARCH-005-testing-standards.md b/.archgate/adrs/ARCH-005-testing-standards.md index e434dc18..85bfc510 100644 --- a/.archgate/adrs/ARCH-005-testing-standards.md +++ b/.archgate/adrs/ARCH-005-testing-standards.md @@ -40,6 +40,8 @@ Use Bun's built-in test runner (`bun test`) for all tests. Test files go in `tes - Use `tests/fixtures/` for sample data files - Use temp directories (`mkdtemp`) for tests that write to the filesystem - Clean up temp directories in `afterEach` or `afterAll` +- **Close external SDK instances** (servers, clients, transports) in `afterEach` or `afterAll` by calling their cleanup method (e.g., `await server.close()`). Manage their lifecycle in `beforeEach`/`afterEach` rather than inside individual test bodies so cleanup is guaranteed. +- **When a test creates a temp git repo and needs to call `git commit`, configure local user identity first** — CI runners have no global git config, so commits fail without explicit local identity. Set it with `await Bun.$\`git config user.email "test@test.com"\`.cwd(tempDir).quiet()`and`await Bun.$\`git config user.name "Test"\`.cwd(tempDir).quiet()`immediately after`git init`. - Test public module interfaces, not private implementation details - Use descriptive test names that explain the expected behavior @@ -48,6 +50,8 @@ Use Bun's built-in test runner (`bun test`) for all tests. Test files go in `tes - Don't test private implementation details — test the public API of each module - Don't depend on network access in unit tests - Don't leave temp files after test runs +- **Don't leave external SDK instances open after tests** — instances from libraries such as `@modelcontextprotocol/sdk` hold internal references (e.g., `AjvJsonSchemaValidator` backed by `ajv`) that keep Bun's event loop alive on Linux, causing `bun test` to hang indefinitely after all tests complete even though every test passes. Always call the cleanup method in `afterEach`. +- **Don't rely on globally-configured git identity in temp git repos** — always set `user.email` and `user.name` locally in any repo that makes commits. Omitting this works locally (where developers have global git config) but fails silently in CI, producing a cryptic `ShellPromise` error with no indication that git identity is the cause. - Don't skip tests without a tracking issue - Don't import test utilities from `node:test` — use Bun's built-in `bun:test` module @@ -83,6 +87,53 @@ describe("runChecks", () => { }); ``` +### Good Example — Temp Git Repo with Commits + +```typescript +// tests/engine/git-files.test.ts +it("returns both staged and unstaged changes", async () => { + await Bun.$`git init`.cwd(tempDir).quiet(); + // GOOD: set local identity before any commit — CI has no global git config + await Bun.$`git config user.email "test@test.com"`.cwd(tempDir).quiet(); + await Bun.$`git config user.name "Test"`.cwd(tempDir).quiet(); + writeFileSync(join(tempDir, "a.ts"), "export const a = 1;"); + await Bun.$`git add a.ts`.cwd(tempDir).quiet(); + await Bun.$`git commit -m "init"`.cwd(tempDir).quiet(); + // ... rest of test +}); +``` + +### Good Example — External SDK Cleanup + +```typescript +// tests/mcp/resources.test.ts +import { describe, expect, test, beforeEach, afterEach } from "bun:test"; +import { mkdtempSync, rmSync, mkdirSync } from "node:fs"; +import { McpServer } from "@modelcontextprotocol/sdk/server/mcp.js"; +import { registerResources } from "../../src/mcp/resources"; + +describe("registerResources", () => { + let tempDir: string; + let server: McpServer; + + // GOOD: lifecycle managed in beforeEach/afterEach, not inside test bodies + beforeEach(() => { + tempDir = mkdtempSync(join(tmpdir(), "archgate-mcp-res-test-")); + mkdirSync(join(tempDir, ".archgate", "adrs"), { recursive: true }); + server = new McpServer({ name: "test", version: "0.0.0" }); + }); + + afterEach(async () => { + await server.close(); // GOOD: releases internal validator references + rmSync(tempDir, { recursive: true, force: true }); + }); + + test("does not throw when registering resources", () => { + expect(() => registerResources(server, tempDir)).not.toThrow(); + }); +}); +``` + ### Bad Example ```typescript @@ -102,6 +153,22 @@ it("writes output", async () => { await Bun.write("/tmp/test-output.json", data); // No cleanup — file persists after test }); + +// BAD: SDK instance created inside test body — no guaranteed cleanup path +it("registers resources", () => { + const server = new McpServer({ name: "test", version: "0.0.0" }); + // server.close() never called — event loop held open on Linux + expect(() => registerResources(server, tempDir)).not.toThrow(); +}); + +// BAD: git commit without local identity — works locally, fails in CI +it("reads changes", async () => { + await Bun.$`git init`.cwd(tempDir).quiet(); + writeFileSync(join(tempDir, "a.ts"), "x"); + await Bun.$`git add a.ts`.cwd(tempDir).quiet(); + await Bun.$`git commit -m "init"`.cwd(tempDir).quiet(); + // Fails in CI: "*** Please tell me who you are." +}); ``` ## Consequences @@ -124,13 +191,15 @@ it("writes output", async () => { - **Mitigation:** The project pins a specific Bun version via `.prototools` (currently 1.3.8). Test runner API changes are caught during controlled Bun upgrades with full test suite validation. - **Coverage reporting gaps** — `bun test --coverage` may not report accurate coverage for all code paths, especially for dynamically imported modules. - **Mitigation:** Coverage is a guideline (80% target), not a hard gate. Critical modules (engine, formats) are tested thoroughly regardless of coverage numbers. +- **Third-party SDK event loop retention** — External SDK instances that hold internal resource references (e.g., `AjvJsonSchemaValidator` inside `@modelcontextprotocol/sdk`) keep Bun's event loop alive on Linux after all tests complete, causing `bun test` to hang indefinitely. This does not surface on macOS (event loop drains normally there), making it a Linux-CI-only failure that is hard to reproduce locally. + - **Mitigation:** Always manage SDK lifecycle in `beforeEach`/`afterEach` and call the cleanup method (`close()`, `destroy()`, `disconnect()`) in `afterEach`. Add `timeout-minutes` to CI jobs as a safety net — the `code-pull-request.yml` job is set to 10 minutes to cap any future regressions. ## Compliance and Enforcement ### Automated Enforcement - **Archgate rule** `ARCH-005/test-mirrors-src`: Scans all source files in `src/` and verifies a corresponding `.test.ts` file exists in `tests/`. Severity: `error`. -- **CI pipeline**: `bun test` runs on every pull request. Test failures block merge. +- **CI pipeline**: `bun test --timeout 60000` runs on every pull request. Test failures and per-test timeouts block merge. All workflow jobs have `timeout-minutes` set to prevent indefinite hangs. ### Manual Enforcement @@ -138,9 +207,12 @@ Code reviewers MUST verify: 1. New source files have corresponding test files 2. Tests use temp directories for filesystem operations (no hardcoded paths) -3. Tests clean up after themselves (`afterEach`/`afterAll` cleanup) +3. Tests clean up after themselves (`afterEach`/`afterAll` cleanup) — including both temp directories and external SDK instances +4. Tests that instantiate SDK objects (servers, clients, connections) manage their lifecycle in `beforeEach`/`afterEach`, not inside individual test bodies +5. Tests that call `git commit` on a temp repo configure `user.email` and `user.name` locally before committing ## References - [Bun test runner documentation](https://bun.sh/docs/cli/test) - [ARCH-001 — Command Structure](./ARCH-001-command-structure.md) — In-process execution enables testing commands directly without process spawning +- [ARCH-006 — Dependency Policy](./ARCH-006-dependency-policy.md) — Third-party dependencies introduce runtime behaviors (like event loop retention) that must be accounted for in test teardown diff --git a/.claude/agent-memory/archgate-developer/MEMORY.md b/.claude/agent-memory/archgate-developer/MEMORY.md index 3800baa5..2c9f1b3c 100644 --- a/.claude/agent-memory/archgate-developer/MEMORY.md +++ b/.claude/agent-memory/archgate-developer/MEMORY.md @@ -26,6 +26,8 @@ Skipping steps 2 or 3 is a workflow violation. The user should NEVER have to inv ## Patterns & Fixes +- **`McpServer` event loop retention on Linux** — `new McpServer()` from `@modelcontextprotocol/sdk` creates an `AjvJsonSchemaValidator` (backed by `ajv` + `ajv-formats`) that keeps Bun's event loop alive on Linux after tests complete, causing `bun test` to hang for 30+ minutes. macOS drains the event loop fine — this is a Linux-CI-only failure. Fix: manage server lifecycle in `beforeEach`/`afterEach` and call `await server.close()` in `afterEach`. Also captured in ARCH-005 Don'ts. +- **`git commit` in temp repos requires local identity** — CI runners have no global `user.email`/`user.name` configured. Any test that runs `git commit` on a temp repo MUST call `git config user.email` and `git config user.name` locally after `git init`. Fails with a cryptic `ShellPromise` error in CI; passes locally. Also captured in ARCH-005 Do's. - **Bun import cache-busting**: Bun caches `import()` per-process. For long-running processes (MCP server), append `?t=${Date.now()}` to the import path to force re-reading from disk. Applied in `src/engine/loader.ts`. - **Never use `bunx prettier` directly** — Always use `bun run format` (to fix) or `bun run format:check` (to verify). Using `bunx prettier` can fail or use a different version than the project's devDependency. The same applies to all dev tools: prefer `bun run