Skip to content

fix: Capture only failed console.assert calls #2239

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.

Sign up for GitHub

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

Jump to bottom
Merged
Parent: [Gitflow] Merge master into develop
merged 1 commit into from
Sep 20, 2019
Merged

fix: Capture only failed console.assert calls #2239

merged 1 commit into from
Sep 20, 2019

Conversation

@kamilogorek
Copy link
Contributor

Fixes #2211

We have this implemented correctly in CaptureConsole integration :<

sentry-javascript/packages/integrations/src/captureconsole.ts

Lines 58 to 66 in ec571ff

if (level === 'assert') {
if (args[0] === false) {
message = `Assertion failed: ${safeJoin(args.slice(1), ' ') || 'console.assert'}`;
scope.setExtra('arguments', normalize(args.slice(1), 3));
hub.captureMessage(message);
}
} else {
hub.captureMessage(message);
}

@kamilogorek kamilogorek requested a review from HazAT September 12, 2019 10:57
@getsentry-bot
Copy link
Contributor

Warnings
⚠️ Please add a changelog entry for your changes.
Messages
📖 ✅ TSLint passed
📖

@sentry/browser bundle gzip'ed minified size: (ES5: 16.207 kB) (ES6: 15.3867 kB)

Generated by 🚫 dangerJS against a522e7f

@HazAT
Copy link
Member

HazAT commented Sep 17, 2019

+Changelog

@kamilogorek kamilogorek merged commit 5907ce6 into master Sep 20, 2019
@kamilogorek kamilogorek deleted the assert-fail-only branch September 20, 2019 10:23
@andrew0 andrew0 mentioned this pull request Apr 25, 2024
Merged
mydea added a commit that referenced this pull request Apr 26, 2024
@andrew0 @mydea
fix(core): Capture only failed console.assert calls (#11799)
18ba344 
I'm seeing a lot of breadcrumbs for `console.assert`s even though they
are passing. This issue was previously fixed in #2239 but I guess there
was a regression at some point. There was already an existing test case
for this, but it was missing an assertion.

---------

Co-authored-by: Francesco Novy <francesconovy@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@HazAT HazAT HazAT approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

console.assert adds a breadcrumb even when assertion doesn't fail

3 participants

@kamilogorek @getsentry-bot @HazAT