From e8cf33daad58a35fffa65aad20b991dcf7885a1b Mon Sep 17 00:00:00 2001 From: A <258483684+la14-1@users.noreply.github.com> Date: Fri, 27 Mar 2026 02:26:34 -0700 Subject: [PATCH] test: remove duplicate and theatrical tests (#3057) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * test: remove duplicate in-memory cache tests and fix missing cache reset Two tests verifying in-memory cache returns the same instance without re-fetching were duplicated across manifest.test.ts and manifest-cache-lifecycle.test.ts. The strongest version (checks both object identity and fetch call count) already lives in the combined-fallback-chain describe block in manifest-cache-lifecycle.test.ts, so the two weaker duplicates are removed. Also fixes missing _resetCacheForTesting() calls in beforeEach for the in-memory cache behavior and combined fallback chain describe blocks — without it, in-memory state from a prior test could contaminate later tests. Co-Authored-By: Claude Sonnet 4.6 * test: remove duplicate and theatrical tests Consolidate 5 near-identical manifest rejection tests into a single data-driven loop, and collapse 4 identical logging-function smoke tests into a data-driven loop. Both changes eliminate copy-paste repetition while preserving exact test coverage. Co-Authored-By: Claude Sonnet 4.6 --------- Co-authored-by: spawn-qa-bot Co-authored-by: Claude Sonnet 4.6 --- .../manifest-cache-lifecycle.test.ts | 13 +- packages/cli/src/__tests__/manifest.test.ts | 119 ++++++------------ packages/cli/src/__tests__/ui-cov.test.ts | 47 ++++--- 3 files changed, 69 insertions(+), 110 deletions(-) diff --git a/packages/cli/src/__tests__/manifest-cache-lifecycle.test.ts b/packages/cli/src/__tests__/manifest-cache-lifecycle.test.ts index 022e837b..9f12badf 100644 --- a/packages/cli/src/__tests__/manifest-cache-lifecycle.test.ts +++ b/packages/cli/src/__tests__/manifest-cache-lifecycle.test.ts @@ -4,7 +4,7 @@ import type { TestEnvironment } from "./test-helpers"; import { afterEach, beforeEach, describe, expect, it, mock } from "bun:test"; import { existsSync, mkdirSync, rmSync, utimesSync, writeFileSync } from "node:fs"; import { join } from "node:path"; -import { agentKeys, countImplemented, loadManifest } from "../manifest"; +import { _resetCacheForTesting, agentKeys, countImplemented, loadManifest } from "../manifest"; import { createMockManifest, setupTestEnvironment, teardownTestEnvironment } from "./test-helpers"; /** @@ -239,6 +239,7 @@ describe("Manifest Cache Lifecycle", () => { beforeEach(() => { env = setupTestEnvironment(); + _resetCacheForTesting(); }); afterEach(() => { @@ -255,15 +256,6 @@ describe("Manifest Cache Lifecycle", () => { // fetch should have been called at least twice (once per forceRefresh) expect(fetchMock.mock.calls.length).toBeGreaterThanOrEqual(2); }); - - it("should return same instance without forceRefresh", async () => { - global.fetch = mock(() => Promise.resolve(new Response(JSON.stringify(mockManifest)))); - - const manifest1 = await loadManifest(true); - const manifest2 = await loadManifest(false); - - expect(manifest1).toBe(manifest2); - }); }); describe("combined fallback chain: invalid fetch + stale cache", () => { @@ -271,6 +263,7 @@ describe("Manifest Cache Lifecycle", () => { beforeEach(() => { env = setupTestEnvironment(); + _resetCacheForTesting(); }); afterEach(() => { diff --git a/packages/cli/src/__tests__/manifest.test.ts b/packages/cli/src/__tests__/manifest.test.ts index e025f52e..8afed3b9 100644 --- a/packages/cli/src/__tests__/manifest.test.ts +++ b/packages/cli/src/__tests__/manifest.test.ts @@ -171,15 +171,6 @@ describe("manifest", () => { expect(global.fetch).toHaveBeenCalled(); }); - it("returns in-memory cache on second call without fetching", async () => { - const fetchMock = mock(async () => new Response(JSON.stringify(mockManifest))); - global.fetch = fetchMock; - await loadManifest(); - const fetchCount = fetchMock.mock.calls.length; - await loadManifest(); - expect(fetchMock.mock.calls.length).toBe(fetchCount); - }); - it("falls back to stale cache when fetch fails", async () => { const cacheDir = join(env.testDir, "spawn"); mkdirSync(cacheDir, { @@ -217,30 +208,22 @@ describe("manifest", () => { await expect(loadManifest(true)).rejects.toThrow("Cannot load manifest"); }); - it("throws when manifest from GitHub is invalid", async () => { - const consoleSpy = spyOn(console, "error").mockImplementation(() => {}); - global.fetch = mock( - async () => + const invalidManifestCases: Array<{ + label: string; + fetchImpl: () => Promise; + }> = [ + { + label: "non-manifest shape", + fetchImpl: async () => new Response( JSON.stringify({ not: "a manifest", }), ), - ); - - const cacheFile = join(env.testDir, "spawn", "manifest.json"); - if (existsSync(cacheFile)) { - rmSync(cacheFile); - } - - await expect(loadManifest(true)).rejects.toThrow("Cannot load manifest"); - consoleSpy.mockRestore(); - }); - - it("rejects manifest with string agents field", async () => { - const consoleSpy = spyOn(console, "error").mockImplementation(() => {}); - global.fetch = mock( - async () => + }, + { + label: "string agents field", + fetchImpl: async () => new Response( JSON.stringify({ agents: "claude", @@ -248,21 +231,10 @@ describe("manifest", () => { matrix: {}, }), ), - ); - - const cacheFile = join(env.testDir, "spawn", "manifest.json"); - if (existsSync(cacheFile)) { - rmSync(cacheFile); - } - - await expect(loadManifest(true)).rejects.toThrow("Cannot load manifest"); - consoleSpy.mockRestore(); - }); - - it("rejects manifest with array clouds field", async () => { - const consoleSpy = spyOn(console, "error").mockImplementation(() => {}); - global.fetch = mock( - async () => + }, + { + label: "array clouds field", + fetchImpl: async () => new Response( JSON.stringify({ agents: {}, @@ -273,21 +245,10 @@ describe("manifest", () => { matrix: {}, }), ), - ); - - const cacheFile = join(env.testDir, "spawn", "manifest.json"); - if (existsSync(cacheFile)) { - rmSync(cacheFile); - } - - await expect(loadManifest(true)).rejects.toThrow("Cannot load manifest"); - consoleSpy.mockRestore(); - }); - - it("rejects manifest with numeric matrix field", async () => { - const consoleSpy = spyOn(console, "error").mockImplementation(() => {}); - global.fetch = mock( - async () => + }, + { + label: "numeric matrix field", + fetchImpl: async () => new Response( JSON.stringify({ agents: {}, @@ -295,31 +256,27 @@ describe("manifest", () => { matrix: 42, }), ), - ); + }, + { + label: "network error", + fetchImpl: async () => { + throw new Error("Network timeout"); + }, + }, + ]; - const cacheFile = join(env.testDir, "spawn", "manifest.json"); - if (existsSync(cacheFile)) { - rmSync(cacheFile); - } - - await expect(loadManifest(true)).rejects.toThrow("Cannot load manifest"); - consoleSpy.mockRestore(); - }); - - it("throws when network errors occur and no cache exists", async () => { - const consoleSpy = spyOn(console, "error").mockImplementation(() => {}); - global.fetch = mock(async () => { - throw new Error("Network timeout"); + for (const { label, fetchImpl } of invalidManifestCases) { + it(`rejects invalid manifest (${label})`, async () => { + const consoleSpy = spyOn(console, "error").mockImplementation(() => {}); + global.fetch = mock(fetchImpl); + const cacheFile = join(env.testDir, "spawn", "manifest.json"); + if (existsSync(cacheFile)) { + rmSync(cacheFile); + } + await expect(loadManifest(true)).rejects.toThrow("Cannot load manifest"); + consoleSpy.mockRestore(); }); - - const cacheFile = join(env.testDir, "spawn", "manifest.json"); - if (existsSync(cacheFile)) { - rmSync(cacheFile); - } - - await expect(loadManifest(true)).rejects.toThrow("Cannot load manifest"); - consoleSpy.mockRestore(); - }); + } }); }); diff --git a/packages/cli/src/__tests__/ui-cov.test.ts b/packages/cli/src/__tests__/ui-cov.test.ts index 2bcf2092..2ec817c0 100644 --- a/packages/cli/src/__tests__/ui-cov.test.ts +++ b/packages/cli/src/__tests__/ui-cov.test.ts @@ -62,25 +62,34 @@ afterEach(() => { // ── Logging functions ────────────────────────────────────────────── describe("logging functions", () => { - it("logInfo writes green text to stderr", () => { - logInfo("test info"); - expect(stderrOutput.join("")).toContain("test info"); - }); - - it("logWarn writes yellow text to stderr", () => { - logWarn("test warn"); - expect(stderrOutput.join("")).toContain("test warn"); - }); - - it("logError writes red text to stderr", () => { - logError("test error"); - expect(stderrOutput.join("")).toContain("test error"); - }); - - it("logStep writes cyan text to stderr", () => { - logStep("test step"); - expect(stderrOutput.join("")).toContain("test step"); - }); + for (const [fn, msg] of [ + [ + logInfo, + "test info", + ], + [ + logWarn, + "test warn", + ], + [ + logError, + "test error", + ], + [ + logStep, + "test step", + ], + ] satisfies Array< + [ + (msg: string) => void, + string, + ] + >) { + it(`${fn.name} writes message to stderr`, () => { + fn(msg); + expect(stderrOutput.join("")).toContain(msg); + }); + } it("logStepInline writes message (newline-terminated in non-TTY)", () => { logStepInline("inline msg");