Consider removing `before(:all)`

[dchelimsky]

The initial intent of before(:all) was to work like a class-level setup method in xUnit tools to set something up in the environment that you only want to set up once. At some point, we made it a cross-example state-sharing mechanism, and since that point it's been nothing but trouble.

The superficial motivation for using this as a state-sharing mechanism is speed, but this really stems from the one-expectation-per-example guideline, which, in turn, stems from the fact that expectations raise errors rather than recording and reporting later.

We should consider removing before(:all) from rspec-core, and supporting a non-fast-fail mode in rspec-expectations.

[dchelimsky]

FYI - this was motivated by issue #500, among others.

[dchelimsky]

297 asks that we rename before(:all) to before(:group). I'd be open to leaving before(:group) as a non-state-sharing group-level setup as we remove before(:all).

[justinko]

This is huge and a major +1

[myronmarston]

FWIW, I like before(:all) and use it on occasion. I think it's a "use with care" power user's tool, but it does have its uses.

[dchelimsky]

@myronmarston what do you think about introducing before(:group) as a non-state-sharing variant of before(:all), and then deprecating before(:all) as it stands today?

[myronmarston]

I have on occasion used before(:all) to share state, and those specs wouldn't work with the new before(:group) hook. Here's an example of where I use it:

I've got a library that does a bunch of web scraping and parsing. The specs are largely integrated (using VCR to record/playback the HTTP requests), and in many places I follow this pattern:

describe "Scraping page X" do
  before(:all) do
    @scrape_summary = MyScraper.perform_on("http://foo-bar.com/bazz")
  end

  it "extracts one kind of thing from the page" do
    @scrape_summary.total_results.should eq(25)
  end

  it "extracts something else from the page" do
    @scrape_summary.link_urls.should include("http://some-expected-url.com/")
  end

  # ...several more of these sorts of examples
end

All of the examples in this case are 100% side-effect free expectations set on the results of scraping. Thus, using before(:all) in this manner is safe, and gives the specs a huge speed boost.

You mention the "one expectation per example" guideline, and that a no-fail-fast mode in rspec-expectations could solve this problem...but it doesn't solve it completely for me. I've never followed "one expectation per example"; instead, it's "one behavior per example, and multiple expectations in the same example may be a sign you are specifying multiple behaviors". For some of the specs like the ones above, I already do put multiple expectations in the same example, when they are related to specifying the same behavior. If I was to rewrite these specs to use a new no-fail-fast mode and condense the multiple examples into one, I'd either wind up with a single mammoth it "does A, B, C, D and E" doc string, or a very generic it "works" or it "scrapes what it should" doc string so as not to have huge long one. I don't really like either option. I like having these as separate examples (since they specify different facets of behavior). It's important to me to have specific, descriptive doc strings.

Overall, I think before(:all) can get messy and is often a design smell. When you're writing de-coupled, well-factored code, you should never need to use it. In the case of a library that does web scraping, isolated testing doesn't work so well; you need to integrate with examples of the actual pages you are scraping and parsing.

[justinko]

I had a discussion on Twitter & Github gists sometime last year about all of this.

Basically, we use before(:all) because of descriptions. That's why I use them anyway. Here are some ideas to get around this:

describe "Scraping page X" do
  let(:scrape_summary) { MyScraper.perform_on("http://foo-bar.com/bazz") }

  it 'extracts the correct elements from the page'  do
    detail "extracts one kind of thing from the page" do
      scrape_summary.total_results.should eq(25)
    end

    detail "extracts something else from the page" do
      scrape_summary.link_urls.should include("http://some-expected-url.com/")
    end
  end
end

describe "Scraping page X" do
  let(:scrape_summary) { MyScraper.perform_on("http://foo-bar.com/bazz") }

  it 'extracts the correct elements from the page (generic description)'  do
    expect('extracts total results', scrape_summary.total_results).to eq(25)
    expect('extracts link urls', scrape_summary.link_urls).to include("http://some-expected-url.com/")
  end
end

Or, we could move the work down to the "example" level:

describe "Scraping page X" do
  let(:scrape_summary) { MyScraper.perform_on("http://foo-bar.com/bazz") }

  # Each example group runs all the zits (obviously not the final method name) at once.

  zit "extracts one kind of thing from the page" do
    scrape_summary.total_results.should eq(25)
  end

  zit "extracts something else from the page" do
    scrape_summary.link_urls.should include("http://some-expected-url.com/")
  end
end

Like any of these ideas?

[myronmarston]

@justinko -- of those options, the first one (the detail one is the one I like best), but I don't think I like the idea of introducing a new construct for this. It's not a very common case, and adding more to the DSL adds additional cognitive load.

@dchelimsky -- is the main reason you want to remove before(:all) because of all the gotchas surrounding the shared state it allows? I think we're all agreed that the shared state feature of before(:all) is usually a bad idea. As I explained above, in a few very particular circumstances I find it useful. Given that...what if we made it a "disabled by default" feature that users must opt in to use? Here's what I mean:

With this change, instance variables initialized in before(:all) would be ignored by RSpec, and would thus not available in examples. However, if you really need this feature, you could do:

describe "Scraping page X" do
  before(:all, :share_state) do
    @scrape_summary = MyScraper.perform_on("http://foo-bar.com/bazz")
  end
end

Or it could just be a config option in the RSpec.configure block--but I kinda like it being a per-before(:all) hook thing.

[justinko]

@myronmarston the discussion here is to entirely remove before(:all) from RSpec. :share_state doesn't change anything -- especially code wise.

[myronmarston]

@myronmarston the discussion here is to entirely remove before(:all) from RSpec.

My understanding is that the discussion is to remove before(:all) in favor of before(:group) that acts the same, except for the fact that it doesn't share state. We could certainly add before(:group) as an alias (or similar-behaving method) but I'd like to see a way to preserve the existing before(:all) behavior, but make it opt-in, rather than turned on by default. Whether that's keeping before(:all) itself or migrating to before(:group) doesn't matter as much to me.

:share_state doesn't change anything -- especially code wise.

:share_state would preserve the existing behavior. The real change I'm suggesting is to make state not shared by default. This forces people to be intentional about "yes, I really do want to share state across all these examples", rather than hacking together some specs that use before(:all) and happen to work but will later cause trouble.

[justinko]

I'm tempted to write a gem that allows you to have "shared state" and remove before(:all) from RSpec:

describe do
  let(:foo) { shared { do_something } }
  # or
  shared(:foo) { do_something }

  it do
    foo # computed first
  end

  it do
    foo # memoized value returned
  end
end

The shared method would handle memorization and "clean up". It could store the shared values in the describe (ExampleGroup) instance. Or, it could use global variables namespaced by "rspec".

Thoughts?

[justinko]

The real change I'm suggesting is to make state not shared by default.

Doesn't matter if it's on or off, we would still have to support the case when it is on. This would only add more code: "if before all shared state is on...".

[myronmarston]

An external gem that provides this behavior may be the way to go. A single shared DSL method that mirrors let has much less cognitive load than the other APIs you proposed above. I also like that putting it in an external gem means it's clear that it's not the "normal" way to write specs.

[justinko]

I would store the "shared" values in Thread.current, btw.

[dchelimsky]

is the main reason you want to remove before(:all) because of all the gotchas surrounding the shared state it allows?

@myronmarston it's more specifically the confusing nature of the relationship between shared state and features that are intended to work per-example, like let and subject. I think shared state is manageable if it's visible (i.e. instance variables in before(:all)).

[myronmarston]

@dchelimsky -- that makes sense. Earlier you said:

I'd be open to leaving before(:group) as a non-state-sharing group-level setup as we remove before(:all).

How would let and subject behave when called from within a before(:group) hook?

[coffeeaddict]

I use before(:all) quite often to perform setup (other then just of the subject) - I do agree that :each and :all feel a bit clumsy - :group on the other hand also feels a tad clumsy. What about before(:context)? Given the fact that there is know verb called context in the DSL, this makes most sense imho

[justinko]

Okay here's the shared method: https://github.com/justinko/rspec-shared

So looks like the roadmap is this:

• Change before(:all) to before(:group) and remove shared instance variables.
• Change/alias before(:each) to before(:example)

[dchelimsky]

I think we should leave before(:all) as/is through the 3.0 series, deprecating it in the last 3.x before 4.0. That gives users a year or so to gradually change before(:all) to before(:group).

[jarmo]

I'm using before(:all) in my acceptance tests to open up the browser and store it's value in an instance variable. If before(:all) would be removed, how would i solve that use-case instead?

[d11wtq]

I'm using before/after all to start and stop a system process in a test rig. Perhaps there's another approach to this? Doing this before/after each would be too slow. The daemon in question is sphinx. I basically write out a "test" sphinx.conf to a temp dir, start the process in a before(:all), run all the examples, emptying the indexes in a before(:each) (this is quite fast), then stop sphinx in a after(:all) and clean up the temporary data directory. I'm not sure how I'd do this without such hooks.

[dchelimsky]

@d11wtq @jarmo the "instance state sharing" provided by before(:all) is a bit misleading because we're actually just copying instance variables around. A more representative solution would be one that forces the user to manage state sharing through global variables or constants. if/when we do this we'll still have a place to put code that should only run once (either before(:group) as originally planned or before(:context) as suggested earlier this thread), but it will be up to you to manage the state e.g.

before(:group) do
  $browser = BrowserSimulator.new
  $browser.start
end

after(:group) do
  $browser.stop
end

This would have the additional advantage of clarifying what is per-example state (using @) and what is per-group state (using $) e.g.

before(:group) do
  $browser = BrowserSimulator.new
  $browser.start
end

before(:example) do
  @page = Page.load(some_path)
end

[wulftone]

This gem is related to the issue: https://github.com/LRDesign/rspec-steps

I use that gem and the :all feature very often. (It would be great to see the steps feature integrated into rspec) Isolating specs is all fine and good, but frequently I just know what I'm doing and want things to run as fast as possible. I'd hate to see this spec group setup thing disappear, however I do like @justinko's detail thing.

He's right, we really just want the group behavior for the descriptions in the spec output, but we're trained to shun having multiple .should's in a single it block, though it's a really natural way to test things (but you don't get a description for each should in a single it block!)

[patmaddox]

+1000 !! death to before(:all) - and any_instance too while we're at it :)

[likethesky]

Has any thought been made to throwing an error if a before(:all) has been used without a corresponding after(:all) ... after all? ;-) Perhaps this would be one way of leaving this feature for the minority of cases where it's really needed, but this error would at least alert the unaware that "something extra" must be thought about before blindly proceeding. IOW, you'd have to have at least an empty after(:all) do end any time you want to use a before(:all) do # something; end ? I also like the gem approach, but that seems more tedious and not as pleasurable, to me.

I also like the before(:group) and before(:example) naming, with deprecation warnings issued for before(:each) perhaps, or just letting it continue to work as-is... Also, what's the thought on before do end (that is, without any scope identifier), will that stay as-is? I note that the docs don't discuss this variant though it's what I see day-in and day-out... Happy to open an issue to get the docs updated, if that's just an oversight.

[EarthCitizen]

I use before(:all) when testing shell scripts and database scripts:

describe "My shell script" do
    context "When I run my script" do
        before(:all) do
              ---code here to run the script---
        end
        it { accomplishes task1 }
        it { accomplishes task2 }
        it { accomplishes task3 }
        it { accomplishes task4 }
    end
end

In this case, I am not checking the output of the run. I am checking to see that it performed it's required objectives. How else could I check multiple objects for one run without a before(:all)?

[soulcutter]

@EarthCitizen With multiple assertions inside a single 'it' block, or just using multiple runs to make separate assertions.

[EarthCitizen]

@soulcutter From what I understand, multiple assertions inside a single example is bad practice. Running the scripts multiple times can be unnecessarily inefficient because often the scripts can have a long run time. So running them before each example would create a huge overhead.

[myronmarston]

From what I understand, multiple assertions inside a single example is bad practice.

There's a time and place for "one assertion per example", but this is definitely not a bad practice. My thoughts on the manner:

https://github.com/andreareginato/betterspecs/issues/5#issuecomment-9110114

[EarthCitizen]

@myronmarston Thanks for the input. I see your points. But if we have to pack multiple assertions into one example for performance reasons, would it not just be better to use before(:all) ?

[myronmarston]

But if we have to pack multiple assertions into one example for performance reasons, would it not just be better to use before(:all) ?

I'm not necessarily convinced that one is better then the other, but lots of tools hook into before(:each) to ensure clean slate between each test. before(:all) can't easily work with such tools -- so I tend to favor before(:each) for such things. With before(:all) you have to be extra careful to manage your state well so you don't wind up with "leaked" state that affects other specs.

But in the case here, it sounds like you've had success using before(:all) and I think that's fine.

[EarthCitizen]

@myronmarston Yeah, I agree with you that it should be used with care. It should be for exceptional cases and not as a standard strategy.

[myronmarston]

@dchelimsky and I discussed this some more, and we've decided to keep before(:all). Misunderstandings about it have definitely been a source of issues, and there's something appealing about the simplicity it would bring to remove it, but it's been around for a long time and is still occasionally useful.

[baash05]

Thank you so much for keeping it. changing the language to something so close just because some people abuse it frustrates migration. It would mean I couldn't ever upgrade my rspec. Rails did it with attribute_accessible and now at all the intro courses I have to explain why 90% of the tutorials on line don't work, and why you can't migrate your code from rails 3 to 4.

Thank you so much!