Make --fail-fast configurable #2065

jackscotti · 2015-08-22T07:33:07Z

The aim of this PR is making fail-fast configurable, requested here: #1874

JonRowe · 2015-08-23T12:17:05Z

features/step_definitions/additional_cli_steps.rb

@@ -98,7 +98,7 @@
 end

 Given /^I have a brand new project with no files$/ do
-  in_current_dir do
+  in_current_directory do


Are these intentional changes? Or have you just not rebased recently...

I think these are why the build is failing BTW, we've upgraded some Aruba things lately

This changes are part of the first commit. I have no idea why, but I had failures locally that could be only silenced by renaming these methods. The build is failing only because of these methods that have been renamed. Give me a second, I'll delete the commit and push the branch up again.

jackscotti · 2015-08-31T10:29:17Z

@JonRowe, I managed to get rid of Rubocop's warning by extracting the logic into its own module. The remaining failures do not appear to be related to my changes (?). Are you familiar with them?

I am going to squash/reword commits and start to look into the parser after having understood these errors.

JonRowe · 2015-09-08T05:37:37Z

The last remaining build failures are due to documentation coverage dropping below 100%

JonRowe · 2015-09-08T05:38:26Z

lib/rspec/core/fail_fast_handler.rb

@@ -0,0 +1,17 @@
+module RSpec
+  module Core
+    module FailFastHandler


There's no docs for this module which is what is putting off the coverage, mark the methods and module with # @api private if they're not intended for public usage.

jackscotti · 2015-09-10T08:52:46Z

I tried doing as you said but the test coverage does not change.

Without #api private:

With:

I also tried following the style used in other modules, that is #api private above the module declaration and # @private above the methods within itself, still nothing changes.

fables-tales · 2015-09-17T20:27:24Z

@jackscotti can you share the full yarddoc output for undocumented classes?

jackscotti · 2015-09-30T07:54:01Z

@samphippen, sorry I have been away from any electronic device for the last 2 weeks. Friday I will restart to work on this (and provide you the yarddoc).

fables-tales · 2015-09-30T07:54:39Z

@jackscotti no problem, 👍

jackscotti · 2015-10-02T10:13:58Z

Before the last commit:

After last commit:

Now the test coverage should be 100%, but Travis says it's not. Any idea?

myronmarston · 2015-10-02T13:26:34Z

You fixed the missing documentation coverage but the build is failing due to missing test coverage. As of #1905 we enforce that rspec-core has 100% test coverage on ruby 2.1 and above. This has helped identify dead code and some bugs. To see this locally, you'll need to run script/rspec_with_simplecov. That's a special binary we have to use to use simplecov with RSpec on itself because simplecov has to be loaded before all the code you are intending to check coverage of -- which means that the normal rspec command won't work because RSpec loads itself first. Running that locally reveals there is missing spec coverage for this line and these lines.

myronmarston · 2015-10-02T13:35:39Z

lib/rspec/core/example_group.rb

@@ -26,7 +26,7 @@ module Core
    # your examples defined in {MemoizedHelpers} and {Pending}.
    class ExampleGroup
      extend Hooks
-
+      extend FailFastHandler


Since FailFastHandler is completely private and doesn't have any APIs we want to expose to users, there's no need for the methods to be added to ExampleGroup like this. After all, it's not a property of the example group class whether or not the failures number has been met. I'd prefer it if you just made the methods exposed by FailFastHandler module methods and called them as e.g. FailFastHandler.failures_number_met? w/o extending the module.

Actually, seeing failures_number_met?(reporter.failed_examples.count) below makes me think that these method defs should perhaps move to the reporter: then it can just be reporter.fail_fast_limit_met?. After all, the reporter holds the state of reporter.failed_examples.count already and it has a reference to the configuration instance. We try to limit how much we access our global state (e.g. RSpec.configuration and RSpec.world) so it's nice to use instances that objects have already been passed such as reporter already having a config instance.

The way of implementing it you're suggesting sounds better, I will address it as soon as I can. Thank you for the great feedback and apologise for the noise, I am pretty unfamiliar with this codebase.

No apology needed. Thanks for contributing! RSpec is better because of contributions like yours.

jackscotti · 2015-10-02T15:58:18Z

Alright, seems like we managed to get Simplecov happy. Thank you again for all the feedback, I will address it next time I'll work again on this. :)

myronmarston · 2015-10-06T01:46:27Z

lib/rspec/core/example_group.rb

-          RSpec.world.wants_to_quit = true if fail_fast? && !succeeded
+          if !succeeded && fail_fast?
+            RSpec.world.wants_to_quit = true if reporter.fail_fast_limit_met?
+          end


I'd still like to see the two if statements here combined into one. Nesting them seems messier and unnecessary to me.

jackscotti · 2015-10-07T14:30:32Z

@myronmarston , I have one error I do not understand and one of code not covered. That line of code hasn't been introduced by me, why do you think Simplecov is picking it up only now? And, are you familiar with the error?

Update:
regarding the error, I just found this: https://bugs.ruby-lang.org/issues/10661 and understood why @JonRowe asked me to rename the variable.

myronmarston · 2015-10-07T16:17:36Z

regarding the error, I just found this: https://bugs.ruby-lang.org/issues/10661 and understood why @JonRowe asked me to rename the variable.

Yeah, the Ruby 2.2.0 warning is extremely annoying, particularly because of RSpec's let feature. It looks like the latest build is still getting a failure from that -- this time from reporter. I noticed your change in the last commit, where you renamed group to the_group in a bunch of places. Usually to fix this you want your local variables to be named differently from your let definitions but you changed them both to the_group which I would have thought would have not fixed the warning (it looks like it did, though -- I'm a bit confused as to how!). So I think you just need to make a similar change to reporter. Sorry for the trouble :(. I really wish that warning had never been introduced in Ruby 2.2.0.

jackscotti · 2015-10-08T12:16:28Z

WOOOOHOOOO!! All green!
@myronmarston , thank you for the good explanation, I was finding really hard to silence those warnings. I rebased that commit, now it has a much cleaner diff.

To note that if I address @JonRowe comment about removing self. the warnings come back.

This PR is now ready for review, I will squash/rename/reword the commits ASAP.
Thank you all for the great feedback and support. :)

JonRowe · 2015-10-08T23:11:07Z

features/configuration/fail_fast.feature


-  ```ruby
-  RSpec.configure { |c| c.fail_fast = true }
-  ```


This is documentation and should be restored (although you can add a comment that true means 1)

Do we still have an example of just a bare --fail-fast?

here: https://github.com/rspec/rspec-core/pull/2065/files#diff-87d87ffcd904e2f75195937563248991L20

JonRowe · 2015-10-08T23:13:37Z

To note that if I address @JonRowe comment about removing self. the warnings come back.

Because you didn't follow the second part and rename the let instead ;)

This PR is now ready for review, I will squash/rename/reword the commits ASAP.

Just a few final tweaks, and yep if you could squash the commits that'd be great :)

jackscotti · 2015-10-30T11:29:05Z

Is there anything I can do to get this PR merged?

fables-tales · 2015-10-30T11:30:44Z

@jackscotti when was the last time you rebased against master? I definitely saw something about aruba flying by in my inbox, so that might solve the problem.

JonRowe · 2015-10-30T11:31:57Z

I'd still like to solve the issue with the CLI, tests pass for me and it works but cucumber doesn't seem to like it

jackscotti · 2015-10-30T12:09:30Z

Thanks for getting back to me so quickly!

Rebased on master, now the error is changed. Don't really undertand what is complaining about? It seems to be related to Aruba and not to the code I've introduced.

myronmarston · 2015-10-30T14:47:29Z

The failure isn't related to aruba other than the fact that it's a cucumber scenario and we use aruba in our cucumber scenarios and "aruba" is in the directory name that gets used for the scenario. The failing scenario is the --fail-fast 3 one that you added in this PR. It looks like it's interpreting the 3 in --fail-fast 3 as a file name and not as an argument to --fail-fast. Probably caused by 46eb102?

jackscotti · 2015-10-30T14:55:44Z

Yes, you are right.

We are back to the original error then. What should the parser accept?
I can delete that commit if you are happy with the =.

myronmarston · 2015-10-30T14:59:32Z

I have no idea how to get it to work without the =. I ran into the same thing with bisect. @JonRowe seems to think it's possible to get it to work but for now remove the commit that took the = out and Jon can always do a follow up PR showing us how to make it work without = if he can figure it out.

myronmarston · 2015-10-30T15:02:10Z

BTW, before merging it would also be nice to not have 19 commits (including some merge commits). Do you mind rebasing and squashing those out so that the PR has a small number of logical commits?

If that's difficult for you (it's one of those git things that's tricky to do for the first time) let me know and I can push up what you've got to a branch so we can get it merged from there.

Also, sorry for the long delay on this. We've done a poor job at helping you get this ready :(.

jackscotti · 2015-10-30T15:10:17Z

Really no need to be sorry, I know you've all got a lot to do!!

I am going to rebase/squash/reword the commits and let you know when I think I am done, so you can tell me whether the final result is good or not.

Thank you for the great feedback and patience.

A configurable fail-fast requires OptionParser to be able to handle Integer parameters and filter out anything non-Integer.

The reporter is already ware of the number of failed examples but it should also know whether the required number of failures has been met. Reporter#fail_fast_limit_met? will be used by ExampleGroup to check whether it should stop running examples.

Set `RSpec.world.wants_to_quit = true` when the required number of failed examples has been met.

Make --fail-fast configurable

- Better warning when an invalid value is passed for `--fail-fast` (including silencing call site since it will not be in user code). - No need to stub `reporter.failures_required`. - No need for the `failures_required` method at all. - Use `fail_fast_limit_met?` in more situations. - Remove unneeded `ExampleGroup.fail_fast?` method.

myronmarston · 2015-11-01T06:56:35Z

Thanks, @jackscotti!

Followups from #2065.

- Better warning when an invalid value is passed for `--fail-fast` (including silencing call site since it will not be in user code). - No need to stub `reporter.failures_required`. - No need for the `failures_required` method at all. - Use `fail_fast_limit_met?` in more situations. - Remove unneeded `ExampleGroup.fail_fast?` method.

Make --fail-fast configurable

- Better warning when an invalid value is passed for `--fail-fast` (including silencing call site since it will not be in user code). - No need to stub `reporter.failures_required`. - No need for the `failures_required` method at all. - Use `fail_fast_limit_met?` in more situations. - Remove unneeded `ExampleGroup.fail_fast?` method.

Followups from rspec#2065.

jackscotti changed the title ~~WIP - Configurable fail fast~~ WIP/RFC - Configurable fail fast Aug 22, 2015

JonRowe reviewed Aug 23, 2015
View reviewed changes

JonRowe reviewed Sep 8, 2015
View reviewed changes

myronmarston reviewed Oct 2, 2015
View reviewed changes

myronmarston reviewed Oct 6, 2015
View reviewed changes

JonRowe reviewed Oct 8, 2015
View reviewed changes

jackscotti and others added 4 commits October 30, 2015 17:12

Make OptionParser handle fail-fast parameters

6caef8b

A configurable fail-fast requires OptionParser to be able to handle Integer parameters and filter out anything non-Integer.

Make ExampleGroup stop running examples when appropriate

c6819f0

Set `RSpec.world.wants_to_quit = true` when the required number of failed examples has been met.

Update fail_fast description in configuration.rb

55f2e23

jackscotti changed the title ~~WIP/RFC - Configurable fail fast~~ Make --fail-fast configurable Oct 30, 2015

jackscotti and others added 2 commits October 30, 2015 19:51

Update fail-fast integration tests

2c1cd3d

Silence ruby 2.2 warnings

37cdf42

myronmarston added a commit that referenced this pull request Nov 1, 2015

Merge pull request #2065 from jackscotti/configurable-fail-fast

b60a1ca

Make --fail-fast configurable

myronmarston merged commit b60a1ca into rspec:master Nov 1, 2015

myronmarston mentioned this pull request Nov 1, 2015

Feature request: configurable --fail-fast #1874

Closed

myronmarston added a commit that referenced this pull request Nov 1, 2015

Merge pull request #2105 from rspec/pr-2065-followups

19761e3

Followups from #2065.

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

Merge pull request rspec#2065 from jackscotti/configurable-fail-fast

9aee075

Make --fail-fast configurable

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

Merge pull request rspec#2105 from rspec/pr-2065-followups

ccd5bac

Followups from rspec#2065.

Uh oh!

Make --fail-fast configurable #2065

Make --fail-fast configurable #2065

Uh oh!

Conversation

jackscotti commented Aug 22, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jackscotti commented Aug 31, 2015

Uh oh!

JonRowe commented Sep 8, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jackscotti commented Sep 10, 2015

Uh oh!

fables-tales commented Sep 17, 2015

Uh oh!

jackscotti commented Sep 30, 2015

Uh oh!

fables-tales commented Sep 30, 2015

Uh oh!

jackscotti commented Oct 2, 2015

Uh oh!

myronmarston commented Oct 2, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jackscotti commented Oct 2, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jackscotti commented Oct 7, 2015

Uh oh!

myronmarston commented Oct 7, 2015

Uh oh!

jackscotti commented Oct 8, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JonRowe commented Oct 8, 2015

Uh oh!

jackscotti commented Oct 30, 2015

Uh oh!

fables-tales commented Oct 30, 2015

Uh oh!

JonRowe commented Oct 30, 2015

Uh oh!

jackscotti commented Oct 30, 2015

Uh oh!

myronmarston commented Oct 30, 2015

Uh oh!

jackscotti commented Oct 30, 2015

Uh oh!

myronmarston commented Oct 30, 2015

Uh oh!

myronmarston commented Oct 30, 2015

Uh oh!

jackscotti commented Oct 30, 2015

Uh oh!

myronmarston commented Nov 1, 2015

Uh oh!

Uh oh!