refactor(testing): migrate to playwright-test from jest-playwright #3133
Conversation
jsjoeio
force-pushed
the
jsjoeio/migrate-to-playwright-test
branch
from
035e24d to
3b1802c
Compare
| "supertest": "^6.1.1", | ||
| "ts-jest": "^26.4.4" | ||
| }, | ||
| "resolutions": { | ||
| "@playwright/test/playwright": "^1.11.0-next-alpha-apr-13-2021" |
There was a problem hiding this comment.
are there any risks we need to be aware of with respect to using an alpha version?
There was a problem hiding this comment.
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.
| fi | ||
| 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 "$@" |
There was a problem hiding this comment.
I know this is just for testing, but would it be useful to randomize the password?
There was a problem hiding this comment.
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
| import { PASSWORD } from "./utils/constants" | ||
| import * as wtfnode from "./utils/wtfnode" | ||
|
|
||
| // Playwright doesn't like that ../src/node/util has an enum in it |
There was a problem hiding this comment.
👎 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 😄
| } | ||
|
|
||
| if (process.env.CI) { | ||
| config.retries = 2 |
There was a problem hiding this comment.
I guess this is to handle flakiness in CI environments? if so, a comment might be nice
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Oops...I guess I thought the flakiness covered the why but maybe not. Working on that 😅
| 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']") |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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:
<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
force-pushed
the
jsjoeio/migrate-to-playwright-test
branch
from
3b1802c to
450fcd5
Compare
This PR refactors our test runner from
jest-playwrighttoplaywright-testat the recommendation of the Playwright Team.Other handy features:
--repeat-eachto repeat the test suite multiple timesMajor changes:
playwright-testautomatically retries failed tests (3 times locally, 2 times in CI)It should allow for a more flexible configuration and help us identify flaky tests faster.
Screenshot
TODOs
Notes
logoutis flaky/failing because of the login rate limiter (#2647) :( See video:fd4a541aca126b2ed2e80158917e3e65.mp4
Fixes #3115 #2998