Backtrace cleaner #843

Merged
merged 21 commits into from Mar 26, 2013

Projects

None yet

3 participants

@samphippen
RSpec member

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?

@dchelimsky
RSpec member

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

@samphippen
RSpec member

@dchelimsky sounds good to me 👍

@dchelimsky dchelimsky and 2 others commented on an outdated diff Mar 24, 2013
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
@dchelimsky
dchelimsky Mar 24, 2013

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?

@samphippen
samphippen Mar 24, 2013

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
@myronmarston
myronmarston Mar 24, 2013

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.

@myronmarston myronmarston commented on an outdated diff Mar 24, 2013
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
@myronmarston
myronmarston Mar 24, 2013

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.

@myronmarston myronmarston commented on an outdated diff Mar 24, 2013
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)
@myronmarston
myronmarston Mar 24, 2013

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.
@myronmarston myronmarston commented on an outdated diff Mar 24, 2013
spec/rspec/core/backtrace_cleaner_spec.rb
+ 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
@myronmarston
myronmarston Mar 24, 2013

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

@myronmarston myronmarston commented on an outdated diff Mar 24, 2013
spec/rspec/core/backtrace_cleaner_spec.rb
+ 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
@myronmarston
myronmarston Mar 24, 2013

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!)

@samphippen
RSpec member

@myronmarston how's it looking now?

@myronmarston myronmarston and 1 other commented on an outdated diff Mar 24, 2013
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
@myronmarston
myronmarston Mar 24, 2013

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.

@samphippen
samphippen Mar 24, 2013

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

@myronmarston
myronmarston Mar 24, 2013

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

@myronmarston myronmarston commented on an outdated diff Mar 24, 2013
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)]
@myronmarston
myronmarston Mar 24, 2013

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.

@myronmarston
RSpec member

@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!

@samphippen
RSpec member

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

@samphippen
RSpec member

@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?

@dchelimsky
RSpec member

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

@myronmarston myronmarston commented on the diff Mar 25, 2013
lib/rspec/core/backtrace_cleaner.rb
+
+ 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
@myronmarston
myronmarston Mar 25, 2013

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

@myronmarston
RSpec member

@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!

@samphippen
RSpec member

@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?

@samphippen
RSpec member

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

@myronmarston myronmarston and 1 other commented on an outdated diff Mar 25, 2013
lib/rspec/core/configuration.rb
@@ -79,18 +80,6 @@ def self.add_setting(name, opts={})
end
end
- # @macro [attach] add_setting
- # @attribute $1
@myronmarston
myronmarston Mar 25, 2013

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

@myronmarston myronmarston commented on the diff Mar 25, 2013
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.
@myronmarston
myronmarston Mar 25, 2013

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

@myronmarston
RSpec member

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

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

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

Signed-off-by: Sam Phippen <samphippen@googlemail.com>
659c23a
@samphippen samphippen Add specs exposing failing attribute setters
Signed-off-by: Sam Phippen <samphippen@googlemail.com>
7908058
@samphippen samphippen Pull backtrace cleaning behaviour into the backtrace_cleaner object
Signed-off-by: Sam Phippen <samphippen@googlemail.com>
0eb2545
@samphippen samphippen Change include/exclude for inclusion/exclusion in the backtrace cleaner
Signed-off-by: Sam Phippen <samphippen@googlemail.com>
09e740a
@samphippen samphippen Change add_setting to match the new cleaner names
Signed-off-by: Sam Phippen <samphippen@googlemail.com>
ccf6c25
@samphippen 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
@samphippen 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
@samphippen samphippen Fix failing html/text_mate formatter specs
Signed-off-by: Sam Phippen <samphippen@googlemail.com>
d2ee4e5
@samphippen samphippen Revert "Remove add_setting calls in backtrace_cleaner"
This reverts commit ae7fb79.
6c23e85
@samphippen 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
@samphippen samphippen Add some documentation for new backtrace behaviuor, move old docs.
Signed-off-by: Sam Phippen <samphippen@googlemail.com>
6268eba
@samphippen samphippen Swap in new exclude/include language in backtrace_cleaner_spec
Signed-off-by: Sam Phippen <samphippen@googlemail.com>
8028d74
@samphippen samphippen Add specs for new default behaviour of backtrace cleaner.
Signed-off-by: Sam Phippen <samphippen@googlemail.com>
81a5b2c
@samphippen
RSpec member

(I rebased off master)

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

I love newlines.

@samphippen
RSpec member

@myronmarston how's this looking now?

@myronmarston myronmarston merged commit 8ac0f7e into rspec:master Mar 26, 2013

1 check passed

Details default The Travis build passed
@myronmarston
RSpec member

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

@myronmarston myronmarston added a commit that referenced this pull request Mar 26, 2013
@myronmarston myronmarston Changelog entries for #843.
[ci skip]
e192cf2
@myronmarston
RSpec member

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).

@samphippen
RSpec member

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. 🚀

@myronmarston myronmarston added a commit that referenced this pull request Sep 17, 2014
@myronmarston 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
@myronmarston myronmarston added a commit that referenced this pull request Sep 17, 2014
@myronmarston 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
@myronmarston myronmarston added a commit that referenced this pull request Sep 18, 2014
@myronmarston 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