-
Notifications
You must be signed in to change notification settings - Fork 26.1k
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
refactor(tests): make chain more "correct" #51728
Conversation
Stats from current PRDefault BuildGeneral
Client Bundles (main, webpack)
Legacy Client Bundles (polyfills)
Client Pages
Client Build Manifests
Rendered Page Sizes
Edge SSR bundle Size
Middleware size
Next Runtimes
|
Tests Passed |
cd81556
to
4f8c6d0
Compare
8150fd2
to
90f3830
Compare
This stack of pull requests is managed by Graphite. Learn more about stacking. Join @ForsakenHarmony and the rest of your teammates on Graphite |
7f5b97a
to
d36aa00
Compare
New and removed dependencies detected. Learn more about Socket for GitHub ↗︎
🚮 Removed packages: npm/@playwright/test@1.35.1, npm/playwright-chromium@1.35.1, npm/playwright-core@1.35.1 |
d223ccd
to
f854146
Compare
e3f0d36
to
732a31a
Compare
This reverts commit 7f5b97a.
732a31a
to
2f960c9
Compare
2f960c9
to
269c86d
Compare
page.on('close', async () => { | ||
await teardown(this.teardownTracing.bind(this)) | ||
}) |
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.
This unfortunately does not work as you have to stop tracing before the page gets closed
Why?
I really dislike the way
.chain
works right now, it shouldn't mutate theBrowserInterface
, this PR changes it so it's just a pure chain without weird side effects.One example with the current version (before this PR):
Additional Changes