From 8002baf44f998f4b621ce9014c08468cf705bc6a Mon Sep 17 00:00:00 2001 From: Dmitry Gozman Date: Thu, 9 Feb 2023 14:50:40 -0800 Subject: [PATCH] chore: move option overrides logic to FixturePool (#20795) --- .../playwright-test/src/common/fixtures.ts | 142 ++++++++++-------- .../playwright-test/src/common/poolBuilder.ts | 33 +--- packages/playwright-test/src/common/types.ts | 1 - 3 files changed, 88 insertions(+), 88 deletions(-) diff --git a/packages/playwright-test/src/common/fixtures.ts b/packages/playwright-test/src/common/fixtures.ts index 8943c952d3..69aedafd29 100644 --- a/packages/playwright-test/src/common/fixtures.ts +++ b/packages/playwright-test/src/common/fixtures.ts @@ -16,7 +16,7 @@ import { formatLocation } from '../util'; import * as crypto from 'crypto'; -import type { FixturesWithLocation, Location } from './types'; +import type { Fixtures, FixturesWithLocation, Location } from './types'; export type FixtureScope = 'test' | 'worker'; type FixtureAuto = boolean | 'all-hooks-included'; @@ -45,21 +45,24 @@ export type FixtureRegistration = { id: string; // A fixture override can use the previous version of the fixture. super?: FixtureRegistration; - // Whether this fixture is an option value set from the config. - fromConfig?: boolean; + // Whether this fixture is an option override value set from the config. + optionOverride?: boolean; }; export type LoadError = { message: string; location: Location; }; - -export type LoadErrorSink = (error: LoadError) => void; +type LoadErrorSink = (error: LoadError) => void; +type OptionOverrides = { + overrides: Fixtures, + location: Location, +}; function isFixtureTuple(value: any): value is FixtureTuple { return Array.isArray(value) && typeof value[1] === 'object' && ('scope' in value[1] || 'auto' in value[1] || 'option' in value[1] || 'timeout' in value[1]); } -export function isFixtureOption(value: any): value is FixtureTuple { +function isFixtureOption(value: any): value is FixtureTuple { return isFixtureTuple(value) && !!value[1].option; } @@ -68,71 +71,88 @@ export class FixturePool { readonly registrations: Map; private _onLoadError: LoadErrorSink; - constructor(fixturesList: FixturesWithLocation[], onLoadError: LoadErrorSink, parentPool?: FixturePool, disallowWorkerFixtures?: boolean) { + constructor(fixturesList: FixturesWithLocation[], onLoadError: LoadErrorSink, parentPool?: FixturePool, disallowWorkerFixtures?: boolean, optionOverrides?: OptionOverrides) { this.registrations = new Map(parentPool ? parentPool.registrations : []); this._onLoadError = onLoadError; - for (const { fixtures, location, fromConfig } of fixturesList) { - for (const entry of Object.entries(fixtures)) { - const name = entry[0]; - let value = entry[1]; - let options: { auto: FixtureAuto, scope: FixtureScope, option: boolean, timeout: number | undefined, customTitle: string | undefined } | undefined; - if (isFixtureTuple(value)) { - options = { - auto: value[1].auto ?? false, - scope: value[1].scope || 'test', - option: !!value[1].option, - timeout: value[1].timeout, - customTitle: (value[1] as any)._title, - }; - value = value[0]; - } - let fn = value as (Function | any); + const allOverrides = optionOverrides?.overrides ?? {}; + const overrideKeys = new Set(Object.keys(allOverrides)); + for (const list of fixturesList) { + this._appendFixtureList(list, !!disallowWorkerFixtures, false); - const previous = this.registrations.get(name); - if (previous && options) { - if (previous.scope !== options.scope) { - this._addLoadError(`Fixture "${name}" has already been registered as a { scope: '${previous.scope}' } fixture defined in ${formatLocation(previous.location)}.`, location); - continue; - } - if (previous.auto !== options.auto) { - this._addLoadError(`Fixture "${name}" has already been registered as a { auto: '${previous.scope}' } fixture defined in ${formatLocation(previous.location)}.`, location); - continue; - } - } else if (previous) { - options = { auto: previous.auto, scope: previous.scope, option: previous.option, timeout: previous.timeout, customTitle: previous.customTitle }; - } else if (!options) { - options = { auto: false, scope: 'test', option: false, timeout: undefined, customTitle: undefined }; - } - - if (!kScopeOrder.includes(options.scope)) { - this._addLoadError(`Fixture "${name}" has unknown { scope: '${options.scope}' }.`, location); - continue; - } - if (options.scope === 'worker' && disallowWorkerFixtures) { - this._addLoadError(`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); - continue; - } - - // Overriding option with "undefined" value means setting it to the default value - // from the config or from the original declaration of the option. - if (fn === undefined && options.option && previous) { - let original = previous; - while (!original.fromConfig && original.super) - original = original.super; - fn = original.fn; - } - - const deps = fixtureParameterNames(fn, location, e => this._onLoadError(e)); - const registration: FixtureRegistration = { id: '', name, location, scope: options.scope, fn, auto: options.auto, option: options.option, timeout: options.timeout, customTitle: options.customTitle, deps, super: previous, fromConfig }; - registrationId(registration); - this.registrations.set(name, registration); + // Process option overrides immediately after original option definitions, + // so that any test.use() override it. + const selectedOverrides: Fixtures = {}; + for (const [key, value] of Object.entries(list.fixtures)) { + if (isFixtureOption(value) && overrideKeys.has(key)) + (selectedOverrides as any)[key] = [(allOverrides as any)[key], value[1]]; } + if (Object.entries(selectedOverrides).length) + this._appendFixtureList({ fixtures: selectedOverrides, location: optionOverrides!.location }, !!disallowWorkerFixtures, true); } this.digest = this.validate(); } + private _appendFixtureList(list: FixturesWithLocation, disallowWorkerFixtures: boolean, isOptionsOverride: boolean) { + const { fixtures, location } = list; + for (const entry of Object.entries(fixtures)) { + const name = entry[0]; + let value = entry[1]; + let options: { auto: FixtureAuto, scope: FixtureScope, option: boolean, timeout: number | undefined, customTitle: string | undefined } | undefined; + if (isFixtureTuple(value)) { + options = { + auto: value[1].auto ?? false, + scope: value[1].scope || 'test', + option: !!value[1].option, + timeout: value[1].timeout, + customTitle: (value[1] as any)._title, + }; + value = value[0]; + } + let fn = value as (Function | any); + + const previous = this.registrations.get(name); + if (previous && options) { + if (previous.scope !== options.scope) { + this._addLoadError(`Fixture "${name}" has already been registered as a { scope: '${previous.scope}' } fixture defined in ${formatLocation(previous.location)}.`, location); + continue; + } + if (previous.auto !== options.auto) { + this._addLoadError(`Fixture "${name}" has already been registered as a { auto: '${previous.scope}' } fixture defined in ${formatLocation(previous.location)}.`, location); + continue; + } + } else if (previous) { + options = { auto: previous.auto, scope: previous.scope, option: previous.option, timeout: previous.timeout, customTitle: previous.customTitle }; + } else if (!options) { + options = { auto: false, scope: 'test', option: false, timeout: undefined, customTitle: undefined }; + } + + if (!kScopeOrder.includes(options.scope)) { + this._addLoadError(`Fixture "${name}" has unknown { scope: '${options.scope}' }.`, location); + continue; + } + if (options.scope === 'worker' && disallowWorkerFixtures) { + this._addLoadError(`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); + continue; + } + + // Overriding option with "undefined" value means setting it to the default value + // from the config or from the original declaration of the option. + if (fn === undefined && options.option && previous) { + let original = previous; + while (!original.optionOverride && original.super) + original = original.super; + fn = original.fn; + } + + const deps = fixtureParameterNames(fn, location, e => this._onLoadError(e)); + const registration: FixtureRegistration = { id: '', name, location, scope: options.scope, fn, auto: options.auto, option: options.option, timeout: options.timeout, customTitle: options.customTitle, deps, super: previous, optionOverride: isOptionsOverride }; + registrationId(registration); + this.registrations.set(name, registration); + } + } + private validate() { const markers = new Map(); const stack: FixtureRegistration[] = []; diff --git a/packages/playwright-test/src/common/poolBuilder.ts b/packages/playwright-test/src/common/poolBuilder.ts index 589a3ffd45..382255bbf8 100644 --- a/packages/playwright-test/src/common/poolBuilder.ts +++ b/packages/playwright-test/src/common/poolBuilder.ts @@ -14,11 +14,11 @@ * limitations under the License. */ -import { FixturePool, isFixtureOption } from './fixtures'; +import { FixturePool } from './fixtures'; import type { LoadError } from './fixtures'; import type { Suite, TestCase } from './test'; import type { TestTypeImpl } from './testType'; -import type { Fixtures, FixturesWithLocation, FullProjectInternal } from './types'; +import type { FullProjectInternal } from './types'; import { formatLocation } from '../util'; import type { TestError } from '../../reporter'; @@ -73,8 +73,11 @@ export class PoolBuilder { private _buildTestTypePool(testType: TestTypeImpl, testErrors?: TestError[]): FixturePool { if (!this._testTypePools.has(testType)) { - const fixtures = this._project ? this._applyConfigUseOptions(this._project, testType) : testType.fixtures; - const pool = new FixturePool(fixtures, e => this._handleLoadError(e, testErrors)); + const optionOverrides = { + overrides: this._project?.use ?? {}, + location: { file: `project#${this._project?._internal.id}`, line: 1, column: 1 } + }; + const pool = new FixturePool(testType.fixtures, e => this._handleLoadError(e, testErrors), undefined, undefined, optionOverrides); this._testTypePools.set(testType, pool); } return this._testTypePools.get(testType)!; @@ -86,26 +89,4 @@ export class PoolBuilder { else throw new Error(`${formatLocation(e.location)}: ${e.message}`); } - - private _applyConfigUseOptions(project: FullProjectInternal, testType: TestTypeImpl): FixturesWithLocation[] { - const projectUse = project.use || {}; - const configKeys = new Set(Object.keys(projectUse)); - if (!configKeys.size) - return testType.fixtures; - const result: FixturesWithLocation[] = []; - for (const f of testType.fixtures) { - result.push(f); - const optionsFromConfig: Fixtures = {}; - for (const [key, value] of Object.entries(f.fixtures)) { - if (isFixtureOption(value) && configKeys.has(key)) - (optionsFromConfig as any)[key] = [(projectUse as any)[key], value[1]]; - } - if (Object.entries(optionsFromConfig).length) { - // Add config options immediately after original option definition, - // so that any test.use() override it. - result.push({ fixtures: optionsFromConfig, location: { file: `project#${project._internal.id}`, line: 1, column: 1 }, fromConfig: true }); - } - } - return result; - } } diff --git a/packages/playwright-test/src/common/types.ts b/packages/playwright-test/src/common/types.ts index ce9ecbd0bb..89be4ecea0 100644 --- a/packages/playwright-test/src/common/types.ts +++ b/packages/playwright-test/src/common/types.ts @@ -26,7 +26,6 @@ export type { Location } from '../../types/testReporter'; export type FixturesWithLocation = { fixtures: Fixtures; location: Location; - fromConfig?: boolean; }; export type Annotation = { type: string, description?: string };