Skip to content

Conversation

@andreiborza
Copy link
Member

No description provided.

@andreiborza andreiborza requested review from Lms24, chargome and lforst July 12, 2024 09:44
@andreiborza andreiborza added the Package: solidstart Issues related to the Sentry SolidStart SDK label Jul 12, 2024
Copy link
Contributor

@lforst lforst left a 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';
Copy link
Contributor

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

Copy link
Member Author

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';
Copy link
Contributor

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.

Copy link
Member Author

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',
Copy link
Contributor

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.

Copy link
Member Author

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',
Copy link
Contributor

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

@andreiborza andreiborza requested a review from lforst July 12, 2024 12:30
@andreiborza andreiborza merged commit 6797044 into develop Jul 12, 2024
@andreiborza andreiborza deleted the ab/solidstart-e2e-performance-client-tests branch July 12, 2024 12:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Package: solidstart Issues related to the Sentry SolidStart SDK

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants