Revert "fix(test runner): ignore undefined values in fixtures definit…ions (#15119)" (#15979)

Revert "fix(test runner): ignore undefined values in fixtures definitions (#15119)"

Revert commit d7b63fa0b45ab12e40d0018b0e6f796ecd2df4b7.
Add a test for the broken behavior.

packages/playwright-test/src/loader.ts

@@ -16,7 +16,7 @@
 
 import { installTransform } from './transform';
 import type { Config, Project, ReporterDescription, FullProjectInternal, FullConfigInternal, Fixtures, FixturesWithLocation } from './types';
-import { getPackageJsonPath, filterUndefinedFixtures, errorWithFile } from './util';
+import { getPackageJsonPath, mergeObjects, errorWithFile } from './util';
 import { setCurrentlyLoadingFileSuite } from './globals';
 import { Suite, type TestCase } from './test';
 import type { SerializedLoaderData } from './ipc';
@@ -98,7 +98,7 @@ export class Loader {
       throw new Error(`Cannot use --browser option when configuration file defines projects. Specify browserName in the projects instead.`);
     config.projects = takeFirst(this._configCLIOverrides.projects, config.projects as any);
     config.workers = takeFirst(this._configCLIOverrides.workers, config.workers);
-    config.use = { ...filterUndefinedFixtures(config.use), ...filterUndefinedFixtures(this._configCLIOverrides.use) };
+    config.use = mergeObjects(config.use, this._configCLIOverrides.use);
     for (const project of config.projects || [])
       this._applyCLIOverridesToProject(project);
 
@@ -232,7 +232,7 @@ export class Loader {
     projectConfig.repeatEach = takeFirst(this._configCLIOverrides.repeatEach, projectConfig.repeatEach);
     projectConfig.retries = takeFirst(this._configCLIOverrides.retries, projectConfig.retries);
     projectConfig.timeout = takeFirst(this._configCLIOverrides.timeout, projectConfig.timeout);
-    projectConfig.use = { ...filterUndefinedFixtures(projectConfig.use), ...filterUndefinedFixtures(this._configCLIOverrides.use) };
+    projectConfig.use = mergeObjects(projectConfig.use, this._configCLIOverrides.use);
   }
 
   private _resolveProject(config: Config, fullConfig: FullConfigInternal, projectConfig: Project, throwawayArtifactsPath: string): FullProjectInternal {
@@ -271,7 +271,7 @@ export class Loader {
       testIgnore: takeFirst(projectConfig.testIgnore, config.testIgnore, []),
       testMatch: takeFirst(projectConfig.testMatch, config.testMatch, '**/?(*.)@(spec|test).*'),
       timeout: takeFirst(projectConfig.timeout, config.timeout, defaultTimeout),
-      use: { ...filterUndefinedFixtures(config.use), ...filterUndefinedFixtures(projectConfig.use) },
+      use: mergeObjects(config.use, projectConfig.use),
     };
   }
 

packages/playwright-test/src/testType.ts

@@ -19,7 +19,7 @@ import { currentlyLoadingFileSuite, currentTestInfo, setCurrentlyLoadingFileSuit
 import { TestCase, Suite } from './test';
 import { wrapFunctionWithLocation } from './transform';
 import type { Fixtures, FixturesWithLocation, Location, TestType } from './types';
-import { errorWithLocation, filterUndefinedFixtures, serializeError } from './util';
+import { errorWithLocation, serializeError } from './util';
 
 const testTypeSymbol = Symbol('testType');
 
@@ -197,7 +197,7 @@ export class TestTypeImpl {
 
   private _use(location: Location, fixtures: Fixtures) {
     const suite = this._ensureCurrentSuite(location, `test.use()`);
-    suite._use.push({ fixtures: filterUndefinedFixtures(fixtures) , location });
+    suite._use.push({ fixtures, location });
   }
 
   private async _step(location: Location, title: string, body: () => Promise<void>): Promise<void> {
@@ -223,7 +223,7 @@ export class TestTypeImpl {
   private _extend(location: Location, fixtures: Fixtures) {
     if ((fixtures as any)[testTypeSymbol])
       throw new Error(`test.extend() accepts fixtures object, not a test object.\nDid you mean to call test._extendTest()?`);
-    const fixturesWithLocation: FixturesWithLocation = { fixtures: filterUndefinedFixtures(fixtures), location };
+    const fixturesWithLocation: FixturesWithLocation = { fixtures, location };
     return new TestTypeImpl([...this.fixtures, fixturesWithLocation]).test;
   }
 

packages/playwright-test/src/util.ts

@@ -153,11 +153,15 @@ export function createTitleMatcher(patterns: RegExp | RegExp[]): Matcher {
   };
 }
 
-export function filterUndefinedFixtures<T extends object>(o: T | undefined): T {
-  // We don't want "undefined" values to actually mean "undefined",
-  // but rather "no opinion about this option", like in all other
-  // places in the config.
-  return Object.fromEntries(Object.entries(o || {}).filter(entry => !Object.is(entry[1], undefined))) as any as T;
+export function mergeObjects<A extends object, B extends object>(a: A | undefined | void, b: B | undefined | void): A & B {
+  const result = { ...a } as any;
+  if (!Object.is(b, undefined)) {
+    for (const [name, value] of Object.entries(b as B)) {
+      if (!Object.is(value, undefined))
+        result[name] = value;
+    }
+  }
+  return result as any;
 }
 
 export function forceRegExp(pattern: string): RegExp {

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

@@ -226,11 +226,11 @@ test('test._extendTest should print nice message when used as extend', async ({
   expect(result.output).toContain('Did you mean to call test.extend() with fixtures instead?');
 });
 
-test('fixture options should ignore undefined value', async ({ runInlineTest }) => {
+test('test.use() with undefined should not be ignored', async ({ runInlineTest }) => {
   const result = await runInlineTest({
     'playwright.config.ts': `
       module.exports = {
-        use: { option: undefined },
+        use: { option: 'config' },
       };
     `,
     'a.test.js': `
@@ -238,13 +238,11 @@ test('fixture options should ignore undefined value', async ({ runInlineTest })
       test('test1', async ({ option }) => {
         console.log('test1-' + option);
       });
-
       test.describe('', () => {
         test.use({ option: 'foo' });
         test('test2', async ({ option }) => {
           console.log('test2-' + option);
         });
-
         test.describe('', () => {
           test.use({ option: undefined });
           test('test3', async ({ option }) => {
@@ -252,7 +250,6 @@ test('fixture options should ignore undefined value', async ({ runInlineTest })
           });
         });
       });
-
       test.extend({ option: undefined })('test4', async ({ option }) => {
         console.log('test4-' + option);
       });
@@ -260,8 +257,8 @@ test('fixture options should ignore undefined value', async ({ runInlineTest })
   });
   expect(result.exitCode).toBe(0);
   expect(result.passed).toBe(4);
-  expect(result.output).toContain('test1-default');
+  expect(result.output).toContain('test1-config');
   expect(result.output).toContain('test2-foo');
-  expect(result.output).toContain('test3-foo');
-  expect(result.output).toContain('test4-default');
+  expect(result.output).toContain('test3-undefined');
+  expect(result.output).toContain('test4-undefined');
 });