From d2d71c4cdbb2eb3139167edc5704a8ba690bc532 Mon Sep 17 00:00:00 2001 From: Pavel Feldman Date: Thu, 12 Aug 2021 07:58:00 -0700 Subject: [PATCH] fix(reporter): group fixture initialization under before hooks (#8072) --- docs/src/test-advanced-js.md | 2 +- src/test/fixtures.ts | 7 +++++-- src/test/workerRunner.ts | 16 +++++++++++----- .../playwright.expect.misc.spec.ts | 2 +- tests/playwright-test/reporter.spec.ts | 8 ++++---- 5 files changed, 22 insertions(+), 13 deletions(-) diff --git a/docs/src/test-advanced-js.md b/docs/src/test-advanced-js.md index 835f698ed5..dafef9d0fe 100644 --- 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 index 9f99adb51c..040bf71131 100644 --- 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 = { @@ -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); } diff --git a/src/test/workerRunner.ts b/src/test/workerRunner.ts index 7e4e3662dd..cb10388e6a 100644 --- 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 index 8b35a99190..3ac77969fb 100644 --- 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 index 677c6c0a18..ef8dde5183 100644 --- a/tests/playwright-test/reporter.spec.ts +++ b/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\":\"\"}}`,