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

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

Merged
merged 9 commits into from
Apr 15, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions .github/workflows/ci.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -350,11 +350,11 @@ jobs:
if: always()
uses: actions/upload-artifact@v2
with:
name: test-videos
path: ./test/e2e/videos
name: failed-test-videos
path: ./test/test-results

- name: Remove release packages and test artifacts
run: rm -rf ./release-packages ./test/e2e/videos
run: rm -rf ./release-packages ./test/test-results

docker-amd64:
runs-on: ubuntu-latest
Expand Down
4 changes: 2 additions & 2 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -16,5 +16,5 @@ node-*
.home
coverage
**/.DS_Store
test/e2e/videos
test/e2e/screenshots
# Failed e2e test videos are saved here
test/test-results
17 changes: 4 additions & 13 deletions ci/dev/test-e2e.sh
Original file line number Diff line number Diff line change
Expand Up @@ -3,19 +3,10 @@ set -euo pipefail

main() {
cd "$(dirname "$0")/../.."
# We must keep jest in a sub-directory. See ../../test/package.json for more
# information. We must also run it from the root otherwise coverage will not
# include our source files.
if [[ -z ${PASSWORD-} ]] || [[ -z ${CODE_SERVER_ADDRESS-} ]]; then
echo "The end-to-end testing suites rely on your local environment"
echo -e "\n"
echo "Please set the following environment variables locally:"
echo " \$PASSWORD"
echo " \$CODE_SERVER_ADDRESS"
echo -e "\n"
exit 1
fi
CS_DISABLE_PLUGINS=true ./test/node_modules/.bin/jest "$@" --config ./test/jest.e2e.config.ts --runInBand
cd test
# We set these environment variables because they're used in the e2e tests
# they don't have to be these values, but these are the defaults
PASSWORD=e45432jklfdsab CODE_SERVER_ADDRESS=http://localhost:8080 yarn folio --config=config.ts --reporter=list "$@"
Copy link
Contributor

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

}

main "$@"
73 changes: 73 additions & 0 deletions test/config.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
import {
ChromiumEnv,
FirefoxEnv,
WebKitEnv,
test,
setConfig,
PlaywrightOptions,
Config,
globalSetup,
} from "@playwright/test"
import * as crypto from "crypto"
import path from "path"
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
Contributor

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 😄

// so I had to copy hash in separately
const hash = (str: string): string => {
return crypto.createHash("sha256").update(str).digest("hex")
}

const cookieToStore = {
sameSite: "Lax" as const,
name: "key",
value: hash(PASSWORD),
domain: "localhost",
path: "/",
expires: -1,
httpOnly: false,
secure: false,
}

globalSetup(async () => {
console.log("\n🚨 Running globalSetup for playwright end-to-end tests")
console.log("👋 Please hang tight...")

if (process.env.WTF_NODE) {
wtfnode.setup()
}

const storage = {
cookies: [cookieToStore],
}

// Save storage state and store as an env variable
// More info: https://playwright.dev/docs/auth?_highlight=authe#reuse-authentication-state
process.env.STORAGE = JSON.stringify(storage)
console.log("✅ globalSetup is now complete.")
})

const config: Config = {
testDir: path.join(__dirname, "e2e"), // Search for tests in this directory.
timeout: 30000, // Each test is given 30 seconds.
retries: 3, // Retry failing tests 2 times
}

if (process.env.CI) {
// In CI, retry failing tests 2 times
// in the event of flakiness
config.retries = 2
Copy link
Contributor

@jawnsy jawnsy Apr 15, 2021

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.

Copy link
Contributor

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 😅

}

setConfig(config)

const options: PlaywrightOptions = {
headless: true, // Run tests in headless browsers.
video: "retain-on-failure",
}

// Run tests in three browsers.
test.runWith(new ChromiumEnv(options), { tag: "chromium" })
test.runWith(new FirefoxEnv(options), { tag: "firefox" })
test.runWith(new WebKitEnv(options), { tag: "webkit" })
48 changes: 14 additions & 34 deletions test/e2e/browser.test.ts
Original file line number Diff line number Diff line change
@@ -1,35 +1,15 @@
/// <reference types="jest-playwright-preset" />

// This test is for nothing more than to make sure
// tests are running in multiple browsers
describe("Browser gutcheck", () => {
beforeEach(async () => {
await jestPlaywright.resetBrowser({
logger: {
isEnabled: (name) => name === "browser",
log: (name, severity, message, args) => console.log(`${name} ${message}`),
},
})
})

test("should display correct browser based on userAgent", async () => {
const displayNames = {
chromium: "Chrome",
firefox: "Firefox",
webkit: "Safari",
}
const userAgent = await page.evaluate("navigator.userAgent")

if (browserName === "chromium") {
expect(userAgent).toContain(displayNames[browserName])
}

if (browserName === "firefox") {
expect(userAgent).toContain(displayNames[browserName])
}

if (browserName === "webkit") {
expect(userAgent).toContain(displayNames[browserName])
}
})
import { test, expect } from "@playwright/test"
import { CODE_SERVER_ADDRESS } from "../utils/constants"

// This is a "gut-check" test to make sure playwright is working as expected
test("browser should display correct userAgent", async ({ page, browserName }) => {
const displayNames = {
chromium: "Chrome",
firefox: "Firefox",
webkit: "Safari",
}
await page.goto(CODE_SERVER_ADDRESS, { waitUntil: "networkidle" })
const userAgent = await page.evaluate("navigator.userAgent")

expect(userAgent).toContain(displayNames[browserName])
})
24 changes: 14 additions & 10 deletions test/e2e/globalSetup.test.ts
Original file line number Diff line number Diff line change
@@ -1,20 +1,24 @@
/// <reference types="jest-playwright-preset" />
import { test, expect } from "@playwright/test"
import { CODE_SERVER_ADDRESS, STORAGE } from "../utils/constants"

// This test is to make sure the globalSetup works as expected
// meaning globalSetup ran and stored the storageState in STORAGE
describe("globalSetup", () => {
beforeEach(async () => {
// Create a new context with the saved storage state
// so we don't have to logged in
test.describe("globalSetup", () => {
// Create a new context with the saved storage state
// so we don't have to logged in
const options: any = {}

// TODO@jsjoeio
// Fix this once https://github.com/microsoft/playwright-test/issues/240
// is fixed
if (STORAGE) {
const storageState = JSON.parse(STORAGE) || {}
await jestPlaywright.resetContext({
options.contextOptions = {
storageState,
})
}
}
test("should keep us logged in using the storageState", options, async ({ page }) => {
await page.goto(CODE_SERVER_ADDRESS, { waitUntil: "networkidle" })
})

it("should keep us logged in using the storageState", async () => {
// Make sure the editor actually loaded
expect(await page.isVisible("div.monaco-workbench"))
})
Expand Down
18 changes: 11 additions & 7 deletions test/e2e/login.test.ts
Original file line number Diff line number Diff line change
@@ -1,13 +1,17 @@
/// <reference types="jest-playwright-preset" />
import { test, expect } from "@playwright/test"
import { CODE_SERVER_ADDRESS, PASSWORD } from "../utils/constants"

describe("login", () => {
beforeEach(async () => {
await jestPlaywright.resetBrowser()
await page.goto(CODE_SERVER_ADDRESS, { waitUntil: "networkidle" })
})
test.describe("login", () => {
// Reset the browser so no cookies are persisted
// by emptying the storageState
const options = {
contextOptions: {
storageState: {},
},
}

it("should be able to login", async () => {
test("should be able to login", options, async ({ page }) => {
await page.goto(CODE_SERVER_ADDRESS, { waitUntil: "networkidle" })
// Type in password
await page.fill(".password", PASSWORD)
// Click the submit button and login
Expand Down
24 changes: 11 additions & 13 deletions test/e2e/loginPage.test.ts
Original file line number Diff line number Diff line change
@@ -1,19 +1,17 @@
/// <reference types="jest-playwright-preset" />

import { test, expect } from "@playwright/test"
import { CODE_SERVER_ADDRESS } from "../utils/constants"

describe("login page", () => {
beforeEach(async () => {
await jestPlaywright.resetContext({
logger: {
isEnabled: (name, severity) => name === "browser",
log: (name, severity, message, args) => console.log(`${name} ${message}`),
},
})
await page.goto(CODE_SERVER_ADDRESS, { waitUntil: "networkidle" })
})
test.describe("login page", () => {
// Reset the browser so no cookies are persisted
// by emptying the storageState
const options = {
contextOptions: {
storageState: {},
},
}

it("should see the login page", async () => {
test("should see the login page", options, async ({ page }) => {
await page.goto(CODE_SERVER_ADDRESS, { waitUntil: "networkidle" })
// It should send us to the login page
expect(await page.title()).toBe("code-server login")
})
Expand Down
17 changes: 10 additions & 7 deletions test/e2e/logout.test.ts
Original file line number Diff line number Diff line change
@@ -1,13 +1,16 @@
/// <reference types="jest-playwright-preset" />
import { test, expect } from "@playwright/test"
import { CODE_SERVER_ADDRESS, PASSWORD } from "../utils/constants"

describe("logout", () => {
beforeEach(async () => {
await jestPlaywright.resetBrowser()
test.describe("logout", () => {
// Reset the browser so no cookies are persisted
// by emptying the storageState
const options = {
contextOptions: {
storageState: {},
},
}
test("should be able login and logout", options, async ({ page }) => {
await page.goto(CODE_SERVER_ADDRESS, { waitUntil: "networkidle" })
})

it("should be able login and logout", async () => {
// Type in password
await page.fill(".password", PASSWORD)
// Click the submit button and login
Expand Down
64 changes: 36 additions & 28 deletions test/e2e/openHelpAbout.test.ts
Original file line number Diff line number Diff line change
@@ -1,38 +1,46 @@
/// <reference types="jest-playwright-preset" />
import { test, expect } from "@playwright/test"
import { CODE_SERVER_ADDRESS, STORAGE } from "../utils/constants"

describe("Open Help > About", () => {
beforeEach(async () => {
// Create a new context with the saved storage state
// so we don't have to logged in
test.describe("Open Help > About", () => {
// Create a new context with the saved storage state
// so we don't have to logged in
const options: any = {}
// TODO@jsjoeio
// Fix this once https://github.com/microsoft/playwright-test/issues/240
// is fixed
if (STORAGE) {
const storageState = JSON.parse(STORAGE) || {}
await jestPlaywright.resetContext({
options.contextOptions = {
storageState,
})
await page.goto(CODE_SERVER_ADDRESS, { waitUntil: "networkidle" })
})
}
}

it("should see a 'Help' then 'About' button in the Application Menu that opens a dialog", async () => {
// Make sure the editor actually loaded
expect(await page.isVisible("div.monaco-workbench"))
test(
"should see a 'Help' then 'About' button in the Application Menu that opens a dialog",
options,
async ({ page }) => {
await page.goto(CODE_SERVER_ADDRESS, { waitUntil: "networkidle" })
// Make sure the editor actually loaded
expect(await page.isVisible("div.monaco-workbench"))

// Click the Application menu
await page.click("[aria-label='Application Menu']")
// See the Help button
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
Contributor

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.

// See the Help button
const helpButton = "a.action-menu-item span[aria-label='Help']"
expect(await page.isVisible(helpButton))

// Hover the helpButton
await page.hover(helpButton)
// Hover the helpButton
await page.hover(helpButton)

// see the About button and click it
const aboutButton = "a.action-menu-item span[aria-label='About']"
expect(await page.isVisible(aboutButton))
// NOTE: it won't work unless you hover it first
await page.hover(aboutButton)
await page.click(aboutButton)
// see the About button and click it
const aboutButton = "a.action-menu-item span[aria-label='About']"
expect(await page.isVisible(aboutButton))
// NOTE: it won't work unless you hover it first
await page.hover(aboutButton)
await page.click(aboutButton)

const codeServerText = "text=code-server"
expect(await page.isVisible(codeServerText))
})
const codeServerText = "text=code-server"
expect(await page.isVisible(codeServerText))
},
)
})
Loading