From da4de439e0d4fd2bf5bc6fd843691987e741871c Mon Sep 17 00:00:00 2001 From: Asher Date: Tue, 22 Jun 2021 16:34:44 -0500 Subject: [PATCH] Spawn a code-server instance for each test suite This uses the current dev build by default but can be overidden with CODE_SERVER_TEST_ENTRY (for example to test a release or some other version). Each instance has a separate state directory. This should make parallelization work. This also means you are no longer required to specify the password and address yourself (or the extension directory once we add a test extension). `yarn test:e2e` should just work as-is. Lastly, it means the tests are no longer subject to yarn watch randomly restarting. --- .github/workflows/ci.yaml | 15 +-- ci/dev/test-e2e.sh | 31 +++++- test/e2e/baseFixture.ts | 47 +++++++-- test/e2e/browser.test.ts | 4 +- test/e2e/codeServer.test.ts | 10 +- test/e2e/globalSetup.test.ts | 4 +- test/e2e/login.test.ts | 4 +- test/e2e/logout.test.ts | 8 +- test/e2e/models/CodeServer.ts | 186 +++++++++++++++++++++++++++------ test/e2e/openHelpAbout.test.ts | 4 +- test/e2e/terminal.test.ts | 4 +- test/playwright.config.ts | 3 +- test/utils/constants.ts | 3 +- test/utils/helpers.ts | 2 +- 14 files changed, 253 insertions(+), 72 deletions(-) diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index 8fcda93a4..f433f6413 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -334,8 +334,9 @@ jobs: needs: package-linux-amd64 runs-on: ubuntu-latest env: - PASSWORD: e45432jklfdsab - CODE_SERVER_ADDRESS: http://localhost:8080 + # Since we build code-server we might as well run tests from the release + # since VS Code will load faster due to the bundling. + CODE_SERVER_TEST_ENTRY: "./release-packages/code-server-linux-amd64" steps: - uses: actions/checkout@v2 @@ -362,9 +363,11 @@ jobs: name: release-packages path: ./release-packages - - name: Untar code-server file + - name: Untar code-server release run: | - cd release-packages && tar -xzf code-server*-linux-amd64.tar.gz + cd release-packages + tar -xzf code-server*-linux-amd64.tar.gz + mv code-server*-linux-amd64 code-server-linux-amd64 - name: Install dependencies if: steps.cache-yarn.outputs.cache-hit != 'true' @@ -380,9 +383,7 @@ jobs: yarn install --check-files - name: Run end-to-end tests - run: | - ./release-packages/code-server*-linux-amd64/bin/code-server --log trace & - yarn test:e2e + run: yarn test:e2e - name: Upload test artifacts if: always() diff --git a/ci/dev/test-e2e.sh b/ci/dev/test-e2e.sh index 1e6dcfa59..47c01cda6 100755 --- a/ci/dev/test-e2e.sh +++ b/ci/dev/test-e2e.sh @@ -3,10 +3,35 @@ set -euo pipefail main() { cd "$(dirname "$0")/../.." + source ./ci/lib.sh + + local dir="$PWD" + if [[ ! ${CODE_SERVER_TEST_ENTRY-} ]]; then + echo "Set CODE_SERVER_TEST_ENTRY to test another build of code-server" + else + pushd "$CODE_SERVER_TEST_ENTRY" + dir="$PWD" + popd + fi + + echo "Testing build in '$dir'" + + # Simple sanity checks to see that we've built. There could still be things + # wrong (native modules version issues, incomplete build, etc). + if [[ ! -d $dir/out ]]; then + echo >&2 "No code-server build detected" + echo >&2 "You can build it with 'yarn build' or 'yarn watch'" + exit 1 + fi + + if [[ ! -d $dir/lib/vscode/out ]]; then + echo >&2 "No VS Code build detected" + echo >&2 "You can build it with 'yarn build:vscode' or 'yarn watch'" + exit 1 + fi + 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 playwright test "$@" + yarn playwright test "$@" } main "$@" diff --git a/test/e2e/baseFixture.ts b/test/e2e/baseFixture.ts index a5fc15511..b4105c227 100644 --- a/test/e2e/baseFixture.ts +++ b/test/e2e/baseFixture.ts @@ -1,12 +1,47 @@ +import { field, logger } from "@coder/logger" import { test as base } from "@playwright/test" -import { CodeServer } from "./models/CodeServer" +import { CodeServer, CodeServerPage } from "./models/CodeServer" -export const test = base.extend<{ codeServerPage: CodeServer }>({ - codeServerPage: async ({ page }, use) => { - const codeServer = new CodeServer(page) - await codeServer.navigate() - await use(codeServer) +/** + * Wraps `test.describe` to create and manage an instance of code-server. If you + * don't use this you will need to create your own code-server instance and pass + * it to `test.use`. + */ +export const describe = (name: string, fn: (codeServer: CodeServer) => void) => { + test.describe(name, () => { + // This will spawn on demand so nothing is necessary on before. + const codeServer = new CodeServer(name) + + // Kill code-server after the suite has ended. This may happen even without + // doing it explicitly but it seems prudent to be sure. + test.afterAll(async () => { + await codeServer.close() + }) + + // This makes `codeServer` available to the extend call below. + test.use({ codeServer }) + + fn(codeServer) + }) +} + +interface TestFixtures { + codeServer: CodeServer + codeServerPage: CodeServerPage +} + +/** + * Create a test that spawns code-server if necessary and ensures the page is + * ready. + */ +export const test = base.extend({ + codeServer: undefined, // No default; should be provided through `test.use`. + codeServerPage: async ({ codeServer, page }, use) => { + const codeServerPage = new CodeServerPage(codeServer, page) + await codeServerPage.navigate() + await use(codeServerPage) }, }) +/** Shorthand for test.expect. */ export const expect = test.expect diff --git a/test/e2e/browser.test.ts b/test/e2e/browser.test.ts index 9e3e01545..078b4e309 100644 --- a/test/e2e/browser.test.ts +++ b/test/e2e/browser.test.ts @@ -1,7 +1,7 @@ -import { test, expect } from "./baseFixture" +import { describe, test, expect } from "./baseFixture" // This is a "gut-check" test to make sure playwright is working as expected -test.describe("browser", () => { +describe("browser", () => { test("browser should display correct userAgent", async ({ codeServerPage, browserName }) => { const displayNames = { chromium: "Chrome", diff --git a/test/e2e/codeServer.test.ts b/test/e2e/codeServer.test.ts index f2a95121a..a40e30d89 100644 --- a/test/e2e/codeServer.test.ts +++ b/test/e2e/codeServer.test.ts @@ -1,12 +1,12 @@ -import { CODE_SERVER_ADDRESS, storageState } from "../utils/constants" -import { test, expect } from "./baseFixture" +import { storageState } from "../utils/constants" +import { describe, test, expect } from "./baseFixture" -test.describe("CodeServer", () => { +describe("CodeServer", () => { test.use({ storageState, }) - test(`should navigate to ${CODE_SERVER_ADDRESS}`, async ({ codeServerPage }) => { + test("should navigate to home page", async ({ codeServerPage }) => { // We navigate codeServer before each test // and we start the test with a storage state // which means we should be logged in @@ -14,7 +14,7 @@ test.describe("CodeServer", () => { const url = codeServerPage.page.url() // We use match because there may be a / at the end // so we don't want it to fail if we expect http://localhost:8080 to match http://localhost:8080/ - expect(url).toMatch(CODE_SERVER_ADDRESS) + expect(url).toMatch(await codeServerPage.address()) }) test("should always see the code-server editor", async ({ codeServerPage }) => { diff --git a/test/e2e/globalSetup.test.ts b/test/e2e/globalSetup.test.ts index e1c54e139..ed3506ea9 100644 --- a/test/e2e/globalSetup.test.ts +++ b/test/e2e/globalSetup.test.ts @@ -1,9 +1,9 @@ import { storageState } from "../utils/constants" -import { test, expect } from "./baseFixture" +import { describe, test, expect } from "./baseFixture" // This test is to make sure the globalSetup works as expected // meaning globalSetup ran and stored the storageState -test.describe("globalSetup", () => { +describe("globalSetup", () => { test.use({ storageState, }) diff --git a/test/e2e/login.test.ts b/test/e2e/login.test.ts index 7975bfcac..b2289740a 100644 --- a/test/e2e/login.test.ts +++ b/test/e2e/login.test.ts @@ -1,7 +1,7 @@ import { PASSWORD } from "../utils/constants" -import { test, expect } from "./baseFixture" +import { describe, test, expect } from "./baseFixture" -test.describe("login", () => { +describe("login", () => { // Reset the browser so no cookies are persisted // by emptying the storageState test.use({ diff --git a/test/e2e/logout.test.ts b/test/e2e/logout.test.ts index bec0f323a..ec480c4f1 100644 --- a/test/e2e/logout.test.ts +++ b/test/e2e/logout.test.ts @@ -1,7 +1,7 @@ -import { CODE_SERVER_ADDRESS, PASSWORD } from "../utils/constants" -import { test, expect } from "./baseFixture" +import { PASSWORD } from "../utils/constants" +import { describe, test, expect } from "./baseFixture" -test.describe("logout", () => { +describe("logout", () => { // Reset the browser so no cookies are persisted // by emptying the storageState test.use({ @@ -39,6 +39,6 @@ test.describe("logout", () => { // https://github.com/microsoft/playwright/issues/1987#issuecomment-620182151 await Promise.all([codeServerPage.page.waitForNavigation(), codeServerPage.page.click(logoutButton)]) const currentUrl = codeServerPage.page.url() - expect(currentUrl).toBe(`${CODE_SERVER_ADDRESS}/login`) + expect(currentUrl).toBe(`${await codeServerPage.address()}/login`) }) }) diff --git a/test/e2e/models/CodeServer.ts b/test/e2e/models/CodeServer.ts index 662c82d40..b3e2bdafa 100644 --- a/test/e2e/models/CodeServer.ts +++ b/test/e2e/models/CodeServer.ts @@ -1,24 +1,163 @@ +import { Logger, logger } from "@coder/logger" +import * as cp from "child_process" import { promises as fs } from "fs" import * as path from "path" import { Page } from "playwright" -import { CODE_SERVER_ADDRESS, workspaceDir } from "../../utils/constants" +import { onLine } from "../../../src/node/util" +import { PASSWORD, workspaceDir } from "../../utils/constants" import { tmpdir } from "../../utils/helpers" -// This is a Page Object Model -// We use these to simplify e2e test authoring -// See Playwright docs: https://playwright.dev/docs/pom/ -export class CodeServer { - private readonly editorSelector = "div.monaco-workbench" +interface CodeServerProcess { + process: cp.ChildProcess + address: string +} - constructor(public readonly page: Page) {} +/** + * Class for spawning and managing code-server. + */ +export class CodeServer { + private process: Promise | undefined + private readonly logger: Logger + private closed = false + + constructor(name: string) { + this.logger = logger.named(name) + } /** - * Navigates to CODE_SERVER_ADDRESS. It will open a newly created random - * directory. + * The address at which code-server can be accessed. Spawns code-server if it + * has not yet been spawned. + */ + async address(): Promise { + if (!this.process) { + this.process = this.spawn() + } + const { address } = await this.process + return address + } + + /** + * Create a random workspace and seed it with settings. + */ + private async createWorkspace(): Promise { + const dir = await tmpdir(workspaceDir) + await fs.mkdir(path.join(dir, ".vscode")) + await fs.writeFile( + path.join(dir, ".vscode/settings.json"), + JSON.stringify({ + "workbench.startupEditor": "none", + }), + "utf8", + ) + return dir + } + + /** + * Spawn a new code-server process with its own workspace and data + * directories. + */ + private async spawn(): Promise { + // This will be used both as the workspace and data directory to ensure + // instances don't bleed into each other. + const dir = await this.createWorkspace() + + return new Promise((resolve, reject) => { + this.logger.debug("spawning") + const proc = cp.spawn( + "node", + [ + process.env.CODE_SERVER_TEST_ENTRY || ".", + // Using port zero will spawn on a random port. + "--bind-addr", + "127.0.0.1:0", + // Setting the XDG variables would be easier and more thorough but the + // modules we import ignores those variables for non-Linux operating + // systems so use these flags instead. + "--config", + path.join(dir, "config.yaml"), + "--user-data-dir", + dir, + // The last argument is the workspace to open. + dir, + ], + { + cwd: path.join(__dirname, "../../.."), + env: { + ...process.env, + PASSWORD, + }, + }, + ) + + proc.on("error", (error) => { + this.logger.error(error.message) + reject(error) + }) + + proc.on("close", () => { + const error = new Error("closed unexpectedly") + if (!this.closed) { + this.logger.error(error.message) + } + reject(error) + }) + + let resolved = false + proc.stdout.setEncoding("utf8") + onLine(proc, (line) => { + // Log the line without the timestamp. + this.logger.trace(line.replace(/\[.+\]/, "")) + if (resolved) { + return + } + const match = line.trim().match(/HTTP server listening on (https?:\/\/[.:\d]+)$/) + if (match) { + // Cookies don't seem to work on IP address so swap to localhost. + // TODO: Investigate whether this is a bug with code-server. + const address = match[1].replace("127.0.0.1", "localhost") + this.logger.debug(`spawned on ${address}`) + resolved = true + resolve({ process: proc, address }) + } + }) + }) + } + + /** + * Close the code-server process. + */ + async close(): Promise { + logger.debug("closing") + if (this.process) { + const proc = (await this.process).process + this.closed = true // To prevent the close handler from erroring. + proc.kill() + } + } +} + +/** + * This is a "Page Object Model" (https://playwright.dev/docs/pom/) meant to + * wrap over a page and represent actions on that page in a more readable way. + * This targets a specific code-server instance which must be passed in. + * Navigation and setup performed by this model will cause the code-server + * process to spawn if it hasn't yet. + */ +export class CodeServerPage { + private readonly editorSelector = "div.monaco-workbench" + + constructor(private readonly codeServer: CodeServer, public readonly page: Page) {} + + address() { + return this.codeServer.address() + } + + /** + * Navigate to code-server. */ async navigate() { - const dir = await this.createWorkspace() - await this.page.goto(`${CODE_SERVER_ADDRESS}?folder=${dir}`, { waitUntil: "networkidle" }) + const address = await this.codeServer.address() + await this.page.goto(address, { waitUntil: "networkidle" }) } /** @@ -45,7 +184,7 @@ export class CodeServer { await this.page.waitForTimeout(1000) reloadCount += 1 if ((await this.isEditorVisible()) && (await this.isConnected())) { - console.log(` Editor became ready after ${reloadCount} reloads`) + logger.debug(`editor became ready after ${reloadCount} reloads`) break } await this.page.reload() @@ -67,7 +206,7 @@ export class CodeServer { async isConnected() { await this.page.waitForLoadState("networkidle") - const host = new URL(CODE_SERVER_ADDRESS).host + const host = new URL(await this.codeServer.address()).host const hostSelector = `[title="Editing on ${host}"]` await this.page.waitForSelector(hostSelector) @@ -107,29 +246,12 @@ export class CodeServer { } /** - * Navigates to CODE_SERVER_ADDRESS - * and reloads until the editor is ready + * Navigates to code-server then reloads until the editor is ready. * - * Helpful for running before tests + * It is recommended to run setup before using this model in any tests. */ async setup() { await this.navigate() await this.reloadUntilEditorIsReady() } - - /** - * Create a random workspace and seed it with settings. - */ - private async createWorkspace(): Promise { - const dir = await tmpdir(workspaceDir) - await fs.mkdir(path.join(dir, ".vscode")) - await fs.writeFile( - path.join(dir, ".vscode/settings.json"), - JSON.stringify({ - "workbench.startupEditor": "none", - }), - "utf8", - ) - return dir - } } diff --git a/test/e2e/openHelpAbout.test.ts b/test/e2e/openHelpAbout.test.ts index 5c48346ed..1cb030540 100644 --- a/test/e2e/openHelpAbout.test.ts +++ b/test/e2e/openHelpAbout.test.ts @@ -1,7 +1,7 @@ import { storageState } from "../utils/constants" -import { test, expect } from "./baseFixture" +import { describe, test, expect } from "./baseFixture" -test.describe("Open Help > About", () => { +describe("Open Help > About", () => { test.use({ storageState, }) diff --git a/test/e2e/terminal.test.ts b/test/e2e/terminal.test.ts index e682f464e..b2b284c74 100644 --- a/test/e2e/terminal.test.ts +++ b/test/e2e/terminal.test.ts @@ -4,9 +4,9 @@ import * as path from "path" import util from "util" import { storageState } from "../utils/constants" import { tmpdir } from "../utils/helpers" -import { expect, test } from "./baseFixture" +import { describe, expect, test } from "./baseFixture" -test.describe("Integrated Terminal", () => { +describe("Integrated Terminal", () => { // Create a new context with the saved storage state // so we don't have to logged in const testFileName = "pipe" diff --git a/test/playwright.config.ts b/test/playwright.config.ts index cb8eae186..9063a04b9 100644 --- a/test/playwright.config.ts +++ b/test/playwright.config.ts @@ -6,8 +6,7 @@ import path from "path" const config: PlaywrightTestConfig = { testDir: path.join(__dirname, "e2e"), // Search for tests in this directory. timeout: 60000, // Each test is given 60 seconds. - retries: process.env.CI ? 2 : 1, // Retry twice in CI due to flakiness. - workers: 1, + retries: process.env.CI ? 2 : 1, // Retry in CI due to flakiness. globalSetup: require.resolve("./utils/globalSetup.ts"), reporter: "list", // Put any shared options on the top level. diff --git a/test/utils/constants.ts b/test/utils/constants.ts index e656145e4..08acb9788 100644 --- a/test/utils/constants.ts +++ b/test/utils/constants.ts @@ -1,4 +1,3 @@ -export const CODE_SERVER_ADDRESS = process.env.CODE_SERVER_ADDRESS || "http://localhost:8080" -export const PASSWORD = process.env.PASSWORD || "e45432jklfdsab" +export const PASSWORD = "e45432jklfdsab" export const storageState = JSON.parse(process.env.STORAGE || "{}") export const workspaceDir = "workspaces" diff --git a/test/utils/helpers.ts b/test/utils/helpers.ts index f299fc130..a6a8d4513 100644 --- a/test/utils/helpers.ts +++ b/test/utils/helpers.ts @@ -25,7 +25,7 @@ export function createLoggerMock() { */ export async function clean(testName: string): Promise { const dir = path.join(os.tmpdir(), `code-server/tests/${testName}`) - await fs.rm(dir, { recursive: true }) + await fs.rmdir(dir, { recursive: true }) } /**