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

`full_backtrace = true` is unhelpful for beginners #1536

Closed
tomstuart opened this Issue May 19, 2014 · 21 comments

Comments

Projects
None yet
8 participants
@tomstuart
Contributor

tomstuart commented May 19, 2014

Setting config.full_backtrace = true when running exactly one spec file is unhelpful for people who are just getting started with RSpec, because those people are likely to both:

  1. uncomment and use the generated spec_helper.rb defaults without knowing what they all mean, and
  2. only have a single spec file, which triggers this behaviour even if they’re running their “entire suite” with bundle exec rspec or similar.

To a beginner, this combination of circumstances makes it look like RSpec doesn’t have a backtrace cleaner, which makes its output much more intimidating and difficult to read.

It’s obviously subjective, but I feel that the generated spec_helper.rb would be better off without this line. As it stands, I have to handwavingly tell people to remove it when I’m teaching them about RSpec, and I’d rather not do that.

@JonRowe

This comment has been minimized.

Show comment
Hide comment
@JonRowe

JonRowe May 19, 2014

Member

Hmm I'm on the fence about this, on the one hand it makes me sad that people don't read and trim down scaffolded files like this, (it's the first thing I do on a fresh project) which means I'd be ok with trimming it down a bit by default ;)

On the other this is one of the more useful ways to educate people about the settings and configuration available to them...

Member

JonRowe commented May 19, 2014

Hmm I'm on the fence about this, on the one hand it makes me sad that people don't read and trim down scaffolded files like this, (it's the first thing I do on a fresh project) which means I'd be ok with trimming it down a bit by default ;)

On the other this is one of the more useful ways to educate people about the settings and configuration available to them...

@tomstuart

This comment has been minimized.

Show comment
Hide comment
@tomstuart

tomstuart May 19, 2014

Contributor

I agree with the educational sentiment, but the motivation behind encouraging this behaviour was “to give a great initial experience”, which I don’t think it does in this case.

To put it another way: config.filter_run :focus and config.run_all_when_everything_filtered = true are great sensible defaults, and ditto for expectations.syntax = :expect, config.order = :random etc; it’s a net win to officially recommend these settings to people who otherwise have no opinion on configuring RSpec. But config.full_backtrace = true is more nuanced; in some cases it might help, and in other cases it might make things more difficult, so it seems like a harder thing to officially “recommend”.

(Full disclosure: I never turn off the backtrace cleaner, because the many lines of .bundle/gems/gems/rspec-core-3.0.0.rc1/lib/rspec/core/… make it impossible to see what’s going on.)

Contributor

tomstuart commented May 19, 2014

I agree with the educational sentiment, but the motivation behind encouraging this behaviour was “to give a great initial experience”, which I don’t think it does in this case.

To put it another way: config.filter_run :focus and config.run_all_when_everything_filtered = true are great sensible defaults, and ditto for expectations.syntax = :expect, config.order = :random etc; it’s a net win to officially recommend these settings to people who otherwise have no opinion on configuring RSpec. But config.full_backtrace = true is more nuanced; in some cases it might help, and in other cases it might make things more difficult, so it seems like a harder thing to officially “recommend”.

(Full disclosure: I never turn off the backtrace cleaner, because the many lines of .bundle/gems/gems/rspec-core-3.0.0.rc1/lib/rspec/core/… make it impossible to see what’s going on.)

@myronmarston

This comment has been minimized.

Show comment
Hide comment
@myronmarston

myronmarston May 19, 2014

Member

I'm on the fence about this.

I completely see your point. But on the other side of things, we've got users who think it's ridiculous that we don't always default to the full backtrace:

https://twitter.com/jamesgolick/status/451469652867448832

The config settings generated by rspec --init came out of common settings I found I was using on project after project that I thought would be widely useful. I find --backtrace to be too noisy for running an entire spec suite but when I'm running one file I find it useful to get the full context...but then again, the rspec lines in the backtrace make more sense to me than just about anyone else given my involvement with the project.

Ultimately, I want the generated config to serve the community's needs (and not be catered to mine). What do others think? (@xaviershay @samphippen @soulcutter @cupakromer)

We can also solicit community feedback on this issue but I want to hear from the other rspec contributors first.

Member

myronmarston commented May 19, 2014

I'm on the fence about this.

I completely see your point. But on the other side of things, we've got users who think it's ridiculous that we don't always default to the full backtrace:

https://twitter.com/jamesgolick/status/451469652867448832

The config settings generated by rspec --init came out of common settings I found I was using on project after project that I thought would be widely useful. I find --backtrace to be too noisy for running an entire spec suite but when I'm running one file I find it useful to get the full context...but then again, the rspec lines in the backtrace make more sense to me than just about anyone else given my involvement with the project.

Ultimately, I want the generated config to serve the community's needs (and not be catered to mine). What do others think? (@xaviershay @samphippen @soulcutter @cupakromer)

We can also solicit community feedback on this issue but I want to hear from the other rspec contributors first.

@tomstuart

This comment has been minimized.

Show comment
Hide comment
@tomstuart

tomstuart May 19, 2014

Contributor

I must admit that I don’t understand @jamesgolick’s complaint. The job of the backtrace cleaner is to remove irrelevant and uninteresting noise (e.g. RSpec internals) from backtraces, not to hide the backtrace completely. If it’s not doing that job properly — as he implies, but doesn’t explain, in these tweets — then that’s a bug in the backtrace cleaner, not a reason to turn it off by default.

Contributor

tomstuart commented May 19, 2014

I must admit that I don’t understand @jamesgolick’s complaint. The job of the backtrace cleaner is to remove irrelevant and uninteresting noise (e.g. RSpec internals) from backtraces, not to hide the backtrace completely. If it’s not doing that job properly — as he implies, but doesn’t explain, in these tweets — then that’s a bug in the backtrace cleaner, not a reason to turn it off by default.

@jamesgolick

This comment has been minimized.

Show comment
Hide comment
@jamesgolick

jamesgolick May 19, 2014

Provided that the backtrace cleaner regularly removes lines that are pertinent to debugging test failures, it is fundamentally intrusive. My experience is that the assertion made in this tweet is wrong:

https://twitter.com/myronmarston/status/451478481415569409

Removing rspec implementation related frames may make sense, but frames related to code in other gems means that any calls from inside of libraries are obscured and confusing.

Just my 2c.

jamesgolick commented May 19, 2014

Provided that the backtrace cleaner regularly removes lines that are pertinent to debugging test failures, it is fundamentally intrusive. My experience is that the assertion made in this tweet is wrong:

https://twitter.com/myronmarston/status/451478481415569409

Removing rspec implementation related frames may make sense, but frames related to code in other gems means that any calls from inside of libraries are obscured and confusing.

Just my 2c.

@tomstuart

This comment has been minimized.

Show comment
Hide comment
@tomstuart

tomstuart May 19, 2014

Contributor

Thanks @jamesgolick, that makes sense. I guess that discussion belongs elsewhere (e.g. should there be several levels of backtrace cleaning, with “only RSpec frames” as a conservative option?), but I’m glad I now understand the objection.

Contributor

tomstuart commented May 19, 2014

Thanks @jamesgolick, that makes sense. I guess that discussion belongs elsewhere (e.g. should there be several levels of backtrace cleaning, with “only RSpec frames” as a conservative option?), but I’m glad I now understand the objection.

@xaviershay

This comment has been minimized.

Show comment
Hide comment
@xaviershay

xaviershay May 19, 2014

Member

but frames related to code in other gems

I don't believe it is our intention to filter these out. Do you have a specific example?

ref: https://github.com/rspec/rspec-support/blob/master/lib/rspec/support/caller_filter.rb

Member

xaviershay commented May 19, 2014

but frames related to code in other gems

I don't believe it is our intention to filter these out. Do you have a specific example?

ref: https://github.com/rspec/rspec-support/blob/master/lib/rspec/support/caller_filter.rb

@tomstuart

This comment has been minimized.

Show comment
Hide comment
@tomstuart
Contributor

tomstuart commented May 19, 2014

@xaviershay

This comment has been minimized.

Show comment
Hide comment
@xaviershay

xaviershay May 19, 2014

Member

oh my bad, I didn't realise we had that code in two places. Hrm ok, need to think about that.

Member

xaviershay commented May 19, 2014

oh my bad, I didn't realise we had that code in two places. Hrm ok, need to think about that.

@jamesgolick

This comment has been minimized.

Show comment
Hide comment
@jamesgolick

jamesgolick May 19, 2014

I don't think giving people the option to filter out all gems actually makes sense, altho perhaps a configurable ability to filter out particular gems would be interesting?

configuration.backtrace_filter.filter_gem "some_noisy_gem" or whatever.

FWIW, this is all very ironic because I wrote the original version of backtrace filtering for rails and now deeply regret it.

jamesgolick commented May 19, 2014

I don't think giving people the option to filter out all gems actually makes sense, altho perhaps a configurable ability to filter out particular gems would be interesting?

configuration.backtrace_filter.filter_gem "some_noisy_gem" or whatever.

FWIW, this is all very ironic because I wrote the original version of backtrace filtering for rails and now deeply regret it.

@myronmarston

This comment has been minimized.

Show comment
Hide comment
@myronmarston

myronmarston May 19, 2014

Member

Thanks for chiming in @jamesgolick -- now I see what you were talking about. Clearly twitter isn't the best medium to discuss these things...

The filterings can be individually enabled/disabled by configuring backtrace_exlusion_patterns:

# Regexps used to exclude lines from backtraces.
#
# Excludes lines from ruby (and jruby) source, installed gems, anything
# in any "bin" directory, and any of the rspec libs (outside gem
# installs) by default.
#
# You can modify the list via the getter, or replace it with the setter.
#
# To override this behaviour and display a full backtrace, use
# `--backtrace`on the command line, in a `.rspec` file, or in the
# `rspec_options` attribute of RSpec's rake task.
def backtrace_exclusion_patterns
@backtrace_formatter.exclusion_patterns
end
# Set regular expressions used to exclude lines in backtrace
# @param patterns [Regexp] set the backtrace exlusion pattern
def backtrace_exclusion_patterns=(patterns)
@backtrace_formatter.exclusion_patterns = patterns
end

...so one could remove the gem part from that, but I'm also open to the idea of always including gem lines in the backtrace. I'd like to understand why we do that, though -- it predates my time. I found the source commit where we started filtering gems (fc591be) but it doesn't provide much context for the decision. @dchelimsky -- do you remember why we chose to filter gems? I noticed that rspec 1 didn't:

https://github.com/dchelimsky/rspec/blob/7c7869d2ca5e9ebb06ce2022f9ebcb7114f4c6c1/lib/spec/runner/backtrace_tweaker.rb#L46-L61

If we do decide to stop filtering gems from the backtrace I worry about cases where you are testing a rack-based HTTP app (e.g. using rack-test). Rack apps tend to have very deep stack traces from many gems, particularly since middlewares are all present in the backtrace for every request. On Rails I can imagine the backtrace before it gets to your controller action being extremely deep, and including it all would be quite noisy.

OTOH, if I'm writing a test against something that uses just one gem, and something blows up in that gem, it's nice to see the backtrace.

e.g. should there be several levels of backtrace cleaning, with “only RSpec frames” as a conservative option?

This is certainly one way to address this problem but having 3 levels of backtrace filtering (none, gems + rspec, rspec only) sounds too complex for me :(.

I don't think giving people the option to filter out all gems actually makes sense, altho perhaps a configurable ability to filter out particular gems would be interesting?

configuration.backtrace_filter.filter_gem "some_noisy_gem" or whatever.

I need to think about it some more, but I think I like that idea. rspec-rails could add rails, rack, etc to the list as they'd be commonly needed. What do others think?

Member

myronmarston commented May 19, 2014

Thanks for chiming in @jamesgolick -- now I see what you were talking about. Clearly twitter isn't the best medium to discuss these things...

The filterings can be individually enabled/disabled by configuring backtrace_exlusion_patterns:

# Regexps used to exclude lines from backtraces.
#
# Excludes lines from ruby (and jruby) source, installed gems, anything
# in any "bin" directory, and any of the rspec libs (outside gem
# installs) by default.
#
# You can modify the list via the getter, or replace it with the setter.
#
# To override this behaviour and display a full backtrace, use
# `--backtrace`on the command line, in a `.rspec` file, or in the
# `rspec_options` attribute of RSpec's rake task.
def backtrace_exclusion_patterns
@backtrace_formatter.exclusion_patterns
end
# Set regular expressions used to exclude lines in backtrace
# @param patterns [Regexp] set the backtrace exlusion pattern
def backtrace_exclusion_patterns=(patterns)
@backtrace_formatter.exclusion_patterns = patterns
end

...so one could remove the gem part from that, but I'm also open to the idea of always including gem lines in the backtrace. I'd like to understand why we do that, though -- it predates my time. I found the source commit where we started filtering gems (fc591be) but it doesn't provide much context for the decision. @dchelimsky -- do you remember why we chose to filter gems? I noticed that rspec 1 didn't:

https://github.com/dchelimsky/rspec/blob/7c7869d2ca5e9ebb06ce2022f9ebcb7114f4c6c1/lib/spec/runner/backtrace_tweaker.rb#L46-L61

If we do decide to stop filtering gems from the backtrace I worry about cases where you are testing a rack-based HTTP app (e.g. using rack-test). Rack apps tend to have very deep stack traces from many gems, particularly since middlewares are all present in the backtrace for every request. On Rails I can imagine the backtrace before it gets to your controller action being extremely deep, and including it all would be quite noisy.

OTOH, if I'm writing a test against something that uses just one gem, and something blows up in that gem, it's nice to see the backtrace.

e.g. should there be several levels of backtrace cleaning, with “only RSpec frames” as a conservative option?

This is certainly one way to address this problem but having 3 levels of backtrace filtering (none, gems + rspec, rspec only) sounds too complex for me :(.

I don't think giving people the option to filter out all gems actually makes sense, altho perhaps a configurable ability to filter out particular gems would be interesting?

configuration.backtrace_filter.filter_gem "some_noisy_gem" or whatever.

I need to think about it some more, but I think I like that idea. rspec-rails could add rails, rack, etc to the list as they'd be commonly needed. What do others think?

@soulcutter

This comment has been minimized.

Show comment
Hide comment
@soulcutter

soulcutter May 19, 2014

Member
  1. I think the inclusion of gems in the filter makes too big of an assumption and should be removed, at least as a default.
  2. As a separate feature, it would be nice to add configurability to the filter beyond on/off. I'm open to different ideas of what that might look like - @jamesgolick 's filter_gem idea is ok. Edit: oh yeah, backtrace_exclusion_patterns - that's nice! filter_gem could be a convenience method that uses that.
  3. The spec_helper.rb boilerplate does not make much sense to me as-is - it makes more sense to me outside the if statement. My desire to see the full backtrace has nothing to do with the number of specs I'm running. Commented-out, outside the if statement seems ok, or just plain removed altogether (personally I think it's plenty discoverable in the command line options… not everyone is going to agree)
Member

soulcutter commented May 19, 2014

  1. I think the inclusion of gems in the filter makes too big of an assumption and should be removed, at least as a default.
  2. As a separate feature, it would be nice to add configurability to the filter beyond on/off. I'm open to different ideas of what that might look like - @jamesgolick 's filter_gem idea is ok. Edit: oh yeah, backtrace_exclusion_patterns - that's nice! filter_gem could be a convenience method that uses that.
  3. The spec_helper.rb boilerplate does not make much sense to me as-is - it makes more sense to me outside the if statement. My desire to see the full backtrace has nothing to do with the number of specs I'm running. Commented-out, outside the if statement seems ok, or just plain removed altogether (personally I think it's plenty discoverable in the command line options… not everyone is going to agree)
@dchelimsky

This comment has been minimized.

Show comment
Hide comment
@dchelimsky

dchelimsky May 19, 2014

Member

@dchelimsky -- do you remember why we chose to filter gems? I noticed that rspec 1 didn't:

Not clearly, but my guess would be I find including gems too noisy. Seems like you're all on a good path here, so don't mind me :)

Member

dchelimsky commented May 19, 2014

@dchelimsky -- do you remember why we chose to filter gems? I noticed that rspec 1 didn't:

Not clearly, but my guess would be I find including gems too noisy. Seems like you're all on a good path here, so don't mind me :)

@jamesgolick

This comment has been minimized.

Show comment
Hide comment
@jamesgolick

jamesgolick May 19, 2014

I really strongly feel that filtering rails is a horrible idea. That automatically means that you can't tell one of your methods is being called by an active record callback. How could that possibly be a good idea?

jamesgolick commented May 19, 2014

I really strongly feel that filtering rails is a horrible idea. That automatically means that you can't tell one of your methods is being called by an active record callback. How could that possibly be a good idea?

@cupakromer

This comment has been minimized.

Show comment
Hide comment
@cupakromer

cupakromer May 19, 2014

Member

I'm a bit mixed on this. I hate to introduce more magic, but in general, I only want to see lines in my backtrace from gems if there's no matching backtrace from my code. Though, it's been a while since I hit this, so I'm not working from a fresh perspective.

I also agree, that having too many config options is a bit annoying to both maintain and consume.

In general, I don't want to see the full backtrace even when I run a single spec. However, this isn't really that big of an issue for me and I don't feel strongly either direction.

Member

cupakromer commented May 19, 2014

I'm a bit mixed on this. I hate to introduce more magic, but in general, I only want to see lines in my backtrace from gems if there's no matching backtrace from my code. Though, it's been a while since I hit this, so I'm not working from a fresh perspective.

I also agree, that having too many config options is a bit annoying to both maintain and consume.

In general, I don't want to see the full backtrace even when I run a single spec. However, this isn't really that big of an issue for me and I don't feel strongly either direction.

@myronmarston

This comment has been minimized.

Show comment
Hide comment
@myronmarston

myronmarston May 19, 2014

Member

I really strongly feel that filtering rails is a horrible idea. That automatically means that you can't tell one of your methods is being called by an active record callback. How could that possibly be a good idea?

Admittedly, I haven't used rails in years so I'm not the best one to make that call. It was just an idea for how to deal with the huuuuuuge backtraces you get when testing something that goes through the full rack stack. I'll leave the choice of whether or not to do that up to those who work on and use rspec-rails, though :).

Member

myronmarston commented May 19, 2014

I really strongly feel that filtering rails is a horrible idea. That automatically means that you can't tell one of your methods is being called by an active record callback. How could that possibly be a good idea?

Admittedly, I haven't used rails in years so I'm not the best one to make that call. It was just an idea for how to deal with the huuuuuuge backtraces you get when testing something that goes through the full rack stack. I'll leave the choice of whether or not to do that up to those who work on and use rspec-rails, though :).

@myronmarston

This comment has been minimized.

Show comment
Hide comment
@myronmarston

myronmarston May 19, 2014

Member

It sounds like there's consensus around removing two things:

  • gems from the default backtrace filtering
  • full_backtrace = true in the spec_helper generated by rspec --init.

Any volunteers to make a PR with those changes?

Member

myronmarston commented May 19, 2014

It sounds like there's consensus around removing two things:

  • gems from the default backtrace filtering
  • full_backtrace = true in the spec_helper generated by rspec --init.

Any volunteers to make a PR with those changes?

@cupakromer

This comment has been minimized.

Show comment
Hide comment
@cupakromer

cupakromer May 19, 2014

Member

fwiw I'd be fine with leaving full_backtrace = true in the generated spec_helper if it was commented out by default. That way, the setting is exposed with the nice comment block, but it's not on by default.

Member

cupakromer commented May 19, 2014

fwiw I'd be fine with leaving full_backtrace = true in the generated spec_helper if it was commented out by default. That way, the setting is exposed with the nice comment block, but it's not on by default.

@myronmarston

This comment has been minimized.

Show comment
Hide comment
@myronmarston

myronmarston May 19, 2014

Member

fwiw I'd be fine with leaving full_backtrace = true in the generated spec_helper if it was commented out by default. That way, the setting is exposed with the nice comment block, but it's not on by default.

It is commented out by default as part of a large section that is commented out using =begin and =end. It was decided awhile back that the only uncommented config settings generated by rspec --init should be things that are slated to become defaults, and that for recommended settings they should be commented out, so that users are (hopefully) made aware of what they are opting in to.

Member

myronmarston commented May 19, 2014

fwiw I'd be fine with leaving full_backtrace = true in the generated spec_helper if it was commented out by default. That way, the setting is exposed with the nice comment block, but it's not on by default.

It is commented out by default as part of a large section that is commented out using =begin and =end. It was decided awhile back that the only uncommented config settings generated by rspec --init should be things that are slated to become defaults, and that for recommended settings they should be commented out, so that users are (hopefully) made aware of what they are opting in to.

@cupakromer

This comment has been minimized.

Show comment
Hide comment
@cupakromer

cupakromer May 19, 2014

Member

Right, I see. I was thinking more of a single line comment. However, based on this reasoning, I see why it should just be removed. Thanks.

Member

cupakromer commented May 19, 2014

Right, I see. I was thinking more of a single line comment. However, based on this reasoning, I see why it should just be removed. Thanks.

@myronmarston

This comment has been minimized.

Show comment
Hide comment
@myronmarston

myronmarston May 24, 2014

Member

Closing as @tomstuart's original issue has been addressed by #1547. There's an open issue for the further improvements discussed here (#1554), which is slated for 3.1.

Member

myronmarston commented May 24, 2014

Closing as @tomstuart's original issue has been addressed by #1547. There's an open issue for the further improvements discussed here (#1554), which is slated for 3.1.

myronmarston added a commit that referenced this issue Jul 16, 2014

Remove /gems/ from default backtrace exclusion patterns.
As discussed in #1536 and #1554, the failure can often be in a gem.

We may add an additional gem filtering API later on but for
now I just want to try this on a few projects and see how the
experience is.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment