fix(steps): step should not get an unrelated error (#22773)

Previously, we would use any error that was added during the step
execution as an error for this particular step.

This produces false positives, for example failing `page.click` call
that happened during `context` teardown was producing an error and
marking teardown is failed. However, in reality, the test itself has
failed, while teardown has not.

New approach uses test step hierarchy to inherit errors from child steps
to the parent step. This does not regress the original fix where
`expect.soft` errors are surfaced in the parent step.

See also #19973 that introduced the original logic.

packages/playwright-test/src/worker/testInfo.ts

@@ -33,6 +33,8 @@ interface TestStepInternal {
   category: string;
   wallTime: number;
   location?: Location;
+  steps: TestStepInternal[];
+  error?: TestInfoError;
 }
 
 export class TestInfoImpl implements TestInfo {
@@ -49,6 +51,7 @@ export class TestInfoImpl implements TestInfo {
   _lastStepId = 0;
   readonly _projectInternal: FullProjectInternal;
   readonly _configInternal: FullConfigInternal;
+  readonly _steps: TestStepInternal[] = [];
 
   // ------------ TestInfo fields ------------
   readonly testId: string;
@@ -205,14 +208,14 @@ export class TestInfoImpl implements TestInfo {
     }
   }
 
-  _addStep(data: Omit<TestStepInternal, 'complete' | 'stepId'>): TestStepInternal {
+  _addStep(data: Omit<TestStepInternal, 'complete' | 'stepId' | 'steps'>): TestStepInternal {
     const stepId = `${data.category}@${data.title}@${++this._lastStepId}`;
     const parentStep = zones.zoneData<TestStepInternal>('stepZone', captureRawStack());
     let callbackHandled = false;
-    const firstErrorIndex = this.errors.length;
     const step: TestStepInternal = {
       stepId,
       ...data,
+      steps: [],
       complete: result => {
         if (callbackHandled)
           return;
@@ -225,10 +228,11 @@ export class TestInfoImpl implements TestInfo {
           // 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];
+          // One of the child steps failed (probably soft expect).
+          // Report this step as failed to make it easier to spot.
+          error = step.steps.map(s => s.error).find(e => !!e);
         }
+        step.error = error;
         const payload: StepEndPayload = {
           testId: this._test.id,
           stepId,
@@ -238,6 +242,8 @@ export class TestInfoImpl implements TestInfo {
         this._onStepEnd(payload);
       }
     };
+    const parentStepList = parentStep ? parentStep.steps : this._steps;
+    parentStepList.push(step);
     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;
@@ -275,7 +281,7 @@ export class TestInfoImpl implements TestInfo {
     this.errors.push(error);
   }
 
-  async _runAsStep<T>(stepInfo: Omit<TestStepInternal, 'complete' | 'wallTime' | 'parentStepId' | 'stepId'>, cb: (step: TestStepInternal) => Promise<T>): Promise<T> {
+  async _runAsStep<T>(stepInfo: Omit<TestStepInternal, 'complete' | 'wallTime' | 'parentStepId' | 'stepId' | 'steps'>, cb: (step: TestStepInternal) => Promise<T>): Promise<T> {
     const step = this._addStep({ ...stepInfo, wallTime: Date.now() });
     return await zones.run('stepZone', step, async () => {
       try {

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

@@ -720,3 +720,117 @@ test('should nest steps based on zones', async ({ runInlineTest }) => {
     }
   ]);
 });
+
+test('should not mark page.close as failed when page.click fails', async ({ runInlineTest }) => {
+  const result = await runInlineTest({
+    'reporter.ts': stepHierarchyReporter,
+    'playwright.config.ts': `
+      module.exports = {
+        reporter: './reporter',
+      };
+    `,
+    'a.test.ts': `
+      import { test, expect } from '@playwright/test';
+      let page: Page;
+
+      test.beforeAll(async ({ browser }) => {
+        page = await browser.newPage();
+      });
+
+      test.afterAll(async () => {
+        await page.close();
+      });
+
+      test('fails', async () => {
+        test.setTimeout(2000);
+        await page.setContent('hello');
+        await page.click('div');
+      });
+    `
+  }, { reporter: '' });
+
+  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([
+    {
+      category: 'hook',
+      title: 'Before Hooks',
+      steps: [
+        {
+          category: 'hook',
+          title: 'beforeAll hook',
+          location: {
+            column: 'number',
+            file: 'a.test.ts',
+            line: 'number',
+          },
+          steps: [
+            {
+              category: 'pw:api',
+              title: 'browserType.launch',
+            },
+            {
+              category: 'pw:api',
+              title: 'browser.newPage',
+              location: {
+                column: 'number',
+                file: 'a.test.ts',
+                line: 'number',
+              },
+            },
+          ],
+        },
+      ],
+    },
+    {
+      category: 'pw:api',
+      title: 'page.setContent',
+      location: {
+        column: 'number',
+        file: 'a.test.ts',
+        line: 'number',
+      },
+    },
+    {
+      category: 'pw:api',
+      title: 'page.click(div)',
+      location: {
+        column: 'number',
+        file: 'a.test.ts',
+        line: 'number',
+      },
+      error: '<error>',
+    },
+
+    {
+      category: 'hook',
+      title: 'After Hooks',
+      steps: [
+        {
+          category: 'hook',
+          title: 'afterAll hook',
+          location: {
+            column: 'number',
+            file: 'a.test.ts',
+            line: 'number',
+          },
+          steps: [
+            {
+              category: 'pw:api',
+              title: 'page.close',
+              location: {
+                column: 'number',
+                file: 'a.test.ts',
+                line: 'number',
+              },
+            },
+          ],
+        },
+        {
+          category: 'pw:api',
+          title: 'browser.close',
+        },
+      ],
+    },
+  ]);
+});