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

Conversation

Skn0tt
Copy link
Member

@Skn0tt Skn0tt commented Oct 4, 2024

Closes #32369

The idea behind ordering is that for pairs of setup/teardown, each pair is only torn down after all pairs executed after it are torn down. This allows "stacking" these setup steps onto another, where one setup depends on the previous setup.

For globalSetup: ['s1', 's2'], globalTeardown: ['t1', 't2'], this results in order s1, s2 ; t2, t1.

There's a second API some people use where setup steps return their respective teardown steps. If that API is used exclusively, the same ordering applies. Mixing both APIs is discouraged, as that gives hard-to-reason-about ordering.

@Skn0tt Skn0tt requested a review from dgozman October 4, 2024 11:15
@Skn0tt Skn0tt self-assigned this Oct 4, 2024
tests/playwright-test/global-setup.spec.ts Outdated Show resolved Hide resolved
tests/playwright-test/global-setup.spec.ts Outdated Show resolved Hide resolved
packages/playwright/src/runner/tasks.ts Show resolved Hide resolved

This comment has been minimized.

@Skn0tt Skn0tt marked this pull request as ready for review October 7, 2024 05:51

This comment has been minimized.

@Skn0tt
Copy link
Member Author

Skn0tt commented Oct 7, 2024

Oof, my change swaps the ordering of existing globalTeardown files and setup-returned teardown functions. That's tricky 🤔

@Skn0tt
Copy link
Member Author

Skn0tt commented Oct 8, 2024

Discussed this with Dima in a call. Reverting the ordering change so we don't break anything. This makes the ordering harder to reason about when folks mix both APIs, but that's discouraged anyways.

@Skn0tt Skn0tt requested a review from dgozman October 8, 2024 14:58

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.

This comment has been minimized.

This comment has been minimized.


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

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.

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.

packages/playwright/src/runner/tasks.ts Show resolved Hide resolved

This comment has been minimized.

tests/playwright-test/global-setup.spec.ts Outdated Show resolved Hide resolved
'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.

packages/playwright/src/common/config.ts Outdated Show resolved Hide resolved
Skn0tt and others added 2 commits October 17, 2024 14:29
Co-authored-by: Dmitry Gozman <dgozman@gmail.com>
Signed-off-by: Simon Knott <info@simonknott.de>

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

@Skn0tt Skn0tt merged commit 0d63df4 into microsoft:main Oct 18, 2024
27 of 29 checks passed
Copy link
Contributor

Test results for "tests 1"

5 flaky ⚠️ [chromium-library] › library/tracing.spec.ts:410:14 › should produce screencast frames fit @chromium-ubuntu-22.04-node22
⚠️ [firefox-page] › page/page-event-request.spec.ts:169:3 › should return response body when Cross-Origin-Opener-Policy is set @firefox-ubuntu-22.04-node18
⚠️ [webkit-library] › library/browsercontext-viewport-mobile.spec.ts:87:5 › mobile viewport › should support window.orientation emulation @webkit-ubuntu-22.04-node18
⚠️ [webkit-library] › library/screenshot.spec.ts:75:14 › page screenshot › should work with device scale factor @webkit-ubuntu-22.04-node18
⚠️ [webkit-library] › library/screenshot.spec.ts:217:14 › element screenshot › element screenshot should work with device scale factor @webkit-ubuntu-22.04-node18

35065 passed, 634 skipped
✔️✔️✔️

Merge workflow run.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature]: Allow multiple global setup and teardown files in the playwright configuration
3 participants