refactor(testing): migrate to playwright-test from jest-playwright #3133

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

Jump to bottom

Merged

repo-ranger merged 9 commits into main from jsjoeio/migrate-to-playwright-test

Apr 15, 2021

Merged

refactor(testing): migrate to playwright-test from jest-playwright #3133

repo-ranger merged 9 commits into main from jsjoeio/migrate-to-playwright-test

Apr 15, 2021

Conversation

jsjoeio

Copy link

Contributor

@jsjoeio jsjoeio commented Apr 14, 2021 •

edited

Loading

This PR refactors our test runner from jest-playwright to playwright-test at the recommendation of the Playwright Team.

Other handy features:

use --repeat-each to repeat the test suite multiple times

Major changes:

only videos for failed tests are saved
playwright-test automatically retries failed tests (3 times locally, 2 times in CI)
now runs tests in all three browsers (before Chromium. now WebKit and Firefox)

It should allow for a more flexible configuration and help us identify flaky tests faster.

Screenshot

image

TODOs

fix flaky tests
fix failing tests
fix logs to not say jest in globalSetup

Notes

logout is flaky/failing because of the login rate limiter (#2647) :( See video:

fd4a541aca126b2ed2e80158917e3e65.mp4

Fixes #3115 #2998

@jsjoeio jsjoeio added this to the v3.9.4 milestone

Apr 14, 2021

@jsjoeio jsjoeio self-assigned this

Apr 14, 2021

@jsjoeio jsjoeio linked an issue

Apr 14, 2021

that may be closed by this pull request

[dev]: enable end-to-end test suite on Safari and Firefox #2998

Closed

@jsjoeio jsjoeio force-pushed the jsjoeio/migrate-to-playwright-test branch from 035e24d to 3b1802c Compare

April 14, 2021 22:04

@jsjoeio jsjoeio marked this pull request as ready for review

April 14, 2021 22:29

@jsjoeio jsjoeio requested a review from a team as a code owner

April 14, 2021 22:29

jawnsy

jawnsy approved these changes

Apr 15, 2021

View reviewed changes

test/package.json

"ts-jest": "^26.4.4"

"resolutions": {

"@playwright/test/playwright": "^1.11.0-next-alpha-apr-13-2021"

Copy link

Choose a reason for hiding this comment

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

are there any risks we need to be aware of with respect to using an alpha version?

Copy link

Contributor Author

Choose a reason for hiding this comment

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

Possibly! I will take responsibility if something breaks.

I had to do this because I was running into bugs that don't occur in the newest (alpha) version of Playwright, specifically this one: microsoft/playwright#6020

I'll keep an eye on this and remove once the next playwright release comes out.

ci/dev/test-e2e.sh

CS_DISABLE_PLUGINS=true ./test/node_modules/.bin/jest "$@" --config ./test/jest.e2e.config.ts --runInBand

cd test

PASSWORD=e45432jklfdsab CODE_SERVER_ADDRESS=http://localhost:8080 yarn folio --config=config.ts --reporter=list "$@"

Copy link

Choose a reason for hiding this comment

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

I know this is just for testing, but would it be useful to randomize the password?

Copy link

Contributor Author

Choose a reason for hiding this comment

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

useful

Probably not. We need to password to be set as an environment variable for the tests in order to log in. I added a comment just now to explain that

test/config.ts

import { PASSWORD } from "./utils/constants"

import * as wtfnode from "./utils/wtfnode"

// Playwright doesn't like that ../src/node/util has an enum in it

Copy link

Choose a reason for hiding this comment

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

👎 that you had to do this, but 👍 that you added a comment... I could certainly be better about doing this myself, it's a great habit 😄

test/config.ts

}

if (process.env.CI) {

config.retries = 2

Copy link

@jawnsy jawnsy Apr 15, 2021 •

edited

Loading

Choose a reason for hiding this comment

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

I guess this is to handle flakiness in CI environments? if so, a comment might be nice

Copy link

Contributor Author

Choose a reason for hiding this comment

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

Yup! I thought the comment on L54 would suffice:
https://github.com/cdr/code-server/pull/3133/files/3b1802c2357c7707e487287391d16e93fb9eb756#diff-8c108cf05702abdfb2e259659b8823f7c917fced47193febd0ab7f4a39134294R54

But I'll add another

Copy link

Choose a reason for hiding this comment

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

that comment describes what the setting does but not why we need it, though :) IMO comments should be about the why because it gives context

Copy link

Contributor Author

Choose a reason for hiding this comment

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

Oops...I guess I thought the flakiness covered the why but maybe not. Working on that 😅

test/e2e/openHelpAbout.test.ts

const helpButton = "a.action-menu-item span[aria-label='Help']"

expect(await page.isVisible(helpButton))

// Click the Application menu

await page.click("[aria-label='Application Menu']")

Copy link

Choose a reason for hiding this comment

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

do people still do page objects or is that not cool anymore? also, is it possible to select by IDs instead of other attributes, or is this the recommended approach with Playwright?

Copy link

Contributor Author

Choose a reason for hiding this comment

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

Lol not sure about page objects. Unfortunately the way the HTML is structured, there isn't an ID to use as a selector. Otherwise, we would:
image

<div class="menubar-menu-button" role="menuitem" tabindex="0" aria-label="Application Menu" title="Application Menu" aria-haspopup="true" style="visibility: visible;"><div class="menubar-menu-title toolbar-toggle-more codicon codicon-menubar-more" role="none" aria-hidden="true"></div></div>

And this HTML comes from VS Code. We could add our own IDs, but there's a chance they get overwritten during a git subtree update to lib/vscode.

This means that if the VS Code Team changes the HTML (i.e. removes the aria-label) then our tests break. This can be annoying for us, but it's the best approach. Plus, aria-labels are less likely to change as frequently as something like a class so at least this attribute is a bit more stable.

jsjoeio added 9 commits

April 15, 2021 11:46

@jsjoeio


 chore(deps): add @playwright/test

0a090bf

@jsjoeio


 refactor: add new e2e config

08cd2d8

@jsjoeio


 refactor: use playwright-test syntax for e2e tests

@jsjoeio


 refactor: add login to config.js for e2e tests

7ea6d22

@jsjoeio


 chore(deps): add resolution for newest playwright

6c3bb10

@jsjoeio


 refactor: test-e2e.sh script

92b7c1e

@jsjoeio


 fix: add retry 2 for failing e2e tests

c9fa931

@jsjoeio


 fix: update path for e2e videos

d6f0725

@jsjoeio


 refactor: pass all args to test-e2e script

450fcd5

@jsjoeio jsjoeio force-pushed the jsjoeio/migrate-to-playwright-test branch from 3b1802c to 450fcd5 Compare

April 15, 2021 18:46

@jsjoeio jsjoeio added the merge when passing label

Apr 15, 2021

@repo-ranger repo-ranger bot merged commit 97fbbfa into main

Apr 15, 2021

@repo-ranger repo-ranger bot deleted the jsjoeio/migrate-to-playwright-test branch

April 15, 2021 19:04

@jsjoeio jsjoeio added the testing label

May 14, 2021

Labels

testing

2 participants

@jsjoeio @jawnsy

refactor(testing): migrate to playwright-test from jest-playwright #3133

refactor(testing): migrate to playwright-test from jest-playwright #3133

Uh oh!

Conversation

@jsjoeio jsjoeio commented Apr 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Screenshot

TODOs

Notes

Uh oh!

@jawnsy jawnsy Apr 14, 2021

Choose a reason for hiding this comment

Uh oh!

@jsjoeio jsjoeio Apr 15, 2021

Choose a reason for hiding this comment

Uh oh!

@jawnsy jawnsy Apr 14, 2021

Choose a reason for hiding this comment

Uh oh!

@jsjoeio jsjoeio Apr 15, 2021

Choose a reason for hiding this comment

Uh oh!

@jawnsy jawnsy Apr 15, 2021

Choose a reason for hiding this comment

Uh oh!

@jawnsy jawnsy Apr 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

@jsjoeio jsjoeio Apr 15, 2021

Choose a reason for hiding this comment

Uh oh!

@jawnsy jawnsy Apr 15, 2021

Choose a reason for hiding this comment

Uh oh!

@jsjoeio jsjoeio Apr 15, 2021

Choose a reason for hiding this comment

Uh oh!

@jawnsy jawnsy Apr 15, 2021

Choose a reason for hiding this comment

Uh oh!

@jsjoeio jsjoeio Apr 15, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

@jsjoeio jsjoeio commented Apr 14, 2021 •

edited

Loading

@jawnsy jawnsy Apr 15, 2021 •

edited

Loading