-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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(react): Add tests for react-router
createHashRouter
usage.
#8362
Conversation
d90efef
to
f631c6f
Compare
size-limit report 📦
|
df34772
to
98d0b71
Compare
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.
Can we add some integration tests that validate the transaction name being set correctly?
98d0b71
to
da57a94
Compare
react-router
createHashRouter
usage.react-router
createHashRouter
usage.
84f9466
to
a6ce831
Compare
.toBe(200); | ||
}); | ||
|
||
test('Sends a pageload transaction to Sentry', async ({ page }) => { |
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.
Now that I see the integration test, I'm a bit hesistant to add the entire app + playwright under the react package tests for maintenance concerns.
Could we just check the transaction name in this test and the navigation test? Sorry for making you add the entire app - but not sure if it's worth to keep 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.
No worries at all, I agree, this ended up with so much code quite fastly. Which I tested now with 2 lines in the e2e test. 😅 Anyways we now have a PoC in case we need to integration test wider extent of React SDK in the future.
faa5f29
to
63dd97d
Compare
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.
Thanks @onurtemizkan!
See comment: #8317 (comment)
Resolves: #6251
Resolves: #8317
Docs Update: getsentry/sentry-docs#7218