fix(test runner): after hooks step should not be nested (#8969)

src/test/dispatcher.ts

@@ -297,7 +297,7 @@ export class Dispatcher {
         return;
       }
       const { test, result, steps, stepStack } = this._testById.get(params.testId)!;
-      const parentStep = [...stepStack].pop();
+      const parentStep = params.forceNoParent ? undefined : [...stepStack].pop();
       const step: TestStep = {
         title: params.title,
         titlePath: () => {

src/test/expect.ts

@@ -75,7 +75,12 @@ function wrap(matcherName: string, matcher: any) {
 
     const INTERNAL_STACK_LENGTH = 3;
     const stackLines = new Error().stack!.split('\n').slice(INTERNAL_STACK_LENGTH + 1);
-    const step = testInfo._addStep('expect', `expect${this.isNot ? '.not' : ''}.${matcherName}`, true);
+    const step = testInfo._addStep({
+      category: 'expect',
+      title: `expect${this.isNot ? '.not' : ''}.${matcherName}`,
+      canHaveChildren: true,
+      forceNoParent: false
+    });
 
     const reportStepEnd = (result: any) => {
       const success = result.pass !== this.isNot;

src/test/index.ts

@@ -212,8 +212,12 @@ export const test = _baseTest.extend<TestFixtures, WorkerAndFileFixtures>({
       }
       (context as any)._csi = {
         onApiCallBegin: (apiCall: string) => {
-          const testInfoImpl = testInfo as any;
+          const step = (testInfo as any)._addStep({
-          const step = testInfoImpl._addStep('pw:api', apiCall, false);
+            category: 'pw:api',
+            title: apiCall,
+            canHaveChildren: false,
+            forceNoParent: false,
+          });
           return { userObject: step };
         },
         onApiCallEnd: (data: { userObject: any }, error?: Error) => {

src/test/ipc.ts

@@ -52,6 +52,7 @@ export type StepBeginPayload = {
   title: string;
   category: string;
   canHaveChildren: boolean;
+  forceNoParent: boolean;
   wallTime: number;  // milliseconds since unix epoch
 };
 

src/test/testType.ts

@@ -186,7 +186,12 @@ export class TestTypeImpl {
     const testInfo = currentTestInfo();
     if (!testInfo)
       throw errorWithLocation(location, `test.step() can only be called from a test`);
-    const step = testInfo._addStep('test.step', title, true);
+    const step = testInfo._addStep({
+      category: 'test.step',
+      title,
+      canHaveChildren: true,
+      forceNoParent: false
+    });
     try {
       await body();
       step.complete();

src/test/types.ts

@@ -27,11 +27,13 @@ export type Annotations = { type: string, description?: string }[];
 
 export interface TestStepInternal {
   complete(error?: Error | TestError): void;
+  title: string;
   category: string;
   canHaveChildren: boolean;
+  forceNoParent: boolean;
 }
 
 export interface TestInfoImpl extends TestInfo {
   _testFinished: Promise<void>;
-  _addStep: (category: string, title: string, canHaveChildren: boolean) => TestStepInternal;
+  _addStep: (data: Omit<TestStepInternal, 'complete'>) => TestStepInternal;
 }

src/test/workerRunner.ts

@@ -267,12 +267,11 @@ export class WorkerRunner extends EventEmitter {
           deadlineRunner.updateDeadline(deadline());
       },
       _testFinished: new Promise(f => testFinishedCallback = f),
-      _addStep: (category: string, title: string, canHaveChildren: boolean) => {
+      _addStep: data => {
-        const stepId = `${category}@${title}@${++lastStepId}`;
+        const stepId = `${data.category}@${data.title}@${++lastStepId}`;
         let callbackHandled = false;
         const step: TestStepInternal = {
-          category,
+          ...data,
-          canHaveChildren,
           complete: (error?: Error | TestError) => {
             if (callbackHandled)
               return;
@@ -292,9 +291,7 @@ export class WorkerRunner extends EventEmitter {
         const payload: StepBeginPayload = {
           testId,
           stepId,
-          category,
+          ...data,
-          canHaveChildren,
-          title,
           wallTime: Date.now(),
         };
         if (reportEvents)
@@ -423,7 +420,12 @@ export class WorkerRunner extends EventEmitter {
   }
 
   private async _runTestWithBeforeHooks(test: TestCase, testInfo: TestInfoImpl) {
-    const step = testInfo._addStep('hook', 'Before Hooks', true);
+    const step = testInfo._addStep({
+      category: 'hook',
+      title: 'Before Hooks',
+      canHaveChildren: true,
+      forceNoParent: true
+    });
     if (test._type === 'test')
       await this._runBeforeHooks(test, testInfo);
 
@@ -457,7 +459,12 @@ export class WorkerRunner extends EventEmitter {
     let step: TestStepInternal | undefined;
     let teardownError: TestError | undefined;
     try {
-      step = testInfo._addStep('hook', 'After Hooks', true);
+      step = testInfo._addStep({
+        category: 'hook',
+        title: 'After Hooks',
+        canHaveChildren: true,
+        forceNoParent: true
+      });
       if (test._type === 'test')
         await this._runHooks(test.parent!, 'afterEach', testInfo);
     } catch (error) {

tests/config/browserTest.ts

@@ -143,7 +143,12 @@ export const playwrightFixtures: Fixtures<PlaywrightTestOptions & PlaywrightTest
       (context as any)._csi = {
         onApiCallBegin: (apiCall: string) => {
           const testInfoImpl = testInfo as any;
-          const step = testInfoImpl._addStep('pw:api', apiCall, false);
+          const step = testInfoImpl._addStep({
+            category: 'pw:api',
+            title: apiCall,
+            canHaveChildren: false,
+            forceNoParent: false
+          });
           return { userObject: step };
         },
         onApiCallEnd: (data: { userObject: any }, error?: Error) => {

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

@@ -16,44 +16,44 @@
 
 import { test, expect } from './playwright-test-fixtures';
 
-test('should report api step hierarchy', async ({ runInlineTest }) => {
+const stepHierarchyReporter = `
-  const expectReporterJS = `
+class Reporter {
-    class Reporter {
+  onBegin(config: FullConfig, suite: Suite) {
-      onBegin(config: FullConfig, suite: Suite) {
+    this.suite = suite;
-        this.suite = suite;
+  }
-      }
 
-      distillStep(step) {
+  distillStep(step) {
-        return {
+    return {
-          ...step,
+      ...step,
-          startTime: undefined,
+      startTime: undefined,
-          duration: undefined,
+      duration: undefined,
-          parent: undefined,
+      parent: undefined,
-          data: undefined,
+      data: undefined,
-          steps: step.steps.length ? step.steps.map(s => this.distillStep(s)) : undefined,
+      steps: step.steps.length ? step.steps.map(s => this.distillStep(s)) : undefined,
-        };
+    };
-      }
+  }
 
-      async onEnd() {
+  async onEnd() {
-        const processSuite = (suite: Suite) => {
+    const processSuite = (suite: Suite) => {
-          for (const child of suite.suites)
+      for (const child of suite.suites)
-            processSuite(child);
+        processSuite(child);
-          for (const test of suite.tests) {
+      for (const test of suite.tests) {
-            for (const result of test.results) {
+        for (const result of test.results) {
-              for (const step of result.steps) {
+          for (const step of result.steps) {
-                console.log('%% ' + JSON.stringify(this.distillStep(step)));
+            console.log('%% ' + JSON.stringify(this.distillStep(step)));
-              }
-            }
           }
-        };
+        }
-        processSuite(this.suite);
       }
-    }
+    };
-    module.exports = Reporter;
+    processSuite(this.suite);
-  `;
+  }
+}
+module.exports = Reporter;
+`;
 
+test('should report api step hierarchy', async ({ runInlineTest }) => {
   const result = await runInlineTest({
-    'reporter.ts': expectReporterJS,
+    'reporter.ts': stepHierarchyReporter,
     'playwright.config.ts': `
       module.exports = {
         reporter: './reporter',
@@ -128,6 +128,54 @@ test('should report api step hierarchy', async ({ runInlineTest }) => {
   ]);
 });
 
+test('should not report nested after hooks', async ({ runInlineTest }) => {
+  const result = await runInlineTest({
+    'reporter.ts': stepHierarchyReporter,
+    'playwright.config.ts': `
+      module.exports = {
+        reporter: './reporter',
+      };
+    `,
+    'a.test.ts': `
+      const { test } = pwt;
+      test('timeout', async ({ page }) => {
+        await test.step('my step', async () => {
+          await new Promise(() => {});
+        });
+      });
+    `
+  }, { reporter: '', workers: 1, timeout: 2000 });
+
+  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: 'pw:api',
+          title: 'browserContext.newPage',
+        },
+      ],
+    },
+    {
+      category: 'test.step',
+      title: 'my step',
+    },
+    {
+      category: 'hook',
+      title: 'After Hooks',
+      steps: [
+        {
+          category: 'pw:api',
+          title: 'browserContext.close',
+        },
+      ],
+    },
+  ]);
+});
+
 test('should report test.step from fixtures', async ({ runInlineTest }) => {
   const expectReporterJS = `
     class Reporter {