chore: box step w/o modifying runtime errors (#28762)

packages/playwright/src/matchers/expect.ts

@@ -248,45 +248,46 @@ class ExpectMetaInfoProxyHandler implements ProxyHandler<any> {
       if (!testInfo)
         return matcher.call(target, ...args);
 
-      const rawStack = captureRawStack();
-      const stackFrames = filteredStackTrace(rawStack);
       const customMessage = this._info.message || '';
       const argsSuffix = computeArgsSuffix(matcherName, args);
 
       const defaultTitle = `expect${this._info.isPoll ? '.poll' : ''}${this._info.isSoft ? '.soft' : ''}${this._info.isNot ? '.not' : ''}.${matcherName}${argsSuffix}`;
       const title = customMessage || defaultTitle;
       const wallTime = Date.now();
-      const step = matcherName !== 'toPass' ? testInfo._addStep({
+
-        location: stackFrames[0],
+      // This looks like it is unnecessary, but it isn't - we need to filter
+      // out all the frames that belong to the test runner from caught runtime errors.
+      const rawStack = captureRawStack();
+      const stackFrames = filteredStackTrace(rawStack);
+
+      // Enclose toPass in a step to maintain async stacks, toPass matcher is always async.
+      const stepInfo = {
         category: 'expect',
         title: trimLongString(title, 1024),
         params: args[0] ? { expected: args[0] } : undefined,
         wallTime,
         infectParentStepsWithError: this._info.isSoft,
         laxParent: true,
-      }) : undefined;
+        isSoft: this._info.isSoft,
+      };
+
+      const step = testInfo._addStep(stepInfo);
 
       const reportStepError = (jestError: ExpectError) => {
         const error = new ExpectError(jestError, customMessage, stackFrames);
-        const serializedError = {
+        step.complete({ error });
-          message: error.message,
+        if (!this._info.isSoft)
-          stack: error.stack,
-        };
-        step?.complete({ error: serializedError });
-        if (this._info.isSoft)
-          testInfo._failWithError(serializedError, false /* isHardError */);
-        else
           throw error;
       };
 
       const finalizer = () => {
-        step?.complete({});
+        step.complete({});
       };
 
-      const expectZone: ExpectZone = { title, wallTime };
-
       try {
-        const result = zones.run<ExpectZone, any>('expectZone', expectZone, () => matcher.call(target, ...args));
+        const expectZone: ExpectZone | null = matcherName !== 'toPass' ? { title, wallTime } : null;
+        const callback = () => matcher.call(target, ...args);
+        const result = expectZone ? zones.run<ExpectZone, any>('expectZone', expectZone, callback) : zones.preserve(callback);
         if (result instanceof Promise)
           return result.then(finalizer).catch(reportStepError);
         finalizer();

packages/playwright/src/matchers/matcherHint.ts

@@ -49,6 +49,7 @@ export class ExpectError extends Error {
     log?: string[];
     timeout?: number;
   };
+
   constructor(jestError: ExpectError, customMessage: string, stackFrames: StackFrame[]) {
     super('');
     // Copy to erase the JestMatcherError constructor name from the console.log(error).

packages/playwright/src/matchers/matchers.ts

@@ -17,13 +17,13 @@
 import type { Locator, Page, APIResponse } from 'playwright-core';
 import type { FrameExpectOptions } from 'playwright-core/lib/client/types';
 import { colors } from 'playwright-core/lib/utilsBundle';
-import { expectTypes, callLogText, filteredStackTrace } from '../util';
+import { expectTypes, callLogText } from '../util';
 import { toBeTruthy } from './toBeTruthy';
 import { toEqual } from './toEqual';
 import { toExpectedTextValues, toMatchText } from './toMatchText';
-import { captureRawStack, constructURLBasedOnBaseURL, isRegExp, isTextualMimeType, pollAgainstDeadline } from 'playwright-core/lib/utils';
+import { constructURLBasedOnBaseURL, isRegExp, isTextualMimeType, pollAgainstDeadline } from 'playwright-core/lib/utils';
 import { currentTestInfo } from '../common/globals';
-import { TestInfoImpl, type TestStepInternal } from '../worker/testInfo';
+import { TestInfoImpl } from '../worker/testInfo';
 import type { ExpectMatcherContext } from './expect';
 
 interface LocatorEx extends Locator {
@@ -369,42 +369,26 @@ export async function toPass(
   const testInfo = currentTestInfo();
   const timeout = options.timeout !== undefined ? options.timeout : 0;
 
-  const rawStack = captureRawStack();
+  const { deadline, timeoutMessage } = testInfo ? testInfo._deadlineForMatcher(timeout) : TestInfoImpl._defaultDeadlineForMatcher(timeout);
-  const stackFrames = filteredStackTrace(rawStack);
+  const result = await pollAgainstDeadline<Error|undefined>(async () => {
-
+    if (testInfo && currentTestInfo() !== testInfo)
-  const runWithOrWithoutStep = async (callback: (step: TestStepInternal | undefined) => Promise<{ pass: boolean; message: () => string; }>) => {
+      return { continuePolling: false, result: undefined };
-    if (!testInfo)
+    try {
-      return await callback(undefined);
+      await callback();
-    return await testInfo._runAsStep({
+      return { continuePolling: !!this.isNot, result: undefined };
-      title: 'expect.toPass',
+    } catch (e) {
-      category: 'expect',
+      return { continuePolling: !this.isNot, result: e };
-      location: stackFrames[0],
-    }, callback);
-  };
-
-  return await runWithOrWithoutStep(async (step: TestStepInternal | undefined) => {
-    const { deadline, timeoutMessage } = testInfo ? testInfo._deadlineForMatcher(timeout) : TestInfoImpl._defaultDeadlineForMatcher(timeout);
-    const result = await pollAgainstDeadline<Error|undefined>(async () => {
-      if (testInfo && currentTestInfo() !== testInfo)
-        return { continuePolling: false, result: undefined };
-      try {
-        await callback();
-        return { continuePolling: !!this.isNot, result: undefined };
-      } catch (e) {
-        return { continuePolling: !this.isNot, result: e };
-      }
-    }, deadline, options.intervals || [100, 250, 500, 1000]);
-
-    if (result.timedOut) {
-      const message = result.result ? [
-        result.result.message,
-        '',
-        `Call Log:`,
-        `- ${timeoutMessage}`,
-      ].join('\n') : timeoutMessage;
-      step?.complete({ error: { message } });
-      return { message: () => message, pass: !!this.isNot };
     }
-    return { pass: !this.isNot, message: () => '' };
+  }, deadline, options.intervals || [100, 250, 500, 1000]);
-  });
+
+  if (result.timedOut) {
+    const message = result.result ? [
+      result.result.message,
+      '',
+      `Call Log:`,
+      `- ${timeoutMessage}`,
+    ].join('\n') : timeoutMessage;
+    return { message: () => message, pass: !!this.isNot };
+  }
+  return { pass: !this.isNot, message: () => '' };
 }

packages/playwright/src/matchers/toMatchSnapshot.ts

@@ -22,7 +22,7 @@ import type { ImageComparatorOptions, Comparator } from 'playwright-core/lib/uti
 import { getComparator, sanitizeForFilePath, zones } from 'playwright-core/lib/utils';
 import type { PageScreenshotOptions } from 'playwright-core/types/types';
 import {
-  addSuffixToFilePath, serializeError,
+  addSuffixToFilePath,
   trimLongString, callLogText,
   expectTypes  } from '../util';
 import { colors } from 'playwright-core/lib/utilsBundle';
@@ -206,7 +206,7 @@ class SnapshotHelper<T extends ImageComparatorOptions> {
       return this.createMatcherResult(message, true);
     }
     if (this.updateSnapshots === 'missing') {
-      this.testInfo._failWithError(serializeError(new Error(message)), false /* isHardError */);
+      this.testInfo._failWithError(new Error(message), false /* isHardError */);
       return this.createMatcherResult('', true);
     }
     return this.createMatcherResult(message, false);

packages/playwright/src/worker/fixtureRunner.ts

@@ -14,7 +14,7 @@
  * limitations under the License.
  */
 
-import { formatLocation, debugTest, filterStackFile, serializeError } from '../util';
+import { formatLocation, debugTest, filterStackFile } from '../util';
 import { ManualPromise, zones } from 'playwright-core/lib/utils';
 import type { TestInfoImpl, TestStepInternal } from './testInfo';
 import type { FixtureDescription, TimeoutManager } from './timeoutManager';
@@ -143,7 +143,7 @@ class Fixture {
       this._selfTeardownComplete?.then(() => {
         afterStep?.complete({});
       }).catch(e => {
-        afterStep?.complete({ error: serializeError(e) });
+        afterStep?.complete({ error: e });
       });
     }
     testInfo._timeoutManager.setCurrentFixture(undefined);

packages/playwright/src/worker/testInfo.ts

@@ -30,7 +30,7 @@ import type { Attachment } from './testTracing';
 import type { StackFrame } from '@protocol/channels';
 
 export interface TestStepInternal {
-  complete(result: { error?: Error | TestInfoError, attachments?: Attachment[] }): void;
+  complete(result: { error?: Error, attachments?: Attachment[] }): void;
   stepId: string;
   title: string;
   category: string;
@@ -45,6 +45,7 @@ export interface TestStepInternal {
   error?: TestInfoError;
   infectParentStepsWithError?: boolean;
   box?: boolean;
+  isSoft?: boolean;
 }
 
 export class TestInfoImpl implements TestInfo {
@@ -231,7 +232,7 @@ export class TestInfoImpl implements TestInfo {
     this.duration = this._timeoutManager.defaultSlotTimings().elapsed | 0;
   }
 
-  async _runAndFailOnError(fn: () => Promise<void>, skips?: 'allowSkips'): Promise<TestInfoError | undefined> {
+  async _runAndFailOnError(fn: () => Promise<void>, skips?: 'allowSkips'): Promise<Error | undefined> {
     try {
       await fn();
     } catch (error) {
@@ -239,9 +240,8 @@ export class TestInfoImpl implements TestInfo {
         if (this.status === 'passed')
           this.status = 'skipped';
       } else {
-        const serialized = serializeError(error);
+        this._failWithError(error, true /* isHardError */);
-        this._failWithError(serialized, true /* isHardError */);
+        return error;
-        return serialized;
       }
     }
   }
@@ -256,10 +256,9 @@ export class TestInfoImpl implements TestInfo {
     let isLaxParent = false;
     if (!parentStep && data.laxParent) {
       const visit = (step: TestStepInternal) => {
-        // Never nest into under another lax element, it could be a series
+        // Do not nest chains of route.continue.
-        // of no-reply actions, ala page.continue().
+        const shouldNest = step.title !== data.title;
-        const canNest = step.category === data.category || step.category === 'expect' && data.category === 'attach';
+        if (!step.endWallTime && shouldNest)
-        if (!step.endWallTime && canNest && !step.laxParent)
           parentStep = step;
         step.steps.forEach(visit);
       };
@@ -283,24 +282,18 @@ export class TestInfoImpl implements TestInfo {
       complete: result => {
         if (step.endWallTime)
           return;
-        step.endWallTime = Date.now();
-        let error: TestInfoError | undefined;
-        if (result.error instanceof Error) {
-          // Step function threw an error.
-          if (data.boxedStack) {
-            const errorTitle = `${result.error.name}: ${result.error.message}`;
-            result.error.stack = `${errorTitle}\n${stringifyStackFrames(data.boxedStack).join('\n')}`;
-          }
-          error = serializeError(result.error);
-        } else if (result.error) {
-          // Internal API step reported an error.
-          if (data.boxedStack)
-            result.error.stack = `${result.error.message}\n${stringifyStackFrames(data.boxedStack).join('\n')}`;
-          error = result.error;
-        }
-        step.error = error;
 
-        if (!error) {
+        step.endWallTime = Date.now();
+        if (result.error) {
+          if (!(result.error as any)[stepSymbol])
+            (result.error as any)[stepSymbol] = step;
+          const error = serializeError(result.error);
+          if (data.boxedStack)
+            error.stack = `${error.message}\n${stringifyStackFrames(data.boxedStack).join('\n')}`;
+          step.error = error;
+        }
+
+        if (!step.error) {
           // Soft errors inside try/catch will make the test fail.
           // In order to locate the failing step, we are marking all the parent
           // steps as failing unconditionally.
@@ -311,18 +304,20 @@ export class TestInfoImpl implements TestInfo {
               break;
             }
           }
-          error = step.error;
         }
 
         const payload: StepEndPayload = {
           testId: this._test.id,
           stepId,
           wallTime: step.endWallTime,
-          error,
+          error: step.error,
         };
         this._onStepEnd(payload);
-        const errorForTrace = error ? { name: '', message: error.message || '', stack: error.stack } : undefined;
+        const errorForTrace = step.error ? { name: '', message: step.error.message || '', stack: step.error.stack } : undefined;
         this._tracing.appendAfterActionForStep(stepId, errorForTrace, result.attachments);
+
+        if (step.isSoft && result.error)
+          this._failWithError(result.error, false /* isHardError */);
       }
     };
     const parentStepList = parentStep ? parentStep.steps : this._steps;
@@ -350,7 +345,7 @@ export class TestInfoImpl implements TestInfo {
       this.status = 'interrupted';
   }
 
-  _failWithError(error: TestInfoError, isHardError: boolean) {
+  _failWithError(error: Error, isHardError: boolean) {
     // Do not overwrite any previous hard errors.
     // Some (but not all) scenarios include:
     //   - expect() that fails after uncaught exception.
@@ -361,7 +356,11 @@ export class TestInfoImpl implements TestInfo {
       this._hasHardError = true;
     if (this.status === 'passed' || this.status === 'skipped')
       this.status = 'failed';
-    this.errors.push(error);
+    const serialized = serializeError(error);
+    const step = (error as any)[stepSymbol] as TestStepInternal | undefined;
+    if (step && step.boxedStack)
+      serialized.stack = `${error.name}: ${error.message}\n${stringifyStackFrames(step.boxedStack).join('\n')}`;
+    this.errors.push(serialized);
   }
 
   async _runAsStepWithRunnable<T>(
@@ -486,3 +485,5 @@ export class TestInfoImpl implements TestInfo {
 
 class SkipError extends Error {
 }
+
+const stepSymbol = Symbol('step');

packages/playwright/src/worker/workerMain.ts

@@ -170,7 +170,7 @@ export class WorkerMain extends ProcessRunner {
     // and unhandled errors - both lead to the test failing. This is good for regular tests,
     // so that you can, e.g. expect() from inside an event handler. The test fails,
     // and we restart the worker.
-    this._currentTest._failWithError(serializeError(error), true /* isHardError */);
+    this._currentTest._failWithError(error, true /* isHardError */);
 
     // For tests marked with test.fail(), this might be a problem when unhandled error
     // is not coming from the user test code (legit failure), but from fixtures or test runner.
@@ -395,7 +395,7 @@ export class WorkerMain extends ProcessRunner {
     const afterHooksSlot = testInfo._didTimeout ? { timeout: this._project.project.timeout, elapsed: 0 } : undefined;
     await testInfo._runAsStepWithRunnable({ category: 'hook', title: 'After Hooks', runnableType: 'afterHooks', runnableSlot: afterHooksSlot }, async step => {
       testInfo._afterHooksStep = step;
-      let firstAfterHooksError: TestInfoError | undefined;
+      let firstAfterHooksError: Error | undefined;
       await testInfo._runWithTimeout(async () => {
         // Note: do not wrap all teardown steps together, because failure in any of them
         // does not prevent further teardown steps from running.
@@ -541,11 +541,11 @@ export class WorkerMain extends ProcessRunner {
       throw beforeAllError;
   }
 
-  private async _runAfterAllHooksForSuite(suite: Suite, testInfo: TestInfoImpl) {
+  private async _runAfterAllHooksForSuite(suite: Suite, testInfo: TestInfoImpl): Promise<Error | undefined> {
     if (!this._activeSuites.has(suite))
       return;
     this._activeSuites.delete(suite);
-    let firstError: TestInfoError | undefined;
+    let firstError: Error | undefined;
     for (const hook of suite._hooks) {
       if (hook.type !== 'afterAll')
         continue;

tests/playwright-test/reporter.spec.ts

@@ -274,7 +274,7 @@ for (const useIntermediateMergeReport of [false, true] as const) {
         `begin {\"title\":\"expect.toBeTruthy\",\"category\":\"expect\"}`,
         `end {\"title\":\"expect.toBeTruthy\",\"category\":\"expect\"}`,
         `begin {\"title\":\"expect.toBeTruthy\",\"category\":\"expect\"}`,
-        `end {\"title\":\"expect.toBeTruthy\",\"category\":\"expect\",\"error\":{\"message\":\"\\u001b[2mexpect(\\u001b[22m\\u001b[31mreceived\\u001b[39m\\u001b[2m).\\u001b[22mtoBeTruthy\\u001b[2m()\\u001b[22m\\n\\nReceived: \\u001b[31mfalse\\u001b[39m\",\"stack\":\"<stack>\",\"location\":\"<location>\",\"snippet\":\"<snippet>\"}}`,
+        `end {\"title\":\"expect.toBeTruthy\",\"category\":\"expect\",\"error\":{\"message\":\"Error: \\u001b[2mexpect(\\u001b[22m\\u001b[31mreceived\\u001b[39m\\u001b[2m).\\u001b[22mtoBeTruthy\\u001b[2m()\\u001b[22m\\n\\nReceived: \\u001b[31mfalse\\u001b[39m\",\"stack\":\"<stack>\",\"location\":\"<location>\",\"snippet\":\"<snippet>\"}}`,
         `begin {\"title\":\"After Hooks\",\"category\":\"hook\"}`,
         `end {\"title\":\"After Hooks\",\"category\":\"hook\"}`,
         `begin {\"title\":\"Before Hooks\",\"category\":\"hook\"}`,

tests/playwright-test/test-step.spec.ts

@@ -44,6 +44,15 @@ class Reporter {
     };
   }
 
+  distillError(error) {
+    return {
+      error: {
+        message: stripAnsi(error.message),
+        stack: stripAnsi(error.stack),
+      }
+    };
+  }
+
   onStdOut(data) {
     process.stdout.write(data.toString());
   }
@@ -61,6 +70,9 @@ class Reporter {
           for (const step of result.steps) {
             console.log('%% ' + JSON.stringify(this.distillStep(step)));
           }
+          for (const error of result.errors) {
+            console.log('%% ' + JSON.stringify(this.distillError(error)));
+          }
         }
       }
     };
@@ -249,6 +261,9 @@ test('should report before hooks step error', async ({ runInlineTest }) => {
       category: 'hook',
       title: 'After Hooks',
     },
+    {
+      error: expect.any(Object)
+    }
   ]);
 });
 
@@ -556,6 +571,9 @@ test('should report custom expect steps', async ({ runInlineTest }) => {
       category: 'hook',
       title: 'After Hooks',
     },
+    {
+      error: expect.any(Object)
+    }
   ]);
 });
 
@@ -633,7 +651,8 @@ test('should mark step as failed when soft expect fails', async ({ runInlineTest
       category: 'test.step',
       location: { file: 'a.test.ts', line: expect.any(Number), column: expect.any(Number) }
     },
-    { title: 'After Hooks', category: 'hook' }
+    { title: 'After Hooks', category: 'hook' },
+    { error: expect.any(Object) }
   ]);
 });
 
@@ -1131,6 +1150,13 @@ test('should show final toPass error', async ({ runInlineTest }) => {
       title: 'After Hooks',
       category: 'hook',
     },
+    {
+      error: {
+        message: expect.stringContaining('Error: expect(received).toBe(expected)'),
+        stack: expect.stringContaining('a.test.ts:6'),
+      }
+    }
+
   ]);
 });
 
@@ -1211,6 +1237,18 @@ test('should propagate nested soft errors', async ({ runInlineTest }) => {
       category: 'hook',
       title: 'After Hooks',
     },
+    {
+      error: {
+        message: expect.stringContaining('Error: expect(received).toBe(expected)'),
+        stack: expect.stringContaining('a.test.ts:6'),
+      }
+    },
+    {
+      error: {
+        message: expect.stringContaining('Error: expect(received).toBe(expected)'),
+        stack: expect.stringContaining('a.test.ts:12'),
+      }
+    }
   ]);
 });
 
@@ -1292,6 +1330,12 @@ test('should not propagate nested hard errors', async ({ runInlineTest }) => {
       category: 'hook',
       title: 'After Hooks',
     },
+    {
+      error: {
+        message: expect.stringContaining('Error: expect(received).toBe(expected)'),
+        stack: expect.stringContaining('a.test.ts:13'),
+      }
+    }
   ]);
 });
 
@@ -1342,6 +1386,12 @@ test('should step w/o box', async ({ runInlineTest }) => {
       category: 'hook',
       title: 'After Hooks',
     },
+    {
+      error: {
+        message: expect.stringContaining('Error: expect(received).toBe(expected)'),
+        stack: expect.stringContaining('a.test.ts:3'),
+      }
+    }
   ]);
 });
 
@@ -1385,6 +1435,12 @@ test('should step w/ box', async ({ runInlineTest }) => {
       category: 'hook',
       title: 'After Hooks',
     },
+    {
+      error: {
+        message: expect.stringContaining('expect(received).toBe(expected)'),
+        stack: expect.not.stringMatching(/a.test.ts:[^8]/),
+      }
+    }
   ]);
 });
 
@@ -1428,6 +1484,12 @@ test('should soft step w/ box', async ({ runInlineTest }) => {
       category: 'hook',
       title: 'After Hooks',
     },
+    {
+      error: {
+        message: expect.stringContaining('Error: expect(received).toBe(expected)'),
+        stack: expect.not.stringMatching(/a.test.ts:[^8]/),
+      }
+    }
   ]);
 });