chore: allow matchers decorate step title (#13199)

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;

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);
     }

packages/playwright-test/src/index.ts

@@ -264,7 +264,7 @@ export const test = _baseTest.extend<TestFixtures, WorkerFixtures>({
         },
         onApiCallEnd: (userData: any, error?: Error) => {
           const step = userData.userObject;
-          step?.complete(error);
+          step?.complete({ error });
         },
       };
     };

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;
 };

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<T extends ImageComparatorOptions> {
   readonly testInfo: TestInfoImpl;
   readonly expectedPath: string;
@@ -84,7 +57,6 @@ class SnapshotHelper<T extends ImageComparatorOptions> {
     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<T extends ImageComparatorOptions> {
         ...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<T extends ImageComparatorOptions> {
     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<T extends ImageComparatorOptions> {
       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<T extends ImageComparatorOptions> {
     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<T extends ImageComparatorOptions> {
       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);

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;
     }
   }

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;
     }
   }

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;

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));

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`,
   ]);
 });