test(browser): Add e2e tests for the `@sentry/browser` package #13125

Zen-cronic · 2024-07-31T01:01:22Z

Addresses #12662

Added e2e tests for @sentry/browser in dev-packages/e2e-tests/test-applications named default-browser.

If you've added code that should be tested, please add tests.
Ensure your code lints and the test suite passes (yarn lint) & (yarn test).

Zen-cronic · 2024-07-31T01:02:27Z

The tests for transaction events will be added next.

Lms24

Hey @Zen-cronic thanks a lot for opening this PR!

Let us know when this is ready to review or if you have questions along the way. I gave your current draft a quick review and have two suggestions:

to add this test to CI, we need to add an entry for the test app in the .github/build.yml matrix
Ideally, we not only test errors but also spans, specifically pageload and navigation spans. You already added browserTracingIntegration to your Sentry.init call, so we should get these events as well. I see you're already perfectly using our usual test pattern so you can take some other performance test files as example.

Great work!

Lms24 · 2024-08-01T12:02:53Z

just FYI: I assigned myself to this PR for internal tracking purposes since I'm gonna review it. So no worries, I'm not taking over or anything. Please continue as before :)

Zen-cronic · 2024-08-01T23:25:58Z

to add this test to CI, we need to add an entry for the test app in the .github/build.yml matrix

Ideally, we not only test errors but also spans, specifically pageload and navigation spans. You already added browserTracingIntegration to your Sentry.init call, so we should get these events as well. I see you're already perfectly using our usual test pattern so you can take some other performance test files as example.

Thanks for the heads up!

I ran into this issue though:

Testing errors with waitForError works just fine; the error event type is emitted and the test succeeds.
However, using waitForTransaction to check for spans and transactions seem to hang. I debugged it and compare the results to other successful test suites (like react-router-6), and it seems that the transaction or span event types are not emitted.

There could be something lacking in my test setup, but waitForError works with the current setup.

test('Should set correct transaction', async ({ page }) => {

 // `pageload` transaction
  const transactionPromise = waitForTransaction('default-browser', async transactionEvent => {
    return !!transactionEvent?.transaction && transactionEvent.contexts?.trace?.op === 'pageload';
  });

  await page.goto(`/`);

  // `waitForTransaction` hangs here because `transaction` (or `span`) event is not emitted,
  // so the code past this line is never executed.
  const pageLoadTransaction = await transactionPromise;

  // ...assertions
});

Lms24 · 2024-08-02T14:19:36Z

@Zen-cronic You can run the pnpm test:assert --ui to run the test in PlayWright's UI mode. This will let you check if reqeusts from the SDK were sent like in browser dev tools. Also, if you enable debug: true in your Sentry.init call, you'll get debug logs that might hint if a span is dropped for some reason.

Also it's helpful to log whatever comes into your waitForTransaction callback, the check looks fine to me on first glance but maybe something is missing

Zen-cronic · 2024-08-03T15:45:39Z

dev-packages/e2e-tests/test-applications/default-browser/src/index.js

@@ -0,0 +1,12 @@
+import * as Sentry from '@sentry/react';


Should be @sentry/browser instead.

Lms24 · 2024-08-06T12:45:36Z

@Zen-cronic anything I can help you with or are you fine for the moment? Don't mean to rush you btw, so just let me know :)

Zen-cronic · 2024-08-06T15:35:45Z

Hey @Lms24

thanks for looking out! I got stuck after some more debugging and testing. Here's what I've tried and observed:

With the debug option, span and transaction related events are not logged when using waitForTransaction. However, in other successful tests (react-19, react-17, react-router-6, etc.), those events are emitted correctly.
I tried logging the event object from beforeSendSpan and beforeSendTransaction options of Sentry.init(), but those functions are not invoked at all. Which verifies that span and transaction are not emitted.
Testing with the --ui flag also doesn't help, but further confirms that span and transaction are not emitted. Because the ui tests still hang when the code encounter the call to waitForTransaction.
The e2e tests for webpack-4 and webpack-5 are missing tests for spans and transactions (only errors). And when I tried testing with waitForTransaction (without changing any setup) the same behaviour is observed as in the default-browser package.

^The last point seems really important, because the same "bug" is found in an already existing package.

Some possible root causes:

webpack. Because the other tests that doesn't use webpack but use waitForTransaction work. I could try a minimal vite setup.
The waitForTransaction function. But after debugging, I don't believe it's the culprit: although it hangs, that's because the span or transaction is not being emitted.

Let me know what you think

Lms24 · 2024-08-06T16:03:00Z

Thanks for the update! I set a reminder to take a look tomorrow. Perhaps there really is a bug that we need to fix.

Zen-cronic · 2024-08-06T16:28:38Z

Sounds great!

Lms24

Hey @Zen-cronic I took a look at the test app and found the reason why transactions were not sent (see comments). I also took the liberty to change some other small things and rebase the PR to the current develop head.

Now before we merge this, there's one thing I think we should still test, which is that we send a navigation span/transaction as well. For this to test, we need to perform a soft navigation in the browser which we can easiest do with one of these actions:

clicking on an element that redirects you to an anchor link (e.g. /#somewhere)
calling something like window.history.pushState({}, '', '/test')

Would you be interested in adding this test? If not just let me know then I'll do it.
Again, great work!

Lms24 · 2024-08-07T08:30:55Z

dev-packages/e2e-tests/test-applications/default-browser/src/index.js

+  integrations: [Sentry.browserTracingIntegration()],
+  tracesSampleRate: 1.0,
+  release: 'e2e-test',
+  environment: 'qa',


just FYI, you couldn't have known this: setting the environment to 'qa' is something we do that, if we actually want to send events from this app to Sentry, we can make sure it arrives and isn't dynamically sampled/dropped by the Sentry backend.

Lms24 · 2024-08-07T08:31:43Z

dev-packages/e2e-tests/test-applications/default-browser/src/index.js

+Sentry.init({
+  dsn: process.env.E2E_TEST_DSN,
+  integrations: [Sentry.browserTracingIntegration()],
+  tracesSampleRate: 1.0,


For spans and transactions to be emitted, you also need to set tracesSampleRate to >0. In case of e2e tests, for consistent results, we set it to 1

This really was the missing link! Only now do i notice that all the e2e suites that test for spans and transactions have this option enabled to 1.0.

Lms24 · 2024-08-07T08:32:01Z

dev-packages/e2e-tests/test-applications/default-browser/src/index.js

@@ -0,0 +1,14 @@
+import * as Sentry from '@sentry/browser';


Changed to @sentry/browser as you already identified :)

Lms24 · 2024-08-07T08:33:45Z

dev-packages/e2e-tests/test-applications/default-browser/tests/performance.test.ts

+  // so the code past this line is never executed.
+  const pageLoadTransaction = await transactionPromise;
+
+  expect(pageLoadTransaction).toEqual({


I thought I'd assert on quite a few fields here since we rarely do it this detailed and I'm curious if we get inconsistencies/flakes in this simple app already. We might need to adjust some checks if they get flaky but at least locally, this 100% consistent.

Lms24 · 2024-08-07T08:34:02Z

.github/workflows/build.yml

@@ -859,6 +859,7 @@ jobs:
            'create-remix-app-express',
            'create-remix-app-express-legacy',
            'create-remix-app-express-vite-dev',
+            'default-browser',


I added the test app to our CI e2e testing matrix

Zen-cronic · 2024-08-07T12:27:59Z

Now before we merge this, there's one thing I think we should still test, which is that we send a navigation span/transaction as well. For this to test, we need to perform a soft navigation in the browser which we can easiest do with one of these actions:

clicking on an element that redirects you to an anchor link (e.g. /#somewhere)

calling something like window.history.pushState({}, '', '/test')

Would you be interested in adding this test? If not just let me know then I'll do it. Again, great work!

I'd definitely like to take it up. Thanks for all the detailed review!

Zen-cronic · 2024-08-07T16:04:59Z

hey @Lms24, the pr is ready whenever you're available. I believe there's no lint issues bcuz running yarn lint in the root of e2e-tests emit no warnings whatsoever on my side.

Let me know if there's anything more needed in this pr.

Lms24

Hey, thanks a lot! The navigation test looks good to me. Let's just move it into performance.test.ts and remove the transaction.test.ts file in favour of it. Then I'll merge the PR. Thanks!

dev-packages/e2e-tests/test-applications/default-browser/tests/transactions.test.ts

Zen-cronic · 2024-08-08T13:57:30Z

Hey, thanks a lot! The navigation test looks good to me. Let's just move it into performance.test.ts and remove the transaction.test.ts file in favour of it. Then I'll merge the PR. Thanks!

Alrighty, all fixed now.

Lms24

Thanks a lot @Zen-cronic! We'll make sure to give you a shoutout in the next release :)

Will merge this soon!

Zen-cronic · 2024-08-08T14:25:29Z

No worries @Lms24 and thanks for your guidance. Glad to have helped!

- tracesSampleRate: 1 - environment: 'qa' - add to CI

- Update the simple web app to include soft navigation action - Include tests for `pageload` and `navigation` transactions/spans

This PR adds the external contributor to the CHANGELOG.md file, so that they are credited for their contribution. See #13125 --- Co-authored-by: Lukas Stracke <[email protected]>

Zen-cronic marked this pull request as draft July 31, 2024 01:04

Zen-cronic marked this pull request as ready for review July 31, 2024 01:08

Zen-cronic marked this pull request as draft July 31, 2024 01:08

Lms24 reviewed Jul 31, 2024

View reviewed changes

Lms24 self-assigned this Aug 1, 2024

Zen-cronic commented Aug 3, 2024

View reviewed changes

Lms24 force-pushed the e2e/default-browser branch from 12ec8b5 to b6dcb74 Compare August 7, 2024 08:22

Lms24 reviewed Aug 7, 2024

View reviewed changes

Zen-cronic marked this pull request as ready for review August 7, 2024 15:56

Zen-cronic changed the title ~~test(e2e): Add e2e tests for the @sentry/browser package~~ test(browser): Add e2e tests for the @sentry/browser package Aug 8, 2024

Lms24 reviewed Aug 8, 2024

View reviewed changes

dev-packages/e2e-tests/test-applications/default-browser/tests/transactions.test.ts Outdated Show resolved Hide resolved

dev-packages/e2e-tests/test-applications/default-browser/tests/transactions.test.ts Outdated Show resolved Hide resolved

Lms24 approved these changes Aug 8, 2024

View reviewed changes

Zen-cronic and others added 6 commits August 13, 2024 09:18

test(browser): Add initial setup for e2e test

1ce8db3

test(browser): Revert unrelated test packages to original values

055fd3d

test(browser): Add error event test

6e91d7c

some fixes

031ce47

- tracesSampleRate: 1 - environment: 'qa' - add to CI

add performance test

3854de4

fix formatting

728cc13

Zen-cronic added 3 commits August 13, 2024 09:18

test(browser): Add tests for span and transaction events

5d3c21b

- Update the simple web app to include soft navigation action - Include tests for `pageload` and `navigation` transactions/spans

test(browser): Update the test app to include soft navigation action

29ac5f5

test(browser): Remove redundant test

ebf9dd9

Lms24 force-pushed the e2e/default-browser branch from d2e459d to ebf9dd9 Compare August 13, 2024 07:18

Lms24 enabled auto-merge (squash) August 13, 2024 07:19

Lms24 merged commit 6aeaf42 into getsentry:develop Aug 13, 2024
121 checks passed

github-actions bot mentioned this pull request Aug 13, 2024

ref: Add external contributor to CHANGELOG.md #13334

Merged

Lms24 mentioned this pull request Sep 10, 2024

Add E2E test app for basic @sentry/browser #12662

Closed

Uh oh!

test(browser): Add e2e tests for the @sentry/browser package #13125

test(browser): Add e2e tests for the @sentry/browser package #13125

Uh oh!

Conversation

Zen-cronic commented Jul 31, 2024

Uh oh!

Zen-cronic commented Jul 31, 2024

Uh oh!

Lms24 left a comment

Choose a reason for hiding this comment

Uh oh!

Lms24 commented Aug 1, 2024

Uh oh!

Zen-cronic commented Aug 1, 2024

Uh oh!

Lms24 commented Aug 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Zen-cronic Aug 3, 2024

Choose a reason for hiding this comment

Uh oh!

Lms24 commented Aug 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Zen-cronic commented Aug 6, 2024

Uh oh!

Lms24 commented Aug 6, 2024

Uh oh!

Zen-cronic commented Aug 6, 2024

Uh oh!

Lms24 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Lms24 Aug 7, 2024

Choose a reason for hiding this comment

Uh oh!

Lms24 Aug 7, 2024

Choose a reason for hiding this comment

Uh oh!

Zen-cronic Aug 7, 2024

Choose a reason for hiding this comment

Uh oh!

Lms24 Aug 7, 2024

Choose a reason for hiding this comment

Uh oh!

Lms24 Aug 7, 2024

Choose a reason for hiding this comment

Uh oh!

Lms24 Aug 7, 2024

Choose a reason for hiding this comment

Uh oh!

Zen-cronic commented Aug 7, 2024

Uh oh!

Zen-cronic commented Aug 7, 2024

Uh oh!

Lms24 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Zen-cronic commented Aug 8, 2024

Uh oh!

Lms24 left a comment

Choose a reason for hiding this comment

Uh oh!

Zen-cronic commented Aug 8, 2024

Uh oh!

Uh oh!

Uh oh!

test(browser): Add e2e tests for the `@sentry/browser` package #13125

test(browser): Add e2e tests for the `@sentry/browser` package #13125

Lms24 commented Aug 2, 2024 •

edited

Loading

Lms24 commented Aug 6, 2024 •

edited

Loading

Lms24 left a comment •

edited

Loading