diff options
Diffstat (limited to 'packages/core/src/tools/edit.test.ts')
| -rw-r--r-- | packages/core/src/tools/edit.test.ts | 162 |
1 files changed, 78 insertions, 84 deletions
diff --git a/packages/core/src/tools/edit.test.ts b/packages/core/src/tools/edit.test.ts index 3bfa023e..3e0dba61 100644 --- a/packages/core/src/tools/edit.test.ts +++ b/packages/core/src/tools/edit.test.ts @@ -27,7 +27,7 @@ vi.mock('../utils/editor.js', () => ({ })); import { describe, it, expect, beforeEach, afterEach, vi, Mock } from 'vitest'; -import { EditTool, EditToolParams } from './edit.js'; +import { applyReplacement, EditTool, EditToolParams } from './edit.js'; import { FileDiff, ToolConfirmationOutcome } from './tools.js'; import { ToolErrorType } from './tool-error.js'; import path from 'path'; @@ -155,45 +155,30 @@ describe('EditTool', () => { fs.rmSync(tempDir, { recursive: true, force: true }); }); - describe('_applyReplacement', () => { - // Access private method for testing - // Note: `tool` is initialized in `beforeEach` of the parent describe block + describe('applyReplacement', () => { it('should return newString if isNewFile is true', () => { - expect((tool as any)._applyReplacement(null, 'old', 'new', true)).toBe( - 'new', - ); - expect( - (tool as any)._applyReplacement('existing', 'old', 'new', true), - ).toBe('new'); + expect(applyReplacement(null, 'old', 'new', true)).toBe('new'); + expect(applyReplacement('existing', 'old', 'new', true)).toBe('new'); }); it('should return newString if currentContent is null and oldString is empty (defensive)', () => { - expect((tool as any)._applyReplacement(null, '', 'new', false)).toBe( - 'new', - ); + expect(applyReplacement(null, '', 'new', false)).toBe('new'); }); it('should return empty string if currentContent is null and oldString is not empty (defensive)', () => { - expect((tool as any)._applyReplacement(null, 'old', 'new', false)).toBe( - '', - ); + expect(applyReplacement(null, 'old', 'new', false)).toBe(''); }); it('should replace oldString with newString in currentContent', () => { - expect( - (tool as any)._applyReplacement( - 'hello old world old', - 'old', - 'new', - false, - ), - ).toBe('hello new world new'); + expect(applyReplacement('hello old world old', 'old', 'new', false)).toBe( + 'hello new world new', + ); }); it('should return currentContent if oldString is empty and not a new file', () => { - expect( - (tool as any)._applyReplacement('hello world', '', 'new', false), - ).toBe('hello world'); + expect(applyReplacement('hello world', '', 'new', false)).toBe( + 'hello world', + ); }); }); @@ -239,15 +224,13 @@ describe('EditTool', () => { filePath = path.join(rootDir, testFile); }); - it('should return false if params are invalid', async () => { + it('should throw an error if params are invalid', async () => { const params: EditToolParams = { file_path: 'relative.txt', old_string: 'old', new_string: 'new', }; - expect( - await tool.shouldConfirmExecute(params, new AbortController().signal), - ).toBe(false); + expect(() => tool.build(params)).toThrow(); }); it('should request confirmation for valid edit', async () => { @@ -259,8 +242,8 @@ describe('EditTool', () => { }; // ensureCorrectEdit will be called by shouldConfirmExecute mockEnsureCorrectEdit.mockResolvedValueOnce({ params, occurrences: 1 }); - const confirmation = await tool.shouldConfirmExecute( - params, + const invocation = tool.build(params); + const confirmation = await invocation.shouldConfirmExecute( new AbortController().signal, ); expect(confirmation).toEqual( @@ -280,9 +263,11 @@ describe('EditTool', () => { new_string: 'new', }; mockEnsureCorrectEdit.mockResolvedValueOnce({ params, occurrences: 0 }); - expect( - await tool.shouldConfirmExecute(params, new AbortController().signal), - ).toBe(false); + const invocation = tool.build(params); + const confirmation = await invocation.shouldConfirmExecute( + new AbortController().signal, + ); + expect(confirmation).toBe(false); }); it('should return false if multiple occurrences of old_string are found (ensureCorrectEdit returns > 1)', async () => { @@ -293,9 +278,11 @@ describe('EditTool', () => { new_string: 'new', }; mockEnsureCorrectEdit.mockResolvedValueOnce({ params, occurrences: 2 }); - expect( - await tool.shouldConfirmExecute(params, new AbortController().signal), - ).toBe(false); + const invocation = tool.build(params); + const confirmation = await invocation.shouldConfirmExecute( + new AbortController().signal, + ); + expect(confirmation).toBe(false); }); it('should request confirmation for creating a new file (empty old_string)', async () => { @@ -310,8 +297,8 @@ describe('EditTool', () => { // as shouldConfirmExecute handles this for diff generation. // If it is called, it should return 0 occurrences for a new file. mockEnsureCorrectEdit.mockResolvedValueOnce({ params, occurrences: 0 }); - const confirmation = await tool.shouldConfirmExecute( - params, + const invocation = tool.build(params); + const confirmation = await invocation.shouldConfirmExecute( new AbortController().signal, ); expect(confirmation).toEqual( @@ -358,9 +345,8 @@ describe('EditTool', () => { }; }, ); - - const confirmation = (await tool.shouldConfirmExecute( - params, + const invocation = tool.build(params); + const confirmation = (await invocation.shouldConfirmExecute( new AbortController().signal, )) as FileDiff; @@ -408,15 +394,13 @@ describe('EditTool', () => { }); }); - it('should return error if params are invalid', async () => { + it('should throw error if params are invalid', async () => { const params: EditToolParams = { file_path: 'relative.txt', old_string: 'old', new_string: 'new', }; - const result = await tool.execute(params, new AbortController().signal); - expect(result.llmContent).toMatch(/Error: Invalid parameters provided/); - expect(result.returnDisplay).toMatch(/Error: File path must be absolute/); + expect(() => tool.build(params)).toThrow(/File path must be absolute/); }); it('should edit an existing file and return diff with fileName', async () => { @@ -433,12 +417,8 @@ describe('EditTool', () => { // ensureCorrectEdit is NOT called by calculateEdit, only by shouldConfirmExecute // So, the default mockEnsureCorrectEdit should correctly return 1 occurrence for 'old' in initialContent - // Simulate confirmation by setting shouldAlwaysEdit - (tool as any).shouldAlwaysEdit = true; - - const result = await tool.execute(params, new AbortController().signal); - - (tool as any).shouldAlwaysEdit = false; // Reset for other tests + const invocation = tool.build(params); + const result = await invocation.execute(new AbortController().signal); expect(result.llmContent).toMatch(/Successfully modified file/); expect(fs.readFileSync(filePath, 'utf8')).toBe(newContent); @@ -461,7 +441,8 @@ describe('EditTool', () => { (mockConfig.getApprovalMode as Mock).mockReturnValueOnce( ApprovalMode.AUTO_EDIT, ); - const result = await tool.execute(params, new AbortController().signal); + const invocation = tool.build(params); + const result = await invocation.execute(new AbortController().signal); expect(result.llmContent).toMatch(/Created new file/); expect(fs.existsSync(newFilePath)).toBe(true); @@ -477,7 +458,8 @@ describe('EditTool', () => { new_string: 'replacement', }; // The default mockEnsureCorrectEdit will return 0 occurrences for 'nonexistent' - const result = await tool.execute(params, new AbortController().signal); + const invocation = tool.build(params); + const result = await invocation.execute(new AbortController().signal); expect(result.llmContent).toMatch( /0 occurrences found for old_string in/, ); @@ -494,7 +476,8 @@ describe('EditTool', () => { new_string: 'new', }; // The default mockEnsureCorrectEdit will return 2 occurrences for 'old' - const result = await tool.execute(params, new AbortController().signal); + const invocation = tool.build(params); + const result = await invocation.execute(new AbortController().signal); expect(result.llmContent).toMatch( /Expected 1 occurrence but found 2 for old_string in file/, ); @@ -512,12 +495,8 @@ describe('EditTool', () => { expected_replacements: 3, }; - // Simulate confirmation by setting shouldAlwaysEdit - (tool as any).shouldAlwaysEdit = true; - - const result = await tool.execute(params, new AbortController().signal); - - (tool as any).shouldAlwaysEdit = false; // Reset for other tests + const invocation = tool.build(params); + const result = await invocation.execute(new AbortController().signal); expect(result.llmContent).toMatch(/Successfully modified file/); expect(fs.readFileSync(filePath, 'utf8')).toBe( @@ -537,7 +516,8 @@ describe('EditTool', () => { new_string: 'new', expected_replacements: 3, // Expecting 3 but only 2 exist }; - const result = await tool.execute(params, new AbortController().signal); + const invocation = tool.build(params); + const result = await invocation.execute(new AbortController().signal); expect(result.llmContent).toMatch( /Expected 3 occurrences but found 2 for old_string in file/, ); @@ -553,7 +533,8 @@ describe('EditTool', () => { old_string: '', new_string: 'new content', }; - const result = await tool.execute(params, new AbortController().signal); + const invocation = tool.build(params); + const result = await invocation.execute(new AbortController().signal); expect(result.llmContent).toMatch(/File already exists, cannot create/); expect(result.returnDisplay).toMatch( /Attempted to create a file that already exists/, @@ -573,7 +554,8 @@ describe('EditTool', () => { (mockConfig.getApprovalMode as Mock).mockReturnValueOnce( ApprovalMode.AUTO_EDIT, ); - const result = await tool.execute(params, new AbortController().signal); + const invocation = tool.build(params); + const result = await invocation.execute(new AbortController().signal); expect(result.llmContent).toMatch( /User modified the `new_string` content/, @@ -593,7 +575,8 @@ describe('EditTool', () => { (mockConfig.getApprovalMode as Mock).mockReturnValueOnce( ApprovalMode.AUTO_EDIT, ); - const result = await tool.execute(params, new AbortController().signal); + const invocation = tool.build(params); + const result = await invocation.execute(new AbortController().signal); expect(result.llmContent).not.toMatch( /User modified the `new_string` content/, @@ -612,7 +595,8 @@ describe('EditTool', () => { (mockConfig.getApprovalMode as Mock).mockReturnValueOnce( ApprovalMode.AUTO_EDIT, ); - const result = await tool.execute(params, new AbortController().signal); + const invocation = tool.build(params); + const result = await invocation.execute(new AbortController().signal); expect(result.llmContent).not.toMatch( /User modified the `new_string` content/, @@ -627,7 +611,8 @@ describe('EditTool', () => { old_string: 'identical', new_string: 'identical', }; - const result = await tool.execute(params, new AbortController().signal); + const invocation = tool.build(params); + const result = await invocation.execute(new AbortController().signal); expect(result.llmContent).toMatch(/No changes to apply/); expect(result.returnDisplay).toMatch(/No changes to apply/); }); @@ -647,7 +632,8 @@ describe('EditTool', () => { old_string: 'any', new_string: 'new', }; - const result = await tool.execute(params, new AbortController().signal); + const invocation = tool.build(params); + const result = await invocation.execute(new AbortController().signal); expect(result.error?.type).toBe(ToolErrorType.FILE_NOT_FOUND); }); @@ -658,7 +644,8 @@ describe('EditTool', () => { old_string: '', new_string: 'new content', }; - const result = await tool.execute(params, new AbortController().signal); + const invocation = tool.build(params); + const result = await invocation.execute(new AbortController().signal); expect(result.error?.type).toBe( ToolErrorType.ATTEMPT_TO_CREATE_EXISTING_FILE, ); @@ -671,7 +658,8 @@ describe('EditTool', () => { old_string: 'not-found', new_string: 'new', }; - const result = await tool.execute(params, new AbortController().signal); + const invocation = tool.build(params); + const result = await invocation.execute(new AbortController().signal); expect(result.error?.type).toBe(ToolErrorType.EDIT_NO_OCCURRENCE_FOUND); }); @@ -683,7 +671,8 @@ describe('EditTool', () => { new_string: 'new', expected_replacements: 3, }; - const result = await tool.execute(params, new AbortController().signal); + const invocation = tool.build(params); + const result = await invocation.execute(new AbortController().signal); expect(result.error?.type).toBe( ToolErrorType.EDIT_EXPECTED_OCCURRENCE_MISMATCH, ); @@ -696,18 +685,18 @@ describe('EditTool', () => { old_string: 'content', new_string: 'content', }; - const result = await tool.execute(params, new AbortController().signal); + const invocation = tool.build(params); + const result = await invocation.execute(new AbortController().signal); expect(result.error?.type).toBe(ToolErrorType.EDIT_NO_CHANGE); }); - it('should return INVALID_PARAMETERS error for relative path', async () => { + it('should throw INVALID_PARAMETERS error for relative path', async () => { const params: EditToolParams = { file_path: 'relative/path.txt', old_string: 'a', new_string: 'b', }; - const result = await tool.execute(params, new AbortController().signal); - expect(result.error?.type).toBe(ToolErrorType.INVALID_TOOL_PARAMS); + expect(() => tool.build(params)).toThrow(); }); it('should return FILE_WRITE_FAILURE on write error', async () => { @@ -720,7 +709,8 @@ describe('EditTool', () => { old_string: 'content', new_string: 'new content', }; - const result = await tool.execute(params, new AbortController().signal); + const invocation = tool.build(params); + const result = await invocation.execute(new AbortController().signal); expect(result.error?.type).toBe(ToolErrorType.FILE_WRITE_FAILURE); }); }); @@ -733,8 +723,9 @@ describe('EditTool', () => { old_string: 'identical_string', new_string: 'identical_string', }; + const invocation = tool.build(params); // shortenPath will be called internally, resulting in just the file name - expect(tool.getDescription(params)).toBe( + expect(invocation.getDescription()).toBe( `No file changes to ${testFileName}`, ); }); @@ -746,9 +737,10 @@ describe('EditTool', () => { old_string: 'this is the old string value', new_string: 'this is the new string value', }; + const invocation = tool.build(params); // shortenPath will be called internally, resulting in just the file name // The snippets are truncated at 30 chars + '...' - expect(tool.getDescription(params)).toBe( + expect(invocation.getDescription()).toBe( `${testFileName}: this is the old string value => this is the new string value`, ); }); @@ -760,7 +752,8 @@ describe('EditTool', () => { old_string: 'old', new_string: 'new', }; - expect(tool.getDescription(params)).toBe(`${testFileName}: old => new`); + const invocation = tool.build(params); + expect(invocation.getDescription()).toBe(`${testFileName}: old => new`); }); it('should truncate long strings in the description', () => { @@ -772,7 +765,8 @@ describe('EditTool', () => { new_string: 'this is a very long new string that will also be truncated', }; - expect(tool.getDescription(params)).toBe( + const invocation = tool.build(params); + expect(invocation.getDescription()).toBe( `${testFileName}: this is a very long old string... => this is a very long new string...`, ); }); @@ -839,8 +833,8 @@ describe('EditTool', () => { content: modifiedContent, }); - const confirmation = await tool.shouldConfirmExecute( - params, + const invocation = tool.build(params); + const confirmation = await invocation.shouldConfirmExecute( new AbortController().signal, ); |