diff --git a/.archgate/adrs/ARCH-005-testing-standards.md b/.archgate/adrs/ARCH-005-testing-standards.md index c5695f06..e3d038d3 100644 --- a/.archgate/adrs/ARCH-005-testing-standards.md +++ b/.archgate/adrs/ARCH-005-testing-standards.md @@ -45,6 +45,7 @@ Use Bun's built-in test runner (`bun test`) for all tests. Test files go in `tes - Test public module interfaces, not private implementation details - Use descriptive test names that explain the expected behavior - **When mocking `fetch` in tests, assign directly to `globalThis.fetch`** — use `globalThis.fetch = mockFn as unknown as typeof fetch`. Restore in `afterEach` via `mock.restore()` (from `bun:test`) or by reassigning the original reference before the test. +- **Wrap `spyOn` / `mockImplementation` calls in `try/finally` to guarantee `mockRestore()` runs** — when `expect()` assertions fail, they throw immediately, skipping any `mockRestore()` that follows. The un-restored spy leaks into subsequent tests, causing false positives or false negatives. Pattern: `const spy = spyOn(...).mockImplementation(() => {}); try { /* assertions */ } finally { spy.mockRestore(); }`. Alternatively, create and restore spies in `beforeEach`/`afterEach` hooks instead of inline. ### Don't @@ -57,6 +58,7 @@ Use Bun's built-in test runner (`bun test`) for all tests. Test files go in `tes - Don't skip tests without a tracking issue - Don't import test utilities from `node:test` — use Bun's built-in `bun:test` module - **Don't use `mock.module("node:fetch", ...)` to intercept HTTP fetch calls** — in Bun, the runtime fetch is `globalThis.fetch` and `mock.module` targeting `node:fetch` does not intercept it. The mock silently has no effect: the real network is hit, making tests non-deterministic and dependent on external services. Assign `globalThis.fetch` directly instead (see Do's above). +- **Don't place `mockRestore()` after assertions without `try/finally` protection** — if the assertion throws, the spy is never restored and contaminates subsequent tests. This is especially dangerous when spying on globals like `console.warn` or `globalThis.fetch`, where a leaked spy silently suppresses or redirects output for every test that follows. ## Implementation Pattern @@ -209,6 +211,7 @@ Code reviewers MUST verify: 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 6. Tests that mock HTTP fetch assign `globalThis.fetch` directly — no `mock.module("node:fetch", ...)` usage +7. Tests that use `spyOn` or `mockImplementation` inline (not in `beforeEach`/`afterEach`) wrap the spy lifecycle in `try/finally` to guarantee `mockRestore()` runs even when assertions fail ## References diff --git a/src/engine/git-files.ts b/src/engine/git-files.ts index 1e998ff7..4611340d 100644 --- a/src/engine/git-files.ts +++ b/src/engine/git-files.ts @@ -4,6 +4,11 @@ import { logDebug, logWarn } from "../helpers/log"; +/** Warn when an ADR's resolved file scope exceeds this many files. */ +export const SCOPE_FILE_WARN_THRESHOLD = 1000; +/** Warn when the glob scan phase takes longer than this (milliseconds). */ +export const SCOPE_SCAN_WARN_MS = 2000; + /** * Run a git command using Bun.spawn (cross-platform, no shell). * Bun.$ hangs on Windows due to pipe handling issues — this is the safe alternative. @@ -79,6 +84,7 @@ export async function resolveScopedFiles( ? await getGitTrackedFiles(projectRoot) : null; + const scanStart = performance.now(); const results = await Promise.all( patterns.map(async (pattern) => { const glob = new Bun.Glob(pattern); @@ -94,8 +100,16 @@ export async function resolveScopedFiles( return files; }) ); + const scanMs = performance.now() - scanStart; const all = [...new Set(results.flat())].sort(); + + if (all.length > SCOPE_FILE_WARN_THRESHOLD || scanMs > SCOPE_SCAN_WARN_MS) { + logWarn( + `${label}: Resolved ${all.length} files from patterns: ${patterns.join(", ")} (scan took ${Math.round(scanMs)}ms). Consider narrowing the \`files\` patterns in the ADR frontmatter to improve performance.` + ); + } + logDebug( "Scoped files resolved:", all.length, diff --git a/tests/engine/git-files.test.ts b/tests/engine/git-files.test.ts index 8be7d997..a3b2cea3 100644 --- a/tests/engine/git-files.test.ts +++ b/tests/engine/git-files.test.ts @@ -10,6 +10,7 @@ import { getStagedFiles, getChangedFiles, resolveScopedFiles, + SCOPE_FILE_WARN_THRESHOLD, } from "../../src/engine/git-files"; import { git, safeRmSync } from "../test-utils"; @@ -211,5 +212,60 @@ describe("git-files", () => { expect(warnCalls.some((msg) => msg.includes("ADR BUILD-001"))).toBe(true); warnSpy.mockRestore(); }); + + test("warns when file scope exceeds threshold", async () => { + await git(["init"], tempDir); + mkdirSync(join(tempDir, "src"), { recursive: true }); + const fileCount = SCOPE_FILE_WARN_THRESHOLD + 1; + for (let i = 0; i < fileCount; i++) { + writeFileSync( + join(tempDir, "src", `file-${String(i).padStart(4, "0")}.ts`), + `export const x${i} = ${i};` + ); + } + await git(["add", "."], tempDir); + const warnSpy = spyOn(console, "warn").mockImplementation(() => {}); + try { + const files = await resolveScopedFiles(tempDir, ["src/**/*.ts"], { + adrId: "SCOPE-001", + }); + expect(files.length).toBeGreaterThan(SCOPE_FILE_WARN_THRESHOLD); + const warnCalls = warnSpy.mock.calls.map((args) => args.join(" ")); + expect( + warnCalls.some( + (msg) => + msg.includes("ADR SCOPE-001") && + msg.includes(`${fileCount} files`) && + msg.includes("scan took") && + msg.includes("Consider narrowing") + ) + ).toBe(true); + } finally { + warnSpy.mockRestore(); + } + }, 30_000); + + test("does not warn when file scope is within threshold", async () => { + await git(["init"], tempDir); + mkdirSync(join(tempDir, "src"), { recursive: true }); + for (let i = 0; i < 10; i++) { + writeFileSync( + join(tempDir, "src", `file-${i}.ts`), + `export const x${i} = ${i};` + ); + } + await git(["add", "."], tempDir); + const warnSpy = spyOn(console, "warn").mockImplementation(() => {}); + try { + const files = await resolveScopedFiles(tempDir, ["src/**/*.ts"]); + expect(files.length).toBeLessThanOrEqual(SCOPE_FILE_WARN_THRESHOLD); + const warnCalls = warnSpy.mock.calls.map((args) => args.join(" ")); + expect( + warnCalls.some((msg) => msg.includes("Consider narrowing")) + ).toBe(false); + } finally { + warnSpy.mockRestore(); + } + }); }); });