Support configurable [no-]fail-fast mode #118

Closed
dchelimsky opened this Issue Feb 14, 2012 · 13 comments

Projects

None yet

6 participants

@dchelimsky
RSpec member

The one-expectation-per-example guideline is motivated by the goal of seeing all failed expectations reported, not just the first one in any example. We could satisfy that need by recording failed expectations and reporting them at the end of each example instead of raising errors. This is how Jasmine works, and it is quite liberating.

Of course, some users will prefer a fail-fast mode, so my proposal is that we have add a config option that defaults to true, e.g.

RSpec.configure do |config|
  config.expectations_fail_fast = true # or false
end

Then we can choose to have it default to false in 3.0 (or not).

Of course, when using rspec-expectations outside of rspec, we'd need a different means of configuring this for that scenario.

See rspec/rspec-core#573

@vrish88

This would be an awesome feature. I've looked into it a little bit and I can see two ways of implementing this.

  1. Removing the raised error in RSpec::Expectations.fail_with and then altering the RSpec core to look for something other than an exception to be raised.
  2. Monkeying around with continuations.

Are these the only two options?

@dchelimsky
RSpec member

@vrish88 #2 is not a viable option IMO. I was thinking along the lines of #1: rspec-expectations would register a container for failure messages on each example, and then matchers would report failures to that container, which would then report out at the end of each example.

@vrish88

@dchelimsky agreed. But in order to maintain compatibility with other frameworks, we still need to raise an error upon completion right? So would we bundle up all the errors into one exception and raise that?

Also, how does this look for basic formatting output?

describe "Example" do
  it "should report all of the failing examples" do
    "This lovely string".should match(/foo/)
    3.should_not == 3
  end
end
Failures:

  1) Example Group should report all of the failing examples
     - Failure/Error: "This lovely string".should match(/foo/)
         expected "This lovely string" to match /foo/
       # ./multiple_failing_expectations_spec.rb:3:in `block (2 levels) in <top (required)>'
     - Failure/Error: 3.should_not == 3
         expected not: == 3
                  got:    3
       # ./multiple_failing_expectations_spec.rb:4:in `block (2 levels) in <top (required)>'

@dchelimsky
RSpec member

That looks good. Keep in mind this needs to be an opt-in (default behavior should stay as it was).

@justinko

@dchelimsky

I'm not sure how this is going to work because of the separation between rspec-core and rspec-expectations.

Right now, a matcher is impervious to an example (as it should be because of stand-alone). How can rspec-expectations be aware of a "container" let alone when the last matcher has executed in an example (to raise an exception)?

I believe all of this functionality is going to have to go in rspec-core (in RSpec::Core::Example). Thoughts?

@myronmarston
RSpec member

I don't think any of this functionality will have to go in core.

  • RSpec::Core::Configuration provides the means for extension libs to add settings.
  • rspec-expectations can add the expectations_fail_fast setting in this manner.
  • rspec-expectations will keep track of failed expectations in some collection object.
  • rspec-expectations can globally add an after(:each) hook that looks at the collection of failed expectations, and, if there are any, raises a nicely formatted message.
@justinko

I was thinking more along the lines of when an example is "eval'd", there would be a rescue clause to catch the matcher exceptions and put them in a collection (using continuations). It would then raise a "collection error" and report on it. This would also allow you to have "no-fail-fast" with other "assertion" libs (t/u, wrong, etc.).

Using an after hook feels strange. I can think of some caveats:

  • We'd have to make sure this after hook runs first, otherwise the failed matchers will never be "reported" (I think).
  • This "collection" would have to be stored globally in rspec-expectations. We all know the problems with global state.

However, all of this is not visible to the end user so I would be okay with this approach, as long as @dchelimsky is.

@vrish88

@myronmarston how would after(:each) work with other test frameworks, like Test::Unit?

@justinko

So, it looks like Matz advises against using continuations, and Rbx and JRuby do not support them.

Because "assertion" libs raises exceptions, which is definitely the right thing to do, I'm going to say this is our options:

  • Forget this feature.
  • Go ahead with @myronmarston's approach with the callbacks.
@tvanderpol

Is there still interest in this? If it is I think this approach is the only one that was being considered here, correct?

@JonRowe
RSpec member

I think this should stay a non-feature, it'd add clutter to the codebase and obfuscate the test output.

In Jasmine this mode leads to false positives and confusion as a single line can break the entire test suite, meaning you can't see the actual failure and you get a order of magnitude more failures beyond what's actually broken.

updated removed unhelpful comments, sorry, pre coffee ;)

@myronmarston
RSpec member

Thanks for resurrecting this discussion, @tvanderpol and @JonRowe. I don't have any experience working with Jasmine, but I can definitely foresee the sorts of problems @JonRowe mentioned arising with this feature enabled. On top of that this would introduce a fair bit of complexity into rspec-expectations, I think. @dchelimsky -- are you OK with us closing this or do you feel like it's still needed?

@dchelimsky
RSpec member

Fine with me.

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