-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
chore(deps): Update playwright to 1.50.0 #15164
Conversation
❌ 3 Tests Failed:
View the top 3 failed tests by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
Hmm, looking some more into this 😬 it seems that Firefox has finally implemented |
transportOptions: { | ||
fetchOptions: { | ||
// See: https://github.com/microsoft/playwright/issues/34497 | ||
keepalive: false, |
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.
Important note: This is apparently a bug in playwright/firefox 🤔 I opened an issue here: microsoft/playwright#34497 but this seems to fix it. I tried to reproduce this in a regular FF instance, and it seems that Sentry data is correctly sent, but cannot be inspected. Possibly this interferes with PW abilities to intercept stuff too.
size-limit report 📦
|
@@ -100,6 +98,7 @@ sentryTest( | |||
data: { | |||
'browser.script.invoker': 'BUTTON#clickme.onclick', | |||
'browser.script.invoker_type': 'event-listener', | |||
'code.filepath': 'https://example.com/path/to/script.js', |
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 is new, but seems correct to me!
@@ -102,6 +100,7 @@ sentryTest( | |||
data: { | |||
'browser.script.invoker': 'BUTTON#clickme.onclick', | |||
'browser.script.invoker_type': 'event-listener', | |||
'code.filepath': 'https://example.com/path/to/script.js', |
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 is new, but seems correct to me!
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.
Probably came from the browser version bump 🤔
// renderTime is 0 because we don't return the `Timing-Allow-Origin` header | ||
// and the image is loaded from a 3rd party origin | ||
expect(eventData.contexts?.trace?.data?.['lcp.renderTime']).toBe(0); | ||
expect(eventData.contexts?.trace?.data?.['lcp.renderTime']).toBeGreaterThan(0); |
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.
@Lms24 are these changes correct? 😅
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 this change can be attributed to browsers now exposing renderTime even without the timing-allow-origin header: https://developer.mozilla.org/en-US/docs/Web/API/PerformanceElementTiming/renderTime#cross-origin_image_render_time
Which makes me think that we can probably merge both tests in this file. Feel free to do in this PR, otherwise I can also do it in a follow-up PR.
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.
ah, I see. I'll merge these then, apparently that changed in chromium versions here!
// renderTime is 0 because we don't return the `Timing-Allow-Origin` header | ||
// and the image is loaded from a 3rd party origin | ||
expect(eventData.contexts?.trace?.data?.['lcp.renderTime']).toBe(0); | ||
expect(eventData.contexts?.trace?.data?.['lcp.renderTime']).toBeGreaterThan(0); |
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 this change can be attributed to browsers now exposing renderTime even without the timing-allow-origin header: https://developer.mozilla.org/en-US/docs/Web/API/PerformanceElementTiming/renderTime#cross-origin_image_render_time
Which makes me think that we can probably merge both tests in this file. Feel free to do in this PR, otherwise I can also do it in a follow-up PR.
3943ca4
to
f5550b7
Compare
if (process.env.CI) { | ||
console.log('::endgroup::'); | ||
} | ||
return paths.map(p => `${path.dirname(p)}/`); |
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 was a bit buggy - e.g. if one dirname was suites/tracing/metrics/web-vitals/test.ts
, it would have made it to suites/tracing/metrics/web-vitals
, which then in turn also matches e.g. web-vitals-cls
etc. dirs. Now, it should be stricter in that regard.
return paths.map(p => path.dirname(p)); | ||
} | ||
|
||
function logError(error: unknown) { |
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.
was not used
@@ -156,7 +127,7 @@ function logError(error: unknown) { | |||
function getApproximateNumberOfTests(testPath: string): number { | |||
try { | |||
const content = fs.readFileSync(path.join(process.cwd(), testPath, 'test.ts'), 'utf-8'); | |||
const matches = content.match(/it\(|test\(|sentryTest\(/g); | |||
const matches = content.match(/sentryTest\(/g); |
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 always use sentryTest
, so no need to check others here IMHO
b0b2f85
to
6865dc2
Compare
Also align to use the same version everywhere, tilde-restricted.
6865dc2
to
aaac579
Compare
Also align to use the same version everywhere, tilde-restricted.