Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add specs for trigger errors #1151

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

feat: add specs for trigger errors #1151

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
80ca3b4
feat: add error thrown
himanshu-dixit Jan 3, 2025
be5fe37
Merge branch 'master' into ft-backend-error-specs
himanshu-dixit Jan 4, 2025
58c2189
Merge branch 'master' into ft-backend-error-specs
himanshu-dixit Jan 7, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 31 additions & 0 deletions js/src/sdk/models/triggers.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);
}

try {
await triggers.getTriggerConfig({
triggerId: "INVALID_TRIGGER_ID",
Comment on lines 100 to +107
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
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);
});

});
} catch (e: unknown) {
const error = e as ComposioError;
expect(error.message).toContain("TriggerNotFoundError");
isErrorThrown = true;
}
expect(isErrorThrown).toBe(true);
});

it("should throw an error if invalid connected account id is provided", async () => {
let isErrorThrown = false;
try {
await triggers.list({
connectedAccountIds: ["xxx"],
});
} catch (e: unknown) {
isErrorThrown = true;
const error = e as ComposioError;
expect(error.message).toContain("BadRequestError");
expect(error.description).toContain(
"Invalid connected account ids provi"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

);
}
expect(isErrorThrown).toBe(true);
});

it("should get the payload of a trigger", async () => {
const res = await triggers.getTriggerInfo({
triggerId: "GMAIL_NEW_GMAIL_MESSAGE",
Expand Down
2 changes: 1 addition & 1 deletion js/src/sdk/utils/errors/src/formatter.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ export const getAPIErrorDetails = (
if (hasNotReceivedResponseFromBE) {
genericMessage = predefinedError.message as string;
} else if (axiosError.config?.baseURL && axiosError.config?.url) {
genericMessage = `${errorNameFromBE || ""} ${errorTypeFromBE ? `- ${errorTypeFromBE}` : ""} on ${axiosError.config?.baseURL! + axiosError.config?.url!}`;
genericMessage = `${errorNameFromBE ? `${errorNameFromBE} - ` : ""}${errorTypeFromBE ? `${errorTypeFromBE}` : ""} on ${axiosError.config?.baseURL! + axiosError.config?.url!}`;
}

switch (errorCode) {
Expand Down
Loading