Skip to content
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

Merged
merged 2 commits into from
Jul 12, 2024

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
Member

@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
Member

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
Member

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
Member

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
Member

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
90 checks passed
@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.

2 participants