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 });