-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
test(browser): Add integration tests for withScope
#4342
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
Conversation
size-limit report
|
withScopewithScope
| Sentry.init({ | ||
| dsn: 'https://[email protected]/1337', | ||
| beforeSend: event => { | ||
| window.events.push(event); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need this beforeSend?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When testing with multiple events, we need to back up copies, as we may be testing them after they're sent and deleted.
This approach is used in the old integration tests , but alternatively I have implemented a helper function there to keep track of multiple requests. It was intended for more complex cases, but we may switch to that everywhere.
And that function is out of scope of that PR :/ I'll move it out. 👍
| @@ -0,0 +1,11 @@ | |||
| import * as Sentry from '@sentry/browser'; | |||
|
|
|||
| window.Sentry = Sentry; | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The SDK should be setting Sentry on the window automatically - we don’t have to manually do this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Weird, it's not set on the test browser. Maybe Playwright blocks auto assignments to window, I'll dig deeper on it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Imo we can just leave it for now to get these tests in!
Something we can come back to.
… into onur/public-api-tests-with-scope
|
@AbhiPrasad, |
|
Ah I didn’t see this till after I merged in the PRs @onurtemizkan - that’s my bad. |
|
No worries at all, @AbhiPrasad, I have sent a patch that seems to resolve that (#4360), not sure how though. |
… into onur/public-api-tests-with-scope
Part of: #4333