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

ref(hub): remove hard cap from maxBreadcrumbs #5873

Merged
merged 3 commits into from
Oct 5, 2022

Conversation

outsideris
Copy link
Contributor

Fix #4693

As decission in #4693, this PR remove hard cap from maxBreadcrumbs.

Signed-off-by: Outsider <outsideris@gmail.com>
@Lms24
Copy link
Member

Lms24 commented Oct 3, 2022

Hi @outsideris and thanks for opening up this PR! I ran CI and it seems like some tests need adjustment. Would you mind taking a look at them? If you don't have the time, don't worry, just let us know. Thanks!

@outsideris
Copy link
Contributor Author

@Lms24 Sure, I want to see the CI results.

@Lms24
Copy link
Member

Lms24 commented Oct 3, 2022

I'll have to get back to you with the actual logs (still need to check what I can share) but I'd recommend just running yarn test in the hub package. Currently, this test is failing. We should probably rewrite this test to check that it is still respecting the maxBreadcrumbs limit.

Also, we should add a test to test a custom set max length.

Don't worry about the E2E test failing for now. We're working on making them less flakey

EDIT: Does this link show you the test logs?

Signed-off-by: Outsider <outsideris@gmail.com>
@outsideris
Copy link
Contributor Author

Sorry, I missed the tests.
I changed the failed test to test higher value than DEFAULT_MAX_BREADCRUMBS.

@outsideris
Copy link
Contributor Author

I want to see the CI results.

It meant that I wanted to CI results when I submitted this PR because CI didn't run since I'm a first-time contributor.
In my local, I confused some failed e2e tests.

Sorry for my poor English. Your kind comment was very helpful.

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.

Sorry for my poor English

No worries, all good! We figured it out after all ;) Yup, repo maintainers need to run the tests manually. Feel free to tag me whenever you need a test run!

packages/hub/test/scope.test.ts Outdated Show resolved Hide resolved
Co-authored-by: Lukas Stracke <lukas.stracke@sentry.io>
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.

Thanks for taking this on! Will merge soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove hard cap from maxBreadcrumbs
3 participants