diff --git a/packages/integration/src/paste-focus-isolation.test.tsx b/packages/integration/src/paste-focus-isolation.test.tsx index a3a6639a..bfd140dd 100644 --- a/packages/integration/src/paste-focus-isolation.test.tsx +++ b/packages/integration/src/paste-focus-isolation.test.tsx @@ -6,37 +6,27 @@ */ import { describe, it, expect, mock, afterEach, beforeEach } from 'bun:test'; import { fireEvent, cleanup, act, within } from '@testing-library/react'; - -// Mock dependencies required by MessagePanel -const mockSendWorkerMessage = mock(() => - Promise.resolve({ - message: { - id: 'msg-1', - sessionId: 'session-1', - fromWorkerId: 'user', - fromWorkerName: 'User', - toWorkerId: 'worker-1', - toWorkerName: 'Worker 1', - content: '', - timestamp: new Date().toISOString(), - }, - }) -); -mock.module('@agent-console/client/src/lib/api', () => ({ - sendWorkerMessage: mockSendWorkerMessage, -})); -mock.module('@agent-console/client/src/lib/worker-websocket', () => ({ - sendInput: mock(() => true), -})); - import { MessagePanel } from '@agent-console/client/src/components/sessions/MessagePanel'; import { renderWithRouter } from '@agent-console/client/src/test/renderWithRouter'; import { _getDraftsMap } from '@agent-console/client/src/hooks/useDraftMessage'; +// MessagePanel resolves its send action via an injected `onSend` prop, not +// via a module-level import of `lib/api` / `lib/worker-websocket` -- the +// prior `mock.module()` of those two modules was a leftover from before +// that DI seam existed and mocked exports MessagePanel no longer reads. +// Neither test below triggers a send, but `onSend` is a required prop. +// Using the real DI seam (Pattern 1) instead of `mock.module()` avoids +// process-globally poisoning sibling integration tests that import +// `lib/api` / `lib/worker-websocket` for real in the same bun:test +// process (e.g. system-api-boundary.test.ts) -- the live #1225-class +// poisoner this file used to be (`.claude/rules/testing.md` Anti-Pattern #2). +const mockOnSend = mock(() => Promise.resolve()); + const defaultProps = { sessionId: 'session-1', targetWorkerId: 'worker-1', newMessage: null, + onSend: mockOnSend, }; describe('Paste Focus Isolation (#523)', () => { diff --git a/packages/server/src/__tests__/api.test.ts b/packages/server/src/__tests__/api.test.ts index e5acef4b..aa0ec972 100644 --- a/packages/server/src/__tests__/api.test.ts +++ b/packages/server/src/__tests__/api.test.ts @@ -11,7 +11,7 @@ import type { } from '@agent-console/shared'; import { setupMemfs, cleanupMemfs, createMockGitRepoFiles } from './utils/mock-fs-helper.js'; import { mockProcess, resetProcessMock } from './utils/mock-process-helper.js'; -import { MockPty, createMockPtyFactory } from './utils/mock-pty.js'; +import { createMockPtyFactory } from './utils/mock-pty.js'; import { mockGit, GitError } from './utils/mock-git-helper.js'; // Set up test config directory BEFORE any service imports to ensure @@ -23,29 +23,8 @@ process.env.AGENT_CONSOLE_HOME = TEST_CONFIG_DIR; // Infrastructure Mocks (must be before any service imports) // ============================================================================= -// Track PTY instances -const mockPtyInstances: MockPty[] = []; -let nextPtyPid = 10000; - -// Mock pty-provider module to avoid spawning real PTY processes in tests -mock.module('../lib/pty-provider.js', () => ({ - bunPtyProvider: { - spawn: () => { - const pty = new MockPty(nextPtyPid++); - mockPtyInstances.push(pty); - return pty; - }, - }, -})); - // Note: process-utils is mocked via mock-process-helper.js (imported above) -// Mock open package to prevent actual file opening -const mockOpen = mock(async () => {}); -mock.module('open', () => ({ - default: mockOpen, -})); - // Mock session-metadata-suggester to avoid running actual agent commands const mockSuggestSessionMetadata = mock(async () => ({ branch: 'suggested-branch', @@ -114,7 +93,7 @@ import { WorkerOutputFileManager } from '../lib/worker-output-file.js'; import { SystemCapabilitiesService } from '../services/system-capabilities-service.js'; import { WorktreeService } from '../services/worktree-service.js'; import type { AppBindings } from '../app-context.js'; -import { asAppContext, TEST_AUTH_USER, ensureTestAuthUser } from './test-utils.js'; +import { asAppContext, TEST_AUTH_USER, ensureTestAuthUser, mockOpen } from './test-utils.js'; import { SingleUserMode } from '../services/user-mode.js'; import { McpTokenRegistry } from '../mcp/mcp-auth.js'; @@ -201,8 +180,6 @@ describe('API Routes Integration', () => { process.env.AGENT_CONSOLE_HOME = TEST_CONFIG_DIR; // Reset PTY tracking - mockPtyInstances.length = 0; - nextPtyPid = 10000; ptyFactory.reset(); // Reset process tracking diff --git a/packages/server/src/routes/__tests__/system.test.ts b/packages/server/src/routes/__tests__/system.test.ts index c5f13bc1..4f8cf54b 100644 --- a/packages/server/src/routes/__tests__/system.test.ts +++ b/packages/server/src/routes/__tests__/system.test.ts @@ -1,15 +1,8 @@ -import { describe, it, expect, beforeEach, afterEach, mock } from 'bun:test'; +import { describe, it, expect, beforeEach, afterEach } from 'bun:test'; import { Hono } from 'hono'; import type { AppBindings } from '../../app-context.js'; import { asAppContext } from '../../__tests__/test-utils.js'; -// Mock open package BEFORE importing mock-fs-helper -// The open package internally uses fs and needs to be mocked first -const mockOpen = mock(async () => {}); -mock.module('open', () => ({ - default: mockOpen, -})); - // Import mock-fs-helper to set up memfs mocks import { setupMemfs, cleanupMemfs } from '../../__tests__/utils/mock-fs-helper.js'; import { createMockSystemCapabilities } from '../../__tests__/utils/mock-system-capabilities-helper.js'; diff --git a/packages/server/src/services/__tests__/worktree-creation-service.test.ts b/packages/server/src/services/__tests__/worktree-creation-service.test.ts index 48419d1e..dd3a57e5 100644 --- a/packages/server/src/services/__tests__/worktree-creation-service.test.ts +++ b/packages/server/src/services/__tests__/worktree-creation-service.test.ts @@ -50,17 +50,11 @@ const mockWorktreeService = { executeHookCommand: mockExecuteHookCommand, }; -// --- Mock logger --- -mock.module('../../lib/logger.js', () => ({ - createLogger: () => ({ - info: () => {}, - warn: () => {}, - error: () => {}, - debug: () => {}, - }), -})); - -// Import after mocks +// No logger mock needed: `lib/logger.js`'s pino instance is already disabled +// (`enabled: !isTest`) whenever `NODE_ENV === 'test'`, which Bun sets by +// default for `bun test` runs. The real (silent) logger is used directly, +// avoiding a process-global `mock.module()` of a module other test files +// import for real (`.claude/rules/testing.md` Anti-Pattern #2). const { createWorktreeWithSession } = await import('../worktree-creation-service.js'); // --- Helpers --- diff --git a/packages/server/src/services/__tests__/worktree-deletion-service.test.ts b/packages/server/src/services/__tests__/worktree-deletion-service.test.ts index 99183f92..2d017651 100644 --- a/packages/server/src/services/__tests__/worktree-deletion-service.test.ts +++ b/packages/server/src/services/__tests__/worktree-deletion-service.test.ts @@ -48,15 +48,11 @@ const mockWorktreeService = { removeOrphanedWorktree: mockRemoveOrphanedWorktree, }; -// --- Mock logger --- -mock.module('../../lib/logger.js', () => ({ - createLogger: () => ({ - info: () => {}, - warn: () => {}, - error: () => {}, - debug: () => {}, - }), -})); +// No logger mock needed: `lib/logger.js`'s pino instance is already disabled +// (`enabled: !isTest`) whenever `NODE_ENV === 'test'`, which Bun sets by +// default for `bun test` runs. The real (silent) logger is used directly, +// avoiding a process-global `mock.module()` of a module other test files +// import for real (`.claude/rules/testing.md` Anti-Pattern #2). // Note: The Bun shell ($) tagged template literal cannot be reliably mocked // via mock.module('bun', ...). The gitStatus capture path runs a real `git -C` diff --git a/packages/server/src/services/inbound/__tests__/diff-worker-handler.test.ts b/packages/server/src/services/inbound/__tests__/diff-worker-handler.test.ts index d23ff2a7..a923c681 100644 --- a/packages/server/src/services/inbound/__tests__/diff-worker-handler.test.ts +++ b/packages/server/src/services/inbound/__tests__/diff-worker-handler.test.ts @@ -1,13 +1,25 @@ -import { describe, it, expect, mock } from 'bun:test'; +import { describe, it, expect, mock, spyOn, beforeEach, afterEach } from 'bun:test'; import type { InboundSystemEvent, Session, Worker } from '@agent-console/shared'; import { createInboundHandlers, type InboundEventHandler, type InboundHandlerDependencies, type EventTarget } from '../handlers.js'; import { buildWorktreeSession } from '../../../__tests__/utils/build-test-data.js'; - -// Mock triggerRefresh at the module level since it's a standalone function import +import * as gitDiffServiceModule from '../../git-diff-service.js'; + +// `triggerRefresh` is a standalone function import in handlers.ts (no DI +// seam via InboundHandlerDependencies exists for it yet -- see the PR +// description for this conversion). spyOn() on the real module keeps this +// test file-scoped and restorable, instead of process-globally poisoning +// every other importer of git-diff-service.js the way mock.module() would +// (`.claude/rules/testing.md` Anti-Pattern #2). const mockTriggerRefresh = mock(() => {}); -mock.module('../../git-diff-service.js', () => ({ - triggerRefresh: mockTriggerRefresh, -})); +let triggerRefreshSpy: ReturnType; + +beforeEach(() => { + triggerRefreshSpy = spyOn(gitDiffServiceModule, 'triggerRefresh').mockImplementation(mockTriggerRefresh); +}); + +afterEach(() => { + triggerRefreshSpy.mockRestore(); +}); function createEvent(type: 'ci:completed' | 'pr:merged' = 'ci:completed'): InboundSystemEvent { return { diff --git a/scripts/__tests__/check-mock-module-poisoners.test.mjs b/scripts/__tests__/check-mock-module-poisoners.test.mjs index 45930206..dc506f69 100644 --- a/scripts/__tests__/check-mock-module-poisoners.test.mjs +++ b/scripts/__tests__/check-mock-module-poisoners.test.mjs @@ -319,8 +319,11 @@ describe('findDefaultFiles — scan glob', () => { }); describe('KNOWN_VIOLATIONS / SANCTIONED_LOCATIONS — baseline integrity', () => { - it('every KNOWN_VIOLATIONS entry has a file, specifier, and one-line reason', () => { - expect(KNOWN_VIOLATIONS.length).toBeGreaterThan(0); + it('every KNOWN_VIOLATIONS entry (if any) has a file, specifier, and one-line reason', () => { + // The original 8-entry baseline was fully converted in Issue #1238; + // the array is expected to be empty until a new justified exception is + // added (see the file-exclusive exception in testing.md Anti-Pattern #2). + expect(KNOWN_VIOLATIONS.length).toBeGreaterThanOrEqual(0); for (const entry of KNOWN_VIOLATIONS) { expect(typeof entry.file).toBe('string'); expect(typeof entry.specifier).toBe('string'); diff --git a/scripts/check-mock-module-poisoners.mjs b/scripts/check-mock-module-poisoners.mjs index 88e6392b..688ccab0 100644 --- a/scripts/check-mock-module-poisoners.mjs +++ b/scripts/check-mock-module-poisoners.mjs @@ -340,56 +340,15 @@ export async function runCheck({ cwd = process.cwd(), files, allowlist = KNOWN_V // --------------------------------------------------------------------------- // KNOWN_VIOLATIONS — see the file header for the allowlist strategy. Keys -// are `file + specifier` pairs. Enumerated by running this detector -// against `main` at the time this script landed (Issue #1226); the -// follow-up cleanup Issue (see .claude/rules/testing.md Anti-Pattern #2) -// converts these to DI seam / spyOn / central registry migration per the -// #977 playbook. Remove an entry here in the same PR that converts it — -// a stale entry left behind fails CI by design. +// are `file + specifier` pairs. The original 8-entry baseline (enumerated +// against `main` at the time this script landed, Issue #1226) was fully +// converted to DI seam / spyOn / central-registry migration per the #977 +// playbook in Issue #1238. Empty now; a new justified entry is added the +// same way -- via the permitted file-exclusive exception in +// `.claude/rules/testing.md` Anti-Pattern #2 -- and removed in the same PR +// that converts it. A stale entry left behind fails CI by design. // --------------------------------------------------------------------------- -export const KNOWN_VIOLATIONS = [ - { - file: 'packages/integration/src/paste-focus-isolation.test.tsx', - specifier: '@agent-console/client/src/lib/api', - reason: - 'Live #1225-class poisoner: mocked here but imported for real by sibling integration tests in the same bun:test process; priority conversion target.', - }, - { - file: 'packages/integration/src/paste-focus-isolation.test.tsx', - specifier: '@agent-console/client/src/lib/worker-websocket', - reason: 'Same file/priority as the api.ts entry above.', - }, - { - file: 'packages/server/src/__tests__/api.test.ts', - specifier: '../lib/pty-provider.js', - reason: 'Ad-hoc baseline; large integration-style test file, not yet converted to DI.', - }, - { - file: 'packages/server/src/__tests__/api.test.ts', - specifier: 'open', - reason: 'Ad-hoc baseline, duplicate of the system.test.ts open mock below.', - }, - { - file: 'packages/server/src/routes/__tests__/system.test.ts', - specifier: 'open', - reason: 'Ad-hoc baseline, duplicate of the api.test.ts open mock above.', - }, - { - file: 'packages/server/src/services/__tests__/worktree-creation-service.test.ts', - specifier: '../../lib/logger.js', - reason: 'Ad-hoc baseline; logger mock, candidate for DI conversion.', - }, - { - file: 'packages/server/src/services/__tests__/worktree-deletion-service.test.ts', - specifier: '../../lib/logger.js', - reason: 'Ad-hoc baseline; logger mock, candidate for DI conversion.', - }, - { - file: 'packages/server/src/services/inbound/__tests__/diff-worker-handler.test.ts', - specifier: '../../git-diff-service.js', - reason: 'Ad-hoc baseline; standalone function mock, candidate for DI via InboundHandlerDependencies.', - }, -]; +export const KNOWN_VIOLATIONS = []; // --------------------------------------------------------------------------- // CLI wrapper