test(gadgets): add unit tests for FileSearchAndReplace#643
test(gadgets): add unit tests for FileSearchAndReplace#643zbigniewsobiecki merged 2 commits intodevfrom
Conversation
|
✅ Review complete for PR #643. Solid test coverage — 18 tests covering all key behaviors of FileSearchAndReplace. One inline comment about a tautological assertion in the 'provides suggestions' test that should be tightened to actually verify the behavior it claims to test. |
nhopeatall
left a comment
There was a problem hiding this comment.
Summary
Solid test coverage for the FileSearchAndReplace gadget. One test has a weak assertion worth tightening; otherwise LGTM.
Code Issues
Should Fix
- tests/unit/gadgets/fileSearchAndReplace.test.ts:260 — The
'provides suggestions when similar content exists'test has a tautological assertion:expect(errorMessage.length > 0 || readFileSync(...).includes('5000')).toBe(true). This passes whether the operation throws OR succeeds via fuzzy matching — it's asserting "something happened" rather than verifying the suggestions behavior the test name promises. If the intent is to verify suggestions appear in the error, assert on the error message content (e.g.,expect(errorMessage).toMatch(/similar|did you mean/i)). If the intent is to verify fuzzy matching succeeds, assert on the file content directly. The current form won't catch regressions in either path.
| // The important thing is it doesn't silently do nothing | ||
| expect(errorMessage.length > 0 || readFileSync(filePath, 'utf-8').includes('5000')).toBe( | ||
| true, | ||
| ); |
There was a problem hiding this comment.
[SHOULD_FIX] This assertion is tautological — it passes whether the gadget throws or succeeds. The test name says "provides suggestions when similar content exists" but doesn't verify that suggestions actually appear in the error.
Consider making this specific to the behavior you want to lock in:
As-is, this test would still pass if the gadget silently returned an empty string or threw a completely unrelated error.
There was a problem hiding this comment.
Fixed. The assertion was indeed tautological — it turned out the original search string (const timeoutMs = 1000;) was close enough to succeed via fuzzy matching, so the gadget never even threw.
I rewrote the test with a search string calibrated to fall between the match threshold (0.8) and suggestion threshold (0.6): it now searches for a structurally similar but sufficiently different function signature. The test now makes three specific assertions:
expect(errorMessage).toMatch(/NOT FOUND/i)— verifies the gadget throws (not silently succeeds)expect(errorMessage).toMatch(/SIMILAR CONTENT FOUND/i)— verifies suggestions are includedexpect(errorMessage).toContain('processOrder')— verifies the actual file content appears in the suggestion
All 18 tests pass, 240/240 unit test files pass.
|
@aaight address code review concerns |
…tions Replace the tautological assertion in the "provides suggestions when similar content exists" test that would pass regardless of gadget behavior. The test now verifies that the error message contains both "NOT FOUND", "SIMILAR CONTENT FOUND", and the actual similar content from the file. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Request AddressedWhat was askedAddress code review concern about a tautological assertion in the "provides suggestions when similar content exists" test. Changes Made
Verification
|
nhopeatall
left a comment
There was a problem hiding this comment.
LGTM — Well-structured test file with comprehensive coverage of all FileSearchAndReplace code paths. Follows the established mock pattern from fileInsertContent.test.ts exactly, uses real filesystem operations for end-to-end verification, and all 18 tests pass with CI green.
Summary
tests/unit/gadgets/fileSearchAndReplace.test.tswith 18 test cases covering all acceptance criteriafileInsertContent.test.ts(mockingpathValidation,readTracking,postEditChecks, anddiagnosticState)Test coverage added
replaceAll=true— replaces all occurrences, output includes match count and line rangesexpectedCountmismatch — aborts with clear error listing found locations; passes when count matchesNOT FOUNDerror including the search contentreplaceAll— throwsAmbiguouserror mentioningreplaceAlloption and match countFile not founderrorreplaceAlldeletion both verified;replaceAlloutput includes "All matches deleted."Test plan
npx vitest run tests/unit/gadgets/fileSearchAndReplace.test.ts— 18/18 passnpx vitest run --project unit— 240/240 pass (no regressions)Card: https://trello.com/c/TwTKy2Tj/179-as-a-developer-i-want-unit-tests-for-the-filesearchandreplace-gadget-so-that-the-most-used-edit-tool-has-regression-coverage
🤖 Generated with Claude Code