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

fix(global setup): simplify ordering #33444

Merged
merged 6 commits into from
Nov 5, 2024
Merged

Conversation

Skn0tt
Copy link
Member

@Skn0tt Skn0tt commented Nov 5, 2024

As.discussed yesterday, this PR follows up on #32955 with some ordering simplification. The new ordering is:

  1. All globalSetup entries, in direct order
  2. All callbacks returned from globalSetup, in reverse order
  3. All globalTeardown entries, in direct order

Before we added support for multiple global setups, a failing callback meant that we wouldn't execute the teardown. This is broken by this change.

@Skn0tt Skn0tt requested a review from dgozman November 5, 2024 07:36
@Skn0tt Skn0tt self-assigned this Nov 5, 2024
expect(result.output).toContain('Error: kaboom');
});

test('globalTeardown runs even if callback failed', async ({ runInlineTest }) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is already covered by globalSetup1 from the previous test, isn't it?

Copy link
Member Author

@Skn0tt Skn0tt Nov 5, 2024

Choose a reason for hiding this comment

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

it is, yes. and it's also covered from the other test I had to touch. but since this is the main breaking behaviour change, and I wanted to have an explicit test to cover this.

This comment has been minimized.

This comment has been minimized.

@Skn0tt
Copy link
Member Author

Skn0tt commented Nov 5, 2024

@dgozman I had to change two more tests here in runner.spec.ts and esm.spec.ts. I'm a little concerned about the runner.spec.ts change: We'll start executing teardown even if setup failed. Let me know if you think this is fine.

@dgozman
Copy link
Contributor

dgozman commented Nov 5, 2024

@dgozman I had to change two more tests here in runner.spec.ts and esm.spec.ts. I'm a little concerned about the runner.spec.ts change: We'll start executing teardown even if setup failed. Let me know if you think this is fine.

Seems fine to me, we have explicitly agreed on this behavior.

@Skn0tt Skn0tt merged commit 8e140a4 into microsoft:main Nov 5, 2024
28 of 29 checks passed
Copy link
Contributor

github-actions bot commented Nov 5, 2024

Test results for "tests 1"

3 failed
❌ [playwright-test] › playwright.spec.ts:181:5 › should respect context options in various contexts @macos-latest-node18-1
❌ [playwright-test] › playwright.trace.spec.ts:137:5 › should not throw with trace: on-first-retry and two retries in the same worker @macos-latest-node18-1
❌ [playwright-test] › ui-mode-test-network-tab.spec.ts:97:5 › should format JSON request body @macos-latest-node18-1

1 flaky ⚠️ [playwright-test] › ui-mode-trace.spec.ts:264:5 › should reveal errors in the sourcetab @macos-latest-node18-1

36757 passed, 678 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.

2 participants