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

Improve error throwing behaviour in browser integration test #11678

Closed
Lms24 opened this issue Apr 18, 2024 · 3 comments · Fixed by #11712
Closed

Improve error throwing behaviour in browser integration test #11678

Lms24 opened this issue Apr 18, 2024 · 3 comments · Fixed by #11712

Comments

@Lms24
Copy link
Member

Lms24 commented Apr 18, 2024

Problem Statement

Currently, it's quite annoying to just throw an error in a browser integration test and to check for the resulting payload. The intuitive way would be to just top-level call throw new Error() in subject.js. However, the resulting error gets thrown as a "Script Error" because it's thrown synchronously when the subject bundle JS is evaluated by the browser. This error is filtered by the SDK by default for good reason.

Solution Brainstorm

Ideally, we can work around this in one way or another so that we can have a simple way of throwing errors in a test that actually make it through the SDK. I have a couple of ideas but we'd need to check what the best approach is. Also I'm definitely open to other ideas.

  1. wrap subject.js code in a setTimeout call. This will cause the initial bundle evaluation to succeed and potential errors should be thrown as intended by testers.
  2. let the test framework accept another file (e.g. async-subject.js) that does what's suggested in 1. subject.js would stay sync as previously.
  3. we could just create a test utility (e.g. triggerError(err: unkown)) that calls page.evaluate. This should work 🤔 but admittedly I didn't test it yet
@onurtemizkan
Copy link
Collaborator

@Lms24, I think the 3rd one would be the most intuitive if it works for every case we test (and the cases we don't properly test due to the current limitations). I can give it a shot if that makes sense?

@Lms24
Copy link
Member Author

Lms24 commented Apr 18, 2024

Yup, sounds good to me!

@AbhiPrasad
Copy link
Member

@onurtemizkan feel free to convert the tests added from #11666 after we merge that in to test everything out!

AbhiPrasad added a commit that referenced this issue 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 issue 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
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants