Aggregate failures Integration #1946

myronmarston · 2015-04-23T08:39:55Z

This is the start of the rspec-core work for the new aggregate_failures feature. The part I'm working on now is getting the aggregated failure to format how we want. To facilitate this, I've extracted a new ExceptionFormatter class that is able to format exceptions according to how rspec-core typically does it for its built-in formatters. We'll then be able to apply this to each of the aggregated failures.

More to come, but I wanted to push what I had and get the travis build going.

JonRowe · 2015-04-27T00:59:45Z

lib/rspec/core/formatters/exception_formatter.rb

+  module Core
+    module Formatters
+      # @private
+      class ExceptionFormatter


Is ExceptionFormatter a good name for this? It's not a "Formatter" in RSpec's sense (even though it does do formatting), we use the term printer elsewhere for this sort of thing.

I thought it was a good name, in spite of the fact that it's not a "Formatter" in the RSpec sense....but ExceptionPrinter is fine by me if you prefer it.

I think I do just so nobody naively thinks it's a formatter in its own right :)

Well, it is labeled @private, so it's not really intended to be available or visible to users...

Yeah I know, but so are the deprecation and bisect formatters etc and they're real formatters. Thanks for making the change though, ❤️

myronmarston · 2015-04-28T23:44:03Z

@JonRowe, I renamed the ExceptionFormatter class to ExceptionPrinter. However, I realized that we do use Formatter for at least one case that's not a formatter in the RSpec sense: BacktraceFormatter. Apart from the normal specific meaning of "formatter" in RSpec, I think "formatter" is a better term than "printer" for what that class does, and I'm tempted to keep the original name. Thoughts?

JonRowe · 2015-04-29T00:04:42Z

Personally I think we should rename BacktraceFormatter, I'm still heavily on the "formatter has a specific meaning" within our lingo here... How about something like FormatException or ExceptionDisplay

myronmarston · 2015-05-02T19:04:51Z

Personally I think we should rename BacktraceFormatter, I'm still heavily on the "formatter has a specific meaning" within our lingo here... How about something like FormatException or ExceptionDisplay

I decided on ExceptionPresenter -- after all, that class is responsible for the presentation of the exception. You OK with that? I went ahead and renamed it....

JonRowe · 2015-05-02T22:22:51Z

Perfect ❤️

myronmarston · 2015-05-12T07:30:13Z

OK, this PR is ready for review as well. It builds on top of rspec/rspec-expectations#733 and integrates that feature. The code changes here provide a couple things:

Improved failure output from aggregate_failures failures.
Lots and lots of refactoring for the exception presentation logic to support this.
:aggregate_failures metadata definition.

Note that this PR can't be merged until rspec/rspec-expectations#733 is, and that one has a @wip cuke, which passes against this rspec-core branch but fails against rspec-core's master branch. So there's a bit of a circular dependency here, at least in terms of the testing, and I'm not sure what to do about it -- see the discussion about the cuke in rspec/rspec-expectations#733 for more info.

There's also an open question around what :aggregate_failures metadata should do when rspec-expectations is not loaded. My currently implementation will yield a NoMethodError (aggregate_failures) which might be OK and is the simplest, but I could see us doing something a bit nicer too...

/cc @rspec/rspec (especially @xaviershay)

xaviershay · 2015-05-12T19:46:47Z

spec/rspec/core_spec.rb

@@ -218,6 +218,14 @@
    end
  end

+  it 'uses only one thread local variable', :run_last do


xaviershay · 2015-05-12T19:49:49Z

There's also an open question around what :aggregate_failures metadata should do when rspec-expectations is not loaded.

I'm allowed to define arbitrary metadata, right? So in that case, not being able to "overload" aggregate_failures if I'm not bringing it in from somewhere else seems like a breach of contract.

Or actually, we're now defining this as a reserved word in rspec-core itself (if we weren't this would be a cyclic dependency I think), therefore we should probably raise something along the lines of "no plugin implements this behaviour" or something.

Not really worth a lot of effort though I don't think.

xaviershay · 2015-05-12T19:50:16Z

All this presentation stuff is gnarly. Good cleanups overall.

myronmarston · 2015-05-12T20:39:09Z

Not really worth a lot of effort though I don't think.

Yep, I'm leading towards "do nothing" for now. We can always do something later -- handling this case a bit more gracefully for users who use rspec-core but not rspec-expectations won't be a breaking API change so there's no urgency to get it in now.

All this presentation stuff is gnarly. Good cleanups overall.

Yeah, it kinda fried my brain :(. I still feel like it's really messy but I don't really want to put the effort into further refactorings now unless there's an obvious easy, big win someone has in mind.

JonRowe · 2015-05-13T01:04:06Z

lib/rspec/core.rb

-  # namespace.
-  def self.thread_local_metadata
-    Thread.current[:_rspec] ||= { :shared_example_group_inclusions => [] }
+    RSpec::Support.thread_local_data[:current_example] = example


myronmarston · 2015-05-15T21:49:55Z

OK, I think this is ready for a final review (and hopefully a merge!). Since @xaviershay's review, I added a cuke (viewable here), and added some improvements so that nested failure aggregation blocks format well.

Want to do a final review, @xaviershay?

I do still have to remove the [Temp] commit that pins this to the rspec-expectations branch, of course. I'll do that after we merge that PR.

xaviershay · 2015-05-16T00:03:38Z

ship it!

This will make it easier to extract an `ExceptionFormatter`.

- Simplifies the notification classes. - Provides something we can use to format multiple expectation failures packaged as a single exception for `aggregate_failures`.

We need to combine some of these cases (such as when we get a `MultipleExpectationsNotMetError` in a pending spec), and to do that we need to combine the options, so having the options listed in the same method is a stepping stone towards that.

To get this to work properly, we have to compose the exception presenter options for pending and for MultipleExpectationsNotMetError.

It is already listed on the aggregate failure backtrace, and would be redundant to list it on each sub-failure.

This should hopefully make the travis build go green...

For rspec/rspec-expectations#733.

- The presence of backticks within that part was causing rendering problems on relish. - Different rubies have different output there so we already ignore that part when comparing the expected to actual output.

This is necessary to support nested aggregation blocks.

This is necessary for niche situations where `aggregate_failures` is nested.

Aggregate failures Integration

[ci skip]

Aggregate failures Integration

[ci skip]

myronmarston force-pushed the aggregate-failures branch from ea6f7fd to a0281fe Compare April 23, 2015 21:52

JonRowe reviewed Apr 27, 2015
View reviewed changes

myronmarston force-pushed the aggregate-failures branch from a0281fe to 9f1bf1b Compare April 28, 2015 23:41

myronmarston force-pushed the aggregate-failures branch 6 times, most recently from 3bb1b61 to d0e02ee Compare May 2, 2015 14:53

myronmarston force-pushed the aggregate-failures branch 5 times, most recently from 5d9eb71 to 7d38d28 Compare May 5, 2015 16:20

Skip test on JRuby 1.8 mode due to the fact it flaps.

76ad12a

myronmarston force-pushed the aggregate-failures branch from 801bd8e to aac27c6 Compare May 12, 2015 07:01

myronmarston mentioned this pull request May 12, 2015

Aggregate failures rspec/rspec-expectations#776

Merged

6 tasks

myronmarston changed the title ~~[WIP] Aggregate failures~~ Aggregate failures Integration May 12, 2015

xaviershay reviewed May 12, 2015
View reviewed changes

spec/rspec/core_spec.rb

@@ -218,6 +218,14 @@

end

end

it 'uses only one thread local variable', :run_last do

Copy link

Member

xaviershay May 12, 2015

Choose a reason for hiding this comment

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

huh, cute.

JonRowe reviewed May 13, 2015
View reviewed changes

myronmarston force-pushed the aggregate-failures branch from 214e86e to 02824d6 Compare May 15, 2015 15:11

myronmarston added 19 commits May 15, 2015 18:06

Include shared group info in output for fixed pending examples.

4473ef2

Favor constructor arg injection over subclass method overriding.

744a397

This will make it easier to extract an `ExceptionFormatter`.

Move exception formatting logic into its own class.

e84ba30

- Simplifies the notification classes. - Provides something we can use to format multiple expectation failures packaged as a single exception for `aggregate_failures`.

Improve indentation support in ExceptionPrinter.

96678ca

Allow Failure/Error line to be used as the exception description.

ca21838

Pass indentation to :detail_formatter as well.

7374753

Implement presenter for aggregated failures.

ce7d9a2

Fix formatting for pending MultipleExpectationsNotMetError.

39fdad5

To get this to work properly, we have to compose the exception presenter options for pending and for MultipleExpectationsNotMetError.

Exclude shared group backtrace from sub-failure backtraces.

93dff11

It is already listed on the aggregate failure backtrace, and would be redundant to list it on each sub-failure.

Mask .java lines in Ruby backtraces.

1f162ed

This should hopefully make the travis build go green...

Standardize on rspec-support’s thread local data hash.

6a18dbd

Implement :aggregate_failures tagging.

7e2d399

For rspec/rspec-expectations#733.

Add cuke for aggregating failures.

c07e241

Remove :in ... portion of backtrace lines from example output.

2cfa3a8

- The presence of backticks within that part was causing rendering problems on relish. - Different rubies have different output there so we already ignore that part when comparing the expected to actual output.

Allow description to be omitted.

86ab1fd

This is necessary to support nested aggregation blocks.

Ensure exception formatting is applied recursively.

2629b79

This is necessary for niche situations where `aggregate_failures` is nested.

Add scenario demonstrating mock expectation aggregation.

d455692

Reword cuke step.

2d17102

myronmarston force-pushed the aggregate-failures branch from a999dc8 to 2d17102 Compare May 16, 2015 01:08

myronmarston added a commit that referenced this pull request May 16, 2015

Merge pull request #1946 from rspec/aggregate-failures

4334568

Aggregate failures Integration

myronmarston merged commit 4334568 into master May 16, 2015

myronmarston deleted the aggregate-failures branch May 16, 2015 02:08

myronmarston added a commit that referenced this pull request May 16, 2015

Add changelog entry for #1946.

98ade33

[ci skip]

MatheusRich pushed a commit to MatheusRich/rspec-core that referenced this pull request Oct 30, 2020

Merge pull request rspec#1946 from rspec/aggregate-failures

87ad8c6

Aggregate failures Integration

MatheusRich pushed a commit to MatheusRich/rspec-core that referenced this pull request Oct 30, 2020

Add changelog entry for rspec#1946.

8f2da5b

[ci skip]

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Aggregate failures Integration #1946

Aggregate failures Integration #1946

myronmarston commented Apr 23, 2015

JonRowe Apr 27, 2015

myronmarston Apr 28, 2015

JonRowe Apr 28, 2015

myronmarston Apr 28, 2015

JonRowe Apr 28, 2015

myronmarston commented Apr 28, 2015

JonRowe commented Apr 29, 2015

myronmarston commented May 2, 2015

JonRowe commented May 2, 2015

myronmarston commented May 12, 2015

xaviershay May 12, 2015

xaviershay commented May 12, 2015

xaviershay commented May 12, 2015

myronmarston commented May 12, 2015

JonRowe May 13, 2015

myronmarston commented May 15, 2015

xaviershay commented May 16, 2015

Aggregate failures Integration #1946

Aggregate failures Integration #1946

Conversation

myronmarston commented Apr 23, 2015

JonRowe Apr 27, 2015

Choose a reason for hiding this comment

myronmarston Apr 28, 2015

Choose a reason for hiding this comment

JonRowe Apr 28, 2015

Choose a reason for hiding this comment

myronmarston Apr 28, 2015

Choose a reason for hiding this comment

JonRowe Apr 28, 2015

Choose a reason for hiding this comment

myronmarston commented Apr 28, 2015

JonRowe commented Apr 29, 2015

myronmarston commented May 2, 2015

JonRowe commented May 2, 2015

myronmarston commented May 12, 2015

xaviershay May 12, 2015

Choose a reason for hiding this comment

xaviershay commented May 12, 2015

xaviershay commented May 12, 2015

myronmarston commented May 12, 2015

JonRowe May 13, 2015

Choose a reason for hiding this comment

myronmarston commented May 15, 2015

xaviershay commented May 16, 2015