chore: do not rely on zones for predefined steps (#29485)

This fixes some edge cases where fixtures and steps inside them were
attached to the wrong parent (see the new test).
This will also allow to replace some `runAsStep` calls with a flat list
of tasks to do that do not rely on lexical scope.

packages/playwright/src/index.ts

@@ -256,7 +256,6 @@ const playwrightFixtures: Fixtures<TestFixtures, WorkerFixtures> = ({
           apiName,
           params,
           wallTime,
-          laxParent: true,
         });
         userData.userObject = step;
       },

packages/playwright/src/matchers/expect.ts

@@ -267,7 +267,6 @@ class ExpectMetaInfoProxyHandler implements ProxyHandler<any> {
         params: args[0] ? { expected: args[0] } : undefined,
         wallTime,
         infectParentStepsWithError: this._info.isSoft,
-        laxParent: true,
         isSoft: this._info.isSoft,
       };
 

packages/playwright/src/worker/fixtureRunner.ts

@@ -99,7 +99,7 @@ class Fixture {
           title: `fixture: ${this.registration.name}`,
           category: 'fixture',
           location: isInternalFixture ? this.registration.location : undefined,
-        }, testInfo._afterHooksStep);
+        });
         mutableStepOnStack!.stepId = afterStep.stepId;
       }
     };

packages/playwright/src/worker/testInfo.ts

@@ -33,12 +33,11 @@ export interface TestStepInternal {
   complete(result: { error?: Error, attachments?: Attachment[] }): void;
   stepId: string;
   title: string;
-  category: string;
+  category: 'hook' | 'fixture' | 'test.step' | string;
   wallTime: number;
   location?: Location;
   boxedStack?: StackFrame[];
   steps: TestStepInternal[];
-  laxParent?: boolean;
   endWallTime?: number;
   apiName?: string;
   params?: Record<string, any>;
@@ -46,6 +45,7 @@ export interface TestStepInternal {
   infectParentStepsWithError?: boolean;
   box?: boolean;
   isSoft?: boolean;
+  forceNoParent?: boolean;
 }
 
 export class TestInfoImpl implements TestInfo {
@@ -65,8 +65,6 @@ export class TestInfoImpl implements TestInfo {
   readonly _projectInternal: FullProjectInternal;
   readonly _configInternal: FullConfigInternal;
   readonly _steps: TestStepInternal[] = [];
-  _beforeHooksStep: TestStepInternal | undefined;
-  _afterHooksStep: TestStepInternal | undefined;
   _onDidFinishTestFunction: (() => Promise<void>) | undefined;
 
   _hasNonRetriableError = false;
@@ -250,24 +248,40 @@ export class TestInfoImpl implements TestInfo {
     }
   }
 
-  _addStep(data: Omit<TestStepInternal, 'complete' | 'stepId' | 'steps'>, parentStep?: TestStepInternal): TestStepInternal {
+  private _findLastNonFinishedStep(filter: (step: TestStepInternal) => boolean) {
+    let result: TestStepInternal | undefined;
+    const visit = (step: TestStepInternal) => {
+      if (!step.endWallTime && filter(step))
+        result = step;
+      step.steps.forEach(visit);
+    };
+    this._steps.forEach(visit);
+    return result;
+  }
+
+  _addStep(data: Omit<TestStepInternal, 'complete' | 'stepId' | 'steps'>): TestStepInternal {
     const stepId = `${data.category}@${++this._lastStepId}`;
     const rawStack = captureRawStack();
-    if (!parentStep)
-      parentStep = zones.zoneData<TestStepInternal>('stepZone', rawStack!) || undefined;
 
-    // For out-of-stack calls, locate the enclosing step.
+    let parentStep: TestStepInternal | undefined;
-    let isLaxParent = false;
+    if (data.category === 'hook' || data.category === 'fixture') {
-    if (!parentStep && data.laxParent) {
+      // Predefined steps form a fixed hierarchy - find the last non-finished one.
-      const visit = (step: TestStepInternal) => {
+      parentStep = this._findLastNonFinishedStep(step => step.category === 'fixture' || step.category === 'hook');
-        // Do not nest chains of route.continue.
+    } else {
-        const shouldNest = step.title !== data.title;
+      parentStep = zones.zoneData<TestStepInternal>('stepZone', rawStack!) || undefined;
-        if (!step.endWallTime && shouldNest)
+      if (!parentStep) {
-          parentStep = step;
+        if (data.category === 'test.step') {
-        step.steps.forEach(visit);
+          // Nest test.step without a good stack in the last non-finished predefined step like a hook.
-      };
+          parentStep = this._findLastNonFinishedStep(step => step.category === 'fixture' || step.category === 'hook');
-      this._steps.forEach(visit);
+        } else {
-      isLaxParent = !!parentStep;
+          // Do not nest chains of route.continue.
+          parentStep = this._findLastNonFinishedStep(step => step.title !== data.title);
+        }
+      }
+    }
+    if (data.forceNoParent) {
+      // This is used to reset step hierarchy after test timeout.
+      parentStep = undefined;
     }
 
     const filteredStack = filteredStackTrace(rawStack);
@@ -281,7 +295,6 @@ export class TestInfoImpl implements TestInfo {
     const step: TestStepInternal = {
       stepId,
       ...data,
-      laxParent: isLaxParent,
       steps: [],
       complete: result => {
         if (step.endWallTime)
@@ -414,7 +427,6 @@ export class TestInfoImpl implements TestInfo {
       title: `attach "${name}"`,
       category: 'attach',
       wallTime: Date.now(),
-      laxParent: true,
     });
     this._attachmentsPush(attachment);
     this._onAttach({

packages/playwright/src/worker/workerMain.ts

@@ -358,8 +358,6 @@ export class WorkerMain extends ProcessRunner {
 
       let testFunctionParams: object | null = null;
       await testInfo._runAsStep({ category: 'hook', title: 'Before Hooks' }, async step => {
-        testInfo._beforeHooksStep = step;
-
         // Run "beforeAll" hooks, unless already run during previous tests.
         for (const suite of suites) {
           didFailBeforeAllForSuite = suite;  // Assume failure, unless reset below.
@@ -416,8 +414,7 @@ export class WorkerMain extends ProcessRunner {
 
     // A timed-out test gets a full additional timeout to run after hooks.
     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 => {
+    await testInfo._runAsStepWithRunnable({ category: 'hook', title: 'After Hooks', runnableType: 'afterHooks', runnableSlot: afterHooksSlot, forceNoParent: true }, async step => {
-      testInfo._afterHooksStep = step;
       let firstAfterHooksError: Error | undefined;
       await testInfo._runWithTimeout(async () => {
         // Note: do not wrap all teardown steps together, because failure in any of them

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

@@ -330,8 +330,8 @@ test('should not override trace file in afterAll', async ({ runInlineTest, serve
     '    fixture: request',
     '      apiRequest.newContext',
     '    apiRequestContext.get',
-    '  fixture: request',
+    '    fixture: request',
-    '    apiRequestContext.dispose',
+    '      apiRequestContext.dispose',
     '  fixture: browser',
   ]);
   expect(trace1.errors).toEqual([`'oh no!'`]);
@@ -836,3 +836,155 @@ test('should not throw when merging traces multiple times', async ({ runInlineTe
   expect(result.passed).toBe(1);
   expect(fs.existsSync(testInfo.outputPath('test-results', 'a-foo', 'trace.zip'))).toBe(true);
 });
+
+test('should record nested steps, even after timeout', async ({ runInlineTest }, testInfo) => {
+  const result = await runInlineTest({
+    'playwright.config.ts': `
+      module.exports = {
+        use: { trace: { mode: 'on' } },
+        timeout: 5000,
+      };
+    `,
+    'a.spec.ts': `
+      import { test as base, expect } from '@playwright/test';
+      const test = base.extend({
+        fooPage: async ({ page }, use) => {
+          expect(1, 'fooPage setup').toBe(1);
+          await new Promise(f => setTimeout(f, 1));  // To avoid same-wall-time sorting issues.
+          await page.setContent('hello');
+          await test.step('step in fooPage setup', async () => {
+            await page.setContent('bar');
+          });
+          await use(page);
+          expect(1, 'fooPage teardown').toBe(1);
+          await new Promise(f => setTimeout(f, 1));  // To avoid same-wall-time sorting issues.
+          await page.setContent('hi');
+          await test.step('step in fooPage teardown', async () => {
+            await page.setContent('bar');
+          });
+        },
+        barPage: async ({ browser }, use) => {
+          expect(1, 'barPage setup').toBe(1);
+          await new Promise(f => setTimeout(f, 1));  // To avoid same-wall-time sorting issues.
+          const page = await browser.newPage();
+          await test.step('step in barPage setup', async () => {
+            await page.setContent('bar');
+          });
+          await use(page);
+          expect(1, 'barPage teardown').toBe(1);
+          await new Promise(f => setTimeout(f, 1));  // To avoid same-wall-time sorting issues.
+          await test.step('step in barPage teardown', async () => {
+            await page.close();
+          });
+        },
+      });
+
+      test.beforeAll(async ({ barPage }) => {
+        expect(1, 'beforeAll start').toBe(1);
+        await new Promise(f => setTimeout(f, 1));  // To avoid same-wall-time sorting issues.
+        await barPage.setContent('hello');
+        await test.step('step in beforeAll', async () => {
+          await barPage.setContent('bar');
+        });
+      });
+
+      test.beforeEach(async ({ fooPage }) => {
+        expect(1, 'beforeEach start').toBe(1);
+        await new Promise(f => setTimeout(f, 1));  // To avoid same-wall-time sorting issues.
+        await fooPage.setContent('hello');
+        await test.step('step in beforeEach', async () => {
+          await fooPage.setContent('hi');
+          // Next line times out. We make sure that after hooks steps
+          // form the expected step tree even when some previous steps have not finished.
+          await new Promise(() => {});
+        });
+      });
+
+      test('example', async ({ fooPage }) => {
+      });
+
+      test.afterEach(async ({ fooPage }) => {
+        expect(1, 'afterEach start').toBe(1);
+        await new Promise(f => setTimeout(f, 1));  // To avoid same-wall-time sorting issues.
+        await fooPage.setContent('hello');
+        await test.step('step in afterEach', async () => {
+          await fooPage.setContent('bar');
+        });
+      });
+
+      test.afterAll(async ({ barPage }) => {
+        expect(1, 'afterAll start').toBe(1);
+        await new Promise(f => setTimeout(f, 1));  // To avoid same-wall-time sorting issues.
+        await barPage.setContent('hello');
+        await test.step('step in afterAll', async () => {
+          await barPage.setContent('bar');
+        });
+      });
+    `,
+  }, { workers: 1 });
+
+  expect(result.exitCode).toBe(1);
+  expect(result.failed).toBe(1);
+  const trace = await parseTrace(testInfo.outputPath('test-results', 'a-example', 'trace.zip'));
+  expect(trace.actionTree).toEqual([
+    'Before Hooks',
+    '  beforeAll hook',
+    '    fixture: browser',
+    '      browserType.launch',
+    '    fixture: barPage',
+    '      barPage setup',
+    '      browser.newPage',
+    '      step in barPage setup',
+    '        page.setContent',
+    '    beforeAll start',
+    '    page.setContent',
+    '    step in beforeAll',
+    '      page.setContent',
+    '    fixture: barPage',
+    '      barPage teardown',
+    '      step in barPage teardown',
+    '        page.close',
+    '  beforeEach hook',
+    '    fixture: context',
+    '      browser.newContext',
+    '    fixture: page',
+    '      browserContext.newPage',
+    '    fixture: fooPage',
+    '      fooPage setup',
+    '      page.setContent',
+    '      step in fooPage setup',
+    '        page.setContent',
+    '    beforeEach start',
+    '    page.setContent',
+    '    step in beforeEach',
+    '      page.setContent',
+    'After Hooks',
+    '  afterEach hook',
+    '    afterEach start',
+    '    page.setContent',
+    '    step in afterEach',
+    '      page.setContent',
+    '  fixture: fooPage',
+    '    fooPage teardown',
+    '    page.setContent',
+    '    step in fooPage teardown',
+    '      page.setContent',
+    '  fixture: page',
+    '  fixture: context',
+    '  afterAll hook',
+    '    fixture: barPage',
+    '      barPage setup',
+    '      browser.newPage',
+    '      step in barPage setup',
+    '        page.setContent',
+    '    afterAll start',
+    '    page.setContent',
+    '    step in afterAll',
+    '      page.setContent',
+    '    fixture: barPage',
+    '      barPage teardown',
+    '      step in barPage teardown',
+    '        page.close',
+    '  fixture: browser',
+  ]);
+});