From c9c1ea6546d74d064150a4880850cc9d2854763e Mon Sep 17 00:00:00 2001 From: Dmitry Gozman Date: Tue, 10 Aug 2021 16:32:32 -0700 Subject: [PATCH] fix(test runner): disallow use(workerFixture) in describes (#8119) Using a worker fixture forces a new worker. This might be unexpected when part of the test file runs in one worker, and another runs in another worker. Top-level use of worker fixtures is still fine. --- src/test/fixtures.ts | 4 +- src/test/project.ts | 23 +++-- src/test/test.ts | 12 ++- src/test/testType.ts | 3 +- tests/playwright-test/shard.spec.ts | 30 +++---- .../{options.spec.ts => test-use.spec.ts} | 85 +++++++++---------- 6 files changed, 78 insertions(+), 79 deletions(-) rename tests/playwright-test/{options.spec.ts => test-use.spec.ts} (71%) diff --git a/src/test/fixtures.ts b/src/test/fixtures.ts index d29b51e9f8..9f99adb51c 100644 --- a/src/test/fixtures.ts +++ b/src/test/fixtures.ts @@ -105,7 +105,7 @@ export class FixturePool { readonly digest: string; readonly registrations: Map; - constructor(fixturesList: FixturesWithLocation[], parentPool?: FixturePool) { + constructor(fixturesList: FixturesWithLocation[], parentPool?: FixturePool, disallowWorkerFixtures?: boolean) { this.registrations = new Map(parentPool ? parentPool.registrations : []); for (const { fixtures, location } of fixturesList) { @@ -136,6 +136,8 @@ export class FixturePool { if (options.scope !== 'test' && options.scope !== 'worker') throw errorWithLocations(`Fixture "${name}" has unknown { scope: '${options.scope}' }.`, { location, name }); + if (options.scope === 'worker' && disallowWorkerFixtures) + throw errorWithLocations(`Cannot use({ ${name} }) in a describe group, because it forces a new worker.\nMake it top-level in the test file or put in the configuration file.`, { location, name }); const deps = fixtureParameterNames(fn, location); const registration: FixtureRegistration = { id: '', name, location, scope: options.scope, fn, auto: options.auto, deps, super: previous }; diff --git a/src/test/project.ts b/src/test/project.ts index 94e79b0f6a..d1383b1532 100644 --- a/src/test/project.ts +++ b/src/test/project.ts @@ -56,19 +56,15 @@ export class ProjectImpl { private buildPool(test: TestCase): FixturePool { if (!this.testPools.has(test)) { let pool = this.buildTestTypePool(test._testType); - const overrides: Fixtures = test.parent!._buildFixtureOverrides(); - if (Object.entries(overrides).length) { - const overridesWithLocation = { - fixtures: overrides, - // TODO: pass location from test.use() callsite. - location: test.location, - }; - pool = new FixturePool([overridesWithLocation], pool); - } - this.testPools.set(test, pool); - pool.validateFunction(test.fn, 'Test', test.location); - for (let parent = test.parent; parent; parent = parent.parent) { + const parents: Suite[] = []; + for (let parent = test.parent; parent; parent = parent.parent) + parents.push(parent); + parents.reverse(); + + for (const parent of parents) { + if (parent._use.length) + pool = new FixturePool(parent._use, pool, parent._isDescribe); for (const hook of parent._eachHooks) pool.validateFunction(hook.fn, hook.type + ' hook', hook.location); for (const hook of parent._allHooks) @@ -76,6 +72,9 @@ export class ProjectImpl { for (const modifier of parent._modifiers) pool.validateFunction(modifier.fn, modifier.type + ' modifier', modifier.location); } + + pool.validateFunction(test.fn, 'Test', test.location); + this.testPools.set(test, pool); } return this.testPools.get(test)!; } diff --git a/src/test/test.ts b/src/test/test.ts index c6712cb997..aeeacb37d9 100644 --- a/src/test/test.ts +++ b/src/test/test.ts @@ -17,7 +17,7 @@ import type { FixturePool } from './fixtures'; import * as reporterTypes from '../../types/testReporter'; import type { TestTypeImpl } from './testType'; -import { Annotations, Location } from './types'; +import { Annotations, FixturesWithLocation, Location } from './types'; class Base { title: string; @@ -48,7 +48,8 @@ export class Suite extends Base implements reporterTypes.Suite { suites: Suite[] = []; tests: TestCase[] = []; location?: Location; - _fixtureOverrides: any = {}; + _use: FixturesWithLocation[] = []; + _isDescribe = false; _entries: (Suite | TestCase)[] = []; _allHooks: TestCase[] = []; _eachHooks: { type: 'beforeEach' | 'afterEach', fn: Function, location: Location }[] = []; @@ -97,20 +98,17 @@ export class Suite extends Base implements reporterTypes.Suite { return items; } - _buildFixtureOverrides(): any { - return this.parent ? { ...this.parent._buildFixtureOverrides(), ...this._fixtureOverrides } : this._fixtureOverrides; - } - _clone(): Suite { const suite = new Suite(this.title); suite._only = this._only; suite.location = this.location; suite._requireFile = this._requireFile; - suite._fixtureOverrides = this._fixtureOverrides; + suite._use = this._use.slice(); suite._eachHooks = this._eachHooks.slice(); suite._timeout = this._timeout; suite._annotations = this._annotations.slice(); suite._modifiers = this._modifiers.slice(); + suite._isDescribe = this._isDescribe; return suite; } } diff --git a/src/test/testType.ts b/src/test/testType.ts index 0b3e0f3974..acf5cbfc4b 100644 --- a/src/test/testType.ts +++ b/src/test/testType.ts @@ -92,6 +92,7 @@ export class TestTypeImpl { const child = new Suite(title); child._requireFile = suite._requireFile; + child._isDescribe = true; child.location = location; suite._addSuite(child); @@ -161,7 +162,7 @@ export class TestTypeImpl { const suite = currentlyLoadingFileSuite(); if (!suite) throw errorWithLocation(location, `test.use() can only be called in a test file`); - suite._fixtureOverrides = { ...suite._fixtureOverrides, ...fixtures }; + suite._use.push({ fixtures, location }); } private async _step(location: Location, title: string, body: () => Promise): Promise { diff --git a/tests/playwright-test/shard.spec.ts b/tests/playwright-test/shard.spec.ts index 97fed8a975..fc84d8402c 100644 --- a/tests/playwright-test/shard.spec.ts +++ b/tests/playwright-test/shard.spec.ts @@ -17,28 +17,28 @@ import { test, expect } from './playwright-test-fixtures'; const tests = { + 'helper.ts': ` + export const headlessTest = pwt.test.extend({ headless: false }); + export const headedTest = pwt.test.extend({ headless: true }); + `, 'a.spec.ts': ` - const test = pwt.test.extend({ foo: 'bar' }); - test.use({ headless: false }); - test('test1', async () => { + import { headlessTest, headedTest } from './helper'; + headlessTest('test1', async () => { console.log('test1-done'); }); - test.describe('suite', () => { - test.use({ headless: true }); - test('test2', async () => { - console.log('test2-done'); - }); + headedTest('test2', async () => { + console.log('test2-done'); }); - test('test3', async () => { + headlessTest('test3', async () => { console.log('test3-done'); }); `, 'b.spec.ts': ` - const test = pwt.test.extend({ bar: 'foo' }); - test('test4', async () => { + import { headlessTest, headedTest } from './helper'; + headlessTest('test4', async () => { console.log('test4-done'); }); - test('test5', async () => { + headedTest('test5', async () => { console.log('test5-done'); }); `, @@ -51,7 +51,7 @@ test('should respect shard=1/2', async ({ runInlineTest }) => { expect(result.skipped).toBe(0); expect(result.output).toContain('test1-done'); expect(result.output).toContain('test3-done'); - expect(result.output).toContain('test2-done'); + expect(result.output).toContain('test4-done'); }); test('should respect shard=2/2', async ({ runInlineTest }) => { @@ -59,7 +59,7 @@ test('should respect shard=2/2', async ({ runInlineTest }) => { expect(result.exitCode).toBe(0); expect(result.passed).toBe(2); expect(result.skipped).toBe(0); - expect(result.output).toContain('test4-done'); + expect(result.output).toContain('test2-done'); expect(result.output).toContain('test5-done'); }); @@ -75,5 +75,5 @@ test('should respect shard=1/2 in config', async ({ runInlineTest }) => { expect(result.skipped).toBe(0); expect(result.output).toContain('test1-done'); expect(result.output).toContain('test3-done'); - expect(result.output).toContain('test2-done'); + expect(result.output).toContain('test4-done'); }); diff --git a/tests/playwright-test/options.spec.ts b/tests/playwright-test/test-use.spec.ts similarity index 71% rename from tests/playwright-test/options.spec.ts rename to tests/playwright-test/test-use.spec.ts index 838a49b0a7..b8fa8b1854 100644 --- a/tests/playwright-test/options.spec.ts +++ b/tests/playwright-test/test-use.spec.ts @@ -71,6 +71,28 @@ test('should run tests with different test options in the same worker', async ({ expect(result.passed).toBe(3); }); +test('should throw when setting worker options in describe', async ({ runInlineTest }) => { + const result = await runInlineTest({ + 'a.test.ts': ` + const test = pwt.test.extend({ + foo: [undefined, { scope: 'worker' }], + }); + test.describe('suite', () => { + test.use({ foo: 'bar' }); + test('test', ({ foo }, testInfo) => { + }); + }); + ` + }, { workers: 1 }); + + expect(result.exitCode).toBe(1); + expect(result.output).toContain([ + `Cannot use({ foo }) in a describe group, because it forces a new worker.`, + `Make it top-level in the test file or put in the configuration file.`, + ` "foo" defined at a.test.ts:9:14`, + ].join('\n')); +}); + test('should run tests with different worker options', async ({ runInlineTest }) => { const result = await runInlineTest({ 'helper.ts': ` @@ -82,58 +104,35 @@ test('should run tests with different worker options', async ({ runInlineTest }) import { test } from './helper'; test('test', ({ foo }, testInfo) => { expect(foo).toBe(undefined); - console.log('\\n%%test=' + testInfo.workerIndex); - }); - - test.describe('suite1', () => { - test.use({ foo: 'bar' }); - test('test1', ({ foo }, testInfo) => { - expect(foo).toBe('bar'); - console.log('\\n%%test1=' + testInfo.workerIndex); - }); - - test.describe('suite2', () => { - test.use({ foo: 'baz' }); - test('test2', ({ foo }, testInfo) => { - expect(foo).toBe('baz'); - console.log('\\n%%test2=' + testInfo.workerIndex); - }); - }); - - test('test3', ({ foo }, testInfo) => { - expect(foo).toBe('bar'); - console.log('\\n%%test3=' + testInfo.workerIndex); - }); + expect(testInfo.workerIndex).toBe(0); }); `, 'b.test.ts': ` import { test } from './helper'; - test.use({ foo: 'qux' }); - test('test4', ({ foo }, testInfo) => { - expect(foo).toBe('qux'); - console.log('\\n%%test4=' + testInfo.workerIndex); + test.use({ foo: 'bar' }); + + test('test1', ({ foo }, testInfo) => { + expect(foo).toBe('bar'); + expect(testInfo.workerIndex).toBe(1); + }); + + test('test2', ({ foo }, testInfo) => { + expect(foo).toBe('bar'); + expect(testInfo.workerIndex).toBe(1); + }); + `, + 'c.test.ts': ` + import { test } from './helper'; + test.use({ foo: 'baz' }); + test('test2', ({ foo }, testInfo) => { + expect(foo).toBe('baz'); + expect(testInfo.workerIndex).toBe(2); }); ` }, { workers: 1 }); expect(result.exitCode).toBe(0); - expect(result.passed).toBe(5); - - const workerIndexMap = new Map(); - const allWorkers = new Set(); - for (const line of result.output.split('\n')) { - if (line.startsWith('%%')) { - const [ name, workerIndex ] = line.substring(2).split('='); - allWorkers.add(workerIndex); - workerIndexMap.set(name, workerIndex); - } - } - - expect(workerIndexMap.size).toBe(5); - expect(workerIndexMap.get('test1')).toBe(workerIndexMap.get('test3')); - expect(allWorkers.size).toBe(4); - for (let i = 0; i < 4; i++) - expect(allWorkers.has(String(i))); + expect(result.passed).toBe(4); }); test('should use options from the config', async ({ runInlineTest }) => {