Initial Cypress e2e wab test migration to Playwright#149
Initial Cypress e2e wab test migration to Playwright#149przemek-qadna wants to merge 8 commits intoplasmicapp:masterfrom
Conversation
|
@AdrianQADNA is attempting to deploy a commit to the Plasmic Team on Vercel. A member of the Team first needs to authorize it. |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Thank you for your submission, we really appreciate it! ❤️ Like many open-source projects, we ask that you sign our Individual Contributor License Agreement before we can accept your contribution. If you are contributing on behalf of a company, please contact us at help@plasmic.app to sign a Corporate Contributor License Agreement. You can sign the individual CLA by posting a comment with the below text. I have read, agree to, and hereby sign Plasmic's Individual Contributor License Agreement You can retrigger this bot by commenting recheck in this Pull Request. Posted by the CLA Assistant Lite bot. |
There was a problem hiding this comment.
Looks good overall, though maybe more abstract than needed in some places. For example, addInteraction -> getInteractionAction -> selectElementWithDataKey.
getInteractionAction is only used in one place. It will probably be reused later, but I'm not sure it's clearer than doing e.g. getElementWithDataKey in addInteraction. It's mostly personal preference, but I'd tend to prefer less indirection up front, adding later when it makes sense.
edit: @jaslong Does it make sense to move this to its own package, like platform/wab-e2e or wab-tests? That's what I usually do to keep heavy dev dependencies out of apps.
| } | ||
|
|
||
| async switchArena(name: string) { | ||
| await this.projectNavButton.click({ force: true }); |
There was a problem hiding this comment.
I think in some cases, these force: true settings copied from Cypress should no longer be necessary.
There was a problem hiding this comment.
+1. Please get rid of as many force: true as possible.
| } | ||
| await this.projectNavSearchInput.fill(name); | ||
| await this.frame.locator(`text=${name}`).click({ force: true }); | ||
| await this.page.waitForTimeout(1000); |
There was a problem hiding this comment.
I haven't tested it, but instead of waiting 1s I think it's sufficient to wait for #proj-nav-button to change to the new arena name. Then, subsequent Playwright selectors should naturally wait if the new content is still loading.
platform/wab/playwright/package.json
Outdated
| "@types/node": "^24.0.13" | ||
| }, | ||
| "scripts": { | ||
| "e2e": "npx dotenvx run -f .env.test -- npx playwright test" |
There was a problem hiding this comment.
Any reason to do this here, instead of loading dotenv in playwright config?
|
|
||
| test("can create all types of text interactions", async ({ pages, page }) => { | ||
| //GIVEN | ||
| const TEST_DATA = { |
There was a problem hiding this comment.
Not a fan of this pattern. It typically just obfuscates the actual values.
As a general rule, only make a variable for a constant if used 3 or more times.
| await pages.projectPage.switchArena(TEST_DATA.arenaName); | ||
|
|
||
| //WHEN | ||
| const contentFrame = await findFrameByText(page, TEST_DATA.setToText); |
There was a problem hiding this comment.
Why aren't these locators in ProjectPage? And why is findFrameByText necessary here instead of the specific frame locators that are already in ProjectPage?
| }; | ||
| }; | ||
|
|
||
| // eslint-disable-next-line no-shadow |
There was a problem hiding this comment.
remove/fix eslint-disable-next-line
| } | ||
|
|
||
| async switchArena(name: string) { | ||
| await this.projectNavButton.click({ force: true }); |
There was a problem hiding this comment.
+1. Please get rid of as many force: true as possible.
| @@ -0,0 +1,21 @@ | |||
| import { getEnvVar } from "../utils/env"; | |||
There was a problem hiding this comment.
Move getEnvVar into this file, do not export it.
There was a problem hiding this comment.
Remove this file and inline types instead.
| }; | ||
|
|
||
| // eslint-disable-next-line no-shadow | ||
| enum Operations { |
There was a problem hiding this comment.
Use string literal unions instead. "new" | "clear"
There was a problem hiding this comment.
Since we have a single-page application AND we have the concept of pages in our app, we should get rid of the "page" terminology. Maybe rename "page" to "model (i.e. rename from "pages/project-page.ts" to "models/StudioModel.ts")? Ultimately, in test code, I want to be able to write something like studio.switchArena("some page").
| @@ -0,0 +1,9 @@ | |||
| import { Page } from "playwright/test"; | |||
|
|
|||
| export class BasePage { | |||
There was a problem hiding this comment.
Get rid of this, just inline
constructor(private readonly page: Page)
in every model.
| } | ||
|
|
||
| await this.closeSidebarButton.click(); | ||
| } |
There was a problem hiding this comment.
I see this file growing to become as large as the existing cypress/support/utils.ts file. How should we divide it into separate files? Maybe by feature?
|
platform/wab-e2e would be fine too! As long as it's clear these tests are for wab. |
|
Need decision later Currentplatform/wab Roote2e-tests In Platformplatform/e2e- |
4 participants
This PR begins the migration of E2E tests from Cypress to Playwright. It includes:
Initial Playwright test structure and config
First migrated wab test as a reference
More tests will be ported in follow-up PRs.
plasmic-first-wab-test.mov