chore: move option overrides logic to FixturePool (#20795)

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.
+  // Whether this fixture is an option override value set from the config.
-  fromConfig?: boolean;
+  optionOverride?: boolean;
 };
 export type LoadError = {
   message: string;
   location: Location;
 };
-
+type LoadErrorSink = (error: LoadError) => void;
-export 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<string, FixtureRegistration>;
   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) {
+    const allOverrides = optionOverrides?.overrides ?? {};
-      for (const entry of Object.entries(fixtures)) {
+    const overrideKeys = new Set(Object.keys(allOverrides));
-        const name = entry[0];
+    for (const list of fixturesList) {
-        let value = entry[1];
+      this._appendFixtureList(list, !!disallowWorkerFixtures, false);
-        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);
+      // Process option overrides immediately after original option definitions,
-        if (previous && options) {
+      // so that any test.use() override it.
-          if (previous.scope !== options.scope) {
+      const selectedOverrides: Fixtures = {};
-            this._addLoadError(`Fixture "${name}" has already been registered as a { scope: '${previous.scope}' } fixture defined in ${formatLocation(previous.location)}.`, location);
+      for (const [key, value] of Object.entries(list.fixtures)) {
-            continue;
+        if (isFixtureOption(value) && overrideKeys.has(key))
-          }
+          (selectedOverrides as any)[key] = [(allOverrides as any)[key], value[1]];
-          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);
       }
+      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<FixtureRegistration, 'visiting' | 'visited'>();
     const stack: FixtureRegistration[] = [];

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 optionOverrides = {
-      const pool = new FixturePool(fixtures, e => this._handleLoadError(e, testErrors));
+        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;
-  }
 }

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