Skip to content

test: improve E2E framework #9469

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

Merged
merged 11 commits into from
Sep 4, 2023
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
8 changes: 8 additions & 0 deletions .github/workflows/ci.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -567,6 +567,14 @@ jobs:
path: ./site/test-results/**/*.webm
retention-days: 7

- name: Upload pprof dumps
if: always() && github.actor != 'dependabot[bot]' && runner.os == 'Linux' && !github.event.pull_request.head.repo.fork
uses: actions/upload-artifact@v3
with:
name: debug-pprof-dumps
path: ./site/test-results/**/debug-pprof-*.txt
retention-days: 7

chromatic:
# REMARK: this is only used to build storybook and deploy it to Chromatic.
runs-on: ubuntu-latest
Expand Down
2 changes: 2 additions & 0 deletions site/e2e/constants.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
// Default port from the server
export const defaultPort = 3000
export const prometheusPort = 2114
export const pprofPort = 6061

// Credentials for the first user
export const username = "admin"
Expand Down
2 changes: 1 addition & 1 deletion site/e2e/global.setup.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import { STORAGE_STATE } from "./playwright.config"
import { Language } from "../src/components/CreateUserForm/CreateUserForm"

test("create first user", async ({ page }) => {
await page.goto("/", { waitUntil: "networkidle" })
await page.goto("/", { waitUntil: "domcontentloaded" })
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we should necessarily change networkidle to domcontentloaded without adding additional wait checks. For instance "wait until element X is present in the html". In a sense, networkidle should be better as it will wait longer compared to domcontentloaded which will fire immediately after dom even if network requests are still outbound.

Copy link
Member Author

Choose a reason for hiding this comment

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

In previous PRs, I added extra "wait until" conditions, so in theory it should be faster and still safe with domcontentloaded.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, that's perfect 👍🏻


await page.getByLabel(Language.usernameLabel).fill(constants.username)
await page.getByLabel(Language.emailLabel).fill(constants.email)
Expand Down
54 changes: 46 additions & 8 deletions site/e2e/helpers.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { expect, Page } from "@playwright/test"
import { spawn } from "child_process"
import { ChildProcess, exec, spawn } from "child_process"
import { randomUUID } from "crypto"
import path from "path"
import express from "express"
Expand All @@ -15,10 +15,12 @@ import {
Resource,
RichParameter,
} from "./provisionerGenerated"
import { prometheusPort, pprofPort } from "./constants"
import { port } from "./playwright.config"
import * as ssh from "ssh2"
import { Duplex } from "stream"
import { WorkspaceBuildParameter } from "api/typesGenerated"
import axios from "axios"

// createWorkspace creates a workspace for a template.
// It does not wait for it to be running, but it does navigate to the page.
Expand All @@ -29,7 +31,7 @@ export const createWorkspace = async (
buildParameters: WorkspaceBuildParameter[] = [],
): Promise<string> => {
await page.goto("/templates/" + templateName + "/workspace", {
waitUntil: "networkidle",
waitUntil: "domcontentloaded",
})
await expect(page).toHaveURL("/templates/" + templateName + "/workspace")

Expand Down Expand Up @@ -57,7 +59,7 @@ export const verifyParameters = async (
expectedBuildParameters: WorkspaceBuildParameter[],
) => {
await page.goto("/@admin/" + workspaceName + "/settings/parameters", {
waitUntil: "networkidle",
waitUntil: "domcontentloaded",
})
await expect(page).toHaveURL(
"/@admin/" + workspaceName + "/settings/parameters",
Expand Down Expand Up @@ -120,7 +122,7 @@ export const createTemplate = async (
content: "window.playwright = true",
})

await page.goto("/templates/new", { waitUntil: "networkidle" })
await page.goto("/templates/new", { waitUntil: "domcontentloaded" })
await expect(page).toHaveURL("/templates/new")

await page.getByTestId("file-upload").setInputFiles({
Expand Down Expand Up @@ -229,7 +231,10 @@ export const buildWorkspaceWithParameters = async (

// startAgent runs the coder agent with the provided token.
// It awaits the agent to be ready before returning.
export const startAgent = async (page: Page, token: string): Promise<void> => {
export const startAgent = async (
page: Page,
token: string,
): Promise<ChildProcess> => {
return startAgentWithCommand(page, token, "go", "run", coderMainPath())
}

Expand Down Expand Up @@ -308,14 +313,14 @@ export const startAgentWithCommand = async (
token: string,
command: string,
...args: string[]
): Promise<void> => {
): Promise<ChildProcess> => {
const cp = spawn(command, [...args, "agent", "--no-reap"], {
env: {
...process.env,
CODER_AGENT_URL: "http://localhost:" + port,
CODER_AGENT_TOKEN: token,
CODER_AGENT_PPROF_ADDRESS: "127.0.0.1:2114",
CODER_AGENT_PROMETHEUS_ADDRESS: "127.0.0.1:6061",
CODER_AGENT_PPROF_ADDRESS: "127.0.0.1:" + pprofPort,
CODER_AGENT_PROMETHEUS_ADDRESS: "127.0.0.1:" + prometheusPort,
},
})
cp.stdout.on("data", (data: Buffer) => {
Expand All @@ -332,6 +337,39 @@ export const startAgentWithCommand = async (
})

await page.getByTestId("agent-status-ready").waitFor({ state: "visible" })
return cp
}

export const stopAgent = async (cp: ChildProcess, goRun: boolean = true) => {
// When the web server is started with `go run`, it spawns a child process with coder server.
// `pkill -P` terminates child processes belonging the same group as `go run`.
// The command `kill` is used to terminate a web server started as a standalone binary.
exec(goRun ? `pkill -P ${cp.pid}` : `kill ${cp.pid}`, (error) => {
if (error) {
throw new Error(`exec error: ${JSON.stringify(error)}`)
}
})
await waitUntilUrlIsNotResponding("http://localhost:" + prometheusPort)
}

const waitUntilUrlIsNotResponding = async (url: string) => {
const maxRetries = 30
const retryIntervalMs = 1000
let retries = 0

while (retries < maxRetries) {
try {
await axios.get(url)
} catch (error) {
return
}

retries++
await new Promise((resolve) => setTimeout(resolve, retryIntervalMs))
}
throw new Error(
`URL ${url} is still responding after ${maxRetries * retryIntervalMs}ms`,
)
}

const coderMainPath = (): string => {
Expand Down
3 changes: 2 additions & 1 deletion site/e2e/playwright.config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,8 @@ export default defineConfig({
`--dangerous-disable-rate-limits ` +
`--provisioner-daemons 10 ` +
`--provisioner-daemons-echo ` +
`--provisioner-daemon-poll-interval 50ms`,
`--provisioner-daemon-poll-interval 50ms ` +
`--pprof-enable`,
env: {
...process.env,

Expand Down
31 changes: 30 additions & 1 deletion site/e2e/reporter.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import fs from "fs"
import type {
FullConfig,
Suite,
Expand All @@ -6,6 +7,7 @@ import type {
FullResult,
Reporter,
} from "@playwright/test/reporter"
import axios from "axios"

class CoderReporter implements Reporter {
onBegin(config: FullConfig, suite: Suite) {
Expand Down Expand Up @@ -38,13 +40,15 @@ class CoderReporter implements Reporter {
)
}

onTestEnd(test: TestCase, result: TestResult) {
async onTestEnd(test: TestCase, result: TestResult) {
// eslint-disable-next-line no-console -- Helpful for debugging
console.log(`Finished test ${test.title}: ${result.status}`)

if (result.status !== "passed") {
// eslint-disable-next-line no-console -- Helpful for debugging
console.log("errors", result.errors, "attachments", result.attachments)
}
await exportDebugPprof(test.title)
}

onEnd(result: FullResult) {
Expand All @@ -53,5 +57,30 @@ class CoderReporter implements Reporter {
}
}

const exportDebugPprof = async (testName: string) => {
const url = "http://127.0.0.1:6060/debug/pprof/goroutine?debug=1"
const outputFile = `test-results/debug-pprof-goroutine-${testName}.txt`

await axios
.get(url)
.then((response) => {
if (response.status !== 200) {
throw new Error(`Error: Received status code ${response.status}`)
}

fs.writeFile(outputFile, response.data, (err) => {
if (err) {
throw new Error(`Error writing to ${outputFile}: ${err.message}`)
} else {
// eslint-disable-next-line no-console -- Helpful for debugging
console.log(`Data from ${url} has been saved to ${outputFile}`)
}
})
})
.catch((error) => {
throw new Error(`Error: ${error.message}`)
})
}

// eslint-disable-next-line no-unused-vars -- Playwright config uses it
export default CoderReporter
17 changes: 13 additions & 4 deletions site/e2e/tests/app.spec.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,13 @@
import { test } from "@playwright/test"
import { randomUUID } from "crypto"
import * as http from "http"
import { createTemplate, createWorkspace, startAgent } from "../helpers"
import {
createTemplate,
createWorkspace,
startAgent,
stopAgent,
stopWorkspace,
} from "../helpers"
import { beforeCoderTest } from "../hooks"

test.beforeEach(async ({ page }) => await beforeCoderTest(page))
Expand Down Expand Up @@ -43,13 +49,16 @@ test("app", async ({ context, page }) => {
},
],
})
await createWorkspace(page, template)
await startAgent(page, token)
const workspaceName = await createWorkspace(page, template)
const agent = await startAgent(page, token)

// Wait for the web terminal to open in a new tab
const pagePromise = context.waitForEvent("page")
await page.getByText(appName).click()
const app = await pagePromise
await app.waitForLoadState("networkidle")
await app.waitForLoadState("domcontentloaded")
await app.getByText(appContent).isVisible()

await stopWorkspace(page, workspaceName)
await stopAgent(agent)
})
4 changes: 2 additions & 2 deletions site/e2e/tests/gitAuth.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ test("git auth device", async ({ page }) => {
})

await page.goto(`/gitauth/${gitAuth.deviceProvider}`, {
waitUntil: "networkidle",
waitUntil: "domcontentloaded",
})
await page.getByText(device.user_code).isVisible()
await sentPending.wait()
Expand Down Expand Up @@ -75,7 +75,7 @@ test("git auth web", async ({ baseURL, page }) => {
)
})
await page.goto(`/gitauth/${gitAuth.webProvider}`, {
waitUntil: "networkidle",
waitUntil: "domcontentloaded",
})
// This endpoint doesn't have the installations URL set intentionally!
await page.waitForSelector("text=You've authenticated with GitHub!")
Expand Down
5 changes: 4 additions & 1 deletion site/e2e/tests/listTemplates.spec.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
import { test, expect } from "@playwright/test"
import { beforeCoderTest } from "../hooks"

test.beforeEach(async ({ page }) => await beforeCoderTest(page))

test("list templates", async ({ page, baseURL }) => {
await page.goto(`${baseURL}/templates`, { waitUntil: "networkidle" })
await page.goto(`${baseURL}/templates`, { waitUntil: "domcontentloaded" })
await expect(page).toHaveTitle("Templates - Coder")
})
11 changes: 8 additions & 3 deletions site/e2e/tests/outdatedAgent.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@ import {
downloadCoderVersion,
sshIntoWorkspace,
startAgentWithCommand,
stopAgent,
stopWorkspace,
} from "../helpers"
import { beforeCoderTest } from "../hooks"

Expand All @@ -32,11 +34,11 @@ test("ssh with agent " + agentVersion, async ({ page }) => {
},
],
})
const workspace = await createWorkspace(page, template)
const workspaceName = await createWorkspace(page, template)
const binaryPath = await downloadCoderVersion(agentVersion)
await startAgentWithCommand(page, token, binaryPath)
const agent = await startAgentWithCommand(page, token, binaryPath)

const client = await sshIntoWorkspace(page, workspace)
const client = await sshIntoWorkspace(page, workspaceName)
await new Promise<void>((resolve, reject) => {
// We just exec a command to be certain the agent is running!
client.exec("exit 0", (err, stream) => {
Expand All @@ -52,4 +54,7 @@ test("ssh with agent " + agentVersion, async ({ page }) => {
})
})
})

await stopWorkspace(page, workspaceName)
await stopAgent(agent, false)
})
11 changes: 8 additions & 3 deletions site/e2e/tests/outdatedCLI.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@ import {
downloadCoderVersion,
sshIntoWorkspace,
startAgent,
stopAgent,
stopWorkspace,
} from "../helpers"
import { beforeCoderTest } from "../hooks"

Expand All @@ -32,11 +34,11 @@ test("ssh with client " + clientVersion, async ({ page }) => {
},
],
})
const workspace = await createWorkspace(page, template)
await startAgent(page, token)
const workspaceName = await createWorkspace(page, template)
const agent = await startAgent(page, token)
const binaryPath = await downloadCoderVersion(clientVersion)

const client = await sshIntoWorkspace(page, workspace, binaryPath)
const client = await sshIntoWorkspace(page, workspaceName, binaryPath)
await new Promise<void>((resolve, reject) => {
// We just exec a command to be certain the agent is running!
client.exec("exit 0", (err, stream) => {
Expand All @@ -52,4 +54,7 @@ test("ssh with client " + clientVersion, async ({ page }) => {
})
})
})

await stopWorkspace(page, workspaceName)
await stopAgent(agent)
})
12 changes: 9 additions & 3 deletions site/e2e/tests/webTerminal.spec.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,10 @@
import { test } from "@playwright/test"
import { createTemplate, createWorkspace, startAgent } from "../helpers"
import {
createTemplate,
createWorkspace,
startAgent,
stopAgent,
} from "../helpers"
import { randomUUID } from "crypto"
import { beforeCoderTest } from "../hooks"

Expand Down Expand Up @@ -28,13 +33,13 @@ test("web terminal", async ({ context, page }) => {
],
})
await createWorkspace(page, template)
await startAgent(page, token)
const agent = await startAgent(page, token)
Copy link
Member

Choose a reason for hiding this comment

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

We still have the unfortunate problem that any error in a test will terminate execution and the agent won't be cleaned up. Can we somehow make it a post-test cleanup hook that always runs and checks if an agent is running?

This isn't a big problem per-se but can cause trouble when running locally -> suddenly all tests fail after one failure. And also cause noise in test output.

Copy link
Member Author

Choose a reason for hiding this comment

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

I was trying to implement an easy way but failed. The clue is to pass the PID of the process/group of process to the test.afterEach. Something like "clean used resources", but not necessarily re-discover them.


// Wait for the web terminal to open in a new tab
const pagePromise = context.waitForEvent("page")
await page.getByTestId("terminal").click()
const terminal = await pagePromise
await terminal.waitForLoadState("networkidle")
await terminal.waitForLoadState("domcontentloaded")

// Ensure that we can type in it
await terminal.keyboard.type("echo hello")
Expand All @@ -50,4 +55,5 @@ test("web terminal", async ({ context, page }) => {
}
await new Promise((r) => setTimeout(r, 250))
}
await stopAgent(agent)
})