Skip to content

One playwright config to rule them all#4072

Merged
zomars merged 18 commits intomainfrom
fixes/e2e-consolidation
Sep 2, 2022
Merged

One playwright config to rule them all#4072
zomars merged 18 commits intomainfrom
fixes/e2e-consolidation

Conversation

@zomars
Copy link
Copy Markdown
Contributor

@zomars zomars commented Sep 1, 2022

What does this PR do?

  • Consolidates App Store and Web E2E test config
  • Planning on consolidating embed as well in a follow-up
  • Makes correct usage of Playwright's Project config
  • Stops using turbo for testing as there was little benefit and it was getting messy
  • Add special .e2e.ts file naming to distinguish between Unit and E2E tests for App Store only, I'm planning to adopt this in the whole monorepo later on

NOTE: Tests are failing because GitHub is trying to run the old YAML files

Type of change

  • Chore (refactoring code, technical debt, workflow improvements)

How should this be tested?

  • yarn test
  • yarn test-e2e
  • yarn test-e2e:app-store

@vercel
Copy link
Copy Markdown

vercel bot commented Sep 1, 2022

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

Name Status Preview Updated
cal ✅ Ready (Inspect) Visit Preview Sep 2, 2022 at 1:51AM (UTC)
swagger ❌ Failed (Inspect) Sep 2, 2022 at 1:51AM (UTC)


export * from "@calcom/app-store/_apps-playwright/lib/testUtils";
export async function cleanUpForms() {
await prisma.app_RoutingForms_Form.deleteMany({
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

No need for this since deleting test users cascades forms as well

}

export async function cleanUpSeededForm(formId: string) {
return await prisma.app_RoutingForms_FormResponse.deleteMany({
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Same here

Comment thread .github/workflows/e2e.yml Outdated
Comment on lines +86 to +87
- run: yarn workspace @calcom/prisma db-seed
- run: yarn build
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

No need for turbo here

@@ -1,7 +1,7 @@
name: E2E Test - Integrations with Third Party
on:
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This was running the same command as normal yarn test-e2e am I missing something? @hariombalhara

Comment on lines +37 to +40
- job: test
paths:
- /apps/web/**
- /packages/**
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Jest runs in parallel now. Doesn't depend on E2E

Comment thread apps/web/package.json
"test-e2e-integrations": "NEXT_PUBLIC_IS_E2E=1 yarn playwright test --config=playwright-integrations/config/playwright.config.ts --project=chromium",
"test-e2e-integrations-quick": "QUICK=true E2E_DEV_SERVER=1 yarn test-e2e-integrations",
"db-setup-tests": "dotenv -e ./test/.env.test -- yarn workspace @calcom/prisma prisma generate",
"playwright-report": "playwright show-report playwright/reports/playwright-html-report",
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

No more individual runners

"scripts": {
"app-e2e": "yarn playwright test --config=playwright/config/playwright.config.ts",
"app-e2e-quick": "QUICK=true yarn app-e2e"
},
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Apps don't need to setup their own runners anymore. You just need to add a *.e2e.ts file and playwright should pick it up.

@zomars zomars marked this pull request as ready for review September 1, 2022 03:33
@zomars zomars changed the title Fixes/e2e consolidation One playwright config to rule them all Sep 1, 2022
@zomars zomars added this to the v.2.0 milestone Sep 1, 2022
@zomars zomars self-assigned this Sep 1, 2022
@zomars zomars requested a review from a team September 1, 2022 03:34
@hariombalhara
Copy link
Copy Markdown
Member

@zomars To get the status of tests for the branch, maybe we can add fixes/e2e-consolidation as branch push target

@hariombalhara
Copy link
Copy Markdown
Member

I like the idea of having just one playwright.config 🙌

Copy link
Copy Markdown
Contributor

@zlwaterfield zlwaterfield left a comment

Choose a reason for hiding this comment

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

This looks good! I do agree with @hariombalhara, it might be good to update the actions config so the E2E can run on this branch to make sure they pass before merging.

@zomars
Copy link
Copy Markdown
Contributor Author

zomars commented Sep 1, 2022

@zlwaterfield @hariombalhara push tests are passing now 🙏🏽

Copy link
Copy Markdown
Contributor

@leog leog left a comment

Choose a reason for hiding this comment

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

  • yarn test worked perfectly
  • For yarn test-e2e I had to run npx playwright install first which makes sense, and then got an error that I had to build before running it. Did that and got an error regarding the database: Please make sure your database server is running at localhost:5450. This try was with PLAYWRIGHT_HEADLESS=true.
  • For yarn test-e2e:app-store I got time outs. Also run with PLAYWRIGHT_HEADLESS=true.

Really great job on putting effort here to make everything more understandable and accessible. The much needed groundwork for your suggestion around having a Testing Expert.

@zomars
Copy link
Copy Markdown
Contributor Author

zomars commented Sep 1, 2022

@leog you're right leog. This is a breaking change since yarn test-e2e doesn't run the seeder and build anymore

@zomars zomars merged commit d27b7ab into main Sep 2, 2022
@zomars zomars deleted the fixes/e2e-consolidation branch September 2, 2022 02:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

No open projects
Status: Done

Development

Successfully merging this pull request may close these issues.

4 participants