test(errors): add unit tests for EmptyResponseError and ToolIdMismatchError#555
test(errors): add unit tests for EmptyResponseError and ToolIdMismatchError#555
Conversation
…hError Co-Authored-By: Giulio Vaccari <io@giuliovaccari.it>
WalkthroughA new test file is introduced with 105 lines of test code for two custom error classes. The test suite verifies that these errors behave like standard Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
Greptile SummaryThis PR adds a new Confidence Score: 4/5Safe to merge; the only finding is a misleading test name that doesn't validate what it claims. Only P2 findings are present. The test suite provides solid coverage of both error classes, and no logic or security issues were found. src/plugin/errors.test.ts — one test description claims to verify singular language but the assertion does not. Important Files Changed
Class Diagram%%{init: {'theme': 'neutral'}}%%
classDiagram
class Error {
+message: string
+name: string
}
class EmptyResponseError {
+provider: string
+model: string
+attempts: number
+constructor(provider, model, attempts, message?)
}
class ToolIdMismatchError {
+expectedIds: string[]
+foundIds: string[]
+constructor(expectedIds, foundIds, message?)
}
Error <|-- EmptyResponseError
Error <|-- ToolIdMismatchError
Prompt To Fix All With AIThis is a comment left during a code review.
Path: src/plugin/errors.test.ts
Line: 38-41
Comment:
**Misleading test name doesn't verify singular language**
The test is titled `"works with attempts = 1 (singular language check)"` but only asserts `toContain("1")`. It never checks that the message uses singular form. The current implementation always interpolates `"attempts"` — so with `attempts = 1` the message reads *"after 1 attempts"*, which is grammatically wrong, yet this test passes. If the intent is to document and verify singular handling, the assertion should match:
```suggestion
it("works with attempts = 1 (singular language check)", () => {
const err = new EmptyResponseError("p", "m", 1);
expect(err.message).toContain("1");
expect(err.message).toMatch(/\b1 attempt\b/);
});
```
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "test(errors): add unit tests for EmptyRe..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| }); | ||
|
|
||
| it("can be thrown and caught as an Error", () => { | ||
| expect(() => { |
There was a problem hiding this comment.
The test is titled "works with attempts = 1 (singular language check)" but only asserts toContain("1"). It never checks that the message uses singular form. The current implementation always interpolates "attempts" — so with attempts = 1 the message reads "after 1 attempts", which is grammatically wrong, yet this test passes. If the intent is to document and verify singular handling, the assertion should match:
| }); | |
| it("can be thrown and caught as an Error", () => { | |
| expect(() => { | |
| it("works with attempts = 1 (singular language check)", () => { | |
| const err = new EmptyResponseError("p", "m", 1); | |
| expect(err.message).toContain("1"); | |
| expect(err.message).toMatch(/\b1 attempt\b/); | |
| }); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/plugin/errors.test.ts
Line: 38-41
Comment:
**Misleading test name doesn't verify singular language**
The test is titled `"works with attempts = 1 (singular language check)"` but only asserts `toContain("1")`. It never checks that the message uses singular form. The current implementation always interpolates `"attempts"` — so with `attempts = 1` the message reads *"after 1 attempts"*, which is grammatically wrong, yet this test passes. If the intent is to document and verify singular handling, the assertion should match:
```suggestion
it("works with attempts = 1 (singular language check)", () => {
const err = new EmptyResponseError("p", "m", 1);
expect(err.message).toContain("1");
expect(err.message).toMatch(/\b1 attempt\b/);
});
```
How can I resolve this? If you propose a fix, please make it concise.![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/plugin/errors.test.ts (2)
98-104: Avoid over-constraining internals via reference-identity assertions.At Line 98, asserting
toBeon arrays locks in reference preservation as part of the public contract, which can block safe future refactors (e.g., defensive copies). Prefer value-level contract unless identity is intentionally required API behavior.Alternative assertion
- expect(err.expectedIds).toBe(expected); - expect(err.foundIds).toBe(found); + expect(err.expectedIds).toEqual(expected); + expect(err.foundIds).toEqual(found);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/plugin/errors.test.ts` around lines 98 - 104, The test is asserting reference identity for arrays which over-constrains ToolIdMismatchError internals; change the two assertions in the "preserves array references exactly" test to value-level equality checks (e.g., use deep equality assertions against err.expectedIds and err.foundIds) so the spec verifies contents not object identity while keeping the public contract for ToolIdMismatchError intact.
24-28: Strengthen default-message assertions to validate formatting, not just tokens.At Line 24 and Line 74,
toContain(...)checks are a bit permissive. Consider asserting the full expected default message for fixed inputs so regressions in phrasing/format are caught earlier.Proposed tightening
- expect(err.message).toContain("2"); - expect(err.message).toContain("attempts"); + expect(err.message).toBe( + "The model returned an empty response after 2 attempts. This may indicate a temporary service issue. Please try again.", + );- expect(err.message).toContain("x"); - expect(err.message).toContain("y"); - expect(err.message).toContain("z"); + expect(err.message).toBe( + "Tool ID mismatch: expected [x, y] but found [z]", + );Also applies to: 74-79
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/plugin/errors.test.ts` around lines 24 - 28, The tests use loose toContain assertions for EmptyResponseError messages; change them to assert the exact expected default message string for given constructor inputs so formatting/regression issues are caught. Update the first spec that constructs new EmptyResponseError("gemini","model-x",2) to expect the full exact message (including the attempt count and the word "attempts") rather than toContain("2")/toContain("attempts"), and likewise tighten the second spec around the other EmptyResponseError case (the block at lines ~74-79) to compare against the full expected message text produced by EmptyResponseError instead of partial contains checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/plugin/errors.test.ts`:
- Around line 98-104: The test is asserting reference identity for arrays which
over-constrains ToolIdMismatchError internals; change the two assertions in the
"preserves array references exactly" test to value-level equality checks (e.g.,
use deep equality assertions against err.expectedIds and err.foundIds) so the
spec verifies contents not object identity while keeping the public contract for
ToolIdMismatchError intact.
- Around line 24-28: The tests use loose toContain assertions for
EmptyResponseError messages; change them to assert the exact expected default
message string for given constructor inputs so formatting/regression issues are
caught. Update the first spec that constructs new
EmptyResponseError("gemini","model-x",2) to expect the full exact message
(including the attempt count and the word "attempts") rather than
toContain("2")/toContain("attempts"), and likewise tighten the second spec
around the other EmptyResponseError case (the block at lines ~74-79) to compare
against the full expected message text produced by EmptyResponseError instead of
partial contains checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 631384db-49b1-4189-b43b-a0a18ce564a7
📒 Files selected for processing (1)
src/plugin/errors.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Greptile Review
- GitHub Check: Test on Node.js
🔇 Additional comments (1)
src/plugin/errors.test.ts (1)
6-105: Solid coverage for both error classes.Nice test ramp-up: inheritance, metadata fields, default/custom messages, and throw/catch behavior are all covered well.
1 participant
Unit test ramp up exercise