fix(test runner): handle fixture option overrides

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<string, FixtureRegistration>;
@@ -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,15 +166,15 @@ 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;
         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, 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);
     }

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

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