From 923f74c5a6f3a7ecdcbf3968b94dcf65113d5ec3 Mon Sep 17 00:00:00 2001 From: Pavel Feldman Date: Wed, 30 Mar 2022 20:52:00 -0800 Subject: [PATCH] chore: allow matchers decorate step title (#13199) --- packages/playwright-test/src/dispatcher.ts | 2 + packages/playwright-test/src/expect.ts | 30 ++++++---- packages/playwright-test/src/index.ts | 2 +- packages/playwright-test/src/ipc.ts | 1 + .../src/matchers/toMatchSnapshot.ts | 59 ++++++------------- packages/playwright-test/src/testInfo.ts | 4 +- packages/playwright-test/src/testType.ts | 4 +- packages/playwright-test/src/types.ts | 2 +- packages/playwright-test/src/workerRunner.ts | 12 ++-- .../to-have-screenshot.spec.ts | 16 ++--- 10 files changed, 58 insertions(+), 74 deletions(-) diff --git a/packages/playwright-test/src/dispatcher.ts b/packages/playwright-test/src/dispatcher.ts index a6c88f90ab..b7c1907646 100644 --- a/packages/playwright-test/src/dispatcher.ts +++ b/packages/playwright-test/src/dispatcher.ts @@ -262,6 +262,8 @@ export class Dispatcher { this._reporter.onStdErr?.('Internal error: step end without step begin: ' + params.stepId, data.test, result); return; } + if (params.refinedTitle) + step.title = params.refinedTitle; step.duration = params.wallTime - step.startTime.getTime(); if (params.error) step.error = params.error; diff --git a/packages/playwright-test/src/expect.ts b/packages/playwright-test/src/expect.ts index f640e17c96..af2c055a82 100644 --- a/packages/playwright-test/src/expect.ts +++ b/packages/playwright-test/src/expect.ts @@ -44,13 +44,19 @@ import { toHaveURL, toHaveValue } from './matchers/matchers'; -import { toMatchSnapshot, toHaveScreenshot, getSnapshotName } from './matchers/toMatchSnapshot'; +import { toMatchSnapshot, toHaveScreenshot } from './matchers/toMatchSnapshot'; import type { Expect, TestError } from './types'; import matchers from 'expect/build/matchers'; import { currentTestInfo } from './globals'; import { serializeError, captureStackTrace, currentExpectTimeout } from './util'; import { monotonicTime } from 'playwright-core/lib/utils/utils'; +// from expect/build/types +export type SyncExpectationResult = { + pass: boolean; + message: () => string; +}; + // #region // Mirrored from https://github.com/facebook/jest/blob/f13abff8df9a0e1148baf3584bcde6d1b479edc7/packages/expect/src/print.ts /** @@ -230,11 +236,6 @@ function wrap(matcherName: string, matcher: any) { if (!testInfo) return matcher.call(this, ...args); - let titleSuffix = ''; - if (matcherName === 'toHaveScreenshot' || matcherName === 'toMatchSnapshot') { - const [received, nameOrOptions, optOptions] = args; - titleSuffix = `(${getSnapshotName(testInfo, received, nameOrOptions, optOptions)})`; - } const stackTrace = captureStackTrace(); const stackLines = stackTrace.frameTexts; const frame = stackTrace.frames[0]; @@ -242,15 +243,16 @@ function wrap(matcherName: string, matcher: any) { const isSoft = expectCallMetaInfo?.isSoft ?? false; const isPoll = expectCallMetaInfo?.isPoll ?? false; const pollTimeout = expectCallMetaInfo?.pollTimeout; + const defaultTitle = `expect${isPoll ? '.poll' : ''}${isSoft ? '.soft' : ''}${this.isNot ? '.not' : ''}.${matcherName}`; const step = testInfo._addStep({ location: frame && frame.file ? { file: path.resolve(process.cwd(), frame.file), line: frame.line || 0, column: frame.column || 0 } : undefined, category: 'expect', - title: customMessage || `expect${isPoll ? '.poll' : ''}${isSoft ? '.soft' : ''}${this.isNot ? '.not' : ''}.${matcherName}${titleSuffix}`, + title: customMessage || defaultTitle, canHaveChildren: true, forceNoParent: false }); - const reportStepEnd = (result: any) => { + const reportStepEnd = (result: any, options: { refinedTitle?: string }) => { const success = result.pass !== this.isNot; let error: TestError | undefined; if (!success) { @@ -276,15 +278,19 @@ function wrap(matcherName: string, matcher: any) { result.message = () => newMessage; } } - step.complete(error); + step.complete({ ...options, error }); return result; }; const reportStepError = (error: Error) => { - step.complete(serializeError(error)); + step.complete({ error: serializeError(error) }); throw error; }; + const refineTitle = (result: SyncExpectationResult & { titleSuffix?: string }): string | undefined => { + return !customMessage && result.titleSuffix ? defaultTitle + result.titleSuffix : undefined; + }; + try { let result; const [receivedOrGenerator, ...otherArgs] = args; @@ -300,8 +306,8 @@ function wrap(matcherName: string, matcher: any) { result = matcher.call(this, ...args); } if (result instanceof Promise) - return result.then(reportStepEnd).catch(reportStepError); - return reportStepEnd(result); + return result.then(result => reportStepEnd(result, { refinedTitle: refineTitle(result) })).catch(reportStepError); + return reportStepEnd(result, { refinedTitle: refineTitle(result) }); } catch (e) { reportStepError(e); } diff --git a/packages/playwright-test/src/index.ts b/packages/playwright-test/src/index.ts index 82aefe58a3..198506430e 100644 --- a/packages/playwright-test/src/index.ts +++ b/packages/playwright-test/src/index.ts @@ -264,7 +264,7 @@ export const test = _baseTest.extend({ }, onApiCallEnd: (userData: any, error?: Error) => { const step = userData.userObject; - step?.complete(error); + step?.complete({ error }); }, }; }; diff --git a/packages/playwright-test/src/ipc.ts b/packages/playwright-test/src/ipc.ts index 49f57c7431..f0a0ef9da9 100644 --- a/packages/playwright-test/src/ipc.ts +++ b/packages/playwright-test/src/ipc.ts @@ -60,6 +60,7 @@ export type StepBeginPayload = { export type StepEndPayload = { testId: string; stepId: string; + refinedTitle?: string; wallTime: number; // milliseconds since unix epoch error?: TestError; }; diff --git a/packages/playwright-test/src/matchers/toMatchSnapshot.ts b/packages/playwright-test/src/matchers/toMatchSnapshot.ts index 3bf5622794..5fcc1b7848 100644 --- a/packages/playwright-test/src/matchers/toMatchSnapshot.ts +++ b/packages/playwright-test/src/matchers/toMatchSnapshot.ts @@ -31,38 +31,11 @@ import fs from 'fs'; import path from 'path'; import * as mime from 'mime'; import { TestInfoImpl } from '../testInfo'; - -// from expect/build/types -type SyncExpectationResult = { - pass: boolean; - message: () => string; -}; +import { SyncExpectationResult } from '../expect'; type NameOrSegments = string | string[]; const SNAPSHOT_COUNTER = Symbol('noname-snapshot-counter'); -export function getSnapshotName( - testInfo: TestInfoImpl, - received: any, - nameOrOptions: NameOrSegments | { name?: NameOrSegments } = {}, - optOptions: any = {} -) { - const [ - anonymousSnapshotExtension, - snapshotPathResolver, - ] = typeof received === 'string' || Buffer.isBuffer(received) ? [ - determineFileExtension(received), - testInfo.snapshotPath.bind(testInfo), - ] : [ - 'png', - testInfo._screenshotPath.bind(testInfo), - ]; - const helper = new SnapshotHelper( - testInfo, snapshotPathResolver, anonymousSnapshotExtension, {}, - nameOrOptions, optOptions, true /* dryRun */); - return path.basename(helper.snapshotPath); -} - class SnapshotHelper { readonly testInfo: TestInfoImpl; readonly expectedPath: string; @@ -84,7 +57,6 @@ class SnapshotHelper { configOptions: ImageComparatorOptions, nameOrOptions: NameOrSegments | { name?: NameOrSegments } & T, optOptions: T, - dryRun: boolean = false, ) { let options: T; let name: NameOrSegments | undefined; @@ -102,8 +74,7 @@ class SnapshotHelper { ...testInfo.titlePath.slice(1), (testInfo as any)[SNAPSHOT_COUNTER] + 1, ].join(' '); - if (!dryRun) - ++(testInfo as any)[SNAPSHOT_COUNTER]; + ++(testInfo as any)[SNAPSHOT_COUNTER]; name = sanitizeForFilePath(trimLongString(fullTitleWithoutSpec)) + '.' + anonymousSnapshotExtension; } @@ -143,19 +114,23 @@ class SnapshotHelper { this.kind = this.mimeType.startsWith('image/') ? 'Screenshot' : 'Snapshot'; } + decorateTitle(result: SyncExpectationResult): SyncExpectationResult & { titleSuffix: string } { + return { ...result, titleSuffix: `(${path.basename(this.snapshotPath)})` }; + } + handleMissingNegated() { const isWriteMissingMode = this.updateSnapshots === 'all' || this.updateSnapshots === 'missing'; const message = `${this.snapshotPath} is missing in snapshots${isWriteMissingMode ? ', matchers using ".not" won\'t write them automatically.' : '.'}`; - return { + return this.decorateTitle({ // NOTE: 'isNot' matcher implies inversed value. pass: true, message: () => message, - }; + }); } handleDifferentNegated() { // NOTE: 'isNot' matcher implies inversed value. - return { pass: false, message: () => '' }; + return this.decorateTitle({ pass: false, message: () => '' }); } handleMatchingNegated() { @@ -165,7 +140,7 @@ class SnapshotHelper { indent('Expected result should be different from the actual one.', ' '), ].join('\n'); // NOTE: 'isNot' matcher implies inversed value. - return { pass: true, message: () => message }; + return this.decorateTitle({ pass: true, message: () => message }); } handleMissing(actual: Buffer | string) { @@ -178,13 +153,13 @@ class SnapshotHelper { if (this.updateSnapshots === 'all') { /* eslint-disable no-console */ console.log(message); - return { pass: true, message: () => message }; + return this.decorateTitle({ pass: true, message: () => message }); } if (this.updateSnapshots === 'missing') { this.testInfo._failWithError(serializeError(new Error(message)), false /* isHardError */); - return { pass: true, message: () => '' }; + return this.decorateTitle({ pass: true, message: () => '' }); } - return { pass: false, message: () => message }; + return this.decorateTitle({ pass: false, message: () => message }); } handleDifferent( @@ -226,11 +201,11 @@ class SnapshotHelper { this.testInfo.attachments.push({ name: path.basename(this.diffPath), contentType: this.mimeType, path: this.diffPath }); output.push(` Diff: ${colors.yellow(this.diffPath)}`); } - return { pass: false, message: () => output.join('\n'), }; + return this.decorateTitle({ pass: false, message: () => output.join('\n'), }); } handleMatching() { - return { pass: true, message: () => '' }; + return this.decorateTitle({ pass: true, message: () => '' }); } } @@ -239,7 +214,7 @@ export function toMatchSnapshot( received: Buffer | string, nameOrOptions: NameOrSegments | { name?: NameOrSegments } & ImageComparatorOptions = {}, optOptions: ImageComparatorOptions = {} -): SyncExpectationResult { +): SyncExpectationResult & { titleSuffix: string } { const testInfo = currentTestInfo(); if (!testInfo) throw new Error(`toMatchSnapshot() must be called during the test`); @@ -269,7 +244,7 @@ export function toMatchSnapshot( writeFileSync(helper.snapshotPath, received); /* eslint-disable no-console */ console.log(helper.snapshotPath + ' does not match, writing actual.'); - return { pass: true, message: () => helper.snapshotPath + ' running with --update-snapshots, writing actual.' }; + return helper.decorateTitle({ pass: true, message: () => helper.snapshotPath + ' running with --update-snapshots, writing actual.' }); } return helper.handleDifferent(received, expected, undefined, result.diff, result.errorMessage, undefined); diff --git a/packages/playwright-test/src/testInfo.ts b/packages/playwright-test/src/testInfo.ts index 70f668e726..ca36e15bc0 100644 --- a/packages/playwright-test/src/testInfo.ts +++ b/packages/playwright-test/src/testInfo.ts @@ -219,10 +219,10 @@ export class TestInfoImpl implements TestInfo { const step = this._addStep(stepInfo); try { const result = await cb(); - step.complete(); + step.complete({}); return result; } catch (e) { - step.complete(e instanceof SkipError ? undefined : serializeError(e)); + step.complete({ error: e instanceof SkipError ? undefined : serializeError(e) }); throw e; } } diff --git a/packages/playwright-test/src/testType.ts b/packages/playwright-test/src/testType.ts index e16bbaa3d9..e3bd8f4103 100644 --- a/packages/playwright-test/src/testType.ts +++ b/packages/playwright-test/src/testType.ts @@ -216,9 +216,9 @@ export class TestTypeImpl { }); try { await body(); - step.complete(); + step.complete({}); } catch (e) { - step.complete(serializeError(e)); + step.complete({ error: serializeError(e) }); throw e; } } diff --git a/packages/playwright-test/src/types.ts b/packages/playwright-test/src/types.ts index 727cff575e..acca6e70a8 100644 --- a/packages/playwright-test/src/types.ts +++ b/packages/playwright-test/src/types.ts @@ -27,7 +27,7 @@ export type FixturesWithLocation = { export type Annotation = { type: string, description?: string }; export interface TestStepInternal { - complete(error?: Error | TestError): void; + complete(result: { refinedTitle?: string, error?: Error | TestError }): void; title: string; category: string; canHaveChildren: boolean; diff --git a/packages/playwright-test/src/workerRunner.ts b/packages/playwright-test/src/workerRunner.ts index 4a3e0777dc..f898641e4f 100644 --- a/packages/playwright-test/src/workerRunner.ts +++ b/packages/playwright-test/src/workerRunner.ts @@ -207,14 +207,14 @@ export class WorkerRunner extends EventEmitter { let callbackHandled = false; const step: TestStepInternal = { ...data, - complete: (error?: Error | TestError) => { + complete: result => { if (callbackHandled) return; callbackHandled = true; - if (error instanceof Error) - error = serializeError(error); + const error = result.error instanceof Error ? serializeError(result.error) : result.error; const payload: StepEndPayload = { testId: test._id, + refinedTitle: result.refinedTitle, stepId, wallTime: Date.now(), error, @@ -346,14 +346,14 @@ export class WorkerRunner extends EventEmitter { // Setup fixtures required by the test. testInfo._timeoutManager.setCurrentRunnable({ type: 'test' }); const params = await this._fixtureRunner.resolveParametersForFunction(test.fn, testInfo); - beforeHooksStep.complete(); // Report fixture hooks step as completed. + beforeHooksStep.complete({}); // Report fixture hooks step as completed. // Now run the test itself. const fn = test.fn; // Extract a variable to get a better stack trace ("myTest" vs "TestCase.myTest [as fn]"). await fn(params, testInfo); }, 'allowSkips'); - beforeHooksStep.complete(maybeError); // Second complete is a no-op. + beforeHooksStep.complete({ error: maybeError }); // Second complete is a no-op. }); if (didFailBeforeAllForSuite) { @@ -425,7 +425,7 @@ export class WorkerRunner extends EventEmitter { }); } - afterHooksStep.complete(firstAfterHooksError); + afterHooksStep.complete({ error: firstAfterHooksError }); this._currentTest = null; setCurrentTestInfo(null); this.emit('testEnd', buildTestEndPayload(testInfo)); diff --git a/tests/playwright-test/to-have-screenshot.spec.ts b/tests/playwright-test/to-have-screenshot.spec.ts index e82a608836..47f704b4d0 100644 --- a/tests/playwright-test/to-have-screenshot.spec.ts +++ b/tests/playwright-test/to-have-screenshot.spec.ts @@ -150,8 +150,8 @@ test('should report toHaveScreenshot step with expectation name in title', async const result = await runInlineTest({ 'reporter.ts': ` class Reporter { - onStepBegin(test, result, step) { - console.log('%% begin ' + step.title); + onStepEnd(test, result, step) { + console.log('%% end ' + step.title); } } module.exports = Reporter; @@ -173,12 +173,12 @@ test('should report toHaveScreenshot step with expectation name in title', async expect(result.exitCode).toBe(0); expect(result.output.split('\n').filter(line => line.startsWith('%%'))).toEqual([ - `%% begin Before Hooks`, - `%% begin browserContext.newPage`, - `%% begin expect.toHaveScreenshot(foo.png)`, - `%% begin expect.toHaveScreenshot(is-a-test-1.png)`, - `%% begin After Hooks`, - `%% begin browserContext.close`, + `%% end browserContext.newPage`, + `%% end Before Hooks`, + `%% end expect.toHaveScreenshot(foo.png)`, + `%% end expect.toHaveScreenshot(is-a-test-1.png)`, + `%% end browserContext.close`, + `%% end After Hooks`, ]); });