Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Backtrace cleaner #843

Merged
merged 21 commits into from

3 participants

Sam Phippen David Chelimsky Myron Marston
Sam Phippen
Collaborator

Related #798

I added the backtrace cleaner object. I felt that discard was a better name than clean given that we now have a second class of line (include) to compare it to. Clean and include seemed less intuitive than discard and include to me. (I did not change the names of the configuration options).

This causes two of the specs to fail on my machine, it looks like it's due to the fact the nature of what lines get included have changed, and this causes the pre-formatted html to be different. Would the best change to make these specs have an empty include patterns configuration option?

David Chelimsky
Owner

How about "exclude" instead of "discard"? Aligns with other parts of rspec (e.g. FilterManager) and clarifies opposition IMO.

Sam Phippen
Collaborator

@dchelimsky sounds good to me :+1:

lib/rspec/core/backtrace_cleaner.rb
@@ -0,0 +1,28 @@
+module RSpec
+ module Core
+ class BacktraceCleaner
+
+ attr_accessor :include_patterns
+ attr_accessor :exclude_patterns
+
+ def initialize(include_patterns, exclude_patterns)
+ @include_patterns = include_patterns
+ @exclude_patterns = exclude_patterns
+ end
+
+ def include?(line)
+ matches_an_include_pattern? line or not matches_an_exclude_pattern? line
David Chelimsky Owner

This is very objective and certainly influenced by my recent work in Clojure, so feel free to disregard: I don't feel redirecting to private functions here buys much i.e. the following would be perfectly clear to any Rubyist, and not require that the reader look elsewhere to understand the meaning.

def include?(line)
  @include_patterns.any? {|p| line =~ p} || @discard_patterns.none? {|p| line =~ p}
end

I'm all for well named methods that describe something that is otherwise non-obvious or exposes something that helps to clarify the domain, but I don't think matches_an_include_pattern? and matches_a_discard_pattern? serve either purpose.

WDYT?

Sam Phippen Collaborator

I could go either way on this one. I'd prefer to use "or" instead of pipes (but I spend so much time coding python and it doesn't have || so that could be my own personal biases). I wasn't aware that none? was a thing, and I think this was primarily borne out of not wanting to do not @discard_patterns.any? {|p| line =~ p}, so I cut the methods out for symmetry. I'd like to garner some more opinions: @alindeman @myronmarston would you prefer it as it stands at the moment or like this?

def include?(line)
  @include_patterns.any? {|p| line =~ p} or @exclude_patterns.none? {|p| line =~ p}
end
Myron Marston Owner

Re: || vs or (and also and vs &&) the precedence rules are different. and and or are for control flow, not for boolean expressions. However, this is subtle and can get quite confusing, so I tend to agree with the github ruby style guide which says:

The and and or keywords are banned. It's just not worth it. Always use && and || instead.

In general, I like short methods, but I think I'd tend to agree with David here: the helper methods don't add much here -- the expression, as David has suggested it, is short and easy enough to understand on its own.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
lib/rspec/core/configuration.rb
@@ -90,6 +91,7 @@ def self.add_setting(name, opts={})
# `--backtrace` on the command line, in a `.rspec` file, or in the
# `rspec_options` attribute of RSpec's rake task.
add_setting :backtrace_clean_patterns
+ add_setting :backtrace_include_patterns
Myron Marston Owner

With the addition of the backtrace_include_patterns, I think it would be preferable if the old option was backtrace_exclude_patterns rather than backtrace_clean_patterns for greater consistency/symmetry. We can't just rename it, though, but what about making backtrace_clean_patterns a deprecated alias to backtrace_exclude_patterns? Then we can remove backtrace_clean_patterns in RSpec 3.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
lib/rspec/core/configuration.rb
@@ -282,7 +285,7 @@ def cleaned_from_backtrace?(line)
# TODO (David 2011-12-25) why are we asking the configuration to do
# stuff? Either use the patterns directly or enapsulate the filtering
# in a BacktraceCleaner object.
- backtrace_clean_patterns.any? { |regex| line =~ regex }
+ not RSpec::Core::BacktraceCleaner.new(backtrace_include_patterns, backtrace_clean_patterns).include?(line)
Myron Marston Owner

3 things:

  • The fact that you need to use not here suggests that the main method exposed off of BacktraceCleaner should be exclude? rather than include? -- then you won't need to use not anymore.
  • Rather than storing the patterns in the configuration object, and constructing a BacktraceCleaner object on the fly, I think I'd prefer to see a single BacktraceCleaner object instantiated on Configuration#instantiate, and then have it store the state of the include/exclude patterns -- backtrace_include_patterns and backtrace_exclude_patterns would simply delegate to the BacktraceCleaner object. Among other things, this results in fewer object allocations (which results in less garbage to clean up). As it stands now, a new BacktraceCleaner instance is instantiated for every single backtrace line from any failure. That seems kinda wasteful.
  • Once you have a Configuration#backtrace_cleaner instance, I think it would be preferrable to update the things that call here to call the method on the cleaner directly. As indicated by David'd TODO comment, it would make more sense to have things asking the backtrace cleaner if a line should be included or not, rather than asking the configuration that.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
spec/rspec/core/backtrace_cleaner_spec.rb
((21 lines not shown))
+ expect(cleaner.include? "apple").to be_true
+ end
+ end
+
+ context "with a discard pattern and a keep pattern" do
+ it "discards lines that match the discard pattern but not the keep pattern" do
+ cleaner = BacktraceCleaner.new([/keep/], [/discard/])
+ expect(cleaner.include? "discard").to be_false
+ end
+
+ it "keeps lines that match the keep pattern and the discard pattern" do
+ cleaner = BacktraceCleaner.new([/hi/], [/.*/])
+ expect(cleaner.include? "hi").to be_true
+ end
+
+ it "keeps lines that do not match the keep pattern, but do not match a discard pattern" do
Myron Marston Owner

I think this would read better as it "keeps lines that do not match either pattern".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
spec/rspec/core/backtrace_cleaner_spec.rb
((27 lines not shown))
+ cleaner = BacktraceCleaner.new([/keep/], [/discard/])
+ expect(cleaner.include? "discard").to be_false
+ end
+
+ it "keeps lines that match the keep pattern and the discard pattern" do
+ cleaner = BacktraceCleaner.new([/hi/], [/.*/])
+ expect(cleaner.include? "hi").to be_true
+ end
+
+ it "keeps lines that do not match the keep pattern, but do not match a discard pattern" do
+ cleaner = BacktraceCleaner.new([/hi/], [/delete/])
+ expect(cleaner.include? "fish").to be_true
+ end
+ end
+ end
+end
Myron Marston Owner

These specs are well-written -- nice work!

My one main suggestion is to adopt the include pattern/exclude pattern terminology here rather than keep pattern/discard pattern. (I think keep and discard are great verbs to use in doc strings, though!)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Sam Phippen
Collaborator

@myronmarston how's it looking now?

lib/rspec/core/configuration.rb
@@ -296,6 +295,37 @@ def mock_framework=(framework)
mock_with framework
end
+ def backtrace_clean_patterns
+ RSpec.deprecate("RSpec::Core::Configuration#backtrace_clean_patterns",
+ "RSpec::Core::Configuration#backtrace_exclude_patterns")
+ backtrace_exclude_patterns
+ end
+
+ def backtrace_clean_patterns=(patterns)
+ RSpec.deprecate("RSpec::Core::Configuration#backtrace_clean_patterns",
+ "RSpec::Core::Configuration#backtrace_exclude_patterns")
+ backtrace_exclude_patterns = patterns
Myron Marston Owner

This doesn't do what you intend. This line is creating a local variable named backtrace_exclude_patterns and assigning it to patterns. It is not calling the #backtrace_exclude_patterns= method as you intend. You have to use self.attr = to set attributes; w/o a receiver, ruby treats it as a local variable.

Sam Phippen Collaborator

You're quite right, I don't know why I missed this. Lemme fix.

Myron Marston Owner

The fact that there's this local variable vs. attribute assignment bug suggests there's a missing spec, too; please add that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
lib/rspec/core/configuration.rb
@@ -201,7 +205,11 @@ def initialize
@color = false
@pattern = '**/*_spec.rb'
@failure_exit_code = 1
- @backtrace_clean_patterns = DEFAULT_BACKTRACE_PATTERNS.dup
+
+ @backtrace_exclude_patterns = DEFAULT_BACKTRACE_PATTERNS.dup
+ @backtrace_include_patterns = [Regexp.new(Dir.getwd)]
Myron Marston Owner

The default values of these patterns seems like a concern that BacktraceCleaner should have rather than Configuration now that there is an object to manage that. So maybe it's worth setting up the initialize method of the BacktraceCleaner so that it sets these defaults?

Also, it seems odd to me to hold two references to the patterns -- one in the configuration object (in the form of @backtrace_x_patterns) and one in the BacktraceCleaner object itself (in the form of @x_patterns).

It seems to me that it's much cleaner to only hold that state in one place (the BacktraceCleaner). That makes it clear where the canonical source of this information is. With this approach, you would do something like:

class Configuration
  attr_reader :backtrace_cleaner

  def initialize
    @backtrace_cleaner = BacktraceCleaner.new
  end

  def backtrace_exclude_patterns
    backtrace_cleaner.exclude_patterns
  end

  def backtrace_exclude_patterns=(patterns)
    backtrace_cleaner.exclude_patterns = patterns
  end

  # do the same for the include patterns, too
end

Then you don't have to mess with the recreate_backtrace_cleaner business.

One last comment: for some reason, I think backtrace_exclusion_patterns and backtrace_inclusion_patterns read better than backtrace_include_patterns and backtrace_exclude_patterns, although I can't really come up with a reason why. Don't feel like you have to change it to this, but if you (and/or @dchelimsky or @alindeman) think it reads better, it might be worth switching to these names.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Myron Marston

@myronmarston how's it looking now?

It's looking quite good. I did have a couple more comments, though. Thanks again for working on this!

Sam Phippen
Collaborator

I do think inclusion/exclusion reads better than include/exclude. I'll patch that out now.

Sam Phippen
Collaborator

@myronmarston is that last add_setting call still necessary now that we have our getter/setter methods. I couldn't quite work out what that does. I also think this is now looks pretty good. Thoughts?

David Chelimsky
Owner

+1 for inclusion/exclusion
+1 for keeping state in one place

Myron Marston myronmarston commented on the diff
lib/rspec/core/backtrace_cleaner.rb
((14 lines not shown))
+
+ attr_accessor :inclusion_patterns
+ attr_accessor :exclusion_patterns
+
+ def initialize(inclusion_patterns=DEFAULT_INCLUSION_PATTERNS.dup, exclusion_patterns=DEFAULT_EXCLUSION_PATTERNS.dup)
+ @inclusion_patterns = inclusion_patterns
+ @exclusion_patterns = exclusion_patterns
+ end
+
+ def exclude?(line)
+ @inclusion_patterns.none? {|p| line =~ p} and @exclusion_patterns.any? {|p| line =~ p}
+ end
+
+ def full_backtrace=(true_or_false)
+ @exclusion_patterns = true_or_false ? [] : DEFAULT_EXCLUSION_PATTERNS.dup
+ end
Myron Marston Owner

Nice work moving full_backtrace here -- I had forgotten about it but it definitely belongs here!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Myron Marston

@myronmarston is that last add_setting call still necessary now that we have our getter/setter methods. I couldn't quite work out what that does. I also think this is now looks pretty good. Thoughts?

The add_setting call isn't needed anymore. It basically just defines the reader, writer and a predicate method for you, and provides support for defining an alias.

Anyhow, this looks great, but I did realize a slight issue that I didn't think about before: there's a way of using bundler (that I personally recommend) that causes all gems to be installed in a subdirectory of your project root...which means that the change here will cause backtraces to always contain all lines from all gems for folks who use bundler like I do. That's bad.

The simplest fix I can think of is to only default backtrace_inclusion_patterns to the current directory if it's really needed, i.e. if the current directory matches any of the exclusion patterns (which was @sferik's case that prompted this issue in the first place). Can anyone think of a better fix?

I took a look at the travis build and noticed two other things:

  • It's failing (you said as such above and I forgot). You can either re-record the html fixtures (using GENERATE = 1) or change the config option for those specs so that the backtraces remain how they were before. If you do re-record them, please open them in a browser to verify they look OK. (I suspect it's easier to just change the config option).
  • There are a bunch of deprecation warnings for the backtrace_clean_patterns option. This creates extra noise in the spec output and suggests that we may be calling it internally somewhere. It'd be good to go through and make sure we are only calling the deprecated method in the couple of specs that are testing that method. In those specs, you can stub RSpec.warn_deprecatation in order to silence the output.

Sorry about keeping coming back with more stuff we'd like to see changed before merging this. This is really close though!

Sam Phippen
Collaborator

@myronmarston would that be to check if Dir.getwd matches any of the exclusion patterns, and if it does put it in the inclusion patterns?

Sam Phippen
Collaborator

@myronmarston apart from that bundler issue you highlighted I've fixed more or less all those issues. This should now pass the travis build.

lib/rspec/core/configuration.rb
@@ -79,18 +80,6 @@ def self.add_setting(name, opts={})
end
end
- # @macro [attach] add_setting
- # @attribute $1
Myron Marston Owner

I think we need to keep this add_setting macro declaration for YARD.

David Chelimsky Owner

Correct.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Myron Marston myronmarston commented on the diff
lib/rspec/core/configuration.rb
@@ -79,18 +80,6 @@ def self.add_setting(name, opts={})
end
end
- # @macro [attach] add_setting
- # @attribute $1
- # Patterns to match against lines in backtraces presented in failure
- # messages in order to filter them out (default:
- # DEFAULT_BACKTRACE_PATTERNS). You can either replace this list using
- # the setter or modify it using the getter.
- #
- # To override this behavior 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.
Myron Marston Owner

Also, seeing this documentation go away reminds me that we need to document the methods you added below.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Myron Marston

@myronmarston would that be to check if Dir.getwd matches any of the exclusion patterns, and if it does put it in the inclusion patterns?

That's the simplest solution I could think of.

@myronmarston apart from that bundler issue you highlighted I've fixed more or less all those issues. This should now pass the travis build.

Thanks for following up on this so promptly!

samphippen added some commits
Sam Phippen samphippen Make backtrace cleaning the responsibility of a class
Signed-off-by: Sam Phippen <samphippen@googlemail.com>
eeafa75
Sam Phippen samphippen Refactor the backtrace cleaner for greater clarity
Signed-off-by: Sam Phippen <samphippen@googlemail.com>
966c6ab
Sam Phippen samphippen Use exclude instead of discard in the backtrace cleaner
Signed-off-by: Sam Phippen <samphippen@googlemail.com>
2351e0a
Sam Phippen samphippen Use "an_exclude" instead of "a_exclude". Grammar is important.
Signed-off-by: Sam Phippen <samphippen@googlemail.com>
d20f78b
Sam Phippen samphippen Change include? for exclude? on backtrace cleaner
This also removes the helper methods.

Signed-off-by: Sam Phippen <samphippen@googlemail.com>
a14f97f
Sam Phippen samphippen Make the description on one of the BacktraceCleaner tests more readable
Signed-off-by: Sam Phippen <samphippen@googlemail.com>
78051dd
Sam Phippen samphippen Rename backtrace_clean_patterns to backtrace_exclude_patterns
This also deprecates backtrace_clean_patterns

Signed-off-by: Sam Phippen <samphippen@googlemail.com>
ffe7c1f
Sam Phippen samphippen Remove Configuration#cleaned_from_backtrace?
We now use config.backtrace_cleaner.exclude?

Signed-off-by: Sam Phippen <samphippen@googlemail.com>
659c23a
Sam Phippen samphippen Add specs exposing failing attribute setters
Signed-off-by: Sam Phippen <samphippen@googlemail.com>
7908058
Sam Phippen samphippen Pull backtrace cleaning behaviour into the backtrace_cleaner object
Signed-off-by: Sam Phippen <samphippen@googlemail.com>
0eb2545
Sam Phippen samphippen Change include/exclude for inclusion/exclusion in the backtrace cleaner
Signed-off-by: Sam Phippen <samphippen@googlemail.com>
09e740a
Sam Phippen samphippen Change add_setting to match the new cleaner names
Signed-off-by: Sam Phippen <samphippen@googlemail.com>
ccf6c25
Sam Phippen samphippen Remove add_setting calls in backtrace_cleaner
this are now unneeded because we've defined instance methods that handle
these configuration options

Signed-off-by: Sam Phippen <samphippen@googlemail.com>
e4a6c2b
Sam Phippen samphippen Kill deprecation warnings for new backtrace specs
This is done by either using the new backtrace_exclusion_patterns or
stubbing RSpec.warn_deprecation

Signed-off-by: Sam Phippen <samphippen@googlemail.com>
e7ae4b2
Sam Phippen samphippen Fix failing html/text_mate formatter specs
Signed-off-by: Sam Phippen <samphippen@googlemail.com>
d2ee4e5
Sam Phippen samphippen Revert "Remove add_setting calls in backtrace_cleaner"
This reverts commit ae7fb79.
6c23e85
Sam Phippen samphippen Change the default inclusion behaviour of the backtrace cleaner
By default the current working directory will only be included if it
matches one of the exclusion patterns

Signed-off-by: Sam Phippen <samphippen@googlemail.com>
5911454
Sam Phippen samphippen Add some documentation for new backtrace behaviuor, move old docs.
Signed-off-by: Sam Phippen <samphippen@googlemail.com>
6268eba
Sam Phippen samphippen Swap in new exclude/include language in backtrace_cleaner_spec
Signed-off-by: Sam Phippen <samphippen@googlemail.com>
8028d74
Sam Phippen samphippen Add specs for new default behaviour of backtrace cleaner.
Signed-off-by: Sam Phippen <samphippen@googlemail.com>
81a5b2c
Sam Phippen
Collaborator

(I rebased off master)

Sam Phippen samphippen Fix weird newline issue
Signed-off-by: Sam Phippen <samphippen@googlemail.com>
56d7c0a
Sam Phippen
Collaborator

I love newlines.

Sam Phippen
Collaborator

@myronmarston how's this looking now?

Myron Marston myronmarston merged commit 8ac0f7e into from
Myron Marston

Looks fantastic. Thanks for taking the time to do such a thorough job!

Myron Marston myronmarston referenced this pull request from a commit
Myron Marston myronmarston Changelog entries for #843.
[ci skip]
e192cf2
Myron Marston

BTW, in the future, please remember to add changelog entries for your stuff, as that saves me from having to add it post-facto (as I did in e192cf2).

Sam Phippen
Collaborator

No problem @myronmarston. One of the longest ones I've ever worked on, lots learned. I'll try to remember the changelog thing in future. Thanks for helping me get this all perfected. :rocket:

Myron Marston myronmarston referenced this pull request from a commit
Myron Marston myronmarston Allow users to filter subdirs of their current dir.
This also greatly simplifies the backtrace formatter;
the system_exclusion_patterns approach made it more
complicated, as did always having an inclusion_pattern.

We only need the current directory as an inclusion_pattern
when it matches one of the built-in exclusion patterns.

Note that this was the approach that we originally had in #843;
it got lost in the refactoring in #1062 (af0b271).
While it seemed like a simpler approach to always put the current dir
in the inclusion_patterns, in practice, this caused a sequence
of whack-a-mole regressions:

- #1328 (fixed by #1329)
- #1604 (fixed by #1616)
- #1705 (fixed by this commit)

I'm hopeful that returning to only including the current dir
in `inclusion_patterns` if it matches a built-in exclusion pattern
will solve these issues once and for all.
5a37680
Myron Marston myronmarston referenced this pull request from a commit
Myron Marston myronmarston Allow users to filter subdirs of their current dir.
This also greatly simplifies the backtrace formatter;
the system_exclusion_patterns approach made it more
complicated, as did always having an inclusion_pattern.

We only need the current directory as an inclusion_pattern
when it matches one of the built-in exclusion patterns.

Note that this was the approach that we originally had in #843;
it got lost in the refactoring in #1062 (af0b271).
While it seemed like a simpler approach to always put the current dir
in the inclusion_patterns, in practice, this caused a sequence
of whack-a-mole regressions:

- #1328 (fixed by #1329)
- #1604 (fixed by #1616)
- #1705 (fixed by this commit)

I'm hopeful that returning to only including the current dir
in `inclusion_patterns` if it matches a built-in exclusion pattern
will solve these issues once and for all.
2cc12ce
Myron Marston myronmarston referenced this pull request from a commit
Myron Marston myronmarston Allow users to filter subdirs of their current dir.
This also greatly simplifies the backtrace formatter;
the system_exclusion_patterns approach made it more
complicated, as did always having an inclusion_pattern.

We only need the current directory as an inclusion_pattern
when it matches one of the built-in exclusion patterns.

Note that this was the approach that we originally had in #843;
it got lost in the refactoring in #1062 (af0b271).
While it seemed like a simpler approach to always put the current dir
in the inclusion_patterns, in practice, this caused a sequence
of whack-a-mole regressions:

- #1328 (fixed by #1329)
- #1604 (fixed by #1616)
- #1705 (fixed by this commit)

I'm hopeful that returning to only including the current dir
in `inclusion_patterns` if it matches a built-in exclusion pattern
will solve these issues once and for all.
9f53daf
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Mar 25, 2013
  1. Sam Phippen

    Make backtrace cleaning the responsibility of a class

    samphippen authored
    Signed-off-by: Sam Phippen <samphippen@googlemail.com>
  2. Sam Phippen

    Refactor the backtrace cleaner for greater clarity

    samphippen authored
    Signed-off-by: Sam Phippen <samphippen@googlemail.com>
  3. Sam Phippen

    Use exclude instead of discard in the backtrace cleaner

    samphippen authored
    Signed-off-by: Sam Phippen <samphippen@googlemail.com>
  4. Sam Phippen

    Use "an_exclude" instead of "a_exclude". Grammar is important.

    samphippen authored
    Signed-off-by: Sam Phippen <samphippen@googlemail.com>
  5. Sam Phippen

    Change include? for exclude? on backtrace cleaner

    samphippen authored
    This also removes the helper methods.
    
    Signed-off-by: Sam Phippen <samphippen@googlemail.com>
  6. Sam Phippen

    Make the description on one of the BacktraceCleaner tests more readable

    samphippen authored
    Signed-off-by: Sam Phippen <samphippen@googlemail.com>
  7. Sam Phippen

    Rename backtrace_clean_patterns to backtrace_exclude_patterns

    samphippen authored
    This also deprecates backtrace_clean_patterns
    
    Signed-off-by: Sam Phippen <samphippen@googlemail.com>
  8. Sam Phippen

    Remove Configuration#cleaned_from_backtrace?

    samphippen authored
    We now use config.backtrace_cleaner.exclude?
    
    Signed-off-by: Sam Phippen <samphippen@googlemail.com>
  9. Sam Phippen

    Add specs exposing failing attribute setters

    samphippen authored
    Signed-off-by: Sam Phippen <samphippen@googlemail.com>
  10. Sam Phippen

    Pull backtrace cleaning behaviour into the backtrace_cleaner object

    samphippen authored
    Signed-off-by: Sam Phippen <samphippen@googlemail.com>
  11. Sam Phippen

    Change include/exclude for inclusion/exclusion in the backtrace cleaner

    samphippen authored
    Signed-off-by: Sam Phippen <samphippen@googlemail.com>
  12. Sam Phippen

    Change add_setting to match the new cleaner names

    samphippen authored
    Signed-off-by: Sam Phippen <samphippen@googlemail.com>
  13. Sam Phippen

    Remove add_setting calls in backtrace_cleaner

    samphippen authored
    this are now unneeded because we've defined instance methods that handle
    these configuration options
    
    Signed-off-by: Sam Phippen <samphippen@googlemail.com>
  14. Sam Phippen

    Kill deprecation warnings for new backtrace specs

    samphippen authored
    This is done by either using the new backtrace_exclusion_patterns or
    stubbing RSpec.warn_deprecation
    
    Signed-off-by: Sam Phippen <samphippen@googlemail.com>
  15. Sam Phippen

    Fix failing html/text_mate formatter specs

    samphippen authored
    Signed-off-by: Sam Phippen <samphippen@googlemail.com>
  16. Sam Phippen
  17. Sam Phippen

    Change the default inclusion behaviour of the backtrace cleaner

    samphippen authored
    By default the current working directory will only be included if it
    matches one of the exclusion patterns
    
    Signed-off-by: Sam Phippen <samphippen@googlemail.com>
  18. Sam Phippen

    Add some documentation for new backtrace behaviuor, move old docs.

    samphippen authored
    Signed-off-by: Sam Phippen <samphippen@googlemail.com>
  19. Sam Phippen

    Swap in new exclude/include language in backtrace_cleaner_spec

    samphippen authored
    Signed-off-by: Sam Phippen <samphippen@googlemail.com>
  20. Sam Phippen

    Add specs for new default behaviour of backtrace cleaner.

    samphippen authored
    Signed-off-by: Sam Phippen <samphippen@googlemail.com>
  21. Sam Phippen

    Fix weird newline issue

    samphippen authored
    Signed-off-by: Sam Phippen <samphippen@googlemail.com>
This page is out of date. Refresh to see the latest.
42 lib/rspec/core/backtrace_cleaner.rb
View
@@ -0,0 +1,42 @@
+module RSpec
+ module Core
+ class BacktraceCleaner
+
+ DEFAULT_EXCLUSION_PATTERNS = [
+ /\/lib\d*\/ruby\//,
+ /org\/jruby\//,
+ /bin\//,
+ %r|/gems/|,
+ /spec\/spec_helper\.rb/,
+ /lib\/rspec\/(core|expectations|matchers|mocks)/
+ ]
+
+ attr_accessor :inclusion_patterns
+ attr_accessor :exclusion_patterns
+
+ def initialize(inclusion_patterns=nil, exclusion_patterns=DEFAULT_EXCLUSION_PATTERNS.dup)
+ @exclusion_patterns = exclusion_patterns
+
+ if inclusion_patterns.nil?
+ @inclusion_patterns = (matches_an_exclusion_pattern? Dir.getwd) ? [Regexp.new(Dir.getwd)] : []
+ else
+ @inclusion_patterns = inclusion_patterns
+ end
+ end
+
+ def exclude?(line)
+ @inclusion_patterns.none? {|p| line =~ p} and matches_an_exclusion_pattern?(line)
+ end
+
+ def full_backtrace=(true_or_false)
+ @exclusion_patterns = true_or_false ? [] : DEFAULT_EXCLUSION_PATTERNS.dup
+ end
Myron Marston Owner

Nice work moving full_backtrace here -- I had forgotten about it but it definitely belongs here!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+
+ private
+
+ def matches_an_exclusion_pattern?(line)
+ @exclusion_patterns.any? {|p| line =~ p}
+ end
+ end
+ end
+end
90 lib/rspec/core/configuration.rb
View
@@ -1,4 +1,5 @@
require 'fileutils'
+require 'rspec/core/backtrace_cleaner'
require 'rspec/core/ruby_project'
module RSpec
@@ -81,15 +82,6 @@ def self.add_setting(name, opts={})
# @macro [attach] add_setting
# @attribute $1
- # Patterns to match against lines in backtraces presented in failure
- # messages in order to filter them out (default:
- # DEFAULT_BACKTRACE_PATTERNS). You can either replace this list using
- # the setter or modify it using the getter.
- #
- # To override this behavior 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.
Myron Marston Owner

Also, seeing this documentation go away reminds me that we need to document the methods you added below.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
- add_setting :backtrace_clean_patterns
# Path to use if no path is provided to the `rspec` command (default:
# `"spec"`). Allows you to just type `rspec` instead of `rspec spec` to
@@ -183,14 +175,7 @@ def self.add_setting(name, opts={})
# @private
attr_accessor :filter_manager
- DEFAULT_BACKTRACE_PATTERNS = [
- /\/lib\d*\/ruby\//,
- /org\/jruby\//,
- /bin\//,
- %r|/gems/|,
- /spec\/spec_helper\.rb/,
- /lib\/rspec\/(core|expectations|matchers|mocks)/
- ]
+ attr_reader :backtrace_cleaner
def initialize
@expectation_frameworks = []
@@ -201,7 +186,9 @@ def initialize
@color = false
@pattern = '**/*_spec.rb'
@failure_exit_code = 1
- @backtrace_clean_patterns = DEFAULT_BACKTRACE_PATTERNS.dup
+
+ @backtrace_cleaner = BacktraceCleaner.new
+
@default_path = 'spec'
@filter_manager = FilterManager.new
@preferred_options = {}
@@ -276,15 +263,6 @@ def add_setting(name, opts={})
send("#{name}=", default) if default
end
- # Used by formatters to ask whether a backtrace line should be displayed
- # or not, based on the line matching any `backtrace_clean_patterns`.
- def cleaned_from_backtrace?(line)
- # TODO (David 2011-12-25) why are we asking the configuration to do
- # stuff? Either use the patterns directly or enapsulate the filtering
- # in a BacktraceCleaner object.
- backtrace_clean_patterns.any? { |regex| line =~ regex }
- end
-
# Returns the configured mock framework adapter module
def mock_framework
mock_with :rspec unless @mock_framework
@@ -296,6 +274,62 @@ def mock_framework=(framework)
mock_with framework
end
+ # The patterns to discard from backtraces. Deprecated, use
+ # Configuration#backtrace_exclusion_patterns instead
+ #
+ # Defaults to RSpec::Core::BacktraceCleaner::DEFAULT_EXCLUSION_PATTERNS
+ #
+ # One can replace the list by using the setter or modify it through the
+ # getter
+ #
+ # 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_clean_patterns
+ RSpec.deprecate("RSpec::Core::Configuration#backtrace_clean_patterns",
+ "RSpec::Core::Configuration#backtrace_exclusion_patterns")
+ @backtrace_cleaner.exclusion_patterns
+ end
+
+ def backtrace_clean_patterns=(patterns)
+ RSpec.deprecate("RSpec::Core::Configuration#backtrace_clean_patterns",
+ "RSpec::Core::Configuration#backtrace_exclusion_patterns")
+ @backtrace_cleaner.exclusion_patterns = patterns
+ end
+
+ # The patterns to always include to backtraces.
+ #
+ # Defaults to [Regexp.new Dir.getwd] if the current working directory
+ # matches any of the exclusion patterns. Otherwise it defaults to empty.
+ #
+ # One can replace the list by using the setter or modify it through the
+ # getter
+ def backtrace_inclusion_patterns
+ @backtrace_cleaner.inclusion_patterns
+ end
+
+ def backtrace_inclusion_patterns=(patterns)
+ @backtrace_cleaner.inclusion_patterns = patterns
+ end
+
+ # The patterns to discard from backtraces.
+ #
+ # Defaults to RSpec::Core::BacktraceCleaner::DEFAULT_EXCLUSION_PATTERNS
+ #
+ # One can replace the list by using the setter or modify it through the
+ # getter
+ #
+ # 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_cleaner.exclusion_patterns
+ end
+
+ def backtrace_exclusion_patterns=(patterns)
+ @backtrace_cleaner.exclusion_patterns = patterns
+ end
+
# Sets the mock framework adapter module.
#
# `framework` can be a Symbol or a Module.
@@ -425,7 +459,7 @@ def expect_with(*frameworks)
end
def full_backtrace=(true_or_false)
- @backtrace_clean_patterns = true_or_false ? [] : DEFAULT_BACKTRACE_PATTERNS
+ @backtrace_cleaner.full_backtrace = true_or_false
end
def color(output=output_stream)
2  lib/rspec/core/formatters/helpers.rb
View
@@ -18,7 +18,7 @@ def format_backtrace(backtrace, options = {})
protected
def backtrace_line(line)
- return nil if RSpec.configuration.cleaned_from_backtrace?(line)
+ return nil if RSpec.configuration.backtrace_cleaner.exclude?(line)
RSpec::Core::Metadata::relative_path(line)
rescue SecurityError
nil
56 spec/rspec/core/backtrace_cleaner_spec.rb
View
@@ -0,0 +1,56 @@
+require "spec_helper"
+
+module RSpec::Core
+ describe BacktraceCleaner do
+ context "with no patterns" do
+ it "keeps all lines" do
+ lines = ["/tmp/a_file", "some_random_text", "hello\330\271!"]
+ cleaner = BacktraceCleaner.new([], [])
+ expect(lines.all? {|line| cleaner.exclude? line}).to be_false
+ end
+ end
+
+ context "with an exclusion pattern but no inclusion patterns" do
+ it "excludes lines that match the exclusion pattern" do
+ cleaner = BacktraceCleaner.new([], [/remove/])
+ expect(cleaner.exclude? "remove me").to be_true
+ end
+
+ it "keeps lines that do not match the exclusion pattern" do
+ cleaner = BacktraceCleaner.new([], [/remove/])
+ expect(cleaner.exclude? "apple").to be_false
+ end
+ end
+
+ context "with an exclusion pattern and an inclusion pattern" do
+ it "excludes lines that match the exclusion pattern but not the inclusion pattern" do
+ cleaner = BacktraceCleaner.new([/keep/], [/discard/])
+ expect(cleaner.exclude? "discard").to be_true
+ end
+
+ it "keeps lines that match the inclusion pattern and the exclusion pattern" do
+ cleaner = BacktraceCleaner.new([/hi/], [/.*/])
+ expect(cleaner.exclude? "hi").to be_false
+ end
+
+ it "keeps lines that match neither pattern" do
+ cleaner = BacktraceCleaner.new([/hi/], [/delete/])
+ expect(cleaner.exclude? "fish").to be_false
+ end
+ end
+
+ context "with an exclusion pattern that matches the current working directory" do
+ it "defaults to having one inclusion pattern, the current working directory" do
+ cleaner = BacktraceCleaner.new(nil, [/.*/])
+ expect(Dir.getwd =~ cleaner.inclusion_patterns.first).to be_true
+ end
+ end
+
+ context "with an exclusion pattern that does not match the current working directory" do
+ it "defaults to having no exclusion patterns" do
+ cleaner = BacktraceCleaner.new(nil, [/i_wont_match_a_directory/])
+ expect(cleaner.inclusion_patterns.length).to be_zero
+ end
+ end
+ end
+end
49 spec/rspec/core/configuration_spec.rb
View
@@ -940,16 +940,16 @@ def metadata_hash(*args)
describe "#full_backtrace=" do
context "given true" do
- it "clears the backtrace clean patterns" do
+ it "clears the backtrace exclusion patterns" do
config.full_backtrace = true
- expect(config.backtrace_clean_patterns).to eq([])
+ expect(config.backtrace_exclusion_patterns).to eq([])
end
end
context "given false" do
it "restores backtrace clean patterns" do
config.full_backtrace = false
- expect(config.backtrace_clean_patterns).to eq(RSpec::Core::Configuration::DEFAULT_BACKTRACE_PATTERNS)
+ expect(config.backtrace_exclusion_patterns).to eq(RSpec::Core::BacktraceCleaner::DEFAULT_EXCLUSION_PATTERNS)
end
end
@@ -958,29 +958,58 @@ def metadata_hash(*args)
config_2 = Configuration.new
config_1.full_backtrace = true
- expect(config_2.backtrace_clean_patterns).not_to be_empty
+ expect(config_2.backtrace_exclusion_patterns).not_to be_empty
end
end
- describe "#cleaned_from_backtrace? defaults" do
+ describe "#backtrace_clean_patterns=" do
+ it "actually receives the new filter values" do
+ RSpec.stub(:warn_deprecation)
+ config = Configuration.new
+ config.backtrace_clean_patterns = [/.*/]
+ expect(config.backtrace_cleaner.exclude? "this").to be_true
+ end
+ end
+
+ describe "#backtrace_clean_patterns" do
+ it "is deprecated" do
+ RSpec.stub(:warn_deprecation)
+ RSpec.should_receive(:warn_deprecation)
+ config = Configuration.new
+ config.backtrace_clean_patterns
+ end
+
+ it "can be appended to" do
+ RSpec.stub(:warn_deprecation)
+ config = Configuration.new
+ config.backtrace_clean_patterns << /.*/
+ expect(config.backtrace_cleaner.exclude? "this").to be_true
+ end
+ end
+
+ describe ".backtrace_cleaner#exclude? defaults" do
it "returns true for rspec files" do
- expect(config.cleaned_from_backtrace?("lib/rspec/core.rb")).to be_true
+ expect(config.backtrace_cleaner.exclude?("lib/rspec/core.rb")).to be_true
end
it "returns true for spec_helper" do
- expect(config.cleaned_from_backtrace?("spec/spec_helper.rb")).to be_true
+ expect(config.backtrace_cleaner.exclude?("spec/spec_helper.rb")).to be_true
end
it "returns true for java files (for JRuby)" do
- expect(config.cleaned_from_backtrace?("org/jruby/RubyArray.java:2336")).to be_true
+ expect(config.backtrace_cleaner.exclude?("org/jruby/RubyArray.java:2336")).to be_true
end
it "returns true for files within installed gems" do
- expect(config.cleaned_from_backtrace?('ruby-1.8.7-p334/gems/mygem-2.3.0/lib/mygem.rb')).to be_true
+ expect(config.backtrace_cleaner.exclude?('ruby-1.8.7-p334/gems/mygem-2.3.0/lib/mygem.rb')).to be_true
end
it "returns false for files in projects containing 'gems' in the name" do
- expect(config.cleaned_from_backtrace?('code/my-gems-plugin/lib/plugin.rb')).to be_false
+ expect(config.backtrace_cleaner.exclude?('code/my-gems-plugin/lib/plugin.rb')).to be_false
+ end
+
+ it "returns false for something in the current working directory" do
+ expect(config.backtrace_cleaner.exclude?("#{Dir.getwd}/arbitrary")).to be_false
end
end
1  spec/rspec/core/formatters/html_formatter_spec.rb
View
@@ -33,6 +33,7 @@ module Formatters
out.set_encoding("utf-8") if out.respond_to?(:set_encoding)
command_line = RSpec::Core::CommandLine.new(options)
+ command_line.instance_variable_get("@configuration").backtrace_cleaner.inclusion_patterns = []
command_line.run(err, out)
out.string.gsub(/\d+\.\d+(s| seconds)/, "n.nnnn\\1")
end
1  spec/rspec/core/formatters/text_mate_formatter_spec.rb
View
@@ -33,6 +33,7 @@ module Formatters
out.set_encoding("utf-8") if out.respond_to?(:set_encoding)
command_line = RSpec::Core::CommandLine.new(options)
+ command_line.instance_variable_get("@configuration").backtrace_cleaner.inclusion_patterns = []
command_line.run(err, out)
out.string.gsub(/\d+\.\d+(s| seconds)/, "n.nnnn\\1")
end
Something went wrong with that request. Please try again.