ref(test): Trigger top-level errors inside sandbox.#11712
ref(test): Trigger top-level errors inside sandbox.#11712AbhiPrasad merged 8 commits intodevelopfrom
Conversation
2a4c4dd to
532b1f7
Compare
Lms24
left a comment
There was a problem hiding this comment.
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! )
| const [, eventData] = await Promise.all([ | ||
| runScriptInSandbox(page, { | ||
| content: ` | ||
| throw { | ||
| type: 'Error', | ||
| otherKey: 'otherValue', | ||
| }; | ||
| `, | ||
| }), | ||
| getFirstSentryEnvelopeRequest<Event>(page), | ||
| ]); |
There was a problem hiding this comment.
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:
| 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?
|
|
||
| // 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), | ||
| }, | ||
| }); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
e9313b0 to
eb9b51c
Compare
size-limit report 📦
|
4a05852 to
5460825
Compare
|
Thanks for the review @Lms24! Can you please take another look? I had to skip these tests on webkit as they're consistently failing. |
|
Do you mind rebasing, I think we should be good to go after! |
a729ca4 to
7deff1b
Compare
|
Seems like firefox test is failing Not sure if it's a flake though. |
|
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? |
128e951 to
955e50a
Compare
Resolves: #11678
Related: #11666
This PR updates
runScriptInSandboxutility, which was implemented (probably for a similar reason) but not used.Ports relevant tests to this pattern.