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

fix: flaky e2e patterns #16696

Open
wants to merge 18 commits into
base: main
Choose a base branch
from
Open

fix: flaky e2e patterns #16696

wants to merge 18 commits into from

Conversation

zomars
Copy link
Member

@zomars zomars commented Sep 18, 2024

What does this PR do?

This PR refactors various end-to-end tests to use a new submitAndWaitForResponse utility function for form submissions and API calls. It also introduces a confirmReschedule helper function to standardize the rescheduling process across tests. These changes improve test reliability and reduce duplication.

Key changes include:

  • Replacing direct API calls and response waiting with submitAndWaitForResponse
  • Updating booking, rescheduling, and cancellation flows to use the new utility functions
  • Refactoring team invitation and webhook tests to use the new helpers
  • Updating various tests related to event types, profiles, and organizations

Mandatory Tasks (DO NOT REMOVE)

  • I have self-reviewed the code (A decent size PR without self-review might be rejected).
  • I have added a Docs issue here if this PR makes changes that would require a documentation change. If N/A, write N/A here and check the checkbox.
  • I confirm automated tests are in place that prove my fix is effective or that my feature works.

How should this be tested?

  1. Run the full end-to-end test suite to ensure all tests pass with the new changes.
  2. Pay special attention to tests involving booking, rescheduling, and API interactions.
  3. Verify that the new utility functions are working as expected across different test scenarios.
  4. Check that no regressions have been introduced in the affected areas (e.g., team invitations, webhooks, event types).

Checklist

  • I have read the contributing guide
  • My code follows the style guidelines of this project
  • I have commented my code, particularly in hard-to-understand areas
  • I have checked if my changes generate no new warnings

Copy link

vercel bot commented Sep 18, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

2 Skipped Deployments
Name Status Preview Comments Updated (UTC)
cal ⬜️ Ignored (Inspect) Visit Preview Sep 19, 2024 8:50pm
calcom-web-canary ⬜️ Ignored (Inspect) Visit Preview Sep 19, 2024 8:50pm

Copy link
Member Author

zomars commented Sep 18, 2024

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @zomars and the rest of your teammates on Graphite Graphite

@keithwillcode keithwillcode added core area: core, team members only foundation labels Sep 18, 2024
@zomars zomars marked this pull request as ready for review September 18, 2024 02:14
@graphite-app graphite-app bot requested a review from a team September 18, 2024 02:14
@dosubot dosubot bot added the automated-tests area: unit tests, e2e tests, playwright label Sep 18, 2024
Copy link

graphite-app bot commented Sep 18, 2024

Graphite Automations

"Add foundation team as reviewer" took an action on this PR • (09/18/24)

1 reviewer was added to this PR based on Keith Williams's automation.

Comment on lines +18 to +27
const TESTING_USERNAMES = [
{
username: "demousernamex",
description: "",
},
{
username: "demo.username",
description: " to include periods(or dots)",
},
];
Copy link
Member Author

Choose a reason for hiding this comment

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

Merged tests into parametized one since it was the exact same code with only username differences

@@ -225,7 +228,6 @@ testBothFutureAndLegacyRoutes.describe("Event Types tests", () => {
await page.locator(`text="Cal Video (Global)"`).click();

await saveEventType(page);
await page.getByTestId("toast-success").waitFor();
Copy link
Member Author

Choose a reason for hiding this comment

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

Already handled on saveEventType

Comment on lines +22 to +24
await submitAndWaitForResponse(page, "/api/trpc/auth/changePassword?batch=1", {
action: () => page.locator("text=Update").click(),
});
Copy link
Contributor

@anikdhabal anikdhabal Sep 18, 2024

Choose a reason for hiding this comment

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

This is perfect to not depending on toast messages🙏

/** Fastest way so far to test for saving changes and form submissions */
/**
* Fastest way so far to test for saving changes and form submissions
* @see https://playwright.dev/docs/api/class-page#page-wait-for-response
Copy link
Member Author

Choose a reason for hiding this comment

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

Added reasoning for this utility.

}) => {
// eslint-disable-next-line playwright/no-skipped-test
test.skip(!process.env.DAILY_API_KEY, "DAILY_API_KEY is needed for this test");
Copy link
Member Author

Choose a reason for hiding this comment

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

IF process.env.DAILY_API_KEY is undefined videoCallUrl will be an empty string which IMO default the purpose of this test

@@ -78,7 +78,6 @@ test.describe("Team", () => {
password: "P4ssw0rd!",
});
await page.locator(`button:text("${t("add")}")`).click();
await page.locator(`[data-testid="copy-invite-link-button"]`).click();
Copy link
Member Author

Choose a reason for hiding this comment

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

Already handled in getInviteLink

Copy link
Member Author

@zomars zomars left a comment

Choose a reason for hiding this comment

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

Ready for review

Copy link
Member Author

@zomars zomars left a comment

Choose a reason for hiding this comment

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

Ready for review

Copy link
Contributor

github-actions bot commented Sep 18, 2024

E2E results are ready!

@anikdhabal
Copy link
Contributor

@zomars Can you check? Some tests are failing, especially login.e2e.

@zomars
Copy link
Member Author

zomars commented Sep 18, 2024

@zomars Can you check? Some tests are failing, especially login.e2e.

I'm waiting on published results to do a deep dive.

Copy link
Member Author

Choose a reason for hiding this comment

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

cc @anikdhabal good alternative to a hard refresh

Comment on lines +43 to +45
const seed = `${username}:${dayjs(startDate).utc().format()}:${new Date().getTime()}:${
workerInfo.workerIndex
}`;
Copy link
Member Author

Choose a reason for hiding this comment

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

Fixes rare collision case

Copy link
Member Author

Choose a reason for hiding this comment

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

Added waitUntil for non blocking promises and updated the unit test

@anikdhabal
Copy link
Contributor

Inviting an existing user and then › create a booking on new link It often gives a strange issue: net::ERR_BLOCKED_BY_RESPONSE

All checks passed🙏. @zomars can you check this weird issue. This is very flaky

@zomars
Copy link
Member Author

zomars commented Sep 19, 2024

Inviting an existing user and then › create a booking on new link It often gives a strange issue: net::ERR_BLOCKED_BY_RESPONSE

All checks passed🙏. @zomars can you check this weird issue. This is very flaky

It happens when we try to waitforUrl and the page does a hard refresh. Causing a response interruption.

@zomars zomars marked this pull request as ready for review September 19, 2024 20:35
Comment on lines +588 to +605
const scheduledTriggersAfterDisabling = await prisma.webhookScheduledTriggers.findMany({
where: {
webhook: {
userId: user.id,
},
},
select: {
payload: true,
webhook: {
select: {
userId: true,
},
},
startAfter: true,
},
});

expect(scheduledTriggersAfterDisabling.length).toBe(0);
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need this? We are already checking the response, and then we are making an extra db query just to check the length.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automated-tests area: unit tests, e2e tests, playwright core area: core, team members only foundation ready-for-e2e 💻 refactor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants