- Notifications
You must be signed in to change notification settings - Fork928
refactor: improve e2e test reporting#10304
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Changes fromall commits
54c4b2d1096c2fe7d17dc011dc55335593a91475e4395a019406b6ed1c6ff27File filter
Filter by extension
Conversations
Uh oh!
There was an error while loading.Please reload this page.
Jump to
Uh oh!
There was an error while loading.Please reload this page.
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -91,6 +91,9 @@ rules: | ||
| - allowSingleExtends: true | ||
| "brace-style": "off" | ||
| "curly": ["error", "all"] | ||
| "eslint-comments/disable-enable-pair": | ||
aslilac marked this conversation as resolved. Show resolvedHide resolvedUh oh!There was an error while loading.Please reload this page. | ||
| - error | ||
| - allowWholeFile: true | ||
| "eslint-comments/require-description": "error" | ||
| eqeqeq: error | ||
| import/default: "off" | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,85 +1,146 @@ | ||
| /* eslint-disable no-console -- Logging is sort of the whole point here */ | ||
| import * as fs from "fs/promises"; | ||
| import type { | ||
| FullConfig, | ||
| Suite, | ||
| TestCase, | ||
| TestResult, | ||
| FullResult, | ||
| Reporter, | ||
| TestError, | ||
| } from "@playwright/test/reporter"; | ||
| import axios from "axios"; | ||
| import type { Writable } from "stream"; | ||
| class CoderReporter implements Reporter { | ||
| config: FullConfig | null = null; | ||
| testOutput = new Map<string, Array<[Writable, string]>>(); | ||
| passedCount = 0; | ||
| failedTests: TestCase[] = []; | ||
| timedOutTests: TestCase[] = []; | ||
| onBegin(config: FullConfig, suite: Suite) { | ||
| this.config = config; | ||
| console.log(`==> Running ${suite.allTests().length} tests`); | ||
| } | ||
| onTestBegin(test: TestCase) { | ||
| this.testOutput.set(test.id, []); | ||
| console.log(`==>Starting test ${test.title}`); | ||
| } | ||
| onStdOut(chunk: string, test?: TestCase, _?: TestResult): void { | ||
| if (!test) { | ||
| for (const line of filteredServerLogLines(chunk)) { | ||
| console.log(`[stdout]${line}`); | ||
| } | ||
| return; | ||
| } | ||
| this.testOutput.get(test.id)!.push([process.stdout, chunk]); | ||
aslilac marked this conversation as resolved. Show resolvedHide resolvedUh oh!There was an error while loading.Please reload this page. | ||
| } | ||
| onStdErr(chunk: string, test?: TestCase, _?: TestResult): void { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. We might be losing here output from It looks like the new reporter simply skips these log records. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. that was sort of intentional. it adds a lot of noise when trying to debug a frontend failure, and since it's not tied to a specific test case it's hard to buffer and display later in a way that makes sense. I also added some logic for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. the idea was supposed to be that backend peeps could set There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. example run, which now has 41 occurrences of the word "error" but succeededhttps://github.com/coder/coder/actions/runs/6549912328/job/17787879014?pr=10304 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. A good compromise would be only filtering out I would leave coderd output as it is helpful with debugging the request flow or provisioner behavior, especially while dealing with flaky issues. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. if we can also ignore lines like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. Agree 👍 | ||
| if (!test) { | ||
| for (const line of filteredServerLogLines(chunk)) { | ||
| console.error(`[stderr]${line}`); | ||
| } | ||
| return; | ||
| } | ||
| this.testOutput.get(test.id)!.push([process.stderr, chunk]); | ||
| } | ||
| async onTestEnd(test: TestCase, result: TestResult) { | ||
| console.log(`==> Finished test ${test.title}: ${result.status}`); | ||
| if (result.status === "passed") { | ||
| this.passedCount++; | ||
| } | ||
| if (result.status === "failed") { | ||
| this.failedTests.push(test); | ||
| } | ||
| if (result.status === "timedOut") { | ||
| this.timedOutTests.push(test); | ||
| } | ||
| const fsTestTitle = test.title.replaceAll(" ", "-"); | ||
| const outputFile = `test-results/debug-pprof-goroutine-${fsTestTitle}.txt`; | ||
| await exportDebugPprof(outputFile); | ||
| if (result.status !== "passed") { | ||
| console.log(`Data from pprof has been saved to ${outputFile}`); | ||
| console.log("==> Output"); | ||
| const output = this.testOutput.get(test.id)!; | ||
| for (const [target, chunk] of output) { | ||
| target.write(`${chunk.replace(/\n$/g, "")}\n`); | ||
| } | ||
| if (result.errors.length > 0) { | ||
| console.log("==> Errors"); | ||
| for (const error of result.errors) { | ||
| reportError(error); | ||
| } | ||
| } | ||
| if (result.attachments.length > 0) { | ||
| console.log("==> Attachments"); | ||
| for (const attachment of result.attachments) { | ||
| console.log(attachment); | ||
| } | ||
| } | ||
| } | ||
| this.testOutput.delete(test.id); | ||
| } | ||
| onEnd(result: FullResult) { | ||
| console.log(`==> Tests ${result.status}`); | ||
| console.log(`${this.passedCount} passed`); | ||
| if (this.failedTests.length > 0) { | ||
| console.log(`${this.failedTests.length} failed`); | ||
| for (const test of this.failedTests) { | ||
| console.log(` ${test.location.file} › ${test.title}`); | ||
| } | ||
| } | ||
| if (this.timedOutTests.length > 0) { | ||
| console.log(`${this.timedOutTests.length} timed out`); | ||
| for (const test of this.timedOutTests) { | ||
| console.log(` ${test.location.file} › ${test.title}`); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| const shouldPrintLine = (line: string) => | ||
| [" error=EOF", "coderd: audit_log"].every((noise) => !line.includes(noise)); | ||
| const filteredServerLogLines = (chunk: string): string[] => | ||
| chunk.trimEnd().split("\n").filter(shouldPrintLine); | ||
| const exportDebugPprof = async (outputFile: string) => { | ||
| const response = await axios.get( | ||
| "http://127.0.0.1:6060/debug/pprof/goroutine?debug=1", | ||
| ); | ||
| if (response.status !== 200) { | ||
| throw new Error(`Error: Received status code ${response.status}`); | ||
| } | ||
| await fs.writeFile(outputFile, response.data); | ||
| }; | ||
| const reportError = (error: TestError) => { | ||
| if (error.location) { | ||
| console.log(`${error.location.file}:${error.location.line}:`); | ||
| } | ||
| if (error.snippet) { | ||
| console.log(error.snippet); | ||
| } | ||
| if (error.message) { | ||
| console.log(error.message); | ||
| } else { | ||
| console.log(error); | ||
| } | ||
| }; | ||
| // eslint-disable-next-line no-unused-vars -- Playwright config uses it | ||