cherry-pick(release-1.14): group fixture initialization under before …

…hooks PR #8072
microsoft · Aug 20, 2021 · 8d77dcb · 8d77dcb
1 parent a83dad5
commit 8d77dcb
Show file tree

Hide file tree

Showing 5 changed files with 18 additions and 13 deletions.
diff --git a/docs/src/test-advanced-js.md b/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:

diff --git a/src/test/fixtures.ts b/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 = {
@@ -242,7 +242,7 @@ export class FixtureRunner {
       throw error;
   }
 
-  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';
@@ -259,6 +259,9 @@ export class FixtureRunner {
       params[name] = fixture.value;
     }
 
+    // Report fixture hooks step as completed.
+    paramsStepCallback?.();
+
     return fn(params, testInfo || workerInfo);
   }
 

diff --git a/src/test/workerRunner.ts b/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);
     }
   }
 

diff --git a/tests/playwright-test/playwright.expect.misc.spec.ts b/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': `

diff --git a/tests/playwright-test/reporter.spec.ts b/tests/playwright-test/reporter.spec.ts
@@ -224,7 +224,6 @@ 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\",\"steps\":[{\"title\":\"browserContext.newPage\",\"category\":\"pw:api\"}]}`,
@@ -278,7 +277,6 @@ test('should report api steps', async ({ runInlineTest }) => {
   expect(result.exitCode).toBe(0);
   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\",\"steps\":[{\"title\":\"browserContext.newPage\",\"category\":\"pw:api\"}]}`,
@@ -326,7 +324,6 @@ 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\",\"steps\":[{\"title\":\"browserContext.newPage\",\"category\":\"pw:api\"}]}`,
@@ -362,7 +359,6 @@ 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\",\"steps\":[{\"title\":\"browserContext.newPage\",\"category\":\"pw:api\"}]}`,