Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(test runner): allow multiple global setups #32955

Merged
merged 17 commits into from
Oct 18, 2024
Merged
8 changes: 4 additions & 4 deletions docs/src/test-api/class-testconfig.md
Original file line number Diff line number Diff line change
Expand Up @@ -110,9 +110,9 @@ export default defineConfig({

## property: TestConfig.globalSetup
* since: v1.10
- type: ?<[string]>
- type: ?<[string]|[Array]<[string]>>

Path to the global setup file. This file will be required and run before all the tests. It must export a single function that takes a [FullConfig] argument.
Path to the global setup file. This file will be required and run before all the tests. It must export a single function that takes a [FullConfig] argument. Pass an array for multiple global setup files.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I kept this pretty short so we don't overload it. Let me know if you think we need to talk about ordering here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We literally overload it here, which is fine. Not sure the wording of your doc is right though, please revise.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's right - what would you recommend instead?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are fine. It felt like a couple of words are missing. "Pass an array of file names to specify multiple global setup files"

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good idea! added the words in c8c483c.


Learn more about [global setup and teardown](../test-global-setup-teardown.md).

Expand All @@ -128,9 +128,9 @@ export default defineConfig({

## property: TestConfig.globalTeardown
* since: v1.10
- type: ?<[string]>
- type: ?<[string]|[Array]<[string]>>

Path to the global teardown file. This file will be required and run after all the tests. It must export a single function. See also [`property: TestConfig.globalSetup`].
Path to the global teardown file. This file will be required and run after all the tests. It must export a single function. See also [`property: TestConfig.globalSetup`]. Pass an array for multiple global teardown files.

Learn more about [global setup and teardown](../test-global-setup-teardown.md).

Expand Down
10 changes: 8 additions & 2 deletions packages/playwright/src/common/config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,9 @@ export class FullConfigInternal {
testIdMatcher?: Matcher;
defineConfigWasUsed = false;

globalSetups: string[] = [];
globalTeardowns: string[] = [];

constructor(location: ConfigLocation, userConfig: Config, configCLIOverrides: ConfigCLIOverrides) {
if (configCLIOverrides.projects && userConfig.projects)
throw new Error(`Cannot use --browser option when configuration file defines projects. Specify browserName in the projects instead.`);
Expand All @@ -70,13 +73,16 @@ export class FullConfigInternal {
const privateConfiguration = (userConfig as any)['@playwright/test'];
this.plugins = (privateConfiguration?.plugins || []).map((p: any) => ({ factory: p }));

this.globalSetups = (Array.isArray(userConfig.globalSetup) ? userConfig.globalSetup : [userConfig.globalSetup]).map(s => resolveScript(s, configDir)).filter((script): script is string => !!script);
Skn0tt marked this conversation as resolved.
Show resolved Hide resolved
this.globalTeardowns = (Array.isArray(userConfig.globalTeardown) ? userConfig.globalTeardown : [userConfig.globalTeardown]).map(s => resolveScript(s, configDir)).filter((script): script is string => !!script);

this.config = {
configFile: resolvedConfigFile,
rootDir: pathResolve(configDir, userConfig.testDir) || configDir,
forbidOnly: takeFirst(configCLIOverrides.forbidOnly, userConfig.forbidOnly, false),
fullyParallel: takeFirst(configCLIOverrides.fullyParallel, userConfig.fullyParallel, false),
globalSetup: takeFirst(resolveScript(userConfig.globalSetup, configDir), null),
globalTeardown: takeFirst(resolveScript(userConfig.globalTeardown, configDir), null),
globalSetup: takeFirst(...this.globalSetups, null),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: there isn't much to take first from, this is pretty much | null

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you prefer this?

Suggested change
globalSetup: takeFirst(...this.globalSetups, null),
globalSetup: this.globalSetups[0] ?? null,

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit means "while you are here if you also find it weird, replace it". Your suggestion works.

globalTeardown: takeFirst(...this.globalTeardowns, null),
globalTimeout: takeFirst(configCLIOverrides.globalTimeout, userConfig.globalTimeout, 0),
grep: takeFirst(userConfig.grep, defaultGrep),
grepInvert: takeFirst(userConfig.grepInvert, null),
Expand Down
16 changes: 14 additions & 2 deletions packages/playwright/src/common/configLoader.ts
Original file line number Diff line number Diff line change
Expand Up @@ -137,13 +137,25 @@ function validateConfig(file: string, config: Config) {
}

if ('globalSetup' in config && config.globalSetup !== undefined) {
if (typeof config.globalSetup !== 'string')
if (Array.isArray(config.globalSetup)) {
config.globalSetup.forEach((item, index) => {
if (typeof item !== 'string')
throw errorWithFile(file, `config.globalSetup[${index}] must be a string`);
});
} else if (typeof config.globalSetup !== 'string') {
throw errorWithFile(file, `config.globalSetup must be a string`);
}
}

if ('globalTeardown' in config && config.globalTeardown !== undefined) {
if (typeof config.globalTeardown !== 'string')
if (Array.isArray(config.globalTeardown)) {
config.globalTeardown.forEach((item, index) => {
if (typeof item !== 'string')
throw errorWithFile(file, `config.globalTeardown[${index}] must be a string`);
});
} else if (typeof config.globalTeardown !== 'string') {
throw errorWithFile(file, `config.globalTeardown must be a string`);
}
}

if ('globalTimeout' in config && config.globalTimeout !== undefined) {
Expand Down
20 changes: 14 additions & 6 deletions packages/playwright/src/runner/tasks.ts
Original file line number Diff line number Diff line change
Expand Up @@ -98,8 +98,11 @@ export function createGlobalSetupTasks(config: FullConfigInternal) {
if (!config.configCLIOverrides.preserveOutputDir && !process.env.PW_TEST_NO_REMOVE_OUTPUT_DIRS)
tasks.push(createRemoveOutputDirsTask());
tasks.push(...createPluginSetupTasks(config));
if (config.config.globalSetup || config.config.globalTeardown)
tasks.push(createGlobalSetupTask());
if (config.globalSetups.length || config.globalTeardowns.length) {
const length = Math.max(config.globalSetups.length, config.globalTeardowns.length);
for (let i = 0; i < length; i++)
tasks.push(createGlobalSetupTask(i, length));
}
return tasks;
}

Expand Down Expand Up @@ -161,15 +164,20 @@ function createPluginBeginTask(plugin: TestRunnerPluginRegistration): Task<TestR
};
}

function createGlobalSetupTask(): Task<TestRun> {
function createGlobalSetupTask(index: number, length: number): Task<TestRun> {
Skn0tt marked this conversation as resolved.
Show resolved Hide resolved
let globalSetupResult: any;
let globalSetupFinished = false;
let teardownHook: any;

let title = 'global setup';
if (length > 1)
title += ` #${index}`;

return {
title: 'global setup',
title,
setup: async ({ config }) => {
const setupHook = config.config.globalSetup ? await loadGlobalHook(config, config.config.globalSetup) : undefined;
teardownHook = config.config.globalTeardown ? await loadGlobalHook(config, config.config.globalTeardown) : undefined;
const setupHook = config.globalSetups[index] ? await loadGlobalHook(config, config.globalSetups[index]) : undefined;
teardownHook = config.globalTeardowns[index] ? await loadGlobalHook(config, config.globalTeardowns[index]) : undefined;
globalSetupResult = setupHook ? await setupHook(config.config) : undefined;
globalSetupFinished = true;
},
Expand Down
10 changes: 6 additions & 4 deletions packages/playwright/types/test.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1077,7 +1077,8 @@ interface TestConfig<TestArgs = {}, WorkerArgs = {}> {

/**
* Path to the global setup file. This file will be required and run before all the tests. It must export a single
* function that takes a [FullConfig](https://playwright.dev/docs/api/class-fullconfig) argument.
* function that takes a [FullConfig](https://playwright.dev/docs/api/class-fullconfig) argument. Pass an array for
* multiple global setup files.
*
* Learn more about [global setup and teardown](https://playwright.dev/docs/test-global-setup-teardown).
*
Expand All @@ -1093,12 +1094,13 @@ interface TestConfig<TestArgs = {}, WorkerArgs = {}> {
* ```
*
*/
globalSetup?: string;
globalSetup?: string|Array<string>;

/**
* Path to the global teardown file. This file will be required and run after all the tests. It must export a single
* function. See also
* [testConfig.globalSetup](https://playwright.dev/docs/api/class-testconfig#test-config-global-setup).
* [testConfig.globalSetup](https://playwright.dev/docs/api/class-testconfig#test-config-global-setup). Pass an array
* for multiple global teardown files.
*
* Learn more about [global setup and teardown](https://playwright.dev/docs/test-global-setup-teardown).
*
Expand All @@ -1114,7 +1116,7 @@ interface TestConfig<TestArgs = {}, WorkerArgs = {}> {
* ```
*
*/
globalTeardown?: string;
globalTeardown?: string|Array<string>;

/**
* Maximum time in milliseconds the whole test suite can run. Zero timeout (default) disables this behavior. Useful on
Expand Down
40 changes: 40 additions & 0 deletions tests/playwright-test/global-setup.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -386,3 +386,43 @@ test('teardown after error', async ({ runInlineTest }) => {
'teardown 1',
]);
});

test('globalSetup should support multiple', async ({ runInlineTest }) => {
const result = await runInlineTest({
'playwright.config.ts': `
module.exports = {
globalSetup: ['./globalSetup1.ts','./globalSetup2.ts','./globalSetup3.ts','./globalSetup4.ts'],
globalTeardown: ['./globalTeardown1.ts', './globalTeardown2.ts'],
};
`,
'globalSetup1.ts': `module.exports = () => { console.log('%%globalSetup1'); return () => { console.log('%%globalSetup1Function'); throw new Error('kaboom'); } };`,
'globalSetup2.ts': `module.exports = () => console.log('%%globalSetup2');`,
'globalSetup3.ts': `module.exports = () => { console.log('%%globalSetup3'); return () => console.log('%%globalSetup3Function'); }`,
'globalSetup4.ts': `module.exports = () => console.log('%%globalSetup4');`,
'globalTeardown1.ts': `module.exports = () => console.log('%%globalTeardown1')`,
'globalTeardown2.ts': `module.exports = () => console.log('%%globalTeardown2');`,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd also throw here, and check that it did not prevent globalTeardown1 from running.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did that in a72ec6e. This behaviour is surprising to me, was somehow expecting a failing teardown to stop further teardowns from being executed.

With that in mind, I don't think that a teardown function returned from globalSetup should behave any differently than a globalTeardown when it throws an error. I've made sure that a failing function doesn't block any globalTeardown script from being executed in 9b78566. Would be lovely if you could give that another look.

I think this could benefit from another refactoring where we pull globalSetup and globalTeardown scripts into separate tasks, but i'd like to get this PR merged first and then refactor in a follow-up.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On a second note, this could be considered a breaking change. If folks are currently relying on the globalSetup result throwing an error to prevent globalTeardown from being executed, that's broken with 9b78566. So maybe we should just stick with the incongruence.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, let's not touch the relative behavior of globalSetup returned function vs globalTeardown. The aim here is to support multiple, and our general rule is that all teardowns run even if one of them fails. This applies to fixtures, afterEach and afterAll hooks, and now to global teardown as well.


'a.test.js': `
import { test } from '@playwright/test';
test('a', () => console.log('%%test a'));
test('b', () => console.log('%%test b'));
`,
}, { reporter: 'line' });
expect(result.passed).toBe(2);

// behaviour: setups in order, teardowns in reverse order.
// setup-returned functions inherit their position, and take precedence over `globalTeardown` scripts.
expect(result.outputLines).toEqual([
'globalSetup1',
'globalSetup2',
'globalSetup3',
'globalSetup4',
'test a',
'test b',
'globalSetup3Function',
'globalTeardown2',
'globalSetup1Function',
// 'globalTeardown1' is missing, because globalTeardown1 errored out.
Skn0tt marked this conversation as resolved.
Show resolved Hide resolved
]);
expect(result.output).toContain('Error: kaboom');
});