fix(test runner): make sure undefined options in config result in default values (#19632)

Note: this keeps existing behavior of `undefined` in `test.use()`
reverting to the config value and not to the original default value.

References #19615.

packages/playwright-test/src/fixtures.ts

@@ -48,6 +48,8 @@ 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;
 };
 
 class Fixture {
@@ -188,7 +190,7 @@ export class FixturePool {
   constructor(fixturesList: FixturesWithLocation[], parentPool?: FixturePool, disallowWorkerFixtures?: boolean) {
     this.registrations = new Map(parentPool ? parentPool.registrations : []);
 
-    for (const { fixtures, location } of fixturesList) {
+    for (const { fixtures, location, fromConfig } of fixturesList) {
       for (const entry of Object.entries(fixtures)) {
         const name = entry[0];
         let value = entry[1];
@@ -223,16 +225,16 @@ export class FixturePool {
           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 });
 
         // Overriding option with "undefined" value means setting it to the default value
-        // from the original declaration of the option.
+        // from the config or from the original declaration of the option.
         if (fn === undefined && options.option && previous) {
           let original = previous;
-          while (original.super)
+          while (!original.fromConfig && original.super)
             original = original.super;
           fn = original.fn;
         }
 
         const deps = fixtureParameterNames(fn, location);
-        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 };
+        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);
       }

packages/playwright-test/src/loader.ts

@@ -435,18 +435,17 @@ class ProjectSuiteBuilder {
       return testType.fixtures;
     const result: FixturesWithLocation[] = [];
     for (const f of testType.fixtures) {
+      result.push(f);
       const optionsFromConfig: Fixtures = {};
-      const originalFixtures: Fixtures = {};
       for (const [key, value] of Object.entries(f.fixtures)) {
         if (isFixtureOption(value) && configKeys.has(key))
           (optionsFromConfig as any)[key] = [(configUse as any)[key], value[1]];
-        else
-          (originalFixtures as any)[key] = value;
       }
-      if (Object.entries(optionsFromConfig).length)
+      if (Object.entries(optionsFromConfig).length) {
-        result.push({ fixtures: optionsFromConfig, location: { file: `project#${this._project._id}`, line: 1, column: 1 } });
+        // Add config options immediately after original option definition,
-      if (Object.entries(originalFixtures).length)
+        // so that any test.use() override it.
-        result.push({ fixtures: originalFixtures, location: f.location });
+        result.push({ fixtures: optionsFromConfig, location: { file: `project#${this._project._id}`, line: 1, column: 1 }, fromConfig: true });
+      }
     }
     return result;
   }

packages/playwright-test/src/types.ts

@@ -24,6 +24,7 @@ export type { Location } from '../types/testReporter';
 export type FixturesWithLocation = {
   fixtures: Fixtures;
   location: Location;
+  fromConfig?: boolean;
 };
 export type Annotation = { type: string, description?: string };
 

tests/playwright-test/playwright.spec.ts

@@ -605,6 +605,7 @@ test('should not throw with many fixtures set to undefined', async ({ runInlineT
   const result = await runInlineTest({
     'playwright.config.ts': `
       module.exports = { use: {
+        browserName: undefined,
         headless: undefined,
         channel: undefined,
         launchOptions: undefined,
@@ -642,6 +643,7 @@ test('should not throw with many fixtures set to undefined', async ({ runInlineT
     'a.spec.ts': `
       const { test } = pwt;
       test.use({
+        browserName: undefined,
         headless: undefined,
         channel: undefined,
         launchOptions: undefined,
@@ -676,6 +678,7 @@ test('should not throw with many fixtures set to undefined', async ({ runInlineT
         contextOptions: undefined,
       });
       test('passes', async ({ page }) => {
+        await page.setContent('text');
       });
     `,
   }, { workers: 1 });

tests/playwright-test/test-extend.spec.ts

@@ -276,3 +276,31 @@ test('test.use() with undefined should not be ignored', async ({ runInlineTest }
   expect(result.output).toContain('test4: option1=config');
   expect(result.output).toContain('test4: option2=default');
 });
+
+test('undefined values in config and test.use should be reverted to default', async ({ runInlineTest }) => {
+  const result = await runInlineTest({
+    'playwright.config.ts': `
+      module.exports = {
+        use: { option1: undefined, option2: undefined },
+      };
+    `,
+    'a.test.js': `
+      const test = pwt.test.extend({
+        option1: [ 'default1', { option: true } ],
+        option2: [ 'default2', { option: true } ],
+        option3: [ 'default3', { option: true } ],
+      });
+      test.use({ option2: undefined, option3: undefined });
+      test('my test', async ({ option1, option2, option3 }) => {
+        console.log('option1=' + option1);
+        console.log('option2=' + option2);
+        console.log('option3=' + option3);
+      });
+    `,
+  });
+  expect(result.exitCode).toBe(0);
+  expect(result.passed).toBe(1);
+  expect(result.output).toContain('option1=default1');
+  expect(result.output).toContain('option2=default2');
+  expect(result.output).toContain('option3=default3');
+});