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

feat(playwright): migrate dashboard spec #1789

Merged
merged 10 commits into from
Mar 15, 2024
Merged

feat(playwright): migrate dashboard spec #1789

merged 10 commits into from
Mar 15, 2024

Conversation

seaerchin
Copy link
Contributor

Problem

Previously we were using cypress, which was slow. this PR is part 1 of shifting to playwright.

Solution

  1. shift from using existing cypress API to using playwright
  2. playwright exposes page, request, context - these are mostly self-explanatory; playwright has test isolation, so each test spec is a new page frame.
  3. use locators - this is a playwright concept, equivalent to cypress selectors.
  4. await for expect to assert that test passes, without the await, playwright will end prematurely and fail.

Copy link
Contributor

@dcshzj dcshzj left a comment

Choose a reason for hiding this comment

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

Non-blocking comments

Comment on lines +3 to +6
push:
branches: [ main, master ]
pull_request:
branches: [ main, master ]
Copy link
Contributor

Choose a reason for hiding this comment

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

nit/question: Not sure if playwright is faster than cypress, should we run this for every PR? Or should we hook onto the existing ci-e2e workflow so that it can be triggered manually?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

playwright is kinda fast but no concrete numbers, more a gut feel based off how they perform. i can hook onto the existing workflow, no worries

branches: [ main, master ]
jobs:
test:
timeout-minutes: 60
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: curious why we are intentionally limiting ourselves to 1 hour instead of the default 6 hours?

Copy link
Contributor

Choose a reason for hiding this comment

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

^ eh lets not leh, our workflow is not so complex, if it takes more than an hour is prob smt wrong

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 workflow is automatically created by playwright ._. fwiw, playwright runs 6 workers at once w/ max timeout of 30s so it should be ok most of the time

@@ -182,6 +184,7 @@
"@types/dompurify": "^3.0.4",
"@types/invariant": "^2.2.35",
"@types/jest": "^29.4.4",
"@types/node": "^20.10.5",
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: is this needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i honestly can't remember - i think yes but i can't recall why.

this package doesn't have any runtime impact (typing) so it shud be ok to keep?

e2e/fixtures/messages.ts Outdated Show resolved Hide resolved
e2e/utils/session.ts Show resolved Hide resolved
e2e/dashboard.spec.ts Outdated Show resolved Hide resolved
push:
branches: [ main, master ]
pull_request:
branches: [ main, master ]
Copy link
Contributor

Choose a reason for hiding this comment

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

hey i think typo here, do u mean push to develop rather than main

branches: [ main, master ]
jobs:
test:
timeout-minutes: 60
Copy link
Contributor

Choose a reason for hiding this comment

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

^ eh lets not leh, our workflow is not so complex, if it takes more than an hour is prob smt wrong

e2e/fixtures/messages.ts Outdated Show resolved Hide resolved
e2e/tsconfig.json Show resolved Hide resolved
* Read environment variables from file.
* https://github.com/motdotla/dotenv
*/
// require('dotenv').config();
Copy link
Contributor

Choose a reason for hiding this comment

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

unintended comment?

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 autogenerated by playwright. i left it in here cos it might be useful next time but can always remove if you feel like it's extra

package.json Outdated Show resolved Hide resolved
@seaerchin
Copy link
Contributor Author

not merging yet, will get playwright to baseline first

@mergify mergify bot requested a review from a team March 8, 2024 15:54
Copy link

mergify bot commented Mar 8, 2024

This pull request has been stale for more than 30 days! Could someone please take a look at it @isomerpages/iso-engineers

@seaerchin seaerchin merged commit 98e04d6 into develop Mar 15, 2024
4 of 5 checks passed
@seaerchin seaerchin deleted the feat/playwright branch March 15, 2024 10:55
@seaerchin seaerchin mentioned this pull request Mar 21, 2024
7 tasks
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.

4 participants