From 8f09935e8161472d4f6be23210e425a7664d39b1 Mon Sep 17 00:00:00 2001 From: Dmitry Gozman Date: Tue, 2 May 2023 11:04:51 -0700 Subject: [PATCH] fix(test runner): separate test fixture scope for beforeAll/afterAll hooks (#22746) There was a single test fixture scope that covers all hooks, modifiers and test function. Now beforeAll-like modifiers, beforeAll and afterAll hooks get a scope each. Fixes #22256. --- .../playwright-test/src/worker/workerMain.ts | 49 +++++++++++++------ tests/playwright-test/fixtures.spec.ts | 10 ++-- tests/playwright-test/hooks.spec.ts | 24 +++++++-- tests/playwright-test/test-step.spec.ts | 8 +-- tests/playwright-test/timeout.spec.ts | 6 ++- 5 files changed, 68 insertions(+), 29 deletions(-) diff --git a/packages/playwright-test/src/worker/workerMain.ts b/packages/playwright-test/src/worker/workerMain.ts index ce7d9bce55..4ec6a669af 100644 --- a/packages/playwright-test/src/worker/workerMain.ts +++ b/packages/playwright-test/src/worker/workerMain.ts @@ -409,23 +409,23 @@ export class WorkerMain extends ProcessRunner { firstAfterHooksError = firstAfterHooksError || afterEachError; } + // Teardown test-scoped fixtures. Attribute to 'test' so that users understand + // they should probably increase the test timeout to fix this issue. + testInfo._timeoutManager.setCurrentRunnable({ type: 'test', slot: afterHooksSlot }); + debugTest(`tearing down test scope started`); + const testScopeError = await testInfo._runAndFailOnError(() => this._fixtureRunner.teardownScope('test', testInfo._timeoutManager)); + debugTest(`tearing down test scope finished`); + firstAfterHooksError = firstAfterHooksError || testScopeError; + // Run "afterAll" hooks for suites that are not shared with the next test. // In case of failure the worker will be stopped and we have to make sure that afterAll - // hooks run before test fixtures teardown. + // hooks run before worker fixtures teardown. for (const suite of reversedSuites) { if (!nextSuites.has(suite) || testInfo._isFailure()) { const afterAllError = await this._runAfterAllHooksForSuite(suite, testInfo); firstAfterHooksError = firstAfterHooksError || afterAllError; } } - - // Teardown test-scoped fixtures. Attribute to 'test' so that users understand - // 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._runAndFailOnError(() => this._fixtureRunner.teardownScope('test', testInfo._timeoutManager)); - debugTest(`tearing down test scope finished`); - firstAfterHooksError = firstAfterHooksError || testScopeError; }); if (testInfo._isFailure()) @@ -439,10 +439,7 @@ export class WorkerMain extends ProcessRunner { // Give it more time for the full cleanup. await testInfo._runWithTimeout(async () => { debugTest(`running full cleanup after the failure`); - for (const suite of reversedSuites) { - const afterAllError = await this._runAfterAllHooksForSuite(suite, testInfo); - firstAfterHooksError = firstAfterHooksError || afterAllError; - } + const teardownSlot = { timeout: this._project.project.timeout, elapsed: 0 }; // 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 }); @@ -450,6 +447,12 @@ export class WorkerMain extends ProcessRunner { const testScopeError = await testInfo._runAndFailOnError(() => this._fixtureRunner.teardownScope('test', testInfo._timeoutManager)); debugTest(`tearing down test scope finished`); firstAfterHooksError = firstAfterHooksError || testScopeError; + + for (const suite of reversedSuites) { + const afterAllError = await this._runAfterAllHooksForSuite(suite, testInfo); + firstAfterHooksError = firstAfterHooksError || afterAllError; + } + // 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`); @@ -512,7 +515,15 @@ export class WorkerMain extends ProcessRunner { category: 'hook', title: `${hook.type} hook`, location: hook.location, - }, () => this._fixtureRunner.resolveParametersAndRunFunction(hook.fn, testInfo, 'all-hooks-only')); + }, async () => { + try { + await this._fixtureRunner.resolveParametersAndRunFunction(hook.fn, testInfo, 'all-hooks-only'); + } finally { + // Each beforeAll hook has its own scope for test fixtures. Attribute to the same runnable and timeSlot. + // Note: we must teardown even after beforeAll fails, because we'll run more beforeAlls. + await this._fixtureRunner.teardownScope('test', testInfo._timeoutManager); + } + }); } catch (e) { // Always run all the hooks, and capture the first error. beforeAllError = beforeAllError || e; @@ -540,7 +551,15 @@ export class WorkerMain extends ProcessRunner { category: 'hook', title: `${hook.type} hook`, location: hook.location, - }, () => this._fixtureRunner.resolveParametersAndRunFunction(hook.fn, testInfo, 'all-hooks-only')); + }, async () => { + try { + await this._fixtureRunner.resolveParametersAndRunFunction(hook.fn, testInfo, 'all-hooks-only'); + } finally { + // Each afterAll hook has its own scope for test fixtures. Attribute to the same runnable and timeSlot. + // Note: we must teardown even after afterAll fails, because we'll run more afterAlls. + await this._fixtureRunner.teardownScope('test', testInfo._timeoutManager); + } + }); }); firstError = firstError || afterAllError; debugTest(`${hook.type} hook at "${formatLocation(hook.location)}" finished`); diff --git a/tests/playwright-test/fixtures.spec.ts b/tests/playwright-test/fixtures.spec.ts index 4edc66c3c1..15a8ef5c75 100644 --- a/tests/playwright-test/fixtures.spec.ts +++ b/tests/playwright-test/fixtures.spec.ts @@ -388,26 +388,26 @@ test('automatic fixtures should work', async ({ runInlineTest }) => { test.beforeEach(async ({}) => { expect(counterWorker).toBe(1); expect(counterTest === 1 || counterTest === 2).toBe(true); - expect(counterHooksIncluded === 1 || counterHooksIncluded === 2).toBe(true); + expect(counterHooksIncluded === 2 || counterHooksIncluded === 3).toBe(true); }); test('test 1', async ({}) => { expect(counterWorker).toBe(1); - expect(counterHooksIncluded).toBe(1); + expect(counterHooksIncluded).toBe(2); expect(counterTest).toBe(1); }); test('test 2', async ({}) => { expect(counterWorker).toBe(1); - expect(counterHooksIncluded).toBe(2); + expect(counterHooksIncluded).toBe(3); expect(counterTest).toBe(2); }); test.afterEach(async ({}) => { expect(counterWorker).toBe(1); expect(counterTest === 1 || counterTest === 2).toBe(true); - expect(counterHooksIncluded === 1 || counterHooksIncluded === 2).toBe(true); + expect(counterHooksIncluded === 2 || counterHooksIncluded === 3).toBe(true); }); test.afterAll(async ({}) => { expect(counterWorker).toBe(1); - expect(counterHooksIncluded).toBe(2); + expect(counterHooksIncluded).toBe(4); expect(counterTest).toBe(2); }); ` diff --git a/tests/playwright-test/hooks.spec.ts b/tests/playwright-test/hooks.spec.ts index 53294a9745..3a61cfbf85 100644 --- a/tests/playwright-test/hooks.spec.ts +++ b/tests/playwright-test/hooks.spec.ts @@ -44,9 +44,15 @@ test('hooks should work with fixtures', async ({ runInlineTest }) => { test.beforeAll(async ({ w, t }) => { global.logs.push('beforeAll-' + w + '-' + t); }); + test.beforeAll(async ({ w, t }) => { + global.logs.push('beforeAll2-' + w + '-' + t); + }); test.afterAll(async ({ w, t }) => { global.logs.push('afterAll-' + w + '-' + t); }); + test.afterAll(async ({ w, t }) => { + global.logs.push('afterAll2-' + w + '-' + t); + }); test.beforeEach(async ({ w, t }) => { global.logs.push('beforeEach-' + w + '-' + t); @@ -65,10 +71,20 @@ test('hooks should work with fixtures', async ({ runInlineTest }) => { '+w', '+t', 'beforeAll-17-42', - 'beforeEach-17-42', - 'test-17-42', - 'afterEach-17-42', - 'afterAll-17-42', + '-t', + '+t', + 'beforeAll2-17-43', + '-t', + '+t', + 'beforeEach-17-44', + 'test-17-44', + 'afterEach-17-44', + '-t', + '+t', + 'afterAll-17-45', + '-t', + '+t', + 'afterAll2-17-46', '-t', '+t', ]); diff --git a/tests/playwright-test/test-step.spec.ts b/tests/playwright-test/test-step.spec.ts index 4552862c09..ae3a6c3b71 100644 --- a/tests/playwright-test/test-step.spec.ts +++ b/tests/playwright-test/test-step.spec.ts @@ -700,6 +700,10 @@ test('should nest steps based on zones', async ({ runInlineTest }) => { ], location: { file: 'a.test.ts', line: 'number', column: 'number' } }, + { + title: 'browserContext.close', + category: 'pw:api' + }, { title: 'afterAll hook', category: 'hook', @@ -712,10 +716,6 @@ test('should nest steps based on zones', async ({ runInlineTest }) => { ], location: { file: 'a.test.ts', line: 'number', column: 'number' } }, - { - title: 'browserContext.close', - category: 'pw:api' - } ] } ]); diff --git a/tests/playwright-test/timeout.spec.ts b/tests/playwright-test/timeout.spec.ts index 74659bbb0c..7fef21b6d4 100644 --- a/tests/playwright-test/timeout.spec.ts +++ b/tests/playwright-test/timeout.spec.ts @@ -331,6 +331,9 @@ test('test timeout should still run hooks before fixtures teardown', async ({ ru await new Promise(f => setTimeout(f, 500)); console.log('\\n%%afterAll-2'); }); + test.afterEach(async () => { + console.log('\\n%%afterEach'); + }); test('test fail', async ({}) => { test.setTimeout(100); console.log('\\n%%test'); @@ -344,9 +347,10 @@ test('test timeout should still run hooks before fixtures teardown', async ({ ru expect(result.outputLines).toEqual([ 'before-auto', 'test', + 'afterEach', + 'after-auto', 'afterAll-1', 'afterAll-2', - 'after-auto', ]); });