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

test: use playwright for integration tests #2634

Merged
merged 78 commits into from
Apr 11, 2022
Merged

Conversation

mcansh
Copy link
Collaborator

@mcansh mcansh commented Apr 4, 2022

the main differences with playwright compared to jest + puppeteer are

  • the lack of a "watch" mode
  • no toMatchInlineSnapshot
  • describe, beforeTest, beforeEach, etc need to be test. prefixed
  • all tests need to use test and not it

Signed-off-by: Logan McAnsh logan@mcan.sh

Closes: #

  • Docs
  • Tests

mcansh added 6 commits April 7, 2022 15:25
Signed-off-by: Logan McAnsh <logan@mcan.sh>
Signed-off-by: Logan McAnsh <logan@mcan.sh>
Signed-off-by: Logan McAnsh <logan@mcan.sh>
Signed-off-by: Logan McAnsh <logan@mcan.sh>
… used across platforms

Signed-off-by: Logan McAnsh <logan@mcan.sh>
Signed-off-by: Logan McAnsh <logan@mcan.sh>
@mcansh
Copy link
Collaborator Author

mcansh commented Apr 7, 2022

gonna look into setting up a playwright "fixture" so the api is a bit closer to what we have with puppeteer.

so instead of passing page from playwright to all the app.whatever calls like this:

test("handles files under upload size limit", async ({ page }) => {
    let uploadFile = path.join(
      fixture.projectDir,
      "toUpload",
      "underLimit.txt"
    );
    let uploadData = Array(1_000).fill("a").join(""); // 1kb
    await fs
      .mkdir(path.dirname(uploadFile), { recursive: true })
      .catch(() => {});
    await fs.writeFile(uploadFile, uploadData, "utf8");

    await app.goto(page, "/file-upload");
    await app.uploadFile(page, "#file", uploadFile);
    await app.clickSubmitButton(page, "/file-upload");
})

we can instead do this

test("handles files under upload size limit", async ({ page, idkthenameyet }) => {
    let uploadFile = path.join(
      fixture.projectDir,
      "toUpload",
      "underLimit.txt"
    );
    let uploadData = Array(1_000).fill("a").join(""); // 1kb
    await fs
      .mkdir(path.dirname(uploadFile), { recursive: true })
      .catch(() => {});
    await fs.writeFile(uploadFile, uploadData, "utf8");

    await idkthenameyet.goto("/file-upload");
    await idkthenameyet.uploadFile("#file", uploadFile);
    await idkthenameyet.clickSubmitButton("/file-upload");
})

actually... being that we need to create our fixture app before we try to use playwright we may have to do it more like so

test("handles files under upload size limit", async ({ page }) => {
    let app = new PlaywrightFixture(appFixture, page);
    let uploadFile = path.join(
      fixture.projectDir,
      "toUpload",
      "underLimit.txt"
    );
    let uploadData = Array(1_000).fill("a").join(""); // 1kb
    await fs
      .mkdir(path.dirname(uploadFile), { recursive: true })
      .catch(() => {});
    await fs.writeFile(uploadFile, uploadData, "utf8");

    await app.goto("/file-upload");
    await app.uploadFile("#file", uploadFile);
    await app.clickSubmitButton("/file-upload");
})

as you can't access the page context from within a beforeAll https://playwright.dev/docs/test-fixtures#creating-a-fixture

Signed-off-by: Logan McAnsh <logan@mcan.sh>
integration/helpers/playwright-fixture.ts Outdated Show resolved Hide resolved
integration/helpers/playwright-fixture.ts Outdated Show resolved Hide resolved
integration/helpers/playwright-fixture.ts Outdated Show resolved Hide resolved
mcansh added 3 commits April 11, 2022 11:19
Signed-off-by: Logan McAnsh <logan@mcan.sh>
Signed-off-by: Logan McAnsh <logan@mcan.sh>
@mcansh mcansh marked this pull request as ready for review April 11, 2022 17:03
Copy link
Member

@kentcdodds kentcdodds left a comment

Choose a reason for hiding this comment

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

Looks pretty good. I just have a couple of things for now. I'll pull this down and try it out soon.

integration/action-test.ts Outdated Show resolved Hide resolved
@@ -29,6 +29,7 @@
"esbuild": "0.14.22",
"esbuild-plugin-cache": "^0.2.9",
"execa": "^5.1.1",
"esbuild-register": "^3.3.2",
Copy link
Member

Choose a reason for hiding this comment

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

Is this needed as a dependency?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

i think it's from a merge conflict, i can remove it

Copy link
Member

Choose a reason for hiding this comment

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

On that note, how much trouble would it be to clean up the commit history on this branch? If it takes more than 10 seconds, don't bother.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

should be pretty easy to do, i'll check after lunch

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

eh my git fu is failing me today

.github/workflows/test.yml Show resolved Hide resolved
.github/workflows/test.yml Outdated Show resolved Hide resolved
Co-authored-by: Michaël De Boey <info@michaeldeboey.be>
Copy link
Member

@kentcdodds kentcdodds left a comment

Choose a reason for hiding this comment

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

Ran things locally and went through each file. Just a couple things. This is way faster 👏👏

.gitignore Show resolved Hide resolved
integration/action-test.ts Show resolved Hide resolved
integration/action-test.ts Show resolved Hide resolved
integration/action-test.ts Show resolved Hide resolved
integration/helpers/utils.ts Outdated Show resolved Hide resolved
integration/playwright.config.ts Outdated Show resolved Hide resolved
integration/playwright.config.ts Outdated Show resolved Hide resolved
integration/playwright.config.ts Outdated Show resolved Hide resolved
integration/playwright.config.ts Outdated Show resolved Hide resolved
integration/resource-routes-test.ts Show resolved Hide resolved
@kentcdodds
Copy link
Member

Oh yeah, could you also update the contributing.md file please?

mcansh added 2 commits April 11, 2022 16:32
Signed-off-by: Logan McAnsh <logan@mcan.sh>
Signed-off-by: Logan McAnsh <logan@mcan.sh>
Copy link
Member

@kentcdodds kentcdodds left a comment

Choose a reason for hiding this comment

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

Solid 👍

@kentcdodds
Copy link
Member

@mcansh
Copy link
Collaborator Author

mcansh commented Apr 11, 2022

CI is giving us this:

image

remix-run/remix/actions/runs/2151038318

forgot to remove the upload action, one sec :)

mcansh added 2 commits April 11, 2022 17:03
Signed-off-by: Logan McAnsh <logan@mcan.sh>
Signed-off-by: Logan McAnsh <logan@mcan.sh>
Copy link
Member

@kentcdodds kentcdodds left a comment

Choose a reason for hiding this comment

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

Just one last question, otherwise I think this is ready to go.

.github/workflows/test.yml Show resolved Hide resolved
@kentcdodds kentcdodds merged commit 00a4643 into dev Apr 11, 2022
@kentcdodds kentcdodds deleted the logan/use-playwright branch April 11, 2022 21:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants