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 breadcrumb tests from karma runner #11436

Merged
merged 2 commits into from
Apr 5, 2024

Conversation

AbhiPrasad
Copy link
Member

I want to remove the karma/mocha based tests in the browser package. To accomplish this, I'll be porting 1 test suite a day from the old integration tests to playwright. Today is Day 2: packages/browser/test/integration/suites/breadcrumbs.js

This adds console and history breadcrumb tests (which didn't exist before), and expands upon the dom and xhr/fetch tests, and cleans up some code here and there as well.

ref #11084
day 1: #11412

@AbhiPrasad AbhiPrasad requested a review from a team April 4, 2024 21:20
@AbhiPrasad AbhiPrasad self-assigned this Apr 4, 2024
@AbhiPrasad AbhiPrasad requested review from stephanie-anderson and lforst and removed request for a team April 4, 2024 21:20
@lforst
Copy link
Member

lforst commented Apr 5, 2024

To accomplish this, I'll be porting 1 test suite a day

This reads like a tiktok challenge

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! Just two small comments


// see: https://github.com/getsentry/sentry-javascript/issues/768
sentryTest(
'should record breadcrumb accessing the target property of an event throws an exception',
Copy link
Member

Choose a reason for hiding this comment

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

l: not sure but I think theres and "if" missing:

Suggested change
'should record breadcrumb accessing the target property of an event throws an exception',
'should record breadcrumb if accessing the target property of an event throws an exception',


const eventData = await promise;

expect(eventData.breadcrumbs).toHaveLength(1);
Copy link
Member

Choose a reason for hiding this comment

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

l: Can we check for the breadcrumb content?

@AbhiPrasad
Copy link
Member Author

This reads like a tiktok challenge

Need to get syntax team to help create some content here 😄

@AbhiPrasad AbhiPrasad enabled auto-merge (squash) April 5, 2024 14:17
@AbhiPrasad AbhiPrasad merged commit 658898c into develop Apr 5, 2024
91 checks passed
@AbhiPrasad AbhiPrasad deleted the abhi-port-breadcrumb-tests branch April 5, 2024 14:33
AbhiPrasad added a commit that referenced this pull request Apr 8, 2024
I want to remove the karma/mocha based tests in the browser package. To
accomplish this, I'll be porting 1 test suite a day from the old
integration tests to playwright. Today is Day 3:
`packages/browser/test/integration/suites/config.js`

I was surprised we never had `ignoreErrors` or `denyUrls` tests in
playwright, so it's good to get the confidence that everything works
here.

ref #11084
day 2: #11436
cadesalaberry pushed a commit to cadesalaberry/sentry-javascript that referenced this pull request Apr 19, 2024
I want to remove the karma/mocha based tests in the browser package. To
accomplish this, I'll be porting 1 test suite a day from the old
integration tests to playwright. Today is Day 2:
`packages/browser/test/integration/suites/breadcrumbs.js`

This adds console and history breadcrumb tests (which didn't exist
before), and expands upon the dom and xhr/fetch tests, and cleans up
some code here and there as well.

ref getsentry#11084
day 1: getsentry#11412
cadesalaberry pushed a commit to cadesalaberry/sentry-javascript that referenced this pull request Apr 19, 2024
…ry#11449)

I want to remove the karma/mocha based tests in the browser package. To
accomplish this, I'll be porting 1 test suite a day from the old
integration tests to playwright. Today is Day 3:
`packages/browser/test/integration/suites/config.js`

I was surprised we never had `ignoreErrors` or `denyUrls` tests in
playwright, so it's good to get the confidence that everything works
here.

ref getsentry#11084
day 2: getsentry#11436
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.

3 participants