Fix test to be windows compatible. (#4875)

packages/core/src/tools/modifiable-tool.test.ts

@@ -4,15 +4,7 @@
  * SPDX-License-Identifier: Apache-2.0
  */
 
-import {
+import { vi, describe, it, expect, beforeEach, afterEach } from 'vitest';
-  vi,
-  describe,
-  it,
-  expect,
-  beforeEach,
-  afterEach,
-  type Mock,
-} from 'vitest';
 import {
   modifyWithEditor,
   ModifyContext,
@@ -21,6 +13,7 @@ import {
 } from './modifiable-tool.js';
 import { EditorType } from '../utils/editor.js';
 import fs from 'fs';
+import fsp from 'fs/promises';
 import os from 'os';
 import * as path from 'path';
 
@@ -36,9 +29,6 @@ vi.mock('diff', () => ({
   createPatch: mockCreatePatch,
 }));
 
-vi.mock('fs');
-vi.mock('os');
-
 interface TestParams {
   filePath: string;
   someOtherParam: string;
@@ -46,7 +36,7 @@ interface TestParams {
 }
 
 describe('modifyWithEditor', () => {
-  let tempDir: string;
+  let testProjectDir: string;
   let mockModifyContext: ModifyContext<TestParams>;
   let mockParams: TestParams;
   let currentContent: string;
@@ -54,17 +44,19 @@ describe('modifyWithEditor', () => {
   let modifiedContent: string;
   let abortSignal: AbortSignal;
 
-  beforeEach(() => {
+  beforeEach(async () => {
     vi.resetAllMocks();
 
-    tempDir = '/tmp/test-dir';
+    testProjectDir = await fsp.mkdtemp(
+      path.join(os.tmpdir(), 'modifiable-tool-test-'),
+    );
     abortSignal = new AbortController().signal;
 
     currentContent = 'original content\nline 2\nline 3';
     proposedContent = 'modified content\nline 2\nline 3';
     modifiedContent = 'user modified content\nline 2\nline 3\nnew line';
     mockParams = {
-      filePath: path.join(tempDir, 'test.txt'),
+      filePath: path.join(testProjectDir, 'test.txt'),
       someOtherParam: 'value',
     };
 
@@ -81,26 +73,18 @@ describe('modifyWithEditor', () => {
         })),
     };
 
-    (os.tmpdir as Mock).mockReturnValue(tempDir);
+    mockOpenDiff.mockImplementation(async (_oldPath, newPath) => {
-
+      await fsp.writeFile(newPath, modifiedContent, 'utf8');
-    (fs.existsSync as Mock).mockReturnValue(true);
-    (fs.mkdirSync as Mock).mockImplementation(() => undefined);
-    (fs.writeFileSync as Mock).mockImplementation(() => {});
-    (fs.unlinkSync as Mock).mockImplementation(() => {});
-
-    (fs.readFileSync as Mock).mockImplementation((filePath: string) => {
-      if (filePath.includes('-new-')) {
-        return modifiedContent;
-      }
-      return currentContent;
     });
 
     mockCreatePatch.mockReturnValue('mock diff content');
-    mockOpenDiff.mockResolvedValue(undefined);
   });
 
-  afterEach(() => {
+  afterEach(async () => {
     vi.restoreAllMocks();
+    await fsp.rm(testProjectDir, { recursive: true, force: true });
+    const diffDir = path.join(os.tmpdir(), 'gemini-cli-tool-modify-diffs');
+    await fsp.rm(diffDir, { recursive: true, force: true });
   });
 
   describe('successful modification', () => {
@@ -120,38 +104,8 @@ describe('modifyWithEditor', () => {
       );
       expect(mockModifyContext.getFilePath).toHaveBeenCalledWith(mockParams);
 
-      expect(fs.writeFileSync).toHaveBeenCalledTimes(2);
+      expect(mockOpenDiff).toHaveBeenCalledOnce();
-      expect(fs.writeFileSync).toHaveBeenNthCalledWith(
+      const [oldFilePath, newFilePath] = mockOpenDiff.mock.calls[0];
-        1,
-        expect.stringContaining(
-          path.join(tempDir, 'gemini-cli-tool-modify-diffs'),
-        ),
-        currentContent,
-        'utf8',
-      );
-      expect(fs.writeFileSync).toHaveBeenNthCalledWith(
-        2,
-        expect.stringContaining(
-          path.join(tempDir, 'gemini-cli-tool-modify-diffs'),
-        ),
-        proposedContent,
-        'utf8',
-      );
-
-      expect(mockOpenDiff).toHaveBeenCalledWith(
-        expect.stringContaining('-old-'),
-        expect.stringContaining('-new-'),
-        'vscode',
-      );
-
-      expect(fs.readFileSync).toHaveBeenCalledWith(
-        expect.stringContaining('-old-'),
-        'utf8',
-      );
-      expect(fs.readFileSync).toHaveBeenCalledWith(
-        expect.stringContaining('-new-'),
-        'utf8',
-      );
 
       expect(mockModifyContext.createUpdatedParams).toHaveBeenCalledWith(
         currentContent,
@@ -171,15 +125,9 @@ describe('modifyWithEditor', () => {
         }),
       );
 
-      expect(fs.unlinkSync).toHaveBeenCalledTimes(2);
+      // Check that temp files are deleted.
-      expect(fs.unlinkSync).toHaveBeenNthCalledWith(
+      await expect(fsp.access(oldFilePath)).rejects.toThrow();
-        1,
+      await expect(fsp.access(newFilePath)).rejects.toThrow();
-        expect.stringContaining('-old-'),
-      );
-      expect(fs.unlinkSync).toHaveBeenNthCalledWith(
-        2,
-        expect.stringContaining('-new-'),
-      );
 
       expect(result).toEqual({
         updatedParams: {
@@ -192,7 +140,8 @@ describe('modifyWithEditor', () => {
     });
 
     it('should create temp directory if it does not exist', async () => {
-      (fs.existsSync as Mock).mockReturnValue(false);
+      const diffDir = path.join(os.tmpdir(), 'gemini-cli-tool-modify-diffs');
+      await fsp.rm(diffDir, { recursive: true, force: true }).catch(() => {});
 
       await modifyWithEditor(
         mockParams,
@@ -201,14 +150,15 @@ describe('modifyWithEditor', () => {
         abortSignal,
       );
 
-      expect(fs.mkdirSync).toHaveBeenCalledWith(
+      const stats = await fsp.stat(diffDir);
-        path.join(tempDir, 'gemini-cli-tool-modify-diffs'),
+      expect(stats.isDirectory()).toBe(true);
-        { recursive: true },
-      );
     });
 
     it('should not create temp directory if it already exists', async () => {
-      (fs.existsSync as Mock).mockReturnValue(true);
+      const diffDir = path.join(os.tmpdir(), 'gemini-cli-tool-modify-diffs');
+      await fsp.mkdir(diffDir, { recursive: true });
+
+      const mkdirSpy = vi.spyOn(fs, 'mkdirSync');
 
       await modifyWithEditor(
         mockParams,
@@ -217,18 +167,15 @@ describe('modifyWithEditor', () => {
         abortSignal,
       );
 
-      expect(fs.mkdirSync).not.toHaveBeenCalled();
+      expect(mkdirSpy).not.toHaveBeenCalled();
+      mkdirSpy.mockRestore();
     });
   });
 
   it('should handle missing old temp file gracefully', async () => {
-    (fs.readFileSync as Mock).mockImplementation((filePath: string) => {
+    mockOpenDiff.mockImplementation(async (oldPath, newPath) => {
-      if (filePath.includes('-old-')) {
+      await fsp.writeFile(newPath, modifiedContent, 'utf8');
-        const error = new Error('ENOENT: no such file or directory');
+      await fsp.unlink(oldPath);
-        (error as NodeJS.ErrnoException).code = 'ENOENT';
-        throw error;
-      }
-      return modifiedContent;
     });
 
     const result = await modifyWithEditor(
@@ -255,13 +202,8 @@ describe('modifyWithEditor', () => {
   });
 
   it('should handle missing new temp file gracefully', async () => {
-    (fs.readFileSync as Mock).mockImplementation((filePath: string) => {
+    mockOpenDiff.mockImplementation(async (_oldPath, newPath) => {
-      if (filePath.includes('-new-')) {
+      await fsp.unlink(newPath);
-        const error = new Error('ENOENT: no such file or directory');
-        (error as NodeJS.ErrnoException).code = 'ENOENT';
-        throw error;
-      }
-      return currentContent;
     });
 
     const result = await modifyWithEditor(
@@ -291,6 +233,8 @@ describe('modifyWithEditor', () => {
     const editorError = new Error('Editor failed to open');
     mockOpenDiff.mockRejectedValue(editorError);
 
+    const writeSpy = vi.spyOn(fs, 'writeFileSync');
+
     await expect(
       modifyWithEditor(
         mockParams,
@@ -300,14 +244,21 @@ describe('modifyWithEditor', () => {
       ),
     ).rejects.toThrow('Editor failed to open');
 
-    expect(fs.unlinkSync).toHaveBeenCalledTimes(2);
+    expect(writeSpy).toHaveBeenCalledTimes(2);
+    const oldFilePath = writeSpy.mock.calls[0][0] as string;
+    const newFilePath = writeSpy.mock.calls[1][0] as string;
+
+    await expect(fsp.access(oldFilePath)).rejects.toThrow();
+    await expect(fsp.access(newFilePath)).rejects.toThrow();
+
+    writeSpy.mockRestore();
   });
 
   it('should handle temp file cleanup errors gracefully', async () => {
     const consoleErrorSpy = vi
       .spyOn(console, 'error')
       .mockImplementation(() => {});
-    (fs.unlinkSync as Mock).mockImplementation((_filePath: string) => {
+    vi.spyOn(fs, 'unlinkSync').mockImplementation(() => {
       throw new Error('Failed to delete file');
     });
 
@@ -327,7 +278,11 @@ describe('modifyWithEditor', () => {
   });
 
   it('should create temp files with correct naming with extension', async () => {
-    const testFilePath = path.join(tempDir, 'subfolder', 'test-file.txt');
+    const testFilePath = path.join(
+      testProjectDir,
+      'subfolder',
+      'test-file.txt',
+    );
     mockModifyContext.getFilePath = vi.fn().mockReturnValue(testFilePath);
 
     await modifyWithEditor(
@@ -337,20 +292,18 @@ describe('modifyWithEditor', () => {
       abortSignal,
     );
 
-    const writeFileCalls = (fs.writeFileSync as Mock).mock.calls;
+    expect(mockOpenDiff).toHaveBeenCalledOnce();
-    expect(writeFileCalls).toHaveLength(2);
+    const [oldFilePath, newFilePath] = mockOpenDiff.mock.calls[0];
-
-    const oldFilePath = writeFileCalls[0][0];
-    const newFilePath = writeFileCalls[1][0];
-
     expect(oldFilePath).toMatch(/gemini-cli-modify-test-file-old-\d+\.txt$/);
     expect(newFilePath).toMatch(/gemini-cli-modify-test-file-new-\d+\.txt$/);
-    expect(oldFilePath).toContain(`${tempDir}/gemini-cli-tool-modify-diffs/`);
+
-    expect(newFilePath).toContain(`${tempDir}/gemini-cli-tool-modify-diffs/`);
+    const diffDir = path.join(os.tmpdir(), 'gemini-cli-tool-modify-diffs');
+    expect(path.dirname(oldFilePath)).toBe(diffDir);
+    expect(path.dirname(newFilePath)).toBe(diffDir);
   });
 
   it('should create temp files with correct naming without extension', async () => {
-    const testFilePath = path.join(tempDir, 'subfolder', 'test-file');
+    const testFilePath = path.join(testProjectDir, 'subfolder', 'test-file');
     mockModifyContext.getFilePath = vi.fn().mockReturnValue(testFilePath);
 
     await modifyWithEditor(
@@ -360,16 +313,14 @@ describe('modifyWithEditor', () => {
       abortSignal,
     );
 
-    const writeFileCalls = (fs.writeFileSync as Mock).mock.calls;
+    expect(mockOpenDiff).toHaveBeenCalledOnce();
-    expect(writeFileCalls).toHaveLength(2);
+    const [oldFilePath, newFilePath] = mockOpenDiff.mock.calls[0];
-
-    const oldFilePath = writeFileCalls[0][0];
-    const newFilePath = writeFileCalls[1][0];
-
     expect(oldFilePath).toMatch(/gemini-cli-modify-test-file-old-\d+$/);
     expect(newFilePath).toMatch(/gemini-cli-modify-test-file-new-\d+$/);
-    expect(oldFilePath).toContain(`${tempDir}/gemini-cli-tool-modify-diffs/`);
+
-    expect(newFilePath).toContain(`${tempDir}/gemini-cli-tool-modify-diffs/`);
+    const diffDir = path.join(os.tmpdir(), 'gemini-cli-tool-modify-diffs');
+    expect(path.dirname(oldFilePath)).toBe(diffDir);
+    expect(path.dirname(newFilePath)).toBe(diffDir);
   });
 });