From d7b63fa0b45ab12e40d0018b0e6f796ecd2df4b7 Mon Sep 17 00:00:00 2001 From: Dmitry Gozman Date: Mon, 27 Jun 2022 11:31:41 -0700 Subject: [PATCH] fix(test runner): ignore undefined values in fixtures definitions (#15119) These mean "I don't want to specify this fixture/option" instead of "I want the value of undefined", aligned with how TypeScript works. We already do similar things in the config. --- packages/playwright-test/src/loader.ts | 8 ++--- packages/playwright-test/src/testType.ts | 6 ++-- packages/playwright-test/src/util.ts | 14 +++----- tests/playwright-test/test-extend.spec.ts | 40 +++++++++++++++++++++++ 4 files changed, 52 insertions(+), 16 deletions(-) diff --git a/packages/playwright-test/src/loader.ts b/packages/playwright-test/src/loader.ts index 5ae753b98b..fc06b6a1f3 100644 --- a/packages/playwright-test/src/loader.ts +++ b/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, mergeObjects, errorWithFile } from './util'; +import { getPackageJsonPath, filterUndefinedFixtures, 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 = mergeObjects(config.use, this._configCLIOverrides.use); + config.use = { ...filterUndefinedFixtures(config.use), ...filterUndefinedFixtures(this._configCLIOverrides.use) }; for (const project of config.projects || []) this._applyCLIOverridesToProject(project); @@ -224,7 +224,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 = mergeObjects(projectConfig.use, this._configCLIOverrides.use); + projectConfig.use = { ...filterUndefinedFixtures(projectConfig.use), ...filterUndefinedFixtures(this._configCLIOverrides.use) }; } private _resolveProject(config: Config, fullConfig: FullConfigInternal, projectConfig: Project, throwawayArtifactsPath: string): FullProjectInternal { @@ -263,7 +263,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: mergeObjects(config.use, projectConfig.use), + use: { ...filterUndefinedFixtures(config.use), ...filterUndefinedFixtures(projectConfig.use) }, }; } diff --git a/packages/playwright-test/src/testType.ts b/packages/playwright-test/src/testType.ts index d12b9ba4a5..a3d1dba3b1 100644 --- a/packages/playwright-test/src/testType.ts +++ b/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, serializeError } from './util'; +import { errorWithLocation, filterUndefinedFixtures, serializeError } from './util'; const testTypeSymbol = Symbol('testType'); @@ -196,7 +196,7 @@ export class TestTypeImpl { private _use(location: Location, fixtures: Fixtures) { const suite = this._ensureCurrentSuite(location, `test.use()`); - suite._use.push({ fixtures, location }); + suite._use.push({ fixtures: filterUndefinedFixtures(fixtures) , location }); } private async _step(location: Location, title: string, body: () => Promise): Promise { @@ -222,7 +222,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, location }; + const fixturesWithLocation: FixturesWithLocation = { fixtures: filterUndefinedFixtures(fixtures), location }; return new TestTypeImpl([...this.fixtures, fixturesWithLocation]).test; } diff --git a/packages/playwright-test/src/util.ts b/packages/playwright-test/src/util.ts index 24053127a2..81f0c9f3f0 100644 --- a/packages/playwright-test/src/util.ts +++ b/packages/playwright-test/src/util.ts @@ -158,15 +158,11 @@ export function createTitleMatcher(patterns: RegExp | RegExp[]): Matcher { }; } -export function mergeObjects(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 filterUndefinedFixtures(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 forceRegExp(pattern: string): RegExp { diff --git a/tests/playwright-test/test-extend.spec.ts b/tests/playwright-test/test-extend.spec.ts index 85704980f2..8028e04895 100644 --- a/tests/playwright-test/test-extend.spec.ts +++ b/tests/playwright-test/test-extend.spec.ts @@ -225,3 +225,43 @@ test('test._extendTest should print nice message when used as extend', async ({ expect(result.passed).toBe(0); expect(result.output).toContain('Did you mean to call test.extend() with fixtures instead?'); }); + +test('fixture options should ignore undefined value', async ({ runInlineTest }) => { + const result = await runInlineTest({ + 'playwright.config.ts': ` + module.exports = { + use: { option: undefined }, + }; + `, + 'a.test.js': ` + const test = pwt.test.extend({ option: [ 'default', { option: true } ] }); + 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 }) => { + console.log('test3-' + option); + }); + }); + }); + + test.extend({ option: undefined })('test4', async ({ option }) => { + console.log('test4-' + option); + }); + `, + }); + expect(result.exitCode).toBe(0); + expect(result.passed).toBe(4); + expect(result.output).toContain('test1-default'); + expect(result.output).toContain('test2-foo'); + expect(result.output).toContain('test3-foo'); + expect(result.output).toContain('test4-default'); +});