fix(reporter): group fixture initialization under before hooks (#8072)

docs/src/test-advanced-js.md

@@ -235,7 +235,7 @@ const config = {
     reuseExistingServer: !process.env.CI,
   },
 };
-mode.exports = config;
+module.exports = config;
 ```
 
 Now you can use a relative path when navigating the page, or use `baseURL` fixture:

src/test/fixtures.ts

@@ -16,7 +16,7 @@
 
 import { formatLocation, wrapInPromise } from './util';
 import * as crypto from 'crypto';
-import { FixturesWithLocation, Location, WorkerInfo, TestInfo } from './types';
+import { FixturesWithLocation, Location, WorkerInfo, TestInfo, CompleteStepCallback } from './types';
 
 type FixtureScope = 'test' | 'worker';
 type FixtureRegistration = {
@@ -233,7 +233,7 @@ export class FixtureRunner {
       this.testScopeClean = true;
   }
 
-  async resolveParametersAndRunHookOrTest(fn: Function, workerInfo: WorkerInfo, testInfo: TestInfo | undefined) {
+  async resolveParametersAndRunHookOrTest(fn: Function, workerInfo: WorkerInfo, testInfo: TestInfo | undefined, paramsStepCallback?: CompleteStepCallback) {
     // Install all automatic fixtures.
     for (const registration of this.pool!.registrations.values()) {
       const shouldSkip = !testInfo && registration.scope === 'test';
@@ -250,6 +250,9 @@ export class FixtureRunner {
       params[name] = fixture.value;
     }
 
+    // Report fixture hooks step as completed.
+    paramsStepCallback?.();
+
     return fn(params, testInfo || workerInfo);
   }
 

src/test/workerRunner.ts

@@ -272,7 +272,11 @@ export class WorkerRunner extends EventEmitter {
         };
         if (reportEvents)
           this.emit('stepBegin', payload);
+        let callbackHandled = false;
         return (error?: Error | TestError) => {
+          if (callbackHandled)
+            return;
+          callbackHandled = true;
           if (error instanceof Error)
             error = serializeError(error);
           const payload: StepEndPayload = {
@@ -378,7 +382,6 @@ export class WorkerRunner extends EventEmitter {
   }
 
   private async _runBeforeHooks(test: TestCase, testInfo: TestInfoImpl) {
-    let completeStep: CompleteStepCallback | undefined;
     try {
       const beforeEachModifiers: Modifier[] = [];
       for (let s = test.parent; s; s = s.parent) {
@@ -390,7 +393,6 @@ export class WorkerRunner extends EventEmitter {
         const result = await this._fixtureRunner.resolveParametersAndRunHookOrTest(modifier.fn, this._workerInfo, testInfo);
         testInfo[modifier.type](!!result, modifier.description!);
       }
-      completeStep = testInfo._addStep('hook', 'Before Hooks');
       await this._runHooks(test.parent!, 'beforeEach', testInfo);
     } catch (error) {
       if (error instanceof SkipError) {
@@ -402,19 +404,21 @@ export class WorkerRunner extends EventEmitter {
       }
       // Continue running afterEach hooks even after the failure.
     }
-    completeStep?.(testInfo.error);
   }
 
   private async _runTestWithBeforeHooks(test: TestCase, testInfo: TestInfoImpl) {
+    const completeStep = testInfo._addStep('hook', 'Before Hooks');
     if (test._type === 'test')
       await this._runBeforeHooks(test, testInfo);
 
     // Do not run the test when beforeEach hook fails.
-    if (testInfo.status === 'failed' || testInfo.status === 'skipped')
+    if (testInfo.status === 'failed' || testInfo.status === 'skipped') {
+      completeStep?.(testInfo.error);
       return;
+    }
 
     try {
-      await this._fixtureRunner.resolveParametersAndRunHookOrTest(test.fn, this._workerInfo, testInfo);
+      await this._fixtureRunner.resolveParametersAndRunHookOrTest(test.fn, this._workerInfo, testInfo, completeStep);
     } catch (error) {
       if (error instanceof SkipError) {
         if (testInfo.status === 'passed')
@@ -428,6 +432,8 @@ export class WorkerRunner extends EventEmitter {
         if (!('error' in  testInfo))
           testInfo.error = serializeError(error);
       }
+    } finally {
+      completeStep?.(testInfo.error);
     }
   }
 

tests/playwright-test/playwright.expect.misc.spec.ts

@@ -159,7 +159,7 @@ test('should support toHaveURL', async ({ runInlineTest }) => {
   expect(result.exitCode).toBe(1);
 });
 
-test('should support respect actionTimeout', async ({ runInlineTest }) => {
+test('should support respect expect.timeout', async ({ runInlineTest }) => {
   const result = await runInlineTest({
     'playwright.config.js': `module.exports = { expect: { timeout: 1000 } }`,
     'a.test.ts': `

tests/playwright-test/reporter.spec.ts

@@ -217,9 +217,9 @@ test('should report expect steps', async ({ runInlineTest }) => {
     `%% begin {\"title\":\"After Hooks\",\"category\":\"hook\"}`,
     `%% end {\"title\":\"After Hooks\",\"category\":\"hook\"}`,
     `%% begin {\"title\":\"Before Hooks\",\"category\":\"hook\"}`,
-    `%% end {\"title\":\"Before Hooks\",\"category\":\"hook\"}`,
     `%% begin {\"title\":\"browserContext.newPage\",\"category\":\"pw:api\"}`,
     `%% end {\"title\":\"browserContext.newPage\",\"category\":\"pw:api\"}`,
+    `%% end {\"title\":\"Before Hooks\",\"category\":\"hook\"}`,
     `%% begin {\"title\":\"expect.not.toHaveTitle\",\"category\":\"expect\"}`,
     `%% begin {\"title\":\"page.title\",\"category\":\"pw:api\"}`,
     `%% end {\"title\":\"page.title\",\"category\":\"pw:api\"}`,
@@ -296,9 +296,9 @@ test('should report api steps', async ({ runInlineTest }) => {
   expect(result.output.split('\n').filter(line => line.startsWith('%%')).map(stripEscapedAscii)).toEqual([
     `%%%% test begin pass`,
     `%% begin {\"title\":\"Before Hooks\",\"category\":\"hook\"}`,
-    `%% end {\"title\":\"Before Hooks\",\"category\":\"hook\"}`,
     `%% begin {\"title\":\"browserContext.newPage\",\"category\":\"pw:api\"}`,
     `%% end {\"title\":\"browserContext.newPage\",\"category\":\"pw:api\"}`,
+    `%% end {\"title\":\"Before Hooks\",\"category\":\"hook\"}`,
     `%% begin {\"title\":\"page.setContent\",\"category\":\"pw:api\"}`,
     `%% end {\"title\":\"page.setContent\",\"category\":\"pw:api\"}`,
     `%% begin {\"title\":\"page.click\",\"category\":\"pw:api\"}`,
@@ -367,9 +367,9 @@ test('should report api step failure', async ({ runInlineTest }) => {
   expect(result.exitCode).toBe(1);
   expect(result.output.split('\n').filter(line => line.startsWith('%%')).map(stripEscapedAscii)).toEqual([
     `%% begin {\"title\":\"Before Hooks\",\"category\":\"hook\"}`,
-    `%% end {\"title\":\"Before Hooks\",\"category\":\"hook\"}`,
     `%% begin {\"title\":\"browserContext.newPage\",\"category\":\"pw:api\"}`,
     `%% end {\"title\":\"browserContext.newPage\",\"category\":\"pw:api\"}`,
+    `%% end {\"title\":\"Before Hooks\",\"category\":\"hook\"}`,
     `%% begin {\"title\":\"page.setContent\",\"category\":\"pw:api\"}`,
     `%% end {\"title\":\"page.setContent\",\"category\":\"pw:api\"}`,
     `%% begin {\"title\":\"page.click\",\"category\":\"pw:api\"}`,
@@ -421,9 +421,9 @@ test('should report test.step', async ({ runInlineTest }) => {
   expect(result.exitCode).toBe(1);
   expect(result.output.split('\n').filter(line => line.startsWith('%%')).map(stripEscapedAscii)).toEqual([
     `%% begin {\"title\":\"Before Hooks\",\"category\":\"hook\"}`,
-    `%% end {\"title\":\"Before Hooks\",\"category\":\"hook\"}`,
     `%% begin {\"title\":\"browserContext.newPage\",\"category\":\"pw:api\"}`,
     `%% end {\"title\":\"browserContext.newPage\",\"category\":\"pw:api\"}`,
+    `%% end {\"title\":\"Before Hooks\",\"category\":\"hook\"}`,
     `%% begin {\"title\":\"First step\",\"category\":\"test.step\"}`,
     `%% begin {\"title\":\"expect.toBe\",\"category\":\"expect\"}`,
     `%% end {\"title\":\"expect.toBe\",\"category\":\"expect\",\"error\":{\"message\":\"expect(received).toBe(expected) // Object.is equality\\n\\nExpected: 2\\nReceived: 1\",\"stack\":\"<stack>\"}}`,