ref(test): Trigger top-level errors inside sandbox. #11712

onurtemizkan · 2024-04-22T08:43:15Z

Resolves: #11678
Related: #11666

This PR updates runScriptInSandbox utility, which was implemented (probably for a similar reason) but not used.
Ports relevant tests to this pattern.

Lms24

Thanks a lot Onur, looks good to me! Just had two suggestions. Also, I have to admit, I completely forgot about injectScriptAndGetEvents. I guess most of us did😅

(Only realized that this is still in draft after starting reviewing, sorry! )

Lms24 · 2024-04-22T11:51:06Z

...s/browser-integration-tests/suites/public-api/instrumentation/onError/non-string-arg/test.ts

+    const [, eventData] = await Promise.all([
+      runScriptInSandbox(page, {
+        content: `
+        throw {
+          type: 'Error',
+          otherKey: 'otherValue',
+        };
+      `,
+      }),
+      getFirstSentryEnvelopeRequest<Event>(page),
+    ]);


Lately, we've been using another pattern in our tests to avoid race conditions and test flakes. While I think in most cases the Promise.all one should work fine, too, I just think the code below is a bit more explicit about the order:

Suggested change

const [, eventData] = await Promise.all([

runScriptInSandbox(page, {

content: `

throw {

type: 'Error',

otherKey: 'otherValue',

};

`,

}),

getFirstSentryEnvelopeRequest<Event>(page),

]);

const errorEventPromise = getFirstSentryEnvelopeRequest<Event>(page);

await runScriptInSandbox(page, {

content: `

throw {

type: 'Error',

otherKey: 'otherValue',

};

`,

});

await errorEventPromise;

(feel free to adjust as necessary)

Would you mind updating to this pattern?

Lms24 · 2024-04-22T15:42:54Z

...ackages/browser-integration-tests/suites/public-api/instrumentation/onError/rethrown/test.ts

-
-    // This is not a refernece error, but another generic error
-    expect(events[1].exception?.values).toHaveLength(1);
-    expect(events[1].exception?.values?.[0]).toMatchObject({
-      type: 'Error',
-      value: 'error 2',
-      mechanism: {
-        type: 'generic',
-        handled: true,
-      },
-      stacktrace: {
-        frames: expect.any(Array),
-      },
-    });


I think this was there before to ensure that the error thrown "in between" is not sent to Sentry. Probably not a super nice way to check that but ensuring something is not sent is always a bit tricky 😅

If you have a better idea how to check for this feel free to go for it; otherwise let's bring it back, wdyt?

I think we can switch back to manually calling window.onerror just for this case here. When we call this without the outer try/catch block, it doesn't call the second captureException as the page crashes.

github-actions · 2024-04-23T15:03:14Z

size-limit report 📦

Path	Size
@sentry/browser	21.64 KB (0%)
@sentry/browser (incl. Tracing)	32.68 KB (0%)
@sentry/browser (incl. Tracing, Replay)	68.03 KB (0%)
@sentry/browser (incl. Tracing, Replay) - with treeshaking flags	61.43 KB (0%)
@sentry/browser (incl. Tracing, Replay with Canvas)	72.07 KB (0%)
@sentry/browser (incl. Tracing, Replay, Feedback)	84.28 KB (0%)
@sentry/browser (incl. Feedback)	37.78 KB (0%)
@sentry/browser (incl. sendFeedback)	26.43 KB (0%)
@sentry/browser (incl. FeedbackAsync)	30.93 KB (0%)
@sentry/react	24.33 KB (0%)
@sentry/react (incl. Tracing)	35.64 KB (0%)
@sentry/vue	25.47 KB (0%)
@sentry/vue (incl. Tracing)	34.47 KB (0%)
@sentry/svelte	21.77 KB (0%)
CDN Bundle	23.95 KB (0%)
CDN Bundle (incl. Tracing)	33.98 KB (0%)
CDN Bundle (incl. Tracing, Replay)	67.67 KB (0%)
CDN Bundle (incl. Tracing, Replay, Feedback)	83.55 KB (0%)
CDN Bundle - uncompressed	70.58 KB (0%)
CDN Bundle (incl. Tracing) - uncompressed	100.94 KB (0%)
CDN Bundle (incl. Tracing, Replay) - uncompressed	210.55 KB (0%)
CDN Bundle (incl. Tracing, Replay, Feedback) - uncompressed	256.94 KB (0%)
@sentry/nextjs (client)	34.86 KB (0%)
@sentry/sveltekit (client)	33.24 KB (0%)
@sentry/node	138.47 KB (+0.01% 🔺)

onurtemizkan · 2024-04-24T09:23:27Z

Thanks for the review @Lms24! Can you please take another look? I had to skip these tests on webkit as they're consistently failing.

AbhiPrasad · 2024-04-25T15:30:16Z

Do you mind rebasing, I think we should be good to go after!

Lms24

Thanks Onur, looks good now (after rebase)!

AbhiPrasad · 2024-04-26T14:33:11Z

Seems like firefox test is failing

[229/231] [firefox] › tracing/browserTracingIntegration/http-timings/test.ts:7:11 › should create fetch spans with http timing @firefox
  1) [firefox] › stacktraces/regular_fn_identifiers/test.ts:7:11 › should parse function identifiers correctly @firefox 

    Error: expect(received).toMatchObject(expected)

    - Expected  - 1
    + Received  + 1

    @@ -10,11 +10,11 @@
        },
        Object {
          "function": "qux/<",
        },
        Object {
    -     "function": "qux/</<",
    +     "function": "qux/<",
        },
        Object {
          "function": "foo",
        },
        Object {

      27 |
      28 |     runInFirefox(() => {
    > 29 |       expect(frames).toMatchObject([
         |                      ^
      30 |         { function: '?' },
      31 |         { function: '?' },
      32 |         { function: 'qux' },

Not sure if it's a flake though.

onurtemizkan · 2024-04-26T15:13:57Z

This looks like the behaviour of the new Firefox version included in the updated Playwright tests. I checked but could not find the related commit there though. Should I update the assertion?

AbhiPrasad · 2024-04-26T15:15:05Z

This looks like the behaviour of the new Firefox version included in the updated Playwright tests. I checked but could not find the related commit there though. Should I update the assertion?

yes let's do that! could we leave a comment also saying in older firefox versions it looks like X?

onurtemizkan force-pushed the onur/browser-test-throws branch 3 times, most recently from 2a4c4dd to 532b1f7 Compare April 22, 2024 14:20

Lms24 reviewed Apr 22, 2024

View reviewed changes

onurtemizkan force-pushed the onur/browser-test-throws branch 2 times, most recently from e9313b0 to eb9b51c Compare April 23, 2024 14:53

onurtemizkan force-pushed the onur/browser-test-throws branch 3 times, most recently from 4a05852 to 5460825 Compare April 24, 2024 09:11

onurtemizkan marked this pull request as ready for review April 24, 2024 09:21

Lms24 approved these changes Apr 25, 2024

View reviewed changes

onurtemizkan force-pushed the onur/browser-test-throws branch 3 times, most recently from a729ca4 to 7deff1b Compare April 26, 2024 09:15

AbhiPrasad approved these changes Apr 26, 2024

View reviewed changes

AbhiPrasad enabled auto-merge (squash) April 26, 2024 13:57

onurtemizkan added 7 commits April 26, 2024 17:27

ref(test): Trigger top-level errors inside sandbox.

0a26b1c

Update playwright to latest.

6b7e918

Switch to Promise.all

8a397c1

Change event waiting pattern.

9f8c64d

Simulate window.onerror in rethrow tests.

c8a819f

Skip runScriptInSandbox tests on webkit.

9144ea4

Await runScriptInSandbox.

38f1421

Update tests to match the new Firefox behaviour.

955e50a

onurtemizkan force-pushed the onur/browser-test-throws branch from 128e951 to 955e50a Compare April 26, 2024 16:27

AbhiPrasad merged commit cda367c into develop Apr 26, 2024
96 checks passed

AbhiPrasad deleted the onur/browser-test-throws branch April 26, 2024 18:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ref(test): Trigger top-level errors inside sandbox. #11712

ref(test): Trigger top-level errors inside sandbox. #11712

onurtemizkan commented Apr 22, 2024

Lms24 left a comment

Lms24 Apr 22, 2024

Lms24 Apr 22, 2024

onurtemizkan Apr 23, 2024

github-actions bot commented Apr 23, 2024 •

edited

Loading

onurtemizkan commented Apr 24, 2024

AbhiPrasad commented Apr 25, 2024

Lms24 left a comment •

edited

Loading

AbhiPrasad commented Apr 26, 2024

onurtemizkan commented Apr 26, 2024

AbhiPrasad commented Apr 26, 2024

ref(test): Trigger top-level errors inside sandbox. #11712

ref(test): Trigger top-level errors inside sandbox. #11712

Conversation

onurtemizkan commented Apr 22, 2024

Lms24 left a comment

Choose a reason for hiding this comment

Lms24 Apr 22, 2024

Choose a reason for hiding this comment

Lms24 Apr 22, 2024

Choose a reason for hiding this comment

onurtemizkan Apr 23, 2024

Choose a reason for hiding this comment

github-actions bot commented Apr 23, 2024 • edited Loading

size-limit report 📦

onurtemizkan commented Apr 24, 2024

AbhiPrasad commented Apr 25, 2024

Lms24 left a comment • edited Loading

Choose a reason for hiding this comment

AbhiPrasad commented Apr 26, 2024

onurtemizkan commented Apr 26, 2024

AbhiPrasad commented Apr 26, 2024

github-actions bot commented Apr 23, 2024 •

edited

Loading

Lms24 left a comment •

edited

Loading