From b8848fb4994c44a20e22a2e3238110a73edad52a Mon Sep 17 00:00:00 2001 From: Dmitry Gozman Date: Wed, 21 Dec 2022 14:34:43 -0800 Subject: [PATCH] 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 | 10 ++++---- packages/playwright-test/src/loader.ts | 13 +++++------ packages/playwright-test/src/types.ts | 1 + tests/playwright-test/playwright.spec.ts | 3 +++ tests/playwright-test/test-extend.spec.ts | 28 +++++++++++++++++++++++ 5 files changed, 44 insertions(+), 11 deletions(-) diff --git a/packages/playwright-test/src/fixtures.ts b/packages/playwright-test/src/fixtures.ts index 2aa919ed5a..e3c546b2d5 100644 --- a/packages/playwright-test/src/fixtures.ts +++ b/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); } diff --git a/packages/playwright-test/src/loader.ts b/packages/playwright-test/src/loader.ts index 77383c6e4a..8be3a75758 100644 --- a/packages/playwright-test/src/loader.ts +++ b/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) - result.push({ fixtures: optionsFromConfig, location: { file: `project#${this._project._id}`, line: 1, column: 1 } }); - if (Object.entries(originalFixtures).length) - result.push({ fixtures: originalFixtures, location: f.location }); + 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#${this._project._id}`, line: 1, column: 1 }, fromConfig: true }); + } } return result; } diff --git a/packages/playwright-test/src/types.ts b/packages/playwright-test/src/types.ts index f9d94baaa0..62b612a3a3 100644 --- a/packages/playwright-test/src/types.ts +++ b/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 }; diff --git a/tests/playwright-test/playwright.spec.ts b/tests/playwright-test/playwright.spec.ts index 7956c6123b..026af19372 100644 --- a/tests/playwright-test/playwright.spec.ts +++ b/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 }); diff --git a/tests/playwright-test/test-extend.spec.ts b/tests/playwright-test/test-extend.spec.ts index 8fd3b0af5a..68350c3646 100644 --- a/tests/playwright-test/test-extend.spec.ts +++ b/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'); +});