-
-
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
test(solidstart): Add client performance e2e tests #12895
test(solidstart): Add client performance e2e tests #12895
Conversation
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.
lgtm, but I would strongly recommend reducing potential leakage between tests. It's not immediately relevant because the tests are running in serial right now, but once there are many tests, pulling them apart to parallelize is a huge pita
|
||
test('sends a pageload transaction', async ({ page }) => { | ||
const transactionPromise = waitForTransaction('solidstart', async transactionEvent => { | ||
return !!transactionEvent?.transaction && transactionEvent.contexts?.trace?.op === 'pageload'; |
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.
You probably want to assert for a particular transaction name, otherwise the tests are gonna start leaking once you add more page.goto()s
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.
Updated
// The sentry solidRouterBrowserTracingIntegration tries to update such | ||
// transactions with the proper name once the `useLocation` hook triggers. | ||
const navigationTxnPromise = waitForTransaction('solidstart', async transactionEvent => { | ||
return !!transactionEvent?.transaction && transactionEvent.contexts?.trace?.op === 'navigation'; |
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.
The waitForTransaction here and the one in the test above may leak into eachother.
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.
Updated
contexts: { | ||
trace: { | ||
op: 'pageload', | ||
origin: 'auto.pageload.browser', |
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.
Follow up task: We likely want to emit auto.pageload.solidstart.router
here.
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.
Thanks, will update this in a follow up PR.
contexts: { | ||
trace: { | ||
op: 'navigation', | ||
origin: 'auto.navigation.solid.solidrouter', |
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.
Follow-up, this should probably be solidstart.solidrouter
No description provided.