diff --git a/packages/playwright/src/common/fixtures.ts b/packages/playwright/src/common/fixtures.ts index 0e69271ab53e8..c4676e130ddc0 100644 --- a/packages/playwright/src/common/fixtures.ts +++ b/packages/playwright/src/common/fixtures.ts @@ -73,6 +73,12 @@ function isFixtureOption(value: any): value is FixtureTuple { return isFixtureTuple(value) && !!value[1].option; } +function isOptionFixture(registration: FixtureRegistration): boolean { + while (!registration.option && registration.super) + registration = registration.super; + return registration.option; +} + export class FixturePool { readonly digest: string; private readonly _registrations: Map; @@ -101,7 +107,7 @@ export class FixturePool { if (optionOverrides) { for (const key of overrideKeys) { const registration = this._registrations.get(key); - if (registration && !registration.option) + if (registration && !isOptionFixture(registration)) this._addLoadError(`Fixture "${key}" cannot be overridden in the configuration "use" section. Only fixtures registered with { option: true } can be set in the config.`, optionOverrides.location); } } @@ -114,12 +120,12 @@ export class FixturePool { 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, box?: boolean | 'self' } | undefined; + let options: { auto: FixtureAuto, scope: FixtureScope, option?: boolean, timeout: number | undefined, customTitle?: string, box?: boolean | 'self' } | undefined; if (isFixtureTuple(value)) { options = { auto: value[1].auto ?? false, scope: value[1].scope || 'test', - option: !!value[1].option, + option: value[1].option, timeout: value[1].timeout, customTitle: value[1].title, box: value[1].box, @@ -138,6 +144,10 @@ export class FixturePool { this._addLoadError(`Fixture "${name}" has already been registered as a { auto: '${previous.scope}' } fixture defined in ${formatLocation(previous.location)}.`, location); continue; } + if (previous.option !== options.option && options.option !== undefined) { + this._addLoadError(`Fixture "${name}" has already been registered as a { option: ${previous.option} } fixture defined in ${formatLocation(previous.location)}.`, location); + continue; + } } else if (previous) { // Note: deliberately not inheriting "options.box" so that fixture override is visible by default. options = { auto: previous.auto, scope: previous.scope, option: previous.option, timeout: previous.timeout, customTitle: previous.customTitle }; @@ -156,7 +166,7 @@ export class FixturePool { // 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) { + if (fn === undefined && previous && isOptionFixture(previous)) { let original = previous; while (!original.optionOverride && original.super) original = original.super; @@ -164,7 +174,7 @@ export class FixturePool { } 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, box: options.box, deps, super: previous, optionOverride: isOptionsOverride }; + const registration: FixtureRegistration = { id: '', name, location, scope: options.scope, fn, auto: options.auto, option: !!options.option, timeout: options.timeout, customTitle: options.customTitle, box: options.box, deps, super: previous, optionOverride: isOptionsOverride }; registrationId(registration); this._registrations.set(name, registration); } diff --git a/tests/playwright-test/fixture-errors.spec.ts b/tests/playwright-test/fixture-errors.spec.ts index 95b848a6f1bca..66bef7a081a3e 100644 --- a/tests/playwright-test/fixture-errors.spec.ts +++ b/tests/playwright-test/fixture-errors.spec.ts @@ -816,3 +816,50 @@ test('should throw when overriding non-option fixture in config', async ({ runIn expect(result.output).not.toContain('Fixture "headless"'); expect(result.output).not.toContain('Fixture "unknownThing"'); }); + +test('should throw when overriding option fixture with non-option', async ({ runInlineTest }) => { + const result = await runInlineTest({ + 'playwright.config.ts': ` + module.exports = {}; + `, + 'a.spec.ts': ` + import { test as base, expect } from '@playwright/test'; + const test = base.extend({ + headless: [ + async ({}, use) => await use(true), + { scope: 'worker', option: false }, + ], + }); + test('works', async ({ headless }) => { + expect(headless).toBe(true); + }); + `, + }); + expect(result.exitCode).toBe(1); + expect(result.output).toContain('Fixture "headless" has already been registered as a { option: true } fixture'); +}); + +test('should allow overriding option both in config and in test.use block', async ({ runInlineTest }) => { + const result = await runInlineTest({ + 'playwright.config.ts': ` + module.exports = { + use: { + headless: true, + }, + }; + `, + 'a.spec.ts': ` + import { test as base, expect } from '@playwright/test'; + const test = base.extend({ + headless: [ + async ({}, use) => await use(true), + { scope: 'worker' }, + ], + }); + test('works', async ({ headless }) => { + expect(headless).toBe(true); + }); + `, + }); + expect(result.exitCode).toBe(0); +}); diff --git a/tests/playwright-test/test-extend.spec.ts b/tests/playwright-test/test-extend.spec.ts index 94ee2bf19a250..cc6bf18f4a77e 100644 --- a/tests/playwright-test/test-extend.spec.ts +++ b/tests/playwright-test/test-extend.spec.ts @@ -287,21 +287,26 @@ test('undefined values in config and test.use should be reverted to default', as const result = await runInlineTest({ 'playwright.config.ts': ` module.exports = { - use: { option1: undefined, option2: undefined }, + use: { option1: undefined, option2: undefined, option4: 'from-config' }, }; `, 'a.test.ts': ` import { test as base, expect } from '@playwright/test'; - const test = base.extend({ + const test1 = base.extend({ option1: [ 'default1', { option: true } ], option2: [ 'default2', { option: true } ], option3: [ 'default3', { option: true } ], + option4: [ 'default4', { option: true } ], + }); + const test = test1.extend({ + option4: [ 'new-default4', { scope: 'test' } ], }); - test.use({ option2: undefined, option3: undefined }); - test('my test', async ({ option1, option2, option3 }) => { + test.use({ option2: undefined, option3: undefined, option4: undefined }); + test('my test', async ({ option1, option2, option3, option4 }) => { console.log('option1=' + option1); console.log('option2=' + option2); console.log('option3=' + option3); + console.log('option4=' + option4); }); `, }); @@ -310,4 +315,5 @@ test('undefined values in config and test.use should be reverted to default', as expect(result.output).toContain('option1=default1'); expect(result.output).toContain('option2=default2'); expect(result.output).toContain('option3=default3'); + expect(result.output).toContain('option4=from-config'); });