Repository navigation
Conversation
Contributor
size-limit report 📦
|
msonnb
changed the base branch from
ms/browser-tests-trace-semantics
to
ms/browser-tests-browser-tracing
October 5, 2026 07:28
msonnb
added this pull request to stack #24891
October 5, 2026 07:28
msonnb
force-pushed
the
ms/browser-tests-requests-web-vitals
branch
from
October 5, 2026 07:41
a8d6d9c to
83fab7a
Compare
msonnb
force-pushed
the
ms/browser-tests-requests-web-vitals
branch
2 times, most recently
from
October 5, 2026 13:23
5a08efb to
dc00794
Compare
msonnb
force-pushed
the
ms/browser-tests-requests-web-vitals
branch
2 times, most recently
from
October 7, 2026 11:27
b188ce7 to
a064fc9
Compare
msonnb
marked this pull request as ready for review
October 7, 2026 12:33
msonnb
requested review from
Lms24 and
logaretm
and removed request for
a team
October 7, 2026 12:33
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
msonnb
force-pushed
the
ms/browser-tests-requests-web-vitals
branch
from
October 7, 2026 12:50
a064fc9 to
3d541db
Compare
msonnb
force-pushed
the
ms/browser-tests-requests-web-vitals
branch
from
October 7, 2026 13:02
3d541db to
0fa7920
Compare
msonnb
force-pushed
the
ms/browser-tests-requests-web-vitals
branch
from
October 7, 2026 13:12
0fa7920 to
136bc99
Compare
msonnb
force-pushed
the
ms/browser-tests-requests-web-vitals
branch
from
October 7, 2026 13:25
136bc99 to
4c8dc91
Compare
Co-Authored-By: GPT-6 <codex@openai.com>
Co-Authored-By: GPT-6 <codex@openai.com>
Co-Authored-By: GPT-6 <codex@openai.com>
msonnb
force-pushed
the
ms/browser-tests-requests-web-vitals
branch
from
October 7, 2026 13:37
4c8dc91 to
52e9455
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 3 total unresolved issues (including 2 from previous reviews).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 52e9455. Configure here.
Co-Authored-By: GPT-6 <codex@openai.com>
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
Comment on lines
+35
to
+36
| expect(lcpSpan.attributes[BROWSER_WEB_VITAL_LCP_ELEMENT]).toEqual({ type: 'string', value: 'body > img' }); | ||
| expect(lcpSpan.attributes[BROWSER_WEB_VITAL_LCP_SIZE]).toEqual({ type: 'integer', value: 107400 }); |
Contributor
There was a problem hiding this comment.
Bug: The LCP test is flaky due to a race condition. It removes a guard for known flakiness and uses a strict assertion that can fail on slow CI machines.
Severity: MEDIUM
Suggested Fix
Reintroduce a mechanism to handle the potential flakiness of LCP element reporting. This could involve using a less strict assertion like expect.stringContaining('body > img') or restoring the conditional logic that checks which element was reported as the LCP before asserting.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location:
dev-packages/browser-integration-tests/suites/tracing/metrics/handlers-lcp/test.ts#L35-L36
Potential issue: The test at `handlers-lcp/test.ts` introduces a race condition.
Previously, the test code included a conditional guard to handle flakiness where the
Largest Contentful Paint (LCP) element could be either an image or a button. This guard
has been removed and replaced with a `waitForFunction` call that waits for the LCP size,
but does not guarantee the LCP element has been finalized. A subsequent button click
finalizes the LCP, but a race condition exists where the button itself can be reported
as the LCP element, causing the new, stricter assertion (`.toEqual({ type: 'string',
value: 'body > img' })`) to fail. This is likely to cause intermittent test failures,
especially in resource-constrained CI environments.
Did we get this right? 👍 / 👎 to inform future reviews.
Member
Author
There was a problem hiding this comment.
the previous test clicked the button immediately, now we wait for the image's size to be reported by the LCP handlers
logaretm
approved these changes
Oct 7, 2026
Lms24
reviewed
Oct 8, 2026
Lms24
left a comment
Lms24
left a comment
Member
There was a problem hiding this comment.
m: I'd prefer keeping one transaction test that asserts on the respective web vital being added as a measurement to the pageload span.
Comment on lines
-18
to
-23
| // A plain (non-streamed) `beforeSendSpan` operates on the v1 `SpanJSON`. INP is sent as a v2 span, | ||
| // so this verifies the static callback still runs and its changes are carried into the v2 span. | ||
| beforeSendSpan: Sentry.withStaticSpan(span => { | ||
| if (span.op === 'ui.interaction.click') { | ||
| span.description = 'scrubbed'; | ||
| span.data['custom.attribute'] = 'from-before-send-span'; |
Member
There was a problem hiding this comment.
m: I think we should keep this test as "static", since the main objective here is asserting that the INP span goes through beforeSendSpan in the v1 format when span streaming is disabled.
So once we remove transactions, this test can be dropped
Comment on lines
+12
to
+22
| 'records connection RTT on pageload and navigation spans', | ||
| async ({ getLocalTestUrl, page, browserName }) => { | ||
| sentryTest.skip(shouldSkipTracingTest() || browserName !== 'chromium'); | ||
| const url = await getLocalTestUrl({ testDir: __dirname }); | ||
| await page.goto(url); | ||
|
|
||
| const pageloadRequest = envelopeRequestParser(await pageloadRequestPromise) as Event; | ||
|
|
||
| const navigationRequestPromise = waitForTransactionRequest( | ||
| page, | ||
| event => event.contexts?.trace?.op === 'navigation', | ||
| ); | ||
| await page.goto(`${url}#foo`); | ||
|
|
||
| const navigationRequest = envelopeRequestParser(await navigationRequestPromise) as Event; | ||
|
|
||
| expect(pageloadRequest.contexts?.trace?.op).toBe('pageload'); | ||
| expect(navigationRequest.contexts?.trace?.op).toBe('navigation'); | ||
| const [pageload] = await waitForStreamedSpanAndTraceHeaderOnUrl(page, url); | ||
| const [navigation] = await waitForStreamedSpanAndTraceHeaderOnUrl(page, `${url}#foo`); | ||
|
|
||
| expect(pageloadRequest.measurements?.['connection.rtt']?.value).toBeDefined(); | ||
| expect(navigationRequest.measurements?.['connection.rtt']).toBeUndefined(); | ||
| expect(pageload.attributes[NETWORK_CONNECTION_RTT]).toEqual({ type: 'integer', value: 0 }); | ||
| expect(navigation.attributes[NETWORK_CONNECTION_RTT]).toEqual(pageload.attributes[NETWORK_CONNECTION_RTT]); | ||
| expect(navigation.attributes[BROWSER_WEB_VITAL_FCP_VALUE]).toBeUndefined(); | ||
| expect(navigation.attributes[BROWSER_WEB_VITAL_TTFB_VALUE]).toBeUndefined(); |
Member
There was a problem hiding this comment.
hmm actually not sure if sending connction.rtt on navigation spans is an SDK bug or expected behaviour now? probably worth looking into. if it's a bug, I'm also fine with merging the test as-is and fixing it in a follow-up.
| expect(streamSpan.end_timestamp).toBeGreaterThan(streamSpan.start_timestamp); | ||
| expect(streamSpan.parent_span_id).toBe(requestSpan.parent_span_id); | ||
| expect(streamSpan.trace_id).toBe(requestSpan.trace_id); | ||
| expect(streamSpan.end_timestamp).toBeGreaterThanOrEqual(streamSpan.start_timestamp); |
Member
There was a problem hiding this comment.
l: any reason this assertion got weaker?
Suggested change
| expect(streamSpan.end_timestamp).toBeGreaterThanOrEqual(streamSpan.start_timestamp); | |
| expect(streamSpan.end_timestamp).toBeGreaterThan(streamSpan.start_timestamp); |
| }); | ||
|
|
||
| expect(requestSpan?.data).not.toHaveProperty('url.fragment'); | ||
| expect(requestSpan?.attributes).not.toHaveProperty('url.fragment'); |
Member
There was a problem hiding this comment.
m: TIL that this check actually asserts that attributes.url.fragment doesn't exist, instead of attributes['url.fragment']. I had no idea 😬
Turns out the correct syntax here is
Suggested change
| expect(requestSpan?.attributes).not.toHaveProperty('url.fragment'); | |
| expect(requestSpan?.attributes).not.toHaveProperty(['url.fragment']); |
Would be amazing if you could go over our tests and check for this pattern. I'm pretty sure we use this more often. But of course as a follow up!
EDIT: Looks like this is "just" a playwright thing. Vitest asserts on the whole key.
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Exercise request instrumentation, web vitals, interactions, and user timing with default span streaming. Retain the existing transaction counterparts for fetch, XHR, resource timing, and TTFB as explicitly pinned
*-staticsuites so the migration preserves coverage of both lifecycles. Check RTT as a span attribute on pageload and navigation.Fixes #24143