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.

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

test: Cover error handling cases in API tests (1/2) #2466

Merged
merged 5 commits into from
Nov 29, 2023

Conversation

DafyddLlyr
Copy link
Contributor

@DafyddLlyr DafyddLlyr commented Nov 20, 2023

What does this PR do?

  • Tidies up a few uncovered branches with test cases following refactors for documentation and stronger enforcing of types
  • Most test cases just cover exception handling, but there's a few others thrown in
  • I see it as a helpful step before trying to take on https://trello.com/c/TfmJiwID/2686-fix-airbrake-error-logging-on-api
  • PR was growing in size so I'll pick up the rest next week - happy for this to be merged upon review

API test coverage change in #2466 & #2482

- Before After
Statements 79.32% 80.41%
Branches 50.69% 55.68%
Functions 67.88% 67.88%
Lines 78.79% 79.66%

Copy link

github-actions bot commented Nov 20, 2023

Removed vultr server and associated DNS entries

@@ -7,7 +7,6 @@ import { Flow } from "../../../types";
const copyPortalAsFlow = async (flowId: string, portalNodeId: string) => {
// fetch the parent flow data
const flow = await getFlowData(flowId);
if (!flow) throw Error("Unknown flowId");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was redundant and unreachable, this exception is already thrown inside getFlowData()

@@ -44,7 +45,7 @@ export async function sendToEmail(
// Prepare email template
const config: EmailSubmissionNotifyConfig = {
personalisation: {
serviceName: flowName || "PlanX",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was also unreachable - all flows must have names.

@@ -54,12 +55,7 @@ export async function sendToEmail(

// Send the email
const response = await sendEmail("submit", sendToEmail, config);
if (response?.message !== "Success") {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Again, this code was unreachable - message is only set to "Success" and nothing else.

@DafyddLlyr DafyddLlyr requested a review from a team November 21, 2023 08:28
@DafyddLlyr DafyddLlyr marked this pull request as ready for review November 21, 2023 08:28
});
await expect(sendEmail("save", TEST_EMAIL, mockConfig)).rejects.toThrow();
});
});
Copy link
Member

Choose a reason for hiding this comment

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

These are super nice & clear 🙌

@DafyddLlyr DafyddLlyr merged commit 66e1bdc into main Nov 29, 2023
12 checks passed
@DafyddLlyr DafyddLlyr deleted the dp/bump-api-coverage branch November 29, 2023 09:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants