From 16a779a5ffc99fe113f371383f0552b80d3bb580 Mon Sep 17 00:00:00 2001 From: Dmitry Gozman Date: Tue, 4 Jan 2022 16:00:55 -0800 Subject: [PATCH] fix(test runner): show codeframe and location from the error top stack frame (#11179) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Previously, reporter would look for a stack frame directly in the test file. Often times, that is not a top stack frame, especially when the test uses some helper functions. This changes error snippets and locations to use the top frame. When top frame does not match the test file, we additionally show the location to avoid confusion: ``` 1) a.spec.ts:7:7 › foobar ======================================================================== Error: oh my at helper.ts:5 3 | 4 | export function ohMy() { > 5 | throw new Error('oh my'); | ^ 6 | } 7 | at ohMy (.../reporter-base-should-print-codeframe-from-a-helper/helper.ts:5:15) at .../reporter-base-should-print-codeframe-from-a-helper/a.spec.ts:8:9 at FixtureRunner.resolveParametersAndRunHookOrTest (.../src/fixtures.ts:281:12) ``` --- .../playwright-test/src/reporters/base.ts | 124 ++++++++---------- .../playwright-test/src/reporters/github.ts | 14 +- .../playwright-test/src/reporters/json.ts | 18 +-- packages/playwright-test/src/reporters/raw.ts | 2 +- tests/playwright-test/blink-diff.spec.ts | 1 - tests/playwright-test/reporter-base.spec.ts | 58 +++++--- 6 files changed, 104 insertions(+), 113 deletions(-) diff --git a/packages/playwright-test/src/reporters/base.ts b/packages/playwright-test/src/reporters/base.ts index 86e8fe023e..676b28e87a 100644 --- a/packages/playwright-test/src/reporters/base.ts +++ b/packages/playwright-test/src/reporters/base.ts @@ -14,35 +14,33 @@ * limitations under the License. */ -import { BabelCodeFrameOptions, codeFrameColumns } from '@babel/code-frame'; +import { codeFrameColumns } from '@babel/code-frame'; import colors from 'colors/safe'; import fs from 'fs'; import milliseconds from 'ms'; import path from 'path'; import StackUtils from 'stack-utils'; -import { FullConfig, TestCase, Suite, TestResult, TestError, Reporter, FullResult, TestStep } from '../../types/testReporter'; +import { FullConfig, TestCase, Suite, TestResult, TestError, Reporter, FullResult, TestStep, Location } from '../../types/testReporter'; const stackUtils = new StackUtils(); export type TestResultOutput = { chunk: string | Buffer, type: 'stdout' | 'stderr' }; export const kOutputSymbol = Symbol('output'); -export type PositionInFile = { column: number; line: number }; type Annotation = { - filePath: string; title: string; message: string; - position?: PositionInFile; + location?: Location; }; type FailureDetails = { tokens: string[]; - position?: PositionInFile; + location?: Location; }; type ErrorDetails = { message: string; - position?: PositionInFile; + location?: Location; }; type TestSummary = { @@ -102,7 +100,7 @@ export class BaseReporter implements Reporter { } onError(error: TestError) { - console.log(formatError(error, colors.enabled).message); + console.log(formatError(this.config, error, colors.enabled).message); } async onEnd(result: FullResult) { @@ -224,11 +222,11 @@ export class BaseReporter implements Reporter { } } -export function formatFailure(config: FullConfig, test: TestCase, options: {index?: number, includeStdio?: boolean, includeAttachments?: boolean, filePath?: string} = {}): { +export function formatFailure(config: FullConfig, test: TestCase, options: {index?: number, includeStdio?: boolean, includeAttachments?: boolean} = {}): { message: string, annotations: Annotation[] } { - const { index, includeStdio, includeAttachments = true, filePath } = options; + const { index, includeStdio, includeAttachments = true } = options; const lines: string[] = []; const title = formatTestTitle(config, test); const annotations: Annotation[] = []; @@ -236,7 +234,7 @@ export function formatFailure(config: FullConfig, test: TestCase, options: {inde lines.push(colors.red(header)); for (const result of test.results) { const resultLines: string[] = []; - const { tokens: resultTokens, position } = formatResultFailure(test, result, ' ', colors.enabled); + const { tokens: resultTokens, location } = formatResultFailure(config, test, result, ' ', colors.enabled); if (!resultTokens.length) continue; if (result.retry) { @@ -281,14 +279,11 @@ export function formatFailure(config: FullConfig, test: TestCase, options: {inde resultLines.push(''); resultLines.push(colors.gray(pad('--- Test output', '-')) + '\n\n' + outputText + '\n' + pad('', '-')); } - if (filePath) { - annotations.push({ - filePath, - position, - title, - message: [header, ...resultLines].join('\n'), - }); - } + annotations.push({ + location, + title, + message: [header, ...resultLines].join('\n'), + }); lines.push(...resultLines); } lines.push(''); @@ -298,7 +293,7 @@ export function formatFailure(config: FullConfig, test: TestCase, options: {inde }; } -export function formatResultFailure(test: TestCase, result: TestResult, initialIndent: string, highlightCode: boolean): FailureDetails { +export function formatResultFailure(config: FullConfig, test: TestCase, result: TestResult, initialIndent: string, highlightCode: boolean): FailureDetails { const resultTokens: string[] = []; if (result.status === 'timedOut') { resultTokens.push(''); @@ -310,17 +305,21 @@ export function formatResultFailure(test: TestCase, result: TestResult, initialI } let error: ErrorDetails | undefined = undefined; if (result.error !== undefined) { - error = formatError(result.error, highlightCode, test.location.file); + error = formatError(config, result.error, highlightCode, test.location.file); resultTokens.push(indent(error.message, initialIndent)); } return { tokens: resultTokens, - position: error?.position, + location: error?.location, }; } +function relativeFilePath(config: FullConfig, file: string): string { + return path.relative(config.rootDir, file) || path.basename(file); +} + function relativeTestPath(config: FullConfig, test: TestCase): string { - return path.relative(config.rootDir, test.location.file) || path.basename(test.location.file); + return relativeFilePath(config, test.location.file); } function stepSuffix(step: TestStep | undefined) { @@ -342,32 +341,39 @@ function formatTestHeader(config: FullConfig, test: TestCase, indent: string, in return pad(header, '='); } -export function formatError(error: TestError, highlightCode: boolean, file?: string): ErrorDetails { +export function formatError(config: FullConfig, error: TestError, highlightCode: boolean, file?: string): ErrorDetails { const stack = error.stack; const tokens = ['']; - let positionInFile: PositionInFile | undefined; + let location: Location | undefined; if (stack) { - const { message, stackLines, position } = prepareErrorStack( - stack, - file - ); - positionInFile = position; - tokens.push(message); - - const codeFrame = generateCodeFrame({ highlightCode }, file, position); - if (codeFrame) { - tokens.push(''); - tokens.push(codeFrame); + const parsed = prepareErrorStack(stack); + tokens.push(parsed.message); + location = parsed.location; + if (location) { + try { + // Stack will have /private/var/folders instead of /var/folders on Mac. + const realFile = fs.realpathSync(location.file); + const source = fs.readFileSync(realFile, 'utf8'); + const codeFrame = codeFrameColumns(source, { start: location }, { highlightCode }); + if (!file || file !== realFile) { + tokens.push(''); + tokens.push(colors.gray(` at `) + `${relativeFilePath(config, realFile)}:${location.line}`); + } + tokens.push(''); + tokens.push(codeFrame); + } catch (e) { + // Failed to read the source file - that's ok. + } } tokens.push(''); - tokens.push(colors.dim(stackLines.join('\n'))); + tokens.push(colors.dim(parsed.stackLines.join('\n'))); } else if (error.message) { tokens.push(error.message); } else if (error.value) { tokens.push(error.value); } return { - position: positionInFile, + location, message: tokens.join('\n'), }; } @@ -382,48 +388,26 @@ function indent(lines: string, tab: string) { return lines.replace(/^(?=.+$)/gm, tab); } -export function generateCodeFrame(options: BabelCodeFrameOptions, file?: string, position?: PositionInFile): string | undefined { - if (!position || !file) - return; - - const source = fs.readFileSync(file!, 'utf8'); - const codeFrame = codeFrameColumns( - source, - { start: position }, - options - ); - - return codeFrame; -} - -export function prepareErrorStack(stack: string, file?: string): { +export function prepareErrorStack(stack: string): { message: string; stackLines: string[]; - position?: PositionInFile; + location?: Location; } { const lines = stack.split('\n'); let firstStackLine = lines.findIndex(line => line.startsWith(' at ')); - if (firstStackLine === -1) firstStackLine = lines.length; + if (firstStackLine === -1) + firstStackLine = lines.length; const message = lines.slice(0, firstStackLine).join('\n'); const stackLines = lines.slice(firstStackLine); - const position = file ? positionInFile(stackLines, file) : undefined; - return { - message, - stackLines, - position, - }; -} - -function positionInFile(stackLines: string[], file: string): PositionInFile | undefined { - // Stack will have /private/var/folders instead of /var/folders on Mac. - file = fs.realpathSync(file); + let location: Location | undefined; for (const line of stackLines) { const parsed = stackUtils.parseLine(line); - if (!parsed || !parsed.file) - continue; - if (path.resolve(process.cwd(), parsed.file) === file) - return { column: parsed.column || 0, line: parsed.line || 0 }; + if (parsed && parsed.file) { + location = { file: path.join(process.cwd(), parsed.file), column: parsed.column || 0, line: parsed.line || 0 }; + break; + } } + return { message, stackLines, location }; } function monotonicTime(): number { diff --git a/packages/playwright-test/src/reporters/github.ts b/packages/playwright-test/src/reporters/github.ts index 757589a6a7..3ee6578f5d 100644 --- a/packages/playwright-test/src/reporters/github.ts +++ b/packages/playwright-test/src/reporters/github.ts @@ -69,7 +69,7 @@ export class GitHubReporter extends BaseReporter { } override onError(error: TestError) { - const errorMessage = formatError(error, false).message; + const errorMessage = formatError(this.config, error, false).message; this.githubLogger.error(errorMessage); } @@ -100,21 +100,19 @@ export class GitHubReporter extends BaseReporter { private _printFailureAnnotations(failures: TestCase[]) { failures.forEach((test, index) => { - const filePath = workspaceRelativePath(test.location.file); const { annotations } = formatFailure(this.config, test, { - filePath, index: index + 1, includeStdio: true, includeAttachments: false, }); - annotations.forEach(({ filePath, title, message, position }) => { + annotations.forEach(({ location, title, message }) => { const options: GitHubLogOptions = { - file: filePath, + file: workspaceRelativePath(location?.file || test.location.file), title, }; - if (position) { - options.line = position.line; - options.col = position.column; + if (location) { + options.line = location.line; + options.col = location.column; } this.githubLogger.error(message, options); }); diff --git a/packages/playwright-test/src/reporters/json.ts b/packages/playwright-test/src/reporters/json.ts index 8d8553a09a..9f5f525ae2 100644 --- a/packages/playwright-test/src/reporters/json.ts +++ b/packages/playwright-test/src/reporters/json.ts @@ -17,7 +17,7 @@ import fs from 'fs'; import path from 'path'; import { FullConfig, TestCase, Suite, TestResult, TestError, TestStep, FullResult, TestStatus, Location, Reporter } from '../../types/testReporter'; -import { PositionInFile, prepareErrorStack } from './base'; +import { prepareErrorStack } from './base'; export interface JSONReport { config: Omit & { @@ -77,7 +77,7 @@ export interface JSONReportTestResult { body?: string; contentType: string; }[]; - errorLocation?: PositionInFile + errorLocation?: Location; } export interface JSONReportTestStep { title: string; @@ -229,12 +229,12 @@ class JSONReporter implements Reporter { annotations: test.annotations, expectedStatus: test.expectedStatus, projectName: test.titlePath()[1], - results: test.results.map(r => this._serializeTestResult(r, test.location.file)), + results: test.results.map(r => this._serializeTestResult(r)), status: test.outcome(), }; } - private _serializeTestResult(result: TestResult, file: string): JSONReportTestResult { + private _serializeTestResult(result: TestResult): JSONReportTestResult { const steps = result.steps.filter(s => s.category === 'test.step'); const jsonResult: JSONReportTestResult = { workerIndex: result.workerIndex, @@ -252,14 +252,8 @@ class JSONReporter implements Reporter { body: a.body?.toString('base64') })), }; - if (result.error?.stack) { - const { position } = prepareErrorStack( - result.error.stack, - file - ); - if (position) - jsonResult.errorLocation = position; - } + if (result.error?.stack) + jsonResult.errorLocation = prepareErrorStack(result.error.stack).location; return jsonResult; } diff --git a/packages/playwright-test/src/reporters/raw.ts b/packages/playwright-test/src/reporters/raw.ts index 300600fdae..cd06bb6b48 100644 --- a/packages/playwright-test/src/reporters/raw.ts +++ b/packages/playwright-test/src/reporters/raw.ts @@ -219,7 +219,7 @@ class RawReporter { startTime: result.startTime.toISOString(), duration: result.duration, status: result.status, - error: formatResultFailure(test, result, '', true).tokens.join('').trim(), + error: formatResultFailure(this.config, test, result, '', true).tokens.join('').trim(), attachments: this._createAttachments(result), steps: dedupeSteps(result.steps.map(step => this._serializeStep(test, step))) }; diff --git a/tests/playwright-test/blink-diff.spec.ts b/tests/playwright-test/blink-diff.spec.ts index 1779723d29..170caa0acb 100644 --- a/tests/playwright-test/blink-diff.spec.ts +++ b/tests/playwright-test/blink-diff.spec.ts @@ -730,7 +730,6 @@ it.describe('Blink-Diff', () => { it('should crop image-a', async () => { instance._cropImageA = { width: 1, height: 2 }; - console.log('A'); const result = instance.runSync(); expect(result.dimension).toBe(2); }); diff --git a/tests/playwright-test/reporter-base.spec.ts b/tests/playwright-test/reporter-base.spec.ts index 6459b1a4ca..42fc23b94b 100644 --- a/tests/playwright-test/reporter-base.spec.ts +++ b/tests/playwright-test/reporter-base.spec.ts @@ -64,26 +64,15 @@ test('print should print the error name without a message', async ({ runInlineTe expect(result.output).toContain('FooBarError'); }); -test('print an error in a codeframe', async ({ runInlineTest }) => { +test('should print an error in a codeframe', async ({ runInlineTest }) => { const result = await runInlineTest({ - 'my-lib.ts': ` - const foobar = () => { - const error = new Error('my-message'); - error.name = 'FooBarError'; - throw error; - } - export default () => { - foobar(); - } - `, 'a.spec.ts': ` - const { test } = pwt; - import myLib from './my-lib'; - test('foobar', async ({}) => { - const error = new Error('my-message'); - error.name = 'FooBarError'; - throw error; - }); + const { test } = pwt; + test('foobar', async ({}) => { + const error = new Error('my-message'); + error.name = 'FooBarError'; + throw error; + }); ` }, {}, { FORCE_COLOR: '0', @@ -91,9 +80,36 @@ test('print an error in a codeframe', async ({ runInlineTest }) => { expect(result.exitCode).toBe(1); expect(result.failed).toBe(1); expect(result.output).toContain('FooBarError: my-message'); - expect(result.output).toContain('test(\'foobar\', async'); - expect(result.output).toContain('throw error;'); - expect(result.output).toContain('import myLib from \'./my-lib\';'); + expect(result.output).not.toContain('at a.spec.ts:7'); + expect(result.output).toContain(` 5 | const { test } = pwt;`); + expect(result.output).toContain(` 6 | test('foobar', async ({}) => {`); + expect(result.output).toContain(`> 7 | const error = new Error('my-message');`); +}); + +test('should print codeframe from a helper', async ({ runInlineTest }) => { + const result = await runInlineTest({ + 'helper.ts': ` + export function ohMy() { + throw new Error('oh my'); + } + `, + 'a.spec.ts': ` + import { ohMy } from './helper'; + const { test } = pwt; + test('foobar', async ({}) => { + ohMy(); + }); + ` + }, {}, { + FORCE_COLOR: '0', + }); + expect(result.exitCode).toBe(1); + expect(result.failed).toBe(1); + expect(result.output).toContain('Error: oh my'); + expect(result.output).toContain(` at helper.ts:5`); + expect(result.output).toContain(` 4 | export function ohMy() {`); + expect(result.output).toContain(`> 5 | throw new Error('oh my');`); + expect(result.output).toContain(` | ^`); }); test('should print slow tests', async ({ runInlineTest }) => {