From 621897c385425807df209a6205a397af13ead908 Mon Sep 17 00:00:00 2001 From: Dmitry Gozman Date: Tue, 22 Oct 2024 11:37:56 +0100 Subject: [PATCH] feat(test runner): run tests with different worker fixtures in a single worker Previously, we forced a new worker for a new set of worker fixtures. Now, we can reuse the worker by tearing down mismatching worker fixtures between tests. The new behavior allows all tests from a single file run in-order, even when mixing various test types with different worker fixtures. This is less efficient, but more predictable for the user. Switching between worker fixtures is reported under "Reset Fixtures" step inside "Before Hooks" and has a separate timeout. We still force a new worker for each project and repeatEach index. --- packages/playwright/src/common/ipc.ts | 1 + packages/playwright/src/common/suiteUtils.ts | 9 +-- packages/playwright/src/runner/dispatcher.ts | 15 +++-- packages/playwright/src/runner/testGroups.ts | 13 ++--- packages/playwright/src/runner/workerHost.ts | 2 + .../playwright/src/worker/fixtureRunner.ts | 56 ++++++++++++------ packages/playwright/src/worker/workerMain.ts | 19 ++++-- tests/playwright-test/fixture-errors.spec.ts | 38 ------------ tests/playwright-test/fixtures.spec.ts | 55 +++++++++++++++++- tests/playwright-test/test-extend.spec.ts | 15 ++--- tests/playwright-test/test-step.spec.ts | 58 +++++++++++++++++++ tests/playwright-test/test-use.spec.ts | 6 +- 12 files changed, 194 insertions(+), 93 deletions(-) diff --git a/packages/playwright/src/common/ipc.ts b/packages/playwright/src/common/ipc.ts index 5ce1991f65..ef59793d0f 100644 --- a/packages/playwright/src/common/ipc.ts +++ b/packages/playwright/src/common/ipc.ts @@ -116,6 +116,7 @@ export type DonePayload = { fatalErrors: TestInfoError[]; skipTestsDueToSetupFailure: string[]; // test ids fatalUnknownTestIds?: string[]; + lastTestPoolDigest?: string; }; export type TestOutputPayload = { diff --git a/packages/playwright/src/common/suiteUtils.ts b/packages/playwright/src/common/suiteUtils.ts index 81dc9d5465..4178289bdc 100644 --- a/packages/playwright/src/common/suiteUtils.ts +++ b/packages/playwright/src/common/suiteUtils.ts @@ -77,10 +77,7 @@ export function bindFileSuiteToProject(project: FullProjectInternal, suite: Suit // Skip annotations imply skipped expectedStatus. if (test.annotations.some(a => a.type === 'skip' || a.type === 'fixme')) test.expectedStatus = 'skipped'; - - // We only compute / set digest in the runner. - if (test._poolDigest) - test._workerHash = `${project.id}-${test._poolDigest}-0`; + test._workerHash = `${project.id}-0`; }); return result; @@ -95,9 +92,7 @@ export function applyRepeatEachIndex(project: FullProjectInternal, fileSuite: Su const testId = suite._fileId + '-' + calculateSha1(testIdExpression).slice(0, 20); test.id = testId; test.repeatEachIndex = repeatEachIndex; - - if (test._poolDigest) - test._workerHash = `${project.id}-${test._poolDigest}-${repeatEachIndex}`; + test._workerHash = `${project.id}-${repeatEachIndex}`; } }); } diff --git a/packages/playwright/src/runner/dispatcher.ts b/packages/playwright/src/runner/dispatcher.ts index 4e971f2475..e03c1c6b74 100644 --- a/packages/playwright/src/runner/dispatcher.ts +++ b/packages/playwright/src/runner/dispatcher.ts @@ -56,8 +56,10 @@ export class Dispatcher { return; const job = this._queue[0]; - // 2. Find a worker with the same hash, or just some free worker. - let index = this._workerSlots.findIndex(w => !w.busy && w.worker && w.worker.hash() === job.workerHash && !w.worker.didSendStop()); + // 2. Find a worker with the same hash, preferably with the same digest, or just some free worker. + let index = this._workerSlots.findIndex(w => !w.busy && w.worker && !w.worker.didSendStop() && w.worker.hash() === job.workerHash && w.worker.poolDigest === job.firstPoolDigest); + if (index === -1) + index = this._workerSlots.findIndex(w => !w.busy && w.worker && !w.worker.didSendStop() && w.worker.hash() === job.workerHash); if (index === -1) index = this._workerSlots.findIndex(w => !w.busy); // No workers available, bail out. @@ -113,6 +115,7 @@ export class Dispatcher { const result = await jobDispatcher.jobResult; this._workerSlots[index].jobDispatcher = undefined; this._updateCounterForWorkerHash(job.workerHash, -1); + worker.poolDigest = result.lastTestPoolDigest || worker.poolDigest; // 4. When worker encounters error, we stop it and create a new one. // We also do not keep the worker alive if it cannot serve any more jobs. @@ -231,7 +234,7 @@ export class Dispatcher { } class JobDispatcher { - jobResult = new ManualPromise<{ newJob?: TestGroup, didFail: boolean }>(); + jobResult = new ManualPromise<{ newJob?: TestGroup, didFail: boolean, lastTestPoolDigest?: string }>(); private _listeners: RegisteredListener[] = []; private _failedTests = new Set(); @@ -402,7 +405,7 @@ class JobDispatcher { // - we are here not because something failed // - no unrecoverable worker error if (!this._remainingByTestId.size && !this._failedTests.size && !params.fatalErrors.length && !params.skipTestsDueToSetupFailure.length && !params.fatalUnknownTestIds && !params.unexpectedExitError) { - this._finished({ didFail: false }); + this._finished({ didFail: false, lastTestPoolDigest: params.lastTestPoolDigest }); return; } @@ -476,7 +479,7 @@ class JobDispatcher { // This job is over, we will schedule another one. const newJob = remaining.length ? { ...this._job, tests: remaining } : undefined; - this._finished({ didFail: true, newJob }); + this._finished({ didFail: true, newJob, lastTestPoolDigest: params.lastTestPoolDigest }); } onExit(data: ProcessExitData) { @@ -486,7 +489,7 @@ class JobDispatcher { this._onDone({ skipTestsDueToSetupFailure: [], fatalErrors: [], unexpectedExitError }); } - private _finished(result: { newJob?: TestGroup, didFail: boolean }) { + private _finished(result: { newJob?: TestGroup, didFail: boolean, lastTestPoolDigest?: string }) { eventsHelper.removeEventListeners(this._listeners); this.jobResult.resolve(result); } diff --git a/packages/playwright/src/runner/testGroups.ts b/packages/playwright/src/runner/testGroups.ts index 5743c7044b..dc083fb142 100644 --- a/packages/playwright/src/runner/testGroups.ts +++ b/packages/playwright/src/runner/testGroups.ts @@ -18,6 +18,7 @@ import type { Suite, TestCase } from '../common/test'; export type TestGroup = { workerHash: string; + firstPoolDigest: string; requireFile: string; repeatEachIndex: number; projectId: string; @@ -25,13 +26,10 @@ export type TestGroup = { }; export function createTestGroups(projectSuite: Suite, expectedParallelism: number): TestGroup[] { - // This function groups tests that can be run together. - // Tests cannot be run together when: - // - They belong to different projects - requires different workers. - // - They have a different repeatEachIndex - requires different workers. - // - They have a different set of worker fixtures in the pool - requires different workers. - // - They have a different requireFile - reuses the worker, but runs each requireFile separately. - // - They belong to a parallel suite. + // This function groups tests that must be run together: + // - in default mode - all tests in a single requireFile; + // - in serial mode - all test from the serial suite. + // Otherwise, tests will be put into separate test groups, that may or may not run in the same worker. // Using the map "workerHash -> requireFile -> group" makes us preserve the natural order // of worker hashes and require files for the simple cases. @@ -54,6 +52,7 @@ export function createTestGroups(projectSuite: Suite, expectedParallelism: numbe const createGroup = (test: TestCase): TestGroup => { return { workerHash: test._workerHash, + firstPoolDigest: test._poolDigest, requireFile: test._requireFile, repeatEachIndex: test.repeatEachIndex, projectId: test._projectId, diff --git a/packages/playwright/src/runner/workerHost.ts b/packages/playwright/src/runner/workerHost.ts index 834dd1facf..bbdab598ac 100644 --- a/packages/playwright/src/runner/workerHost.ts +++ b/packages/playwright/src/runner/workerHost.ts @@ -29,6 +29,7 @@ export class WorkerHost extends ProcessHost { readonly parallelIndex: number; readonly workerIndex: number; private _hash: string; + poolDigest: string; private _params: WorkerInitParams; private _didFail = false; @@ -42,6 +43,7 @@ export class WorkerHost extends ProcessHost { this.workerIndex = workerIndex; this.parallelIndex = parallelIndex; this._hash = testGroup.workerHash; + this.poolDigest = testGroup.firstPoolDigest; this._params = { workerIndex: this.workerIndex, diff --git a/packages/playwright/src/worker/fixtureRunner.ts b/packages/playwright/src/worker/fixtureRunner.ts index e67393eb0d..0c1a4ef447 100644 --- a/packages/playwright/src/worker/fixtureRunner.ts +++ b/packages/playwright/src/worker/fixtureRunner.ts @@ -179,15 +179,30 @@ export class FixtureRunner { setPool(pool: FixturePool) { if (!this.testScopeClean) throw new Error('Did not teardown test scope'); - if (this.pool && pool.digest !== this.pool.digest) { - throw new Error([ - `Playwright detected inconsistent test.use() options.`, - `Most common mistakes that lead to this issue:`, - ` - Calling test.use() outside of the test file, for example in a common helper.`, - ` - One test file imports from another test file.`, - ].join('\n')); + if (!this.pool) { + this.pool = pool; + return; } - this.pool = pool; + + // Teardown fixtures in the reverse order. + const fixtures = Array.from(this.instanceForId.values()).reverse(); + const collector = new Set(); + for (const fixture of fixtures) { + // Note: compare id instead of instances to allow identical registrations to be reused, + // for example from a single test.use() applied to multiple tests. + if (pool.resolve(fixture.registration.name)?.id !== fixture.registration.id) + fixture._collectFixturesInTeardownOrder('worker', collector); + } + + if (!collector.size) { + this.pool = pool; + return; + } + + return async (testInfo: TestInfoImpl, runnable: RunnableDescription) => { + await this._teardownFixtureList(collector, testInfo, runnable); + this.pool = pool; + }; } private _collectFixturesInSetupOrder(registration: FixtureRegistration, collector: Set) { @@ -200,26 +215,33 @@ export class FixtureRunner { collector.add(registration); } - async teardownScope(scope: FixtureScope, testInfo: TestInfoImpl, runnable: RunnableDescription) { - // Teardown fixtures in the reverse order. - const fixtures = Array.from(this.instanceForId.values()).reverse(); - const collector = new Set(); - for (const fixture of fixtures) - fixture._collectFixturesInTeardownOrder(scope, collector); + private async _teardownFixtureList(fixtures: Set, testInfo: TestInfoImpl, runnable: RunnableDescription) { let firstError: Error | undefined; - for (const fixture of collector) { + for (const fixture of fixtures) { try { await fixture.teardown(testInfo, runnable); } catch (error) { firstError = firstError ?? error; } } - if (scope === 'test') - this.testScopeClean = true; if (firstError) throw firstError; } + async teardownScope(scope: FixtureScope, testInfo: TestInfoImpl, runnable: RunnableDescription) { + // Teardown fixtures in the reverse order. + const fixtures = Array.from(this.instanceForId.values()).reverse(); + const collector = new Set(); + for (const fixture of fixtures) + fixture._collectFixturesInTeardownOrder(scope, collector); + try { + await this._teardownFixtureList(collector, testInfo, runnable); + } finally { + if (scope === 'test') + this.testScopeClean = true; + } + } + async resolveParametersForFunction(fn: Function, testInfo: TestInfoImpl, autoFixtures: 'worker' | 'test' | 'all-hooks-only', runnable: RunnableDescription): Promise { const collector = new Set(); diff --git a/packages/playwright/src/worker/workerMain.ts b/packages/playwright/src/worker/workerMain.ts index f180f3d08b..1916fd5d3c 100644 --- a/packages/playwright/src/worker/workerMain.ts +++ b/packages/playwright/src/worker/workerMain.ts @@ -54,6 +54,7 @@ export class WorkerMain extends ProcessRunner { private _currentTest: TestInfoImpl | null = null; private _lastRunningTests: TestCase[] = []; private _totalRunningTests = 0; + private _lastTestPoolDigest = ''; // Suites that had their beforeAll hooks, but not afterAll hooks executed. // These suites still need afterAll hooks to be executed for the proper cleanup. // Contains dynamic annotations originated by modifiers with a callback, e.g. `test.skip(() => true)`. @@ -230,7 +231,8 @@ export class WorkerMain extends ProcessRunner { const donePayload: DonePayload = { fatalErrors: this._fatalErrors, skipTestsDueToSetupFailure: [], - fatalUnknownTestIds + fatalUnknownTestIds, + lastTestPoolDigest: this._lastTestPoolDigest, }; for (const test of this._skipRemainingTestsInSuite?.allTests() || []) { if (entries.has(test.id)) @@ -266,9 +268,6 @@ export class WorkerMain extends ProcessRunner { } }; - if (!this._isStopped) - this._fixtureRunner.setPool(test._pool!); - const suites = getSuites(test); const reversedSuites = suites.slice().reverse(); const nextSuites = new Set(getSuites(nextTest)); @@ -333,6 +332,18 @@ export class WorkerMain extends ProcessRunner { let testFunctionParams: object | null = null; await testInfo._runAsStage({ title: 'Before Hooks', stepInfo: { category: 'hook' } }, async () => { + const resetFixturesCallback = this._fixtureRunner.setPool(test._pool!); + if (resetFixturesCallback) { + await testInfo._runAsStage({ title: 'Reset Fixtures', stepInfo: { category: 'hook' } }, async () => { + // Tear down worker fixtures from the previous test that are incompatible with this test. + // Unfortunately, we do not know about new worker fixtures at the end of previous test. + // Give it a separate timeout - not sure what else could it be. + const fixturesResetSlot = { timeout: this._project.project.timeout, elapsed: 0 }; + await resetFixturesCallback(testInfo, { type: 'test', slot: fixturesResetSlot }); + }); + } + this._lastTestPoolDigest = test._pool!.digest; + // Run "beforeAll" hooks, unless already run during previous tests. for (const suite of suites) await this._runBeforeAllHooksForSuite(suite, testInfo); diff --git a/tests/playwright-test/fixture-errors.spec.ts b/tests/playwright-test/fixture-errors.spec.ts index 51c928f156..916ee024b9 100644 --- a/tests/playwright-test/fixture-errors.spec.ts +++ b/tests/playwright-test/fixture-errors.spec.ts @@ -676,44 +676,6 @@ test('should not run user fn when require fixture has failed', async ({ runInlin ]); }); -test('should provide helpful error message when digests do not match', async ({ runInlineTest }) => { - const result = await runInlineTest({ - 'helper.ts': ` - import { test as base } from '@playwright/test'; - export * from '@playwright/test'; - export const test = base.extend({ - foo: [ async ({}, use) => use(), { scope: 'worker' } ], - }); - - test.use({ foo: 'foo' }); - `, - 'a.spec.ts': ` - import { test, expect } from './helper'; - - test('test-a', ({ foo }) => { - expect(foo).toBe('foo'); - }); - `, - 'b.spec.ts': ` - import { test, expect } from './helper'; - - test('test-b', ({ foo }) => { - expect(foo).toBe('foo'); - }); - `, - 'c.spec.ts': ` - import { test, expect } from './helper'; - - test('test-c', ({ foo }) => { - expect(foo).toBe('foo'); - }); - `, - }, { workers: 1 }); - expect(result.exitCode).toBe(1); - expect(result.failed).toBe(1); - expect(result.output).toContain('Playwright detected inconsistent test.use() options.'); -}); - test('tear down base fixture after error in derived', async ({ runInlineTest }) => { const result = await runInlineTest({ 'a.test.ts': ` diff --git a/tests/playwright-test/fixtures.spec.ts b/tests/playwright-test/fixtures.spec.ts index a130f8dfcb..8f669914c9 100644 --- a/tests/playwright-test/fixtures.spec.ts +++ b/tests/playwright-test/fixtures.spec.ts @@ -664,7 +664,7 @@ test('should not create a new worker for test fixtures', async ({ runInlineTest expect(result.passed).toBe(3); }); -test('should create a new worker for worker fixtures', async ({ runInlineTest }) => { +test('should not create a new worker for worker fixtures', async ({ runInlineTest }) => { const result = await runInlineTest({ 'a.test.ts': ` import { test, expect } from '@playwright/test'; @@ -699,7 +699,7 @@ test('should create a new worker for worker fixtures', async ({ runInlineTest }) expect(result.output).toContain('bar-b'); const baseWorker = +result.output.match(/%%base-(\d)/)![1]; expect(result.output).toContain(`%%base-${baseWorker}`); - expect(result.output).toContain(`%%a-${1 - baseWorker}`); + expect(result.output).toContain(`%%a-${baseWorker}`); expect(result.output).toContain(`%%b-${baseWorker}`); expect(result.passed).toBe(3); }); @@ -816,3 +816,54 @@ test('automatic worker fixtures should start before automatic test fixtures', as 'WORKER FIXTURE 2', ]); }); + +test('should teardown necessary worker fixtures when switching pools', async ({ runInlineTest }) => { + const result = await runInlineTest({ + 'a.spec.ts': ` + import { test as original, expect } from '@playwright/test'; + const base = original.extend({ + base: [async ({}, use) => { + console.log('\\n%%base-setup'); + await use('base'); + console.log('\\n%%base-teardown'); + }, { scope: 'worker' }], + }); + + const worker1 = base.extend({ + worker1: [async ({ base }, use) => { + console.log('\\n%%worker1-setup'); + await use(base + '1'); + console.log('\\n%%worker1-teardown'); + }, { scope: 'worker' }], + }); + + const worker2 = base.extend({ + worker2: [async ({ base }, use) => { + console.log('\\n%%worker2-setup'); + await use(base + '2'); + console.log('\\n%%worker2-teardown'); + }, { scope: 'worker' }], + }); + + worker1('test1', async ({ base, worker1 }) => { + console.log('\\n%%test1-' + base + '-' + worker1); + }); + + worker2('test2', async ({ base, worker2 }) => { + console.log('\\n%%test2-' + base + '-' + worker2); + }); + `, + }); + expect(result.exitCode).toBe(0); + expect(result.passed).toBe(2); + expect(result.outputLines).toEqual([ + 'base-setup', + 'worker1-setup', + 'test1-base-base1', + 'worker1-teardown', + 'worker2-setup', + 'test2-base-base2', + 'worker2-teardown', + 'base-teardown', + ]); +}); diff --git a/tests/playwright-test/test-extend.spec.ts b/tests/playwright-test/test-extend.spec.ts index 94ee2bf19a..2ed76e4a08 100644 --- a/tests/playwright-test/test-extend.spec.ts +++ b/tests/playwright-test/test-extend.spec.ts @@ -46,8 +46,11 @@ test('test.extend should work', async ({ runInlineTest }) => { global.logs.push('beforeAll-' + suffix); await run(); global.logs.push('afterAll-' + suffix); - if (suffix.includes('base')) + if (suffix.includes('base')) { + global.logs.push('end of worker'); console.log(global.logs.join('\\n')); + global.logs = []; + } }, { scope: 'worker' }], baseTest: async ({ suffix, derivedWorker }, run) => { @@ -85,10 +88,6 @@ test('test.extend should work', async ({ runInlineTest }) => { 'afterEach-e1', 'afterEach-base1', 'afterAll-e1', - 'afterAll-base1', - ].join('\n')); - expect(output).toContain([ - 'beforeAll-base1', 'beforeAll-e2', 'beforeEach-base1', 'beforeEach-e2', @@ -97,6 +96,7 @@ test('test.extend should work', async ({ runInlineTest }) => { 'afterEach-base1', 'afterAll-e2', 'afterAll-base1', + 'end of worker', ].join('\n')); expect(output).toContain([ 'beforeAll-base2', @@ -107,10 +107,6 @@ test('test.extend should work', async ({ runInlineTest }) => { 'afterEach-e1', 'afterEach-base2', 'afterAll-e1', - 'afterAll-base2', - ].join('\n')); - expect(output).toContain([ - 'beforeAll-base2', 'beforeAll-e2', 'beforeEach-base2', 'beforeEach-e2', @@ -119,6 +115,7 @@ test('test.extend should work', async ({ runInlineTest }) => { 'afterEach-base2', 'afterAll-e2', 'afterAll-base2', + 'end of worker', ].join('\n')); }); diff --git a/tests/playwright-test/test-step.spec.ts b/tests/playwright-test/test-step.spec.ts index cbd4ec38c4..9dead894d0 100644 --- a/tests/playwright-test/test-step.spec.ts +++ b/tests/playwright-test/test-step.spec.ts @@ -1267,3 +1267,61 @@ hook |After Hooks }); expect(exitCode).toBe(0); }); + +test('should teardown necessary worker fixtures when switching pools', async ({ runInlineTest }) => { + const result = await runInlineTest({ + 'reporter.ts': stepIndentReporter, + 'playwright.config.ts': `module.exports = { reporter: './reporter' }`, + 'a.spec.ts': ` + import { test as original, expect } from '@playwright/test'; + const base = original.extend({ + base: [async ({}, use) => { + await use('base'); + }, { scope: 'worker' }], + }); + + const worker1 = base.extend({ + worker1: [async ({ base }, use) => { + await use(base + '1'); + }, { scope: 'worker' }], + }); + + const worker2 = base.extend({ + worker2: [async ({ base }, use) => { + await use(base + '2'); + }, { scope: 'worker' }], + }); + + worker1('test1', async ({ base, worker1 }) => { + expect(base).toBe('base'); + expect(worker1).toBe('base1'); + }); + + worker2('test2', async ({ base, worker2 }) => { + expect(base).toBe('base'); + expect(worker2).toBe('failure'); + }); + `, + }, { reporter: '' }); + expect(result.exitCode).toBe(1); + expect(stripAnsi(result.output)).toBe(` +hook |Before Hooks +fixture | fixture: base @ a.spec.ts:3 +fixture | fixture: worker1 @ a.spec.ts:9 +expect |expect.toBe @ a.spec.ts:22 +expect |expect.toBe @ a.spec.ts:23 +hook |After Hooks +hook |Before Hooks +hook | Reset Fixtures +fixture | fixture: worker1 @ a.spec.ts:9 +fixture | fixture: worker2 @ a.spec.ts:15 +expect |expect.toBe @ a.spec.ts:27 +expect |expect.toBe @ a.spec.ts:28 +expect |↪ error: Error: expect(received).toBe(expected) // Object.is equality +hook |After Hooks +hook |Worker Cleanup +fixture | fixture: worker2 @ a.spec.ts:15 +fixture | fixture: base @ a.spec.ts:3 + |Error: expect(received).toBe(expected) // Object.is equality +`); +}); diff --git a/tests/playwright-test/test-use.spec.ts b/tests/playwright-test/test-use.spec.ts index 8a2bcd2401..9ac5fc8594 100644 --- a/tests/playwright-test/test-use.spec.ts +++ b/tests/playwright-test/test-use.spec.ts @@ -119,12 +119,12 @@ test('should run tests with different worker options', async ({ runInlineTest }) test('test1', ({ foo }, testInfo) => { expect(foo).toBe('bar'); - expect(testInfo.workerIndex).toBe(1); + expect(testInfo.workerIndex).toBe(0); }); test('test2', ({ foo }, testInfo) => { expect(foo).toBe('bar'); - expect(testInfo.workerIndex).toBe(1); + expect(testInfo.workerIndex).toBe(0); }); `, 'c.test.ts': ` @@ -132,7 +132,7 @@ test('should run tests with different worker options', async ({ runInlineTest }) test.use({ foo: 'baz' }); test('test2', ({ foo }, testInfo) => { expect(foo).toBe('baz'); - expect(testInfo.workerIndex).toBe(2); + expect(testInfo.workerIndex).toBe(0); }); ` }, { workers: 1 });