From a387e2dcb5d43c0aabed6b8311d8a4ff3aa8172f Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 14 Dec 2025 19:44:36 +0000 Subject: [PATCH] refactor(tests): align test files with unit testing rubric - ai-tools.test.ts: Add @scaffold label, replace brittle key counting with spread-equality assertion, add collision detection test - page-write-tools.test.ts: Remove weak "only called" assertion, rely on observable outcomes (result.success, result.linesReplaced) - mcp-bridge.test.ts: Remove call count assertion, keep payload assertions - drive-tools.test.ts: Remove unused chain mocks, add @scaffold note with TODO for DriveRepository seam - search-tools.test.ts: Remove unused chain mocks, add @scaffold note with TODO for SearchService seam These changes follow the unit testing rubric principles: - Test observable outcomes, not internal mechanics - Mock only at boundary seams - Label scaffold/aggregation tests appropriately - Document deferred happy path coverage with seam proposals --- .../lib/ai/core/__tests__/ai-tools.test.ts | 134 ++++++++---------- .../ai/tools/__tests__/drive-tools.test.ts | 27 ++-- .../tools/__tests__/page-write-tools.test.ts | 3 +- .../ai/tools/__tests__/search-tools.test.ts | 42 +++--- .../src/lib/mcp/__tests__/mcp-bridge.test.ts | 3 +- 5 files changed, 92 insertions(+), 117 deletions(-) diff --git a/apps/web/src/lib/ai/core/__tests__/ai-tools.test.ts b/apps/web/src/lib/ai/core/__tests__/ai-tools.test.ts index 94357b934c..fa100273d3 100644 --- a/apps/web/src/lib/ai/core/__tests__/ai-tools.test.ts +++ b/apps/web/src/lib/ai/core/__tests__/ai-tools.test.ts @@ -1,3 +1,12 @@ +/** + * @scaffold - aggregation tripwire + * + * This test validates that pageSpaceTools correctly aggregates all tool modules. + * It's a scaffold test because: + * - The unit is pure wiring/aggregation with no business logic + * - Changes here indicate tools were added/removed/renamed + * - Failures should prompt review of whether the change was intentional + */ import { describe, it, expect, vi } from 'vitest'; // Mock all tool modules to avoid database dependencies @@ -64,91 +73,66 @@ vi.mock('../../tools/web-search-tools', () => ({ })); import { pageSpaceTools } from '../ai-tools'; +import { driveTools } from '../../tools/drive-tools'; +import { pageReadTools } from '../../tools/page-read-tools'; +import { pageWriteTools } from '../../tools/page-write-tools'; +import { searchTools } from '../../tools/search-tools'; +import { taskManagementTools } from '../../tools/task-management-tools'; +import { agentTools } from '../../tools/agent-tools'; +import { agentCommunicationTools } from '../../tools/agent-communication-tools'; +import { webSearchTools } from '../../tools/web-search-tools'; describe('ai-tools', () => { describe('pageSpaceTools aggregation', () => { - it('aggregates all tools from individual modules', () => { - expect(pageSpaceTools).toBeDefined(); - expect(typeof pageSpaceTools).toBe('object'); + it('equals the merged object of all tool modules', () => { + expect(pageSpaceTools).toEqual({ + ...driveTools, + ...pageReadTools, + ...pageWriteTools, + ...searchTools, + ...taskManagementTools, + ...agentTools, + ...agentCommunicationTools, + ...webSearchTools, + }); }); - it('includes driveTools', () => { - expect(pageSpaceTools.list_drives).toBeDefined(); - expect(pageSpaceTools.create_drive).toBeDefined(); - expect(pageSpaceTools.rename_drive).toBeDefined(); - }); - - it('includes pageReadTools', () => { - expect(pageSpaceTools.list_pages).toBeDefined(); - expect(pageSpaceTools.read_page).toBeDefined(); - expect(pageSpaceTools.list_trash).toBeDefined(); - }); - - it('includes pageWriteTools', () => { - expect(pageSpaceTools.replace_lines).toBeDefined(); - expect(pageSpaceTools.create_page).toBeDefined(); - expect(pageSpaceTools.rename_page).toBeDefined(); - expect(pageSpaceTools.trash).toBeDefined(); - expect(pageSpaceTools.restore).toBeDefined(); - expect(pageSpaceTools.move_page).toBeDefined(); - expect(pageSpaceTools.edit_sheet_cells).toBeDefined(); - }); - - it('includes searchTools', () => { - expect(pageSpaceTools.regex_search).toBeDefined(); - expect(pageSpaceTools.glob_search).toBeDefined(); - expect(pageSpaceTools.multi_drive_search).toBeDefined(); - }); - - it('includes taskManagementTools', () => { - expect(pageSpaceTools.update_task).toBeDefined(); - }); - - it('includes agentTools', () => { - expect(pageSpaceTools.update_agent_config).toBeDefined(); - }); - - it('includes agentCommunicationTools', () => { - expect(pageSpaceTools.list_agents).toBeDefined(); - expect(pageSpaceTools.multi_drive_list_agents).toBeDefined(); - expect(pageSpaceTools.ask_agent).toBeDefined(); - }); - - it('includes webSearchTools', () => { - expect(pageSpaceTools.web_search).toBeDefined(); - }); - - it('has expected number of tools', () => { - const toolCount = Object.keys(pageSpaceTools).length; - // Total expected: 3 drive + 3 read + 7 write + 3 search + 1 task + 1 agent + 3 agent-comm + 1 web = 22 - expect(toolCount).toBe(22); - }); - - it('has no undefined tools', () => { - for (const [, tool] of Object.entries(pageSpaceTools)) { - expect(tool).toBeDefined(); - expect(tool).not.toBeNull(); + it('has no key collisions between tool modules', () => { + const moduleKeysets = [ + Object.keys(driveTools), + Object.keys(pageReadTools), + Object.keys(pageWriteTools), + Object.keys(searchTools), + Object.keys(taskManagementTools), + Object.keys(agentTools), + Object.keys(agentCommunicationTools), + Object.keys(webSearchTools), + ]; + + const allKeys = moduleKeysets.flat(); + const uniqueKeys = new Set(allKeys); + + // If there are duplicates, find them for a helpful error message + if (uniqueKeys.size !== allKeys.length) { + const seen = new Set(); + const duplicates: string[] = []; + for (const key of allKeys) { + if (seen.has(key)) { + duplicates.push(key); + } + seen.add(key); + } + throw new Error(`Tool name collisions detected: ${duplicates.join(', ')}`); } - }); - it('has no duplicate tool names', () => { - const toolNames = Object.keys(pageSpaceTools); - const uniqueNames = new Set(toolNames); - expect(uniqueNames.size).toBe(toolNames.length); + expect(uniqueKeys.size).toBe(allKeys.length); }); - }); - describe('tool structure', () => { - it('each tool has a name property', () => { + it('has no undefined or null tools', () => { for (const [name, tool] of Object.entries(pageSpaceTools)) { - expect((tool as { name: string }).name).toBe(name); - } - }); - - it('each tool has a description property', () => { - for (const [, tool] of Object.entries(pageSpaceTools)) { - expect((tool as { description: string }).description).toBeDefined(); - expect(typeof (tool as { description: string }).description).toBe('string'); + if (tool === undefined || tool === null) { + throw new Error(`Tool "${name}" is ${tool}`); + } } }); }); diff --git a/apps/web/src/lib/ai/tools/__tests__/drive-tools.test.ts b/apps/web/src/lib/ai/tools/__tests__/drive-tools.test.ts index 3ee0600467..63f7878b05 100644 --- a/apps/web/src/lib/ai/tools/__tests__/drive-tools.test.ts +++ b/apps/web/src/lib/ai/tools/__tests__/drive-tools.test.ts @@ -1,31 +1,18 @@ import { describe, it, expect, vi, beforeEach } from 'vitest'; -// Mock database and dependencies +// Mock database - only mock what's actually used in tests vi.mock('@pagespace/db', () => ({ db: { - select: vi.fn().mockReturnThis(), - from: vi.fn().mockReturnThis(), - where: vi.fn(), - insert: vi.fn().mockReturnThis(), - values: vi.fn().mockReturnThis(), - returning: vi.fn(), - update: vi.fn().mockReturnThis(), - set: vi.fn().mockReturnThis(), query: { drives: { findFirst: vi.fn() }, - driveMembers: { findFirst: vi.fn() }, }, }, drives: { id: 'id', ownerId: 'ownerId' }, - driveMembers: { driveId: 'driveId' }, eq: vi.fn(), and: vi.fn(), - or: vi.fn(), - isNull: vi.fn(), })); vi.mock('@pagespace/lib/server', () => ({ - getUserDriveAccess: vi.fn(), loggers: { ai: { child: vi.fn(() => ({ @@ -53,6 +40,17 @@ import type { ToolExecutionContext } from '../../core'; const mockDb = vi.mocked(db); +/** + * @scaffold - happy path coverage deferred + * + * These tests cover authentication and validation error paths. + * Happy path tests (list_drives returning data, create_drive success, etc.) + * are deferred because they require either: + * - A repository seam to avoid query-builder chain mocking, OR + * - Integration tests against a real database + * + * TODO: Add DriveRepository seam and test happy paths against it. + */ describe('drive-tools', () => { beforeEach(() => { vi.clearAllMocks(); @@ -72,7 +70,6 @@ describe('drive-tools', () => { driveTools.list_drives.execute({}, context) ).rejects.toThrow('User authentication required'); }); - }); describe('create_drive', () => { diff --git a/apps/web/src/lib/ai/tools/__tests__/page-write-tools.test.ts b/apps/web/src/lib/ai/tools/__tests__/page-write-tools.test.ts index 1709bf6faf..d4cfe60807 100644 --- a/apps/web/src/lib/ai/tools/__tests__/page-write-tools.test.ts +++ b/apps/web/src/lib/ai/tools/__tests__/page-write-tools.test.ts @@ -178,10 +178,11 @@ describe('page-write-tools', () => { context ); + // Observable outcomes prove the operation succeeded expect(result.success).toBe(true); expect(result.linesReplaced).toBe(1); + // Verify permission check was called with correct arguments expect(mockCanUserEditPage).toHaveBeenCalledWith('user-123', 'page-1'); - expect(mockDb.update).toHaveBeenCalled(); }); }); diff --git a/apps/web/src/lib/ai/tools/__tests__/search-tools.test.ts b/apps/web/src/lib/ai/tools/__tests__/search-tools.test.ts index eed709fb18..924d82ae00 100644 --- a/apps/web/src/lib/ai/tools/__tests__/search-tools.test.ts +++ b/apps/web/src/lib/ai/tools/__tests__/search-tools.test.ts @@ -1,29 +1,8 @@ import { describe, it, expect, vi, beforeEach } from 'vitest'; -// Mock database and dependencies -vi.mock('@pagespace/db', () => ({ - db: { - select: vi.fn().mockReturnThis(), - selectDistinct: vi.fn().mockReturnThis(), - from: vi.fn().mockReturnThis(), - where: vi.fn(), - limit: vi.fn(), - query: { - pages: { findFirst: vi.fn() }, - drives: { findFirst: vi.fn() }, - }, - }, - pages: { id: 'id', driveId: 'driveId', content: 'content', title: 'title' }, - drives: { id: 'id', isTrashed: 'isTrashed' }, - eq: vi.fn(), - and: vi.fn(), - sql: vi.fn(), - inArray: vi.fn(), -})); - +// Mock only the boundary we actually test vi.mock('@pagespace/lib/server', () => ({ getUserDriveAccess: vi.fn(), - getUserAccessiblePagesInDriveWithDetails: vi.fn(), })); import { searchTools } from '../search-tools'; @@ -32,6 +11,23 @@ import type { ToolExecutionContext } from '../../core'; const mockGetUserDriveAccess = vi.mocked(getUserDriveAccess); +/** + * @scaffold - happy path coverage deferred + * + * These tests cover authentication and authorization error paths. + * Happy path tests (actual search results, filtering, pagination) are deferred + * because they require either: + * - A SearchRepository seam to avoid complex DB iteration mocking, OR + * - Integration tests against a real database with seeded content + * + * The search logic involves iterating over pages and matching patterns, + * which is impractical to mock without coupling to implementation details. + * + * TODO: Add SearchService seam with methods like: + * - searchByRegex(driveId, pattern, options): SearchResult[] + * - searchByGlob(driveId, pattern, options): SearchResult[] + * Then test happy paths against that seam. + */ describe('search-tools', () => { beforeEach(() => { vi.clearAllMocks(); @@ -70,7 +66,6 @@ describe('search-tools', () => { ) ).rejects.toThrow("You don't have access to this drive"); }); - }); describe('glob_search', () => { @@ -104,7 +99,6 @@ describe('search-tools', () => { ) ).rejects.toThrow("You don't have access to this drive"); }); - }); describe('multi_drive_search', () => { diff --git a/apps/web/src/lib/mcp/__tests__/mcp-bridge.test.ts b/apps/web/src/lib/mcp/__tests__/mcp-bridge.test.ts index cdbb7b1fbd..81f6b2aabc 100644 --- a/apps/web/src/lib/mcp/__tests__/mcp-bridge.test.ts +++ b/apps/web/src/lib/mcp/__tests__/mcp-bridge.test.ts @@ -92,8 +92,7 @@ describe('MCPBridge', () => { // Start the request but don't await it yet const promise = bridge.executeTool('user-123', 'my-server', 'my-tool', { foo: 'bar' }); - // Verify send was called - expect(mockWs.send).toHaveBeenCalledTimes(1); + // Verify send was called with correct payload const sentData = JSON.parse((mockWs.send as ReturnType).mock.calls[0][0]); expect(sentData.type).toBe('tool_execute'); expect(sentData.serverName).toBe('my-server');