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: Port onerror tests to playwright #11666

Merged
merged 7 commits into from
Apr 18, 2024
Merged

Conversation

AbhiPrasad
Copy link
Member

Trying #11543 again.

This ports packages/browser/test/integration/suites/onerror.js to playwright. Because I couldn't throw top level errors without generating script errors, I elected to simulate window.onerror being called.

ref #11084

@AbhiPrasad AbhiPrasad requested a review from a team April 18, 2024 00:52
@AbhiPrasad AbhiPrasad self-assigned this Apr 18, 2024
@AbhiPrasad AbhiPrasad requested review from Lms24 and s1gr1d and removed request for a team April 18, 2024 00:52
Copy link
Member

@Lms24 Lms24 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

@Lms24
Copy link
Member

Lms24 commented Apr 18, 2024

General comment; not blocking this PR:

Long-term, I'd like us to move away from these kinds of workarounds where we simulate something that rather than doing it more "naturally". This behaviour doesn't resemble how errors would actually be thrown a real-life project. Because of the "Script Error" limitation we have very few tests where we just throw errors naturally.

To be fair, calling window.onerror at least triggers our GlobalHandlers patch so it's better than calling captureException. Maybe we can look into some kind of asynchronously triggered subject.js script so that we don't always have to manually trigger an async/button click event to throw an error that's not dropped by the SDK.

I opened #11678 to track this more broadly.

…umentation/onError/non-string-arg/test.ts

Co-authored-by: Lukas Stracke <lukas.stracke@sentry.io>
Copy link
Contributor

github-actions bot commented Apr 18, 2024

size-limit report 📦

Path Size
@sentry/browser 21.67 KB (0%)
@sentry/browser (incl. Tracing) 31.4 KB (0%)
@sentry/browser (incl. Tracing, Replay) 66.73 KB (0%)
@sentry/browser (incl. Tracing, Replay) - with treeshaking flags 60.14 KB (0%)
@sentry/browser (incl. Tracing, Replay with Canvas) 70.57 KB (0%)
@sentry/browser (incl. Tracing, Replay, Feedback) 80.44 KB (0%)
@sentry/browser (incl. Feedback) 35.25 KB (0%)
@sentry/browser (incl. Feedback, Feedback Modal) 35.25 KB (0%)
@sentry/browser (incl. Feedback, Feedback Modal, Feedback Screenshot) 37.28 KB (0%)
@sentry/browser (incl. sendFeedback) 26.46 KB (0%)
@sentry/react 24.35 KB (0%)
@sentry/react (incl. Tracing) 34.31 KB (0%)
@sentry/vue 25.2 KB (0%)
@sentry/vue (incl. Tracing) 33.12 KB (0%)
@sentry/svelte 21.79 KB (0%)
CDN Bundle 24.03 KB (-0.02% 🔽)
CDN Bundle (incl. Tracing) 32.71 KB (-0.01% 🔽)
CDN Bundle (incl. Tracing, Replay) 66.37 KB (-0.01% 🔽)
CDN Bundle (incl. Tracing, Replay, Feedback) 82.57 KB (-0.01% 🔽)
CDN Bundle - uncompressed 70.85 KB (-0.02% 🔽)
CDN Bundle (incl. Tracing) - uncompressed 97.5 KB (-0.02% 🔽)
CDN Bundle (incl. Tracing, Replay) - uncompressed 207.16 KB (-0.01% 🔽)
@sentry/nextjs (client) 33.64 KB (0%)
@sentry/sveltekit (client) 31.9 KB (0%)
@sentry/node 156.04 KB (+0.12% 🔺)

@AbhiPrasad AbhiPrasad enabled auto-merge (squash) April 18, 2024 17:01
@AbhiPrasad AbhiPrasad merged commit b64b2ae into develop Apr 18, 2024
92 checks passed
@AbhiPrasad AbhiPrasad deleted the abhi-karma-test-port branch April 18, 2024 17:16
AbhiPrasad added a commit that referenced this pull request Apr 24, 2024
ref #11084

This test ports
`packages/browser/test/integration/suites/onunhandledrejection.js`
playwright. Because of the same limitations as outlined with the on
error tests #11666, I
had to use calls to `window.onunhandledrejection` to simulate these
tests instead of just using `Promise.reject` to test the handler.

#11678 tracks being
able to fix this so we can avoid directly calling
`window.onunhandledrejection` to test.

As `onunhandledrejection.js` was the last suite to use the old
integration tests, I fully removed that code and the corresponding GH
action workflow. I also removed the monorepo deps on `karma`, `chai` and
`sinon`. Extremely satisfying.
AbhiPrasad pushed a commit that referenced this pull request Apr 26, 2024
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants