Initial Cypress e2e wab test migration to Playwright #149

Name	Status	Preview	Comments	Updated (UTC)
examples-plasmic-cms-i18n	❌ Failed (Inspect)	Aug 5, 2025 1:52pm
examples-react-email	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Aug 5, 2025 1:52pm

@github-actions

Copy link

github-actions bot commented Jul 15, 2025 •

edited

Loading

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.}

@vercel vercel bot deployed to Preview – examples-react-email

July 15, 2025 07:41

View deployment

@vercel vercel bot had a problem deploying to Preview – examples-plasmic-cms-i18n

July 15, 2025 07:41

Failure

sampullman

sampullman reviewed

Jul 16, 2025

View reviewed changes

Copy link

Contributor

@sampullman sampullman left a comment •

edited

Loading

Choose a reason for hiding this comment

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

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.

platform/wab/playwright/pages/project-page.ts Outdated

}

async switchArena(name: string) {

await this.projectNavButton.click({ force: true });

Copy link

Contributor

@sampullman sampullman Jul 16, 2025

Choose a reason for hiding this comment

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

I think in some cases, these force: true settings copied from Cypress should no longer be necessary.

Copy link

Member

@jaslong jaslong Jul 16, 2025

Choose a reason for hiding this comment

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

+1. Please get rid of as many force: true as possible.

platform/wab/playwright/pages/project-page.ts Outdated

}

await this.projectNavSearchInput.fill(name);

await this.frame.locator(`text=${name}`).click({ force: true });

await this.page.waitForTimeout(1000);

Copy link

Contributor

@sampullman sampullman Jul 16, 2025

Choose a reason for hiding this comment

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

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"

Copy link

Contributor

@sampullman sampullman Jul 16, 2025

Choose a reason for hiding this comment

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

Any reason to do this here, instead of loading dotenv in playwright config?

jaslong

jaslong reviewed

Jul 16, 2025

View reviewed changes

platform/wab/playwright/e2e/interactions-text.spec.ts Outdated

test("can create all types of text interactions", async ({ pages, page }) => {

//GIVEN

const TEST_DATA = {

Copy link

Member

@jaslong jaslong Jul 16, 2025

Choose a reason for hiding this comment

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

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.

platform/wab/playwright/e2e/interactions-text.spec.ts Outdated

await pages.projectPage.switchArena(TEST_DATA.arenaName);

//WHEN

const contentFrame = await findFrameByText(page, TEST_DATA.setToText);

Copy link

Member

@jaslong jaslong Jul 16, 2025

Choose a reason for hiding this comment

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

Why aren't these locators in ProjectPage? And why is findFrameByText necessary here instead of the specific frame locators that are already in ProjectPage?

platform/wab/playwright/types/interaction.ts Outdated

};

// eslint-disable-next-line no-shadow

Copy link

Member

@jaslong jaslong Jul 16, 2025

Choose a reason for hiding this comment

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

remove/fix eslint-disable-next-line

platform/wab/playwright/pages/project-page.ts Outdated

}

async switchArena(name: string) {

await this.projectNavButton.click({ force: true });

Copy link

Member

@jaslong jaslong Jul 16, 2025

Choose a reason for hiding this comment

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

+1. Please get rid of as many force: true as possible.

platform/wab/playwright/types/environment.ts Outdated

@@ -0,0 +1,21 @@

import { getEnvVar } from "../utils/env";

Copy link

Member

@jaslong jaslong Jul 16, 2025

Choose a reason for hiding this comment

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

Move getEnvVar into this file, do not export it.

platform/wab/playwright/types/interaction.ts Outdated

Copy link

Member

@jaslong jaslong Jul 16, 2025

Choose a reason for hiding this comment

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

Remove this file and inline types instead.

platform/wab/playwright/types/interaction.ts Outdated

};

// eslint-disable-next-line no-shadow

enum Operations {

Copy link

Member

@jaslong jaslong Jul 16, 2025

Choose a reason for hiding this comment

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

Use string literal unions instead. "new" | "clear"

platform/wab/playwright/pages/project-page.ts Outdated

Copy link

Member

@jaslong jaslong Jul 16, 2025

Choose a reason for hiding this comment

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

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").

platform/wab/playwright/pages/base-page.ts Outdated

@@ -0,0 +1,9 @@

import { Page } from "playwright/test";

export class BasePage {

Copy link

Member

@jaslong jaslong Jul 16, 2025

Choose a reason for hiding this comment

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

Get rid of this, just inline

constructor(private readonly page: Page)

in every model.

platform/wab/playwright/pages/project-page.ts Outdated

}

await this.closeSidebarButton.click();

}

Copy link

Member

@jaslong jaslong Jul 16, 2025

Choose a reason for hiding this comment

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

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?