-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
Changes from 7 commits
16536be
d7b52fb
b24f625
4316a01
38b7dee
3ef8178
d1273ad
8740761
c8c483c
5eb3734
02d7236
d544059
a72ec6e
9b78566
13a4d19
d8d044e
d841c67
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -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.`); | ||||||
|
@@ -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), | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would you prefer this?
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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), | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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');`, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 With that in mind, I don't think that a teardown function returned from I think this could benefit from another refactoring where we pull There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree, let's not touch the relative behavior of |
||
|
||
'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'); | ||
}); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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"
There was a problem hiding this comment.
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.