don't remove anonymous functions from stack trace #290
Conversation
| }()); | ||
| }()); | ||
| } catch(ex) { | ||
| trace = TraceKit.computeStackTrace(ex); |
There was a problem hiding this comment.
Hmm, this test will be brittle since it's tied specifically to how it's handled by Chrome. Could we mock an exception object instead?
There was a problem hiding this comment.
I wrote it that way because I assumed it'd be run from the command line only, through v8, so it would act just like Chrome. But that might be a bad assumption! I don't know Mocha very well. I'm guessing from your comment that the tests can be run in-browser too.
There was a problem hiding this comment.
Technically they can be, but they don't pass for all browsers yet. That's an ideal goal though, so I'd rather not back in more platform specific things than we need to. :)
|
fwiw, the patch itself is fine. Just not a fan of the test since it'll be brittle. Ideally at some point, I'd like to do actual browser testing and let the test suite run in FF, IE, etc. This will be problematic. I'd rather test over a fixture with a hardcoded stack trace as a string. |
|
Sure thing, I can make that happen. |
|
Hey, do you want a second commit on this PR, or a fresh PR with the two commits squashed? |
|
If you're comfortable with a: |
TraceKit's stacktrace regex for Chrome didn't match anonymous functions, so all anonymous functions in the stacktrace were being removed before reporting to Sentry. This caused errors to be attributed to the wrong files, and was generally extremely confusing. This change updates the regex to not *require* a function name be present in the stacktrace.
a64e907 to
d94cb5f
Compare
|
Sure thing, I don't care about the historical integrity of that branch. :) See if this test is more to your liking. |
|
Boom, I like this much better. :) Now there's no excuse for other stack trace manipulations to not write tests! Thanks! 🍰 ✨ |
|
Cake AND sparkles?! Dang! Raven contributions get the best emojis. |
don't remove anonymous functions from stack trace
TraceKit's stacktrace regex for Chrome didn't match anonymous functions, so all anonymous functions in the stacktrace were being removed before reporting to Sentry. This caused errors to be attributed to the wrong files, and was generally extremely confusing. This change updates the regex to not require a function name be present in the stacktrace.
see #289