test: add comprehensive ListFilesTool test suite (40 tests)#213
test: add comprehensive ListFilesTool test suite (40 tests)#213proyectoauraorg wants to merge 2 commits into
Conversation
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: 📥 CommitsReviewing files that changed from the base of the PR and between 90d2ad37da811718ae72abb935b1be73c5680494 and c7c5888. 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a comprehensive Vitest test suite for ListFilesTool that mocks dependencies and validates execution, approval gating, path resolution, partial/stream handling, error cases, edge cases, and the module's singleton export. ChangesListFilesTool Test Suite
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsStopped waiting for pipeline failures after 30000ms. One of your pipelines takes longer than our 30000ms fetch window to run, so review may not consider pipeline-failure results for inline comments if any failures occurred after the fetch window. Increase the timeout if you want to wait longer or run a 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.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@src/core/tools/__tests__/listFilesTool.spec.ts`:
- Around line 385-395: The test claims "should not reset consecutive mistake
count on error" but never asserts the counter; update the test for the
tool.execute error path to explicitly assert that
mockTask.consecutiveMistakeCount remains unchanged (e.g., still 2) after the
mocked listFiles rejection; locate the test using
mockTask.consecutiveMistakeCount, vi.mocked(listFiles).mockRejectedValue,
tool.execute(params, mockTask, mockCallbacks) and mockCallbacks.handleError and
add an assertion that mockTask.consecutiveMistakeCount equals the original
value.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 49e701af-160b-4450-bdaf-2d7ad74ba50e
📒 Files selected for processing (1)
src/core/tools/__tests__/listFilesTool.spec.ts
| it("should not reset consecutive mistake count on error", async () => { | ||
| mockTask.consecutiveMistakeCount = 2 | ||
| vi.mocked(listFiles).mockRejectedValue(new Error("fail")) | ||
| const params = { path: "src", recursive: false } | ||
|
|
||
| await tool.execute(params, mockTask, mockCallbacks) | ||
|
|
||
| // The mistake count should NOT be reset since the error path doesn't reset it | ||
| // Note: it may be reset before the error occurs since the validation passed | ||
| expect(mockCallbacks.handleError).toHaveBeenCalled() | ||
| }) |
There was a problem hiding this comment.
Assert the stated behavior for consecutiveMistakeCount
Line 385 says this test verifies no reset on error, but Lines 392-395 never assert the counter value. This test currently passes even if reset behavior regresses.
✅ Proposed fix
it("should not reset consecutive mistake count on error", async () => {
mockTask.consecutiveMistakeCount = 2
vi.mocked(listFiles).mockRejectedValue(new Error("fail"))
const params = { path: "src", recursive: false }
await tool.execute(params, mockTask, mockCallbacks)
- // The mistake count should NOT be reset since the error path doesn't reset it
- // Note: it may be reset before the error occurs since the validation passed
expect(mockCallbacks.handleError).toHaveBeenCalled()
+ expect(mockTask.consecutiveMistakeCount).toBe(2)
})🤖 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__/listFilesTool.spec.ts` around lines 385 - 395, The
test claims "should not reset consecutive mistake count on error" but never
asserts the counter; update the test for the tool.execute error path to
explicitly assert that mockTask.consecutiveMistakeCount remains unchanged (e.g.,
still 2) after the mocked listFiles rejection; locate the test using
mockTask.consecutiveMistakeCount, vi.mocked(listFiles).mockRejectedValue,
tool.execute(params, mockTask, mockCallbacks) and mockCallbacks.handleError and
add an assertion that mockTask.consecutiveMistakeCount equals the original
value.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
proyectoauraorg
force-pushed
the
test/add-list-files-tool-tests
branch
from
90d2ad3 to
2ccef1b
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/core/tools/__tests__/listFilesTool.spec.ts (1)
397-407:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAssert the stated behavior for
consecutiveMistakeCountThe test name claims to verify that the consecutive mistake count is NOT reset on error, but the test never asserts the counter value. Compare with Line 179, which explicitly asserts
consecutiveMistakeCountequals 0 on success. This test should similarly assert that the count remains 2 after the error.✅ Proposed fix
it("should not reset consecutive mistake count on error", async () => { mockTask.consecutiveMistakeCount = 2 vi.mocked(listFiles).mockRejectedValue(new Error("fail")) const params = { path: "src", recursive: false } await tool.execute(params, mockTask, mockCallbacks) - // The mistake count should NOT be reset since the error path doesn't reset it - // Note: it may be reset before the error occurs since the validation passed expect(mockCallbacks.handleError).toHaveBeenCalled() + expect(mockTask.consecutiveMistakeCount).toBe(2) })🤖 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__/listFilesTool.spec.ts` around lines 397 - 407, The test titled "should not reset consecutive mistake count on error" never asserts the counter value; update the test for mockTask.consecutiveMistakeCount (set to 2 before calling tool.execute which mocks listFiles to reject) to assert it remains 2 after execution (and after expecting mockCallbacks.handleError). Specifically, in the test that calls tool.execute(params, mockTask, mockCallbacks) with vi.mocked(listFiles).mockRejectedValue(new Error("fail")), add an assertion like expect(mockTask.consecutiveMistakeCount).toBe(2) after the existing expect(mockCallbacks.handleError).toHaveBeenCalled() so the test verifies the counter was not reset on error.
🤖 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.
Duplicate comments:
In `@src/core/tools/__tests__/listFilesTool.spec.ts`:
- Around line 397-407: The test titled "should not reset consecutive mistake
count on error" never asserts the counter value; update the test for
mockTask.consecutiveMistakeCount (set to 2 before calling tool.execute which
mocks listFiles to reject) to assert it remains 2 after execution (and after
expecting mockCallbacks.handleError). Specifically, in the test that calls
tool.execute(params, mockTask, mockCallbacks) with
vi.mocked(listFiles).mockRejectedValue(new Error("fail")), add an assertion like
expect(mockTask.consecutiveMistakeCount).toBe(2) after the existing
expect(mockCallbacks.handleError).toHaveBeenCalled() so the test verifies the
counter was not reset on error.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 6ebc2dc4-39e8-42dd-869b-1aefc30a70b3
📒 Files selected for processing (1)
src/core/tools/__tests__/listFilesTool.spec.ts
Add full test coverage for ListFilesTool including: - Parameter validation (missing path) - Basic and recursive file listing - Approval flow and user cancellation - Relative and absolute path resolution - Error handling (nonexistent dirs, permission denied) - Partial message streaming (handlePartial) - Edge cases (empty dirs, large file sets, root/dot paths) - Singleton instance verification - RooIgnore/RooProtected controller passthrough Fixes cross-platform compatibility for Windows CI by using path.resolve() instead of hardcoded Unix paths, and removing the unnecessary path module mock that caused failures on Windows. Closes Zoo-Code-Org#209
proyectoauraorg
force-pushed
the
test/add-list-files-tool-tests
branch
from
2ccef1b to
833b22f
Compare
…rror path Addresses CodeRabbit review: the 'no reset on error' test never asserted the counter, so it would pass even if the reset behavior regressed. Renamed to reflect actual behavior (validation success resets to 0 before listFiles throws) and added the missing assertion. 40 tests pass. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
Good catch @coderabbitai — the test asserted nothing about the counter. Fixed: it now asserts `consecutiveMistakeCount === 0` and the name/comment reflect the real behavior (validation success resets the count to 0 before `listFiles` throws; the catch path leaves it untouched). |
|
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! |
1 participant
Description
This PR adds a comprehensive test suite for
ListFilesToolwith 40 test cases covering all critical code paths of the tool.Closes #206
Test Coverage
Parameter Validation (5 tests)
<hint>tagsHappy Path - Non-Recursive (7 tests)
list_filestool-specific approval filesHappy Path - Recursive (4 tests)
Provider State (3 tests)
Approval Flow (6 tests)
Path Handling (6 tests)
handlePartial (4 tests)
Edge Cases (5 tests)
Tests Results
Checklist
check-typespasses successfullySummary by CodeRabbit
Tests
Note