Skip to content
This repository has been archived by the owner on Oct 23, 2023. It is now read-only.

Truncate long source context lines #329

Merged
merged 2 commits into from
Jun 7, 2017
Merged

Conversation

LewisJEllis
Copy link
Contributor

This should address most of the cases causing #153.

/cc @mattrobenolt @MaxBittker

frame.context_line.trim().should.startWith('{snip}');
frame.context_line.trim().should.endWith('{snip}');
frame.post_context.should.be.an.instanceOf(Array);
frame.post_context[0].trim().should.endWith('{snip}');
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel it's not sufficient to just test for presence of {snip} – can we improve this test to also assert the content of the snipped code is as expected (e.g. what appears between {snip})?

@LewisJEllis
Copy link
Contributor Author

LewisJEllis commented Jun 7, 2017

@benvinegar updated the test. I just check the length and the presence of the throw statement since checking exact string equality just feels like tautologically testing that the code does what it does. Let me know if there was any other check you had in mind.

@benvinegar
Copy link
Contributor

It's an improvement.

feels like tautologically testing that the code does what it does.

Yes.

@LewisJEllis LewisJEllis merged commit fc0e1d5 into master Jun 7, 2017
@LewisJEllis LewisJEllis deleted the truncate-long-stuff branch June 7, 2017 00:48
@dpehrson
Copy link

dpehrson commented Jun 9, 2017

Has this been released yet? Just signed up for sentry (looks awesome) and this was unfortunately the first thing I hit.

We deploy an isomorphic react app that leverages webpack asset url hashing so we webpack the server code as well.

@benvinegar
Copy link
Contributor

Not yet. We'll publish something soon.

@joevbruno
Copy link

@benvinegar any progress / timeline on this?

@benvinegar
Copy link
Contributor

benvinegar commented Jun 16, 2017

@joevbruno – I wanted to do this earlier in the week, but now that it's Friday I'd rather wait until the weekend is over first. There's a bunch of stuff on master that could potentially be problematic (it's well tested, but you never know), and even this truncation change could cause some issues to group differently. We want to be on-hand just in case.

@joevbruno
Copy link

@benvinegar Understood. Hopefully it will happen sometime this week

MaxBittker added a commit that referenced this pull request Jun 20, 2017
Version 2.1.0
- Truncate long lines in surrounding source to avoid sending large amounts of minified code [See #329]
- Refactor automatic breadcrumb instrumentation of modules to accommodate compilation tools [See #322]
- Testing for Node 8 [See #328]
@MaxBittker
Copy link
Contributor

this has been released @joevbruno let me know if you have any problems with it!

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

Successfully merging this pull request may close these issues.

None yet

5 participants