feat(soft expect): mark steps with failed soft expects as failed (#19973)

Fixes #19673.

packages/playwright-test/src/testInfo.ts

@@ -18,7 +18,7 @@ import fs from 'fs';
 import path from 'path';
 import { monotonicTime } from 'playwright-core/lib/utils';
 import type { TestInfoError, TestInfo, TestStatus } from '../types/test';
-import type { WorkerInitParams } from './ipc';
+import type { StepBeginPayload, StepEndPayload, WorkerInitParams } from './ipc';
 import type { Loader } from './loader';
 import type { TestCase } from './test';
 import { TimeoutManager } from './timeoutManager';
@@ -26,7 +26,8 @@ import type { Annotation, FullConfigInternal, FullProjectInternal, TestStepInter
 import { getContainedPath, normalizeAndSaveAttachment, sanitizeForFilePath, serializeError, trimLongString } from './util';
 
 export class TestInfoImpl implements TestInfo {
-  private _addStepImpl: (data: Omit<TestStepInternal, 'complete'>) => TestStepInternal;
+  private _onStepBegin: (payload: StepBeginPayload) => void;
+  private _onStepEnd: (payload: StepEndPayload) => void;
   readonly _test: TestCase;
   readonly _timeoutManager: TimeoutManager;
   readonly _startTime: number;
@@ -34,6 +35,7 @@ export class TestInfoImpl implements TestInfo {
   private _hasHardError: boolean = false;
   readonly _onTestFailureImmediateCallbacks = new Map<() => Promise<void>, string>(); // fn -> title
   _didTimeout = false;
+  _lastStepId = 0;
 
   // ------------ TestInfo fields ------------
   readonly repeatEachIndex: number;
@@ -85,10 +87,12 @@ export class TestInfoImpl implements TestInfo {
     workerParams: WorkerInitParams,
     test: TestCase,
     retry: number,
-    addStepImpl: (data: Omit<TestStepInternal, 'complete'>) => TestStepInternal,
+    onStepBegin: (payload: StepBeginPayload) => void,
+    onStepEnd: (payload: StepEndPayload) => void,
   ) {
     this._test = test;
-    this._addStepImpl = addStepImpl;
+    this._onStepBegin = onStepBegin;
+    this._onStepEnd = onStepEnd;
     this._startTime = monotonicTime();
     this._startWallTime = Date.now();
 
@@ -183,8 +187,50 @@ export class TestInfoImpl implements TestInfo {
     }
   }
 
-  _addStep(data: Omit<TestStepInternal, 'complete'>) {
-    return this._addStepImpl(data);
+  _addStep(data: Omit<TestStepInternal, 'complete'>): TestStepInternal {
+    const stepId = `${data.category}@${data.title}@${++this._lastStepId}`;
+    let callbackHandled = false;
+    const firstErrorIndex = this.errors.length;
+    const step: TestStepInternal = {
+      ...data,
+      complete: result => {
+        if (callbackHandled)
+          return;
+        callbackHandled = true;
+        let error: TestInfoError | undefined;
+        if (result.error instanceof Error) {
+          // Step function threw an error.
+          error = serializeError(result.error);
+        } else if (result.error) {
+          // Internal API step reported an error.
+          error = result.error;
+        } else {
+          // There was some other error (porbably soft expect) during step execution.
+          // Report step as failed to make it easier to spot.
+          error = this.errors[firstErrorIndex];
+        }
+        const payload: StepEndPayload = {
+          testId: this._test.id,
+          refinedTitle: step.refinedTitle,
+          stepId,
+          wallTime: Date.now(),
+          error,
+        };
+        this._onStepEnd(payload);
+      }
+    };
+    const hasLocation = data.location && !data.location.file.includes('@playwright');
+    // Sanitize location that comes from user land, it might have extra properties.
+    const location = data.location && hasLocation ? { file: data.location.file, line: data.location.line, column: data.location.column } : undefined;
+    const payload: StepBeginPayload = {
+      testId: this._test.id,
+      stepId,
+      ...data,
+      location,
+      wallTime: Date.now(),
+    };
+    this._onStepBegin(payload);
+    return step;
   }
 
   _failWithError(error: TestInfoError, isHardError: boolean) {

packages/playwright-test/src/workerRunner.ts

@@ -18,11 +18,11 @@ import { colors, rimraf } from 'playwright-core/lib/utilsBundle';
 import util from 'util';
 import { EventEmitter } from 'events';
 import { debugTest, formatLocation, relativeFilePath, serializeError } from './util';
-import type { TestBeginPayload, TestEndPayload, RunPayload, DonePayload, WorkerInitParams, StepBeginPayload, StepEndPayload, TeardownErrorsPayload, WatchTestResolvedPayload } from './ipc';
+import type { TestBeginPayload, TestEndPayload, RunPayload, DonePayload, WorkerInitParams, TeardownErrorsPayload, WatchTestResolvedPayload } from './ipc';
 import { setCurrentTestInfo } from './globals';
 import { Loader } from './loader';
 import type { Suite, TestCase } from './test';
-import type { Annotation, FullProjectInternal, TestInfoError, TestStepInternal } from './types';
+import type { Annotation, FullProjectInternal, TestInfoError } from './types';
 import { FixtureRunner } from './fixtures';
 import { ManualPromise } from 'playwright-core/lib/utils/manualPromise';
 import { TestInfoImpl } from './testInfo';
@@ -226,40 +226,9 @@ export class WorkerRunner extends EventEmitter {
   }
 
   private async _runTest(test: TestCase, retry: number, nextTest: TestCase | undefined) {
-    let lastStepId = 0;
-    const testInfo = new TestInfoImpl(this._loader, this._project, this._params, test, retry, data => {
-      const stepId = `${data.category}@${data.title}@${++lastStepId}`;
-      let callbackHandled = false;
-      const step: TestStepInternal = {
-        ...data,
-        complete: result => {
-          if (callbackHandled)
-            return;
-          callbackHandled = true;
-          const error = result.error instanceof Error ? serializeError(result.error) : result.error;
-          const payload: StepEndPayload = {
-            testId: test.id,
-            refinedTitle: step.refinedTitle,
-            stepId,
-            wallTime: Date.now(),
-            error,
-          };
-          this.emit('stepEnd', payload);
-        }
-      };
-      const hasLocation = data.location && !data.location.file.includes('@playwright');
-      // Sanitize location that comes from user land, it might have extra properties.
-      const location = data.location && hasLocation ? { file: data.location.file, line: data.location.line, column: data.location.column } : undefined;
-      const payload: StepBeginPayload = {
-        testId: test.id,
-        stepId,
-        ...data,
-        location,
-        wallTime: Date.now(),
-      };
-      this.emit('stepBegin', payload);
-      return step;
-    });
+    const testInfo = new TestInfoImpl(this._loader, this._project, this._params, test, retry,
+        stepBeginPayload => this.emit('stepBegin', stepBeginPayload),
+        stepEndPayload => this.emit('stepEnd', stepEndPayload));
 
     const processAnnotation = (annotation: Annotation) => {
       testInfo.annotations.push(annotation);

tests/playwright-test/reporter-html.spec.ts

@@ -482,7 +482,7 @@ test('should warn user when viewing via file:// protocol', async ({ runInlineTes
   });
 });
 
-test('should show timed out steps and hooks', async ({ runInlineTest, page, showReport }) => {
+test('should show failed and timed out steps and hooks', async ({ runInlineTest, page, showReport }) => {
   const result = await runInlineTest({
     'playwright.config.js': `
       module.exports = { timeout: 3000 };
@@ -508,6 +508,12 @@ test('should show timed out steps and hooks', async ({ runInlineTest, page, show
         console.log('afterAll 1');
       });
       test('fails', async ({ page }) => {
+        await test.step('outer error', async () => {
+          await test.step('inner error', async () => {
+            expect.soft(1).toBe(2);
+          });
+        });
+
         await test.step('outer step', async () => {
           await test.step('inner step', async () => {
             await new Promise(() => {});
@@ -521,9 +527,16 @@ test('should show timed out steps and hooks', async ({ runInlineTest, page, show
 
   await showReport();
   await page.click('text=fails');
+
+  await page.click('.tree-item:has-text("outer error") >> text=outer error');
+  await page.click('.tree-item:has-text("outer error") >> .tree-item >> text=inner error');
+  await expect(page.locator('.tree-item:has-text("outer error") svg.color-text-danger')).toHaveCount(3);
+  await expect(page.locator('.tree-item:has-text("expect.soft.toBe"):not(:has-text("inner"))')).toBeVisible();
+
   await page.click('text=outer step');
   await expect(page.locator('.tree-item:has-text("outer step") svg.color-text-danger')).toHaveCount(2);
   await expect(page.locator('.tree-item:has-text("inner step") svg.color-text-danger')).toHaveCount(2);
+
   await page.click('text=Before Hooks');
   await expect(page.locator('.tree-item:has-text("Before Hooks") .tree-item')).toContainText([
     /beforeAll hook/,

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

@@ -35,6 +35,7 @@ class Reporter {
         column: step.location.column ? typeof step.location.column : 0
       } : undefined,
       steps: step.steps.length ? step.steps.map(s => this.distillStep(s)) : undefined,
+      error: step.error ? '<error>' : undefined,
     };
   }
 
@@ -401,3 +402,57 @@ test('should return value from step', async ({ runInlineTest }) => {
   expect(result.output).toContain('v1 = 10');
   expect(result.output).toContain('v2 = 20');
 });
+
+test('should mark step as failed when soft expect fails', async ({ runInlineTest }) => {
+  const result = await runInlineTest({
+    'reporter.ts': stepHierarchyReporter,
+    'playwright.config.ts': `
+      module.exports = {
+        reporter: './reporter',
+      };
+    `,
+    'a.test.ts': `
+      const { test } = pwt;
+      test('pass', async ({}) => {
+        await test.step('outer', async () => {
+          await test.step('inner', async () => {
+            expect.soft(1).toBe(2);
+          });
+        });
+        await test.step('passing', () => {});
+      });
+    `
+  }, { reporter: '', workers: 1 });
+
+  expect(result.exitCode).toBe(1);
+  const objects = result.output.split('\n').filter(line => line.startsWith('%% ')).map(line => line.substring(3).trim()).filter(Boolean).map(line => JSON.parse(line));
+  expect(objects).toEqual([
+    { title: 'Before Hooks', category: 'hook' },
+    {
+      title: 'outer',
+      category: 'test.step',
+      error: '<error>',
+      steps: [{
+        title: 'inner',
+        category: 'test.step',
+        error: '<error>',
+        steps: [
+          {
+            title: 'expect.soft.toBe',
+            category: 'expect',
+            location: { file: 'a.test.ts', line: 'number', column: 'number' },
+            error: '<error>'
+          }
+        ],
+        location: { file: 'a.test.ts', line: 'number', column: 'number' }
+      }],
+      location: { file: 'a.test.ts', line: 'number', column: 'number' }
+    },
+    {
+      title: 'passing',
+      category: 'test.step',
+      location: { file: 'a.test.ts', line: 'number', column: 'number' }
+    },
+    { title: 'After Hooks', category: 'hook' }
+  ]);
+});