refactor(testing): migrate to playwright-test from jest-playwright by jsjoeio · Pull Request #3133 · coder/code-server · GitHub

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

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

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

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.

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

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

👎 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 😄

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

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

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

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

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?

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 added this to the v3.9.4 milestone Apr 14, 2021

jsjoeio self-assigned this Apr 14, 2021

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 force-pushed the jsjoeio/migrate-to-playwright-test branch from 035e24d to 3b1802c Compare April 14, 2021 22:04

jsjoeio marked this pull request as ready for review April 14, 2021 22:29

jsjoeio requested a review from a team as a code owner April 14, 2021 22:29

jawnsy approved these changes Apr 15, 2021

View reviewed changes

jsjoeio added 9 commits April 15, 2021 11:46

chore(deps): add @playwright/test

0a090bf

refactor: add new e2e config

08cd2d8

refactor: use playwright-test syntax for e2e tests

5258670

refactor: add login to config.js for e2e tests

7ea6d22

chore(deps): add resolution for newest playwright

6c3bb10

refactor: test-e2e.sh script

92b7c1e

fix: add retry 2 for failing e2e tests

c9fa931

fix: update path for e2e videos

d6f0725

refactor: pass all args to test-e2e script

450fcd5

jsjoeio force-pushed the jsjoeio/migrate-to-playwright-test branch from 3b1802c to 450fcd5 Compare April 15, 2021 18:46

jsjoeio added the merge when passing label Apr 15, 2021

repo-ranger Bot merged commit 97fbbfa into main Apr 15, 2021

repo-ranger Bot deleted the jsjoeio/migrate-to-playwright-test branch April 15, 2021 19:04

jsjoeio added the testing Anything related to testing label May 14, 2021

                   "ts-jest": "^26.4.4"
+                },
+                "resolutions": {
+                  "@playwright/test/playwright": "^1.11.0-next-alpha-apr-13-2021"

-                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 "$@"

-                  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']")

refactor(testing): migrate to playwright-test from jest-playwright by jsjoeio · Pull Request #3133 · coder/code-server · GitHub

Uh oh!

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

refactor(testing): migrate to playwright-test from jest-playwright#3133
repo-ranger[bot] merged 9 commits into
mainfrom
jsjoeio/migrate-to-playwright-test

jsjoeio commented Apr 14, 2021 •
edited
Loading

Uh oh!

Uh oh!

jawnsy Apr 14, 2021

Uh oh!

jsjoeio Apr 15, 2021

Uh oh!

jawnsy Apr 14, 2021

Uh oh!

jsjoeio Apr 15, 2021

Uh oh!

jawnsy Apr 15, 2021

Uh oh!

jawnsy Apr 15, 2021 •
edited
Loading

Uh oh!

Uh oh!

jsjoeio Apr 15, 2021

Uh oh!

jawnsy Apr 15, 2021

Uh oh!

jsjoeio Apr 15, 2021

Uh oh!

jawnsy Apr 15, 2021

Uh oh!

jsjoeio Apr 15, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

+              import { PASSWORD } from "./utils/constants"
+              import * as wtfnode from "./utils/wtfnode"
+              // Playwright doesn't like that ../src/node/util has an enum in it

+              }
+              if (process.env.CI) {
+                config.retries = 2

refactor(testing): migrate to playwright-test from jest-playwright by jsjoeio · Pull Request #3133 · coder/code-server · GitHub

Uh oh!

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

refactor(testing): migrate to playwright-test from jest-playwright#3133repo-ranger[bot] merged 9 commits intomaincoder/code-server:mainfrom jsjoeio/migrate-to-playwright-testcoder/code-server:jsjoeio/migrate-to-playwright-testCopy head branch name to clipboard

Conversation

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

refactor(testing): migrate to playwright-test from jest-playwright#3133
repo-ranger[bot] merged 9 commits into
mainfrom
jsjoeio/migrate-to-playwright-test

jsjoeio commented Apr 14, 2021 •
edited
Loading

jawnsy Apr 15, 2021 •
edited
Loading