chore: separate concerns of testinfo helpers (#22491)

packages/playwright-test/src/common/testType.ts

@@ -213,10 +213,9 @@ export class TestTypeImpl {
     const testInfo = currentTestInfo();
     if (!testInfo)
       throw new Error(`test.step() can only be called from a test`);
-    return testInfo._runAsStep(body, {
+    return testInfo._runAsStep({ category: 'test.step', title, location }, async step => {
-      category: 'test.step',
+      // Make sure that internal "step" is not leaked to the user callback.
-      title,
+      return await body();
-      location,
     });
   }
 

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

@@ -193,21 +193,15 @@ export class TestInfoImpl implements TestInfo {
     this.duration = this._timeoutManager.defaultSlotTimings().elapsed | 0;
   }
 
-  async _runFn(fn: () => Promise<void>, skips?: 'allowSkips', stepInfo?: Omit<TestStepInternal, 'complete' | 'wallTime' | 'parentStepId' | 'stepId'>): Promise<TestInfoError | undefined> {
+  async _runAndFailOnError(fn: () => Promise<void>, skips?: 'allowSkips'): Promise<TestInfoError | undefined> {
-    const step = stepInfo ? this._addStep({ ...stepInfo, wallTime: Date.now() }) : undefined;
     try {
-      if (step)
+      await fn();
-        await zones.run('stepZone', step, fn);
-      else
-        await fn();
-      step?.complete({});
     } catch (error) {
       if (skips === 'allowSkips' && error instanceof SkipError) {
         if (this.status === 'passed')
           this.status = 'skipped';
       } else {
         const serialized = serializeError(error);
-        step?.complete({ error: serialized });
         this._failWithError(serialized, true /* isHardError */);
         return serialized;
       }
@@ -285,7 +279,7 @@ export class TestInfoImpl implements TestInfo {
     this.errors.push(error);
   }
 
-  async _runAsStep<T>(cb: (step: TestStepInternal) => Promise<T>, stepInfo: Omit<TestStepInternal, 'complete' | 'wallTime' | 'parentStepId' | 'stepId'>): Promise<T> {
+  async _runAsStep<T>(stepInfo: Omit<TestStepInternal, 'complete' | 'wallTime' | 'parentStepId' | 'stepId'>, cb: (step: TestStepInternal) => Promise<T>): Promise<T> {
     const step = this._addStep({ ...stepInfo, wallTime: Date.now() });
     return await zones.run('stepZone', step, async () => {
       try {

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

@@ -320,57 +320,58 @@ export class WorkerMain extends ProcessRunner {
         return;
       }
 
-      let params: object | null = null;
+      let testFunctionParams: object | null = null;
-      // Note: wrap all preparation steps together, because failure/skip in any of them
+      await testInfo._runAsStep({ category: 'hook', title: 'Before Hooks' }, async step => {
-      // prevents further setup and/or test from running.
+        // Note: wrap all preparation steps together, because failure/skip in any of them
-      await testInfo._runFn(async () => {
+        // prevents further setup and/or test from running.
-        // Run "beforeAll" modifiers on parent suites, unless already run during previous tests.
+        const beforeHooksError = await testInfo._runAndFailOnError(async () => {
-        for (const suite of suites) {
+          // Run "beforeAll" modifiers on parent suites, unless already run during previous tests.
-          if (this._extraSuiteAnnotations.has(suite))
+          for (const suite of suites) {
-            continue;
+            if (this._extraSuiteAnnotations.has(suite))
-          const extraAnnotations: Annotation[] = [];
+              continue;
-          this._extraSuiteAnnotations.set(suite, extraAnnotations);
+            const extraAnnotations: Annotation[] = [];
-          didFailBeforeAllForSuite = suite;  // Assume failure, unless reset below.
+            this._extraSuiteAnnotations.set(suite, extraAnnotations);
-          // Separate timeout for each "beforeAll" modifier.
+            didFailBeforeAllForSuite = suite;  // Assume failure, unless reset below.
-          const timeSlot = { timeout: this._project.project.timeout, elapsed: 0 };
+            // Separate timeout for each "beforeAll" modifier.
-          await this._runModifiersForSuite(suite, testInfo, 'worker', timeSlot, extraAnnotations);
+            const timeSlot = { timeout: this._project.project.timeout, elapsed: 0 };
-        }
+            await this._runModifiersForSuite(suite, testInfo, 'worker', timeSlot, extraAnnotations);
+          }
 
-        // Run "beforeAll" hooks, unless already run during previous tests.
+          // Run "beforeAll" hooks, unless already run during previous tests.
-        for (const suite of suites) {
+          for (const suite of suites) {
-          didFailBeforeAllForSuite = suite;  // Assume failure, unless reset below.
+            didFailBeforeAllForSuite = suite;  // Assume failure, unless reset below.
-          await this._runBeforeAllHooksForSuite(suite, testInfo);
+            await this._runBeforeAllHooksForSuite(suite, testInfo);
-        }
+          }
 
-        // Running "beforeAll" succeeded for all suites!
+          // Running "beforeAll" succeeded for all suites!
-        didFailBeforeAllForSuite = undefined;
+          didFailBeforeAllForSuite = undefined;
 
-        // Run "beforeEach" modifiers.
+          // Run "beforeEach" modifiers.
-        for (const suite of suites)
+          for (const suite of suites)
-          await this._runModifiersForSuite(suite, testInfo, 'test', undefined);
+            await this._runModifiersForSuite(suite, testInfo, 'test', undefined);
 
-        // Run "beforeEach" hooks. Once started with "beforeEach", we must run all "afterEach" hooks as well.
+          // Run "beforeEach" hooks. Once started with "beforeEach", we must run all "afterEach" hooks as well.
-        shouldRunAfterEachHooks = true;
+          shouldRunAfterEachHooks = true;
-        await this._runEachHooksForSuites(suites, 'beforeEach', testInfo, undefined);
+          await this._runEachHooksForSuites(suites, 'beforeEach', testInfo, undefined);
 
-        // Setup fixtures required by the test.
+          // Setup fixtures required by the test.
-        testInfo._timeoutManager.setCurrentRunnable({ type: 'test' });
+          testInfo._timeoutManager.setCurrentRunnable({ type: 'test' });
-        params = await this._fixtureRunner.resolveParametersForFunction(test.fn, testInfo, 'test');
+          testFunctionParams = await this._fixtureRunner.resolveParametersForFunction(test.fn, testInfo, 'test');
-      }, 'allowSkips', {
+        }, 'allowSkips');
-        category: 'hook',
+        if (beforeHooksError)
-        title: 'Before Hooks',
+          step.complete({ error: beforeHooksError });
       });
 
-      if (params === null) {
+      if (testFunctionParams === null) {
         // Fixture setup failed, we should not run the test now.
         return;
       }
 
-      await testInfo._runFn(async () => {
+      await testInfo._runAndFailOnError(async () => {
         // Now run the test itself.
         debugTest(`test function started`);
         const fn = test.fn; // Extract a variable to get a better stack trace ("myTest" vs "TestCase.myTest [as fn]").
-        await fn(params, testInfo);
+        await fn(testFunctionParams, testInfo);
         debugTest(`test function finished`);
       }, 'allowSkips');
     });
@@ -388,7 +389,7 @@ export class WorkerMain extends ProcessRunner {
       afterHooksSlot = { timeout: this._project.project.timeout, elapsed: 0 };
       testInfo._timeoutManager.setCurrentRunnable({ type: 'afterEach', slot: afterHooksSlot });
     }
-    await testInfo._runAsStep(async step => {
+    await testInfo._runAsStep({ category: 'hook', title: 'After Hooks' }, async step => {
       let firstAfterHooksError: TestInfoError | undefined;
       await testInfo._runWithTimeout(async () => {
         // Note: do not wrap all teardown steps together, because failure in any of them
@@ -396,14 +397,11 @@ export class WorkerMain extends ProcessRunner {
 
         // Run "immediately upon test failure" callbacks.
         if (testInfo._isFailure()) {
-          const onFailureError = await testInfo._runFn(async () => {
+          const onFailureError = await testInfo._runAndFailOnError(async () => {
             testInfo._timeoutManager.setCurrentRunnable({ type: 'test', slot: afterHooksSlot });
             for (const [fn, title] of testInfo._onTestFailureImmediateCallbacks) {
               debugTest(`on-failure callback started`);
-              await testInfo._runAsStep(fn, {
+              await testInfo._runAsStep({ category: 'hook', title }, fn);
-                category: 'hook',
-                title,
-              });
               debugTest(`on-failure callback finished`);
             }
           });
@@ -412,7 +410,7 @@ export class WorkerMain extends ProcessRunner {
 
         // Run "afterEach" hooks, unless we failed at beforeAll stage.
         if (shouldRunAfterEachHooks) {
-          const afterEachError = await testInfo._runFn(() => this._runEachHooksForSuites(reversedSuites, 'afterEach', testInfo, afterHooksSlot));
+          const afterEachError = await testInfo._runAndFailOnError(() => this._runEachHooksForSuites(reversedSuites, 'afterEach', testInfo, afterHooksSlot));
           firstAfterHooksError = firstAfterHooksError || afterEachError;
         }
 
@@ -430,7 +428,7 @@ export class WorkerMain extends ProcessRunner {
         // they should probably increate the test timeout to fix this issue.
         testInfo._timeoutManager.setCurrentRunnable({ type: 'test', slot: afterHooksSlot });
         debugTest(`tearing down test scope started`);
-        const testScopeError = await testInfo._runFn(() => this._fixtureRunner.teardownScope('test', testInfo._timeoutManager));
+        const testScopeError = await testInfo._runAndFailOnError(() => this._fixtureRunner.teardownScope('test', testInfo._timeoutManager));
         debugTest(`tearing down test scope finished`);
         firstAfterHooksError = firstAfterHooksError || testScopeError;
       });
@@ -454,22 +452,19 @@ export class WorkerMain extends ProcessRunner {
           // Attribute to 'test' so that users understand they should probably increate the test timeout to fix this issue.
           testInfo._timeoutManager.setCurrentRunnable({ type: 'test', slot: teardownSlot });
           debugTest(`tearing down test scope started`);
-          const testScopeError = await testInfo._runFn(() => this._fixtureRunner.teardownScope('test', testInfo._timeoutManager));
+          const testScopeError = await testInfo._runAndFailOnError(() => this._fixtureRunner.teardownScope('test', testInfo._timeoutManager));
           debugTest(`tearing down test scope finished`);
           firstAfterHooksError = firstAfterHooksError || testScopeError;
           // Attribute to 'teardown' because worker fixtures are not perceived as a part of a test.
           testInfo._timeoutManager.setCurrentRunnable({ type: 'teardown', slot: teardownSlot });
           debugTest(`tearing down worker scope started`);
-          const workerScopeError = await testInfo._runFn(() => this._fixtureRunner.teardownScope('worker', testInfo._timeoutManager));
+          const workerScopeError = await testInfo._runAndFailOnError(() => this._fixtureRunner.teardownScope('worker', testInfo._timeoutManager));
           debugTest(`tearing down worker scope finished`);
           firstAfterHooksError = firstAfterHooksError || workerScopeError;
         });
       }
       if (firstAfterHooksError)
         step.complete({ error: firstAfterHooksError });
-    }, {
-      category: 'hook',
-      title: 'After Hooks',
     });
 
     this._currentTest = null;
@@ -489,11 +484,11 @@ export class WorkerMain extends ProcessRunner {
         continue;
       debugTest(`modifier at "${formatLocation(modifier.location)}" started`);
       testInfo._timeoutManager.setCurrentRunnable({ type: modifier.type, location: modifier.location, slot: timeSlot });
-      const result = await testInfo._runAsStep(() => this._fixtureRunner.resolveParametersAndRunFunction(modifier.fn, testInfo, scope), {
+      const result = await testInfo._runAsStep({
         category: 'hook',
         title: `${modifier.type} modifier`,
         location: modifier.location,
-      });
+      }, () => this._fixtureRunner.resolveParametersAndRunFunction(modifier.fn, testInfo, scope));
       debugTest(`modifier at "${formatLocation(modifier.location)}" finished`);
       if (result && extraAnnotations)
         extraAnnotations.push({ type: modifier.type, description: modifier.description });
@@ -514,11 +509,11 @@ export class WorkerMain extends ProcessRunner {
         // Separate time slot for each "beforeAll" hook.
         const timeSlot = { timeout: this._project.project.timeout, elapsed: 0 };
         testInfo._timeoutManager.setCurrentRunnable({ type: 'beforeAll', location: hook.location, slot: timeSlot });
-        await testInfo._runAsStep(() => this._fixtureRunner.resolveParametersAndRunFunction(hook.fn, testInfo, 'all-hooks-only'), {
+        await testInfo._runAsStep({
           category: 'hook',
           title: `${hook.type} hook`,
           location: hook.location,
-        });
+        }, () => this._fixtureRunner.resolveParametersAndRunFunction(hook.fn, testInfo, 'all-hooks-only'));
       } catch (e) {
         // Always run all the hooks, and capture the first error.
         beforeAllError = beforeAllError || e;
@@ -538,15 +533,15 @@ export class WorkerMain extends ProcessRunner {
       if (hook.type !== 'afterAll')
         continue;
       debugTest(`${hook.type} hook at "${formatLocation(hook.location)}" started`);
-      const afterAllError = await testInfo._runFn(async () => {
+      const afterAllError = await testInfo._runAndFailOnError(async () => {
         // Separate time slot for each "afterAll" hook.
         const timeSlot = { timeout: this._project.project.timeout, elapsed: 0 };
         testInfo._timeoutManager.setCurrentRunnable({ type: 'afterAll', location: hook.location, slot: timeSlot });
-        await testInfo._runAsStep(() => this._fixtureRunner.resolveParametersAndRunFunction(hook.fn, testInfo, 'all-hooks-only'), {
+        await testInfo._runAsStep({
           category: 'hook',
           title: `${hook.type} hook`,
           location: hook.location,
-        });
+        }, () => this._fixtureRunner.resolveParametersAndRunFunction(hook.fn, testInfo, 'all-hooks-only'));
       });
       firstError = firstError || afterAllError;
       debugTest(`${hook.type} hook at "${formatLocation(hook.location)}" finished`);
@@ -560,11 +555,11 @@ export class WorkerMain extends ProcessRunner {
     for (const hook of hooks) {
       try {
         testInfo._timeoutManager.setCurrentRunnable({ type, location: hook.location, slot: timeSlot });
-        await testInfo._runAsStep(() => this._fixtureRunner.resolveParametersAndRunFunction(hook.fn, testInfo, 'test'), {
+        await testInfo._runAsStep({
           category: 'hook',
           title: `${hook.type} hook`,
           location: hook.location,
-        });
+        }, () => this._fixtureRunner.resolveParametersAndRunFunction(hook.fn, testInfo, 'test'));
       } catch (e) {
         // Always run all the hooks, and capture the first error.
         error = error || e;

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

@@ -39,6 +39,14 @@ class Reporter {
     };
   }
 
+  onStdOut(data) {
+    process.stdout.write(data.toString());
+  }
+
+  onStdErr(data) {
+    process.stderr.write(data.toString());
+  }
+
   async onEnd() {
     const processSuite = (suite: Suite) => {
       for (const child of suite.suites)
@@ -164,6 +172,51 @@ test('should report api step hierarchy', async ({ runInlineTest }) => {
   ]);
 });
 
+test('should report before hooks step error', 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';
+      test.beforeEach(async ({}) => {
+        throw new Error('oh my');
+      });
+      test('pass', async ({}) => {
+      });
+    `
+  }, { 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([
+    {
+      category: 'hook',
+      title: 'Before Hooks',
+      error: '<error>',
+      steps: [
+        {
+          category: 'hook',
+          title: 'beforeEach hook',
+          error: '<error>',
+          location: {
+            column: 'number',
+            file: 'a.test.ts',
+            line: 'number',
+          },
+        }
+      ],
+    },
+    {
+      category: 'hook',
+      title: 'After Hooks',
+    },
+  ]);
+});
+
 test('should not report nested after hooks', async ({ runInlineTest }) => {
   const result = await runInlineTest({
     'reporter.ts': stepHierarchyReporter,
@@ -382,16 +435,18 @@ test('should report custom expect steps', async ({ runInlineTest }) => {
   ]);
 });
 
-test('should return value from step', async ({ runInlineTest }) => {
+test('should not pass arguments and return value from step', async ({ runInlineTest }) => {
   const result = await runInlineTest({
     'a.test.ts': `
       import { test, expect } from '@playwright/test';
       test('steps with return values', async ({ page }) => {
-        const v1 = await test.step('my step', () => {
+        const v1 = await test.step('my step', (...args) => {
+          expect(args.length).toBe(0);
           return 10;
         });
         console.log('v1 = ' + v1);
-        const v2 = await test.step('my step', async () => {
+        const v2 = await test.step('my step', async (...args) => {
+          expect(args.length).toBe(0);
           return new Promise(f => setTimeout(() => f(v1 + 10), 100));
         });
         console.log('v2 = ' + v2);