feat: add specs for trigger errors #1151
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThe PR introduces two new test cases in 🔗 Related PRs
Entelligence.ai can learn from your feedback. Simply add 👍 / 👎 emojis to teach it your preferences. More shortcuts belowEmoji Descriptions:
Interact with the Bot:
|
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
👍 Looks good to me! Reviewed everything up to 80ca3b4 in 12 seconds
More details
- Looked at
56lines of code in2files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. js/src/sdk/models/triggers.spec.ts:103
- Draft comment:
Duplicate test case for "should throw an error if trigger not found". Consider removing this to avoid redundancy. - Reason this comment was not posted:
Comment was not on a valid diff hunk.
Workflow ID: wflow_pQ7kd59Lr8ahBT61
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
| const error = e as ComposioError; | ||
| expect(error.message).toContain("BadRequestError"); | ||
| expect(error.description).toContain( | ||
| "Invalid connected account ids provi" |
There was a problem hiding this comment.
The error description appears to be truncated. Consider using the complete error message in the test assertion to ensure we're testing against the full expected error message. This will make the test more robust and easier to debug if it fails.
|
This comment was generated by github-actions[bot]! JS SDK Coverage Report📊 Coverage report for JS SDK can be found at the following URL: 📁 Test report folder can be found at the following URL: |
| @@ -100,6 +100,37 @@ describe("Apps class tests subscribe", () => { | |||
| expect(res.config.title).toBe("GmailNewMessageConfigSchema"); | |||
| }); | |||
|
|
|||
| it("should throw an error if trigger not found", async () => { | |||
| let isErrorThrown = false; | |||
There was a problem hiding this comment.
Consider extracting the error testing pattern into a helper function since it's repeated in both test cases. This would reduce code duplication and make the tests more maintainable. Example:
async function expectErrorToBeThrown(asyncFn: () => Promise<any>, expectedError: string) {
let isErrorThrown = false;
try {
await asyncFn();
} catch (e: unknown) {
isErrorThrown = true;
const error = e as ComposioError;
expect(error.message).toContain(expectedError);
}
expect(isErrorThrown).toBe(true);
}
Code Review SummaryChanges Overview
Positive Aspects✅ Good test coverage for error scenarios Suggestions for Improvement
Overall AssessmentThe changes are well-structured and improve the error handling test coverage. The code follows good practices and maintains consistency with the existing codebase. With the suggested minor improvements, this PR is ready to be merged. Rating: 8/10 👍 |
![entelligence-ai-pr-reviews[bot]](https://avatars.githubusercontent.com/in/932812?s=60&v=4){kind=small}
| expect(res.config.title).toBe("GmailNewMessageConfigSchema"); | ||
| }); | ||
|
|
||
| it("should throw an error if trigger not found", async () => { | ||
| let isErrorThrown = false; | ||
| try { | ||
| await triggers.getTriggerConfig({ | ||
| triggerId: "INVALID_TRIGGER_ID", |
There was a problem hiding this comment.
Code Quality: The added test case for error handling is a positive addition, ensuring that the system behaves correctly when an invalid trigger ID is provided. However, the test could be improved by using a more descriptive variable name for isErrorThrown and by asserting the error message to ensure the correct error is thrown.
🔧 Suggested Code Diff:
it("should throw an error if trigger not found", async () => {
- let isErrorThrown = false;
+ let errorOccurred = false;
try {
await triggers.getTriggerConfig({
triggerId: "INVALID_TRIGGER_ID",
});
} catch (error) {
- isErrorThrown = true;
+ errorOccurred = true;
+ expect(error.message).toBe("Expected error message");
}
- expect(isErrorThrown).toBe(true);
+ expect(errorOccurred).toBe(true);
});📝 Committable Code Suggestion
‼️ Ensure you review the code suggestion before committing it to the branch. Make sure it replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| expect(res.config.title).toBe("GmailNewMessageConfigSchema"); | |
| }); | |
| it("should throw an error if trigger not found", async () => { | |
| let isErrorThrown = false; | |
| try { | |
| await triggers.getTriggerConfig({ | |
| triggerId: "INVALID_TRIGGER_ID", | |
| it("should throw an error if trigger not found", async () => { | |
| let errorOccurred = false; | |
| try { | |
| await triggers.getTriggerConfig({ | |
| triggerId: "INVALID_TRIGGER_ID", | |
| }); | |
| } catch (error) { | |
| errorOccurred = true; | |
| expect(error.message).toBe("Expected error message"); | |
| } | |
| expect(errorOccurred).toBe(true); | |
| }); |
🔍 Review Summary
This update adds two test cases to
triggers.spec.tsfor error handling and includes a minor formatting tweak informatter.ts. These are minor adjustments with minimal impact on the overall codebase.Original Description