test(readFileTool): add comprehensive test coverage for ReadFileTool#222
test(readFileTool): add comprehensive test coverage for ReadFileTool#222proyectoauraorg wants to merge 4 commits into
Conversation
- Add 44 new tests covering edge cases and untested code paths - Tests for legacy path handling, handlePartial errors, getStartLine fallback - Tests for getLineSnippet when lines exceed file length - Tests for batch approval flow and media provider integration - Tests for file path normalization and edge cases - Total test count: 77 (was 33, added 44)
proyectoauraorg
requested review from
JamesRobert20,
edelauna,
hannesrudolph,
navedmerchant and
taltas
as code owners
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds ~852 lines of tests expanding ReadFileTool coverage (legacy-format detection and handling, partial/block flows, indentation and slice edge cases, approval messaging, provider-state image limits, and error scenarios) and a small types tweak to detect bare legacy ChangesReadFileTool test coverage expansion
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
src/core/tools/__tests__/readFileTool.spec.tsESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/core/tools/__tests__/readFileTool.spec.ts (1)
771-972: ⚡ Quick winClean up console.warn spies to prevent test leakage.
Multiple tests create
console.warnspies without restoring them after the test completes. Whilevi.clearAllMocks()inbeforeEachclears call history, it doesn't restore original implementations, which can cause spy leakage between tests.The first test (lines 742-749) demonstrates the correct pattern with
consoleWarnSpy.mockRestore(). Apply the same pattern here for consistency and proper test isolation.🧹 Suggested fix: Add spy restoration
For each test creating a console.warn spy, capture the spy and restore it:
it("should handle multiple files in legacy format", async () => { const mockTask = createMockTask() const callbacks = createMockCallbacks() mockTask.ask.mockResolvedValue({ response: "yesButtonClicked", text: undefined, images: undefined }) mockedFsReadFile.mockResolvedValueOnce(Buffer.from("file1 content")) mockedFsReadFile.mockResolvedValueOnce(Buffer.from("file2 content")) - vi.spyOn(console, "warn").mockImplementation(() => {}) + const consoleWarnSpy = vi.spyOn(console, "warn").mockImplementation(() => {}) await readFileTool.execute( { files: [{ path: "file1.ts" }, { path: "file2.ts" }] } as any, mockTask as any, callbacks, ) expect(callbacks.pushToolResult).toHaveBeenCalledWith(expect.stringContaining("file1 content")) expect(callbacks.pushToolResult).toHaveBeenCalledWith(expect.stringContaining("file2 content")) + consoleWarnSpy.mockRestore() })Apply this pattern to all affected tests at lines 771, 787, 800, 814, 827, 857, 878, 892, 905, 923, 939, 959, and 972.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/tools/__tests__/readFileTool.spec.ts` around lines 771 - 972, Several tests in readFileTool.spec.ts create console.warn spies via vi.spyOn(console, "warn").mockImplementation(() => {}) but do not restore them, causing leakage; update each affected test that calls vi.spyOn(console, "warn") to assign the spy to a variable (e.g., const consoleWarnSpy = vi.spyOn(console, "warn").mockImplementation(() => {})) and call consoleWarnSpy.mockRestore() at the end of the test (after awaiting readFileTool.execute and assertions) to restore the original implementation; look for usages in tests like "should block rooignore-protected files in legacy format", "should deny legacy file read when user clicks no", "should handle directory path in legacy format", and similar, replacing the raw spy calls with a captured const and restoring it.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/core/tools/__tests__/readFileTool.spec.ts`:
- Around line 771-972: Several tests in readFileTool.spec.ts create console.warn
spies via vi.spyOn(console, "warn").mockImplementation(() => {}) but do not
restore them, causing leakage; update each affected test that calls
vi.spyOn(console, "warn") to assign the spy to a variable (e.g., const
consoleWarnSpy = vi.spyOn(console, "warn").mockImplementation(() => {})) and
call consoleWarnSpy.mockRestore() at the end of the test (after awaiting
readFileTool.execute and assertions) to restore the original implementation;
look for usages in tests like "should block rooignore-protected files in legacy
format", "should deny legacy file read when user clicks no", "should handle
directory path in legacy format", and similar, replacing the raw spy calls with
a captured const and restoring it.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c21b1200-c403-42b0-afbb-da0f264e09e6
📒 Files selected for processing (1)
src/core/tools/__tests__/readFileTool.spec.ts
- Enhance isLegacyReadFileParams to also detect bare 'files' array (real legacy chat data) - Fix batch approval test: add explicit readWithSlice mocks for file content - Fix partial display test: mock fs.readFile to return string for utf8 encoding
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/types/src/tool-params.ts (1)
92-96: 💤 Low valueConsider validating
filesarray element shape for stronger type safety.The type guard returns
params is LegacyReadFileParamswhen afilesproperty exists and is an array, but doesn't verify that array elements match theFileEntryinterface. This could allow malformed inputs to pass the guard.While this is pragmatic for backward compatibility with real legacy chat data (the stated goal), consider adding a lightweight shape check if type safety is a concern:
const hasFilesArray = "files" in params && Array.isArray((params as unknown as Record<string, unknown>).files) && (params as any).files.every((f: any) => f && typeof f.path === 'string')However, given the explicit goal of supporting legacy chat data and the low likelihood of misuse, the current implementation is acceptable.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/types/src/tool-params.ts` around lines 92 - 96, The type guard currently treats any array on params.files as LegacyReadFileParams which can let malformed entries through; update the hasFilesArray check in the same function (the variables hasFilesArray and hasLegacyFlag that return params is LegacyReadFileParams) to perform a lightweight shape validation on each element (e.g., ensure each item is an object and has a string path and any other required FileEntry fields) before returning true so that only arrays of valid FileEntry-like objects pass the guard.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/types/src/tool-params.ts`:
- Around line 92-96: The type guard currently treats any array on params.files
as LegacyReadFileParams which can let malformed entries through; update the
hasFilesArray check in the same function (the variables hasFilesArray and
hasLegacyFlag that return params is LegacyReadFileParams) to perform a
lightweight shape validation on each element (e.g., ensure each item is an
object and has a string path and any other required FileEntry fields) before
returning true so that only arrays of valid FileEntry-like objects pass the
guard.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f2e883f2-72c6-4206-969c-504915323a29
📒 Files selected for processing (2)
packages/types/src/tool-params.tssrc/core/tools/__tests__/readFileTool.spec.ts
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Addresses CodeRabbit nitpick: several tests installed vi.spyOn(console, "warn") without restoring it, leaking the mock across tests. Added a scoped afterEach that restores console.warn only (via vi.isMockFunction guard) so the suite's module-level mocks are left intact. 75 tests pass. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
Addressed the spy leakage @coderabbitai — added a scoped `afterEach` that restores `console.warn` (guarded by `vi.isMockFunction`) so it no longer leaks across tests, while leaving the suite-level module mocks intact. 75 tests pass locally. |
|
Tip For best results, initiate chat on the files or code changes.
If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
Cast through `unknown` before `ReturnType<typeof vi.spyOn>` so tsc accepts the conversion (console.warn's signature doesn't overlap with MockInstance). Unblocks the `compile` CI job. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
1 participant
Summary
Adds comprehensive test coverage for ReadFileTool, increasing from 33 to 77 tests (+44 new tests).
New Test Coverage
handlePartial error handling:
getStartLine fallback logic:
getLineSnippet edge cases:
Path normalization:
Legacy parameters:
Direct file reading:
Batch approval flow:
params getter:
Test Results
Summary by CodeRabbit