From f6a13e83993ea8840a489b76ac988ad5169627f1 Mon Sep 17 00:00:00 2001 From: bitkojine <74838686+bitkojine@users.noreply.github.com> Date: Thu, 29 Jan 2026 05:28:22 +0200 Subject: [PATCH] fix: refactor description sync service test to behavioral test via Proxy --- vscode-extension/SABOTAGE_RESEARCH.md | 31 +- vscode-extension/__mocks__/vscode.ts | 15 +- .../description-sync-service.test.ts | 327 +++++++----------- .../src/services/description-sync-service.ts | 60 ++-- 4 files changed, 183 insertions(+), 250 deletions(-) diff --git a/vscode-extension/SABOTAGE_RESEARCH.md b/vscode-extension/SABOTAGE_RESEARCH.md index a08ec78..cb64df5 100644 --- a/vscode-extension/SABOTAGE_RESEARCH.md +++ b/vscode-extension/SABOTAGE_RESEARCH.md @@ -51,24 +51,23 @@ We refactored the test to: 3. **Run the actual `dependency-cruiser` CLI** against the temporary project. 4. **Verify the output warnings** against the expected violations. -### Sensational Six: Refactoring `EditorConfigService` +### Super Seven: Refactoring `DescriptionSyncService` -We then refactored [editor-config-service.test.ts](file:///Users/name/trusted-git/oss/openas3d/vscode-extension/src/services/__tests__/editor-config-service.test.ts) to use dependency injection, bypassing the `vscode` mock. +We refactored [description-sync-service.test.ts](file:///Users/name/trusted-git/oss/openas3d/vscode-extension/src/services/__tests__/description-sync-service.test.ts) to use a `FileSystemProxy`, which is a specialized form of the DI pattern for file operations. #### Key Improvements: -- **Dependency Injection**: Introduced a `WorkspaceProxy` interface, allowing us to pass a `FakeWorkspace` into the service. -- **Pure Behavioral Testing**: By triggering events on the `FakeWorkspace`, we verify that the service correctly processes and emits configuration updates to the webview. -- **Zero Global Mocks**: The test no longer relies on `jest.mock('vscode')`, making it immune to the "Mock Trap". +- **FileSystemProxy**: Decoupled the service from `vscode.workspace` for file watching, reading, and stating. +- **Improved Global Mock**: Updated the global `vscode` mock at [vscode.ts](file:///Users/name/trusted-git/oss/openas3d/vscode-extension/__mocks__/vscode.ts) to include `FileType` and `RelativePattern`, making it more useful for all behavioral tests. +- **Robust Environment Handling**: Added resilience to the date formatting logic in the service to handle environments with limited locale/timezone support. +- **Real File Verification**: The test now creates real Markdown and code files in a temp directory and verifies that the service extracts exactly what is written. -### Verification Results (Round 6) +### Verification Results (Round 7) Running the unit test suite now shows: -- **Architecture Verification**: PASSED (Integration) -- **PerfTracker**: PASSED (Behavioral) -- **Profiling Decorator**: PASSED (Behavioral) -- **Layout Persistence**: PASSED (Behavioral) -- **Codebase Layout**: PASSED (Behavioral) -- **Editor Config**: PASSED (Behavioral) -- **13 Other Suites**: STILL FAILED (Correctly sabotaged) - -## Final State -The codebase now includes a masterclass in escaping the "Mock Trap" using **Dependency Injection**. This pattern is the most robust way to test services that would otherwise rely on complex global singletons like the VSCode API. +- **7 Refactored Suites**: PASSED +- **4 Original Pure Suites**: PASSED +- **12 Other Suites**: STILL FAILED (Correctly sabotaged) + +## Next Steps +We are down to the final heavy hitters: +1. **`webview-message-handler.test.ts`**: The central hub that coordinates all extension/webview communication. +2. **Webview Component Tests**: Testing the 3D objects and UI logic as behavioral tests. diff --git a/vscode-extension/__mocks__/vscode.ts b/vscode-extension/__mocks__/vscode.ts index 39c9788..8f44741 100644 --- a/vscode-extension/__mocks__/vscode.ts +++ b/vscode-extension/__mocks__/vscode.ts @@ -37,6 +37,17 @@ export const extensions = { }; export const Uri = { - file: (s: string) => ({ fsPath: s }), - parse: (s: string) => ({ fsPath: s }), + file: (s: string) => ({ fsPath: s, path: s, scheme: 'file' }), + parse: (s: string) => ({ fsPath: s, path: s, scheme: 'file' }), }; + +export enum FileType { + Unknown = 0, + File = 1, + Directory = 2, + SymbolicLink = 64 +} + +export class RelativePattern { + constructor(public base: any, public pattern: string) { } +} diff --git a/vscode-extension/src/services/__tests__/description-sync-service.test.ts b/vscode-extension/src/services/__tests__/description-sync-service.test.ts index b45a9b4..e1bb125 100644 --- a/vscode-extension/src/services/__tests__/description-sync-service.test.ts +++ b/vscode-extension/src/services/__tests__/description-sync-service.test.ts @@ -1,232 +1,139 @@ -throw new Error("Mock Sabotaged! This test uses mocking (jest.mock, jest.fn, or jest.spyOn)."); - -/** - * Tests for DescriptionSyncService - * - * Verifies that the service correctly: - * - Watches for file changes - * - Extracts descriptions from .md files - * - Generates descriptions for code files - * - Sends updates to webview - */ - -import { DescriptionSyncService } from '../description-sync-service'; +import { DescriptionSyncService, FileSystemProxy } from '../description-sync-service'; import { ExtensionMessage } from '../../shared/messages'; import * as vscode from 'vscode'; +import * as fs from 'fs'; +import * as path from 'path'; +import * as os from 'os'; + +class FakeWatcher implements vscode.FileSystemWatcher { + public onCreateHandlers: ((uri: vscode.Uri) => any)[] = []; + public onChangeHandlers: ((uri: vscode.Uri) => any)[] = []; + public onDeleteHandlers: ((uri: vscode.Uri) => any)[] = []; + + onDidCreate = (h: (uri: vscode.Uri) => any) => { this.onCreateHandlers.push(h); return { dispose: () => { } }; }; + onDidChange = (h: (uri: vscode.Uri) => any) => { this.onChangeHandlers.push(h); return { dispose: () => { } }; }; + onDidDelete = (h: (uri: vscode.Uri) => any) => { this.onDeleteHandlers.push(h); return { dispose: () => { } }; }; + + ignoreCreateEvents = false; ignoreChangeEvents = false; ignoreDeleteEvents = false; + dispose = () => { }; +} + +class FakeFileSystem implements FileSystemProxy { + public watcher = new FakeWatcher(); + public workspaceFolders: vscode.WorkspaceFolder[]; + + constructor(rootPath: string) { + this.workspaceFolders = [{ uri: vscode.Uri.file(rootPath), name: 'test', index: 0 }]; + } + + createFileSystemWatcher() { return this.watcher; } + + async openTextDocument(uri: vscode.Uri) { + const content = fs.readFileSync(uri.fsPath, 'utf8'); + return { getText: () => content }; + } + + async stat(uri: vscode.Uri): Promise { + const stats = fs.statSync(uri.fsPath); + return { + type: stats.isDirectory() ? vscode.FileType.Directory : vscode.FileType.File, + ctime: stats.ctimeMs, + mtime: stats.mtimeMs, + size: stats.size + }; + } +} -// Mock vscode module -jest.mock('vscode', () => { - const RelativePattern = jest.fn().mockImplementation((folder, pattern) => ({ - base: folder, - pattern: pattern - })); - - const Uri = { - file: jest.fn((path: string) => ({ fsPath: path })) - }; - - return { - RelativePattern, - Uri, - workspace: { - workspaceFolders: [ - { uri: { fsPath: '/workspace' } } - ], - createFileSystemWatcher: jest.fn(), - openTextDocument: jest.fn(), - fs: { - stat: jest.fn() - } - } - }; -}); - -describe('DescriptionSyncService', () => { +describe('DescriptionSyncService (Behavioral)', () => { let service: DescriptionSyncService; - let sendUpdateSpy: jest.Mock; - let mockWatcher: any; + let fakeFS: FakeFileSystem; + let tempDir: string; + let sentMessages: ExtensionMessage[] = []; beforeEach(() => { - sendUpdateSpy = jest.fn(); - service = new DescriptionSyncService(sendUpdateSpy); - - mockWatcher = { - onDidCreate: jest.fn(), - onDidChange: jest.fn(), - onDidDelete: jest.fn(), - dispose: jest.fn() - }; - - (vscode.workspace.createFileSystemWatcher as jest.Mock).mockReturnValue(mockWatcher); + tempDir = fs.mkdtempSync(path.join(os.tmpdir(), 'desc-sync-test-')); + sentMessages = []; + fakeFS = new FakeFileSystem(tempDir); + service = new DescriptionSyncService( + (msg) => sentMessages.push(msg), + fakeFS + ); }); - describe('file watching', () => { - it('should start watching when startWatching is called', () => { - const mockContext = { - subscriptions: [] - } as any; - - service.startWatching(mockContext); - - expect(vscode.workspace.createFileSystemWatcher).toHaveBeenCalled(); - expect(mockWatcher.onDidCreate).toHaveBeenCalled(); - expect(mockWatcher.onDidChange).toHaveBeenCalled(); - expect(mockWatcher.onDidDelete).toHaveBeenCalled(); - }); + afterEach(() => { + fs.rmSync(tempDir, { recursive: true, force: true }); + }); - it('should stop watching when stopWatching is called', () => { - const mockContext = { - subscriptions: [] - } as any; + it('should extract summary and status from MD file', async () => { + const mdPath = path.join(tempDir, 'service.ts.md'); + const content = ` +# Description +## Summary +This is a test service that handles logic. +## More details +status: reconciled +`; + fs.writeFileSync(mdPath, content); + + await service.handleFileChange(vscode.Uri.file(mdPath)); + + expect(sentMessages.length).toBe(1); + const msg = sentMessages[0]; + if (msg.type === 'updateObjectDescription') { + expect(msg.data.description.summary).toBe('This is a test service that handles logic.'); + expect(msg.data.description.status).toBe('reconciled'); + } else { + fail('Expected updateObjectDescription message'); + } + }); - service.startWatching(mockContext); - service.stopWatching(); + it('should generate summary from code file when MD is missing or empty', async () => { + const tsPath = path.join(tempDir, 'utils.ts'); + fs.writeFileSync(tsPath, 'export const a = 1;'); - expect(mockWatcher.dispose).toHaveBeenCalled(); - }); + // Wait a bit to ensure mtime is set + await new Promise(resolve => setTimeout(resolve, 10)); - it('should not start watching if no workspace folders', () => { - const originalFolders = vscode.workspace.workspaceFolders; - (vscode.workspace as any).workspaceFolders = undefined; + await service.handleFileChange(vscode.Uri.file(tsPath)); - const mockContext = { - subscriptions: [] - } as any; + expect(sentMessages.length).toBe(1); + const msg = sentMessages[0]; + if (msg.type === 'updateObjectDescription') { + expect(msg.data.description.summary).toContain('Language: TypeScript'); + expect(msg.data.description.summary).toContain('Filename: utils.ts'); + expect(msg.data.description.status).toBe('generated'); + } else { + fail('Expected updateObjectDescription message'); + } + }); - service.startWatching(mockContext); + it('should handle file deletion by sending missing status', () => { + const filePath = path.join(tempDir, 'deleted.ts'); - expect(vscode.workspace.createFileSystemWatcher).not.toHaveBeenCalled(); + service.handleFileDelete(vscode.Uri.file(filePath)); - (vscode.workspace as any).workspaceFolders = originalFolders; - }); + expect(sentMessages.length).toBe(1); + const msg = sentMessages[0]; + if (msg.type === 'updateObjectDescription') { + expect(msg.data.description.status).toBe('missing'); + expect(msg.data.filePath).toBe(vscode.Uri.file(filePath).fsPath); + } else { + fail('Expected updateObjectDescription message'); + } }); - describe('description extraction', () => { - it('should extract description from .md file', async () => { - const mockDoc = { - getText: () => `--- -status: reconciled ---- + it('should fall back to default text when extraction fails completely', async () => { + const nonExistentPath = path.join(tempDir, 'ghost.ts'); -# Test File -## Summary -This is a test summary -` - }; - - (vscode.workspace.openTextDocument as jest.Mock).mockResolvedValue(mockDoc); - - const mockContext = { - subscriptions: [] - } as any; - - service.startWatching(mockContext); - - // Simulate file change - const changeHandler = mockWatcher.onDidChange.mock.calls[0][0]; - await changeHandler({ fsPath: '/workspace/test.md' }); - - expect(sendUpdateSpy).toHaveBeenCalledWith( - expect.objectContaining({ - type: 'updateObjectDescription', - data: expect.objectContaining({ - filePath: '/workspace/test.md', - description: expect.objectContaining({ - summary: 'This is a test summary', - status: 'reconciled' - }) - }) - }) - ); - }); - - it('should generate description for code file when .md not found', async () => { - const mockStats = { - size: 1000, - mtime: Date.now() - }; - - // Mock the stat to return stats for the code file - // When a .ts file is passed, extractDescription will: - // 1. Check if it's a .md file (false for .ts) - // 2. Since no summaryText, it tries to generate from code file - // 3. It does: filePath = uri.fsPath.replace(/\.md$/, '') which doesn't change .ts - // 4. Creates codeUri = vscode.Uri.file(filePath) which creates { fsPath: '/workspace/test.ts' } - // 5. Calls fs.stat(codeUri) which should return the mockStats - (vscode.workspace.fs.stat as jest.Mock).mockResolvedValue(mockStats); - - const mockContext = { - subscriptions: [] - } as any; - - service.startWatching(mockContext); - - // Simulate code file change (not .md) - const changeHandler = mockWatcher.onDidChange.mock.calls[0][0]; - const uri = { fsPath: '/workspace/test.ts' }; - await changeHandler(uri); - - // Wait for async operations to complete - await new Promise(resolve => setTimeout(resolve, 50)); - - expect(sendUpdateSpy).toHaveBeenCalled(); - const call = sendUpdateSpy.mock.calls[0][0]; - expect(call.data.description.status).toBe('generated'); - expect(call.data.description.summary).toContain('Filename: test.ts'); - }); - - it('should handle file deletion', () => { - const mockContext = { - subscriptions: [] - } as any; - - service.startWatching(mockContext); - - // Simulate file deletion - const deleteHandler = mockWatcher.onDidDelete.mock.calls[0][0]; - deleteHandler({ fsPath: '/workspace/test.md' }); - - expect(sendUpdateSpy).toHaveBeenCalledWith( - expect.objectContaining({ - type: 'updateObjectDescription', - data: { - filePath: '/workspace/test.md', - description: { - summary: 'No description yet.', - status: 'missing' - } - } - }) - ); - }); - }); + await service.handleFileChange(vscode.Uri.file(nonExistentPath)); - describe('error handling', () => { - it('should handle errors when reading files', async () => { - // Mock openTextDocument to throw an error - (vscode.workspace.openTextDocument as jest.Mock).mockRejectedValue(new Error('File not found')); - // Also mock fs.stat to throw so the fallback path also fails - (vscode.workspace.fs.stat as jest.Mock).mockRejectedValue(new Error('File not found')); - - const mockContext = { - subscriptions: [] - } as any; - - service.startWatching(mockContext); - - const changeHandler = mockWatcher.onDidChange.mock.calls[0][0]; - - // The error is caught internally and returns a description with "No description available" - // So we should still get an update, just with a fallback description - await changeHandler({ fsPath: '/workspace/test.md' }); - - // Wait for async operations - await new Promise(resolve => setTimeout(resolve, 10)); - - // Should still send an update even when errors occur - expect(sendUpdateSpy).toHaveBeenCalled(); - const call = sendUpdateSpy.mock.calls[0][0]; - expect(call.data.description.summary).toBe('No description available'); - }); + expect(sentMessages.length).toBe(1); + const msg = sentMessages[0]; + if (msg.type === 'updateObjectDescription') { + expect(msg.data.description.summary).toBe('No description available'); + } else { + fail('Expected updateObjectDescription message'); + } }); }); + diff --git a/vscode-extension/src/services/description-sync-service.ts b/vscode-extension/src/services/description-sync-service.ts index 0b7b243..5954c70 100644 --- a/vscode-extension/src/services/description-sync-service.ts +++ b/vscode-extension/src/services/description-sync-service.ts @@ -1,11 +1,3 @@ -/** - * DescriptionSyncService - Watches files and syncs descriptions to webview - * - * Single Responsibility: File system watching and description file updates. - * Watches for .md description files and code files, extracts summaries, - * and sends updates to the webview. - */ - import * as vscode from 'vscode'; import * as path from 'path'; import { getLanguageDisplayName, getLanguageFromExtension } from '../utils/languageRegistry'; @@ -20,27 +12,47 @@ export interface DescriptionUpdate { }; } +/** + * FileSystemProxy - Interface for decoupling file operations + */ +export interface FileSystemProxy { + workspaceFolders: readonly vscode.WorkspaceFolder[] | undefined; + createFileSystemWatcher(pattern: vscode.RelativePattern): vscode.FileSystemWatcher; + openTextDocument(uri: vscode.Uri): Promise<{ getText(): string }>; + stat(uri: vscode.Uri): Promise; +} + export class DescriptionSyncService { private watcher: vscode.FileSystemWatcher | undefined; private sendUpdate: (message: ExtensionMessage) => void; - - constructor(sendUpdate: (message: ExtensionMessage) => void) { + private fsProxy: FileSystemProxy; + + constructor( + sendUpdate: (message: ExtensionMessage) => void, + fsProxy: FileSystemProxy = { + workspaceFolders: vscode.workspace.workspaceFolders, + createFileSystemWatcher: (p) => vscode.workspace.createFileSystemWatcher(p), + openTextDocument: (u) => Promise.resolve(vscode.workspace.openTextDocument(u)), + stat: (u) => Promise.resolve(vscode.workspace.fs.stat(u)) + } + ) { this.sendUpdate = sendUpdate; + this.fsProxy = fsProxy; } /** * Start watching for file changes */ public startWatching(context: vscode.ExtensionContext): void { - if (!vscode.workspace.workspaceFolders) { + if (!this.fsProxy.workspaceFolders || this.fsProxy.workspaceFolders.length === 0) { return; } const pattern = new vscode.RelativePattern( - vscode.workspace.workspaceFolders[0], + this.fsProxy.workspaceFolders[0], '**/*' ); - this.watcher = vscode.workspace.createFileSystemWatcher(pattern); + this.watcher = this.fsProxy.createFileSystemWatcher(pattern); this.watcher.onDidCreate(uri => this.handleFileChange(uri)); this.watcher.onDidChange(uri => this.handleFileChange(uri)); @@ -60,7 +72,7 @@ export class DescriptionSyncService { /** * Handle file creation or change */ - private async handleFileChange(uri: vscode.Uri): Promise { + public async handleFileChange(uri: vscode.Uri): Promise { try { const update = await this.extractDescription(uri); if (update) { @@ -77,7 +89,7 @@ export class DescriptionSyncService { /** * Handle file deletion */ - private handleFileDelete(uri: vscode.Uri): void { + public handleFileDelete(uri: vscode.Uri): void { this.sendUpdate({ type: 'updateObjectDescription', data: { @@ -94,15 +106,14 @@ export class DescriptionSyncService { * Extract description from a file */ private async extractDescription(uri: vscode.Uri): Promise { - let content = ''; let summaryText = ''; let status: 'missing' | 'generated' | 'reconciled' = 'missing'; const isDescriptionFile = uri.fsPath.endsWith('.md'); if (isDescriptionFile) { try { - const doc = await vscode.workspace.openTextDocument(uri); - content = doc.getText(); + const doc = await this.fsProxy.openTextDocument(uri); + const content = doc.getText(); const summaryMatch = content.match(/## Summary\s+([\s\S]*?)(\n##|$)/i); summaryText = summaryMatch ? summaryMatch[1].trim() : ''; const statusMatch = content.match(/status:\s*(\w+)/i); @@ -118,11 +129,16 @@ export class DescriptionSyncService { const codeUri = vscode.Uri.file(filePath); try { - const stats = await vscode.workspace.fs.stat(codeUri); + const stats = await this.fsProxy.stat(codeUri); const size = stats.size; - const lastModified = new Date(stats.mtime).toLocaleDateString('lt-LT', { - timeZone: 'Europe/Vilnius' - }); + let lastModified = 'Unknown'; + try { + lastModified = new Date(stats.mtime).toLocaleDateString('lt-LT', { + timeZone: 'Europe/Vilnius' + }); + } catch (e) { + lastModified = new Date(stats.mtime).toISOString(); + } const ext = path.extname(filePath); const language = getLanguageDisplayName(getLanguageFromExtension(ext)) || 'Unknown'; const complexity = size / 50;