Improve `rspec --init`. #1219

Merged
merged 3 commits into from Dec 23, 2013

Conversation

Projects
None yet
7 participants
@myronmarston
Member

myronmarston commented Dec 15, 2013

  • Add lots more suggested settings to spec_helper.rb
    to give a great initial experience.
  • Add a cucumber scenario that demonstrates that the
    settings generated by rspec --init are all valid.
    It doesn't demonstrate that they do what they are
    intended to do, but it protects against misspelled
    settings and whatnot.
  • Remove old cruft.
  • Put the init contents in their own files. Much
    cleaner than using heredocs.

I'd like some 馃憖 on this to see what others think about all the settings this adds. Is this overkill? I think it'll make for a better starting experience to have more in the generated spec_helper.rb file. Are there any settings I've added that shouldn't be included? Anything I'm missing?

It would also be good to find a way to allow rspec-rails' generator to leverage this and build on top of it for the additional rails-specific stuff it adds. I'm not sure of the best way to do that, though.

/cc @alindeman @JonRowe @xaviershay @soulcutter @samphippen

Improve `rspec --init`.
- Add lots more suggested settings to spec_helper.rb
  to give a great initial experience.
- Add a cucumber scenario that demonstrates that the
  settings generated by `rspec --init` are all valid.
  It doesn't demonstrate that they do what they are
  intended to do, but it protects against misspelled
  settings and whatnot.
- Remove old cruft.
- Put the init contents in their own files. Much
  cleaner than using heredocs.
@@ -0,0 +1,3 @@
+--color
+--warnings
+-rspec_helper

This comment has been minimized.

@yujinakayama

yujinakayama Dec 15, 2013

Member

I think this is prone to be confused as if there's -rspec_helper option. Maybe --require spec_helper would be more explicit.

@yujinakayama

yujinakayama Dec 15, 2013

Member

I think this is prone to be confused as if there's -rspec_helper option. Maybe --require spec_helper would be more explicit.

This comment has been minimized.

@myronmarston

myronmarston Dec 15, 2013

Member

Great idea. Fixed in 9387778.

@myronmarston

myronmarston Dec 15, 2013

Member

Great idea. Fixed in 9387778.

@JonRowe

This comment has been minimized.

Show comment
Hide comment
@JonRowe

JonRowe Dec 15, 2013

Member

LGTM, 馃憤 on the explanations for each setting

Member

JonRowe commented Dec 15, 2013

LGTM, 馃憤 on the explanations for each setting

@@ -16,3 +17,19 @@ Feature: --init option
"""
When I run `rspec --init`
Then the output should contain "exist .rspec"
+
+ Scenario: Accept and use the recommended settings in spec_helper (which are initially commented out).

This comment has been minimized.

@xaviershay

xaviershay Dec 15, 2013

Member

This cuke is a good idea.

@xaviershay

xaviershay Dec 15, 2013

Member

This cuke is a good idea.

@@ -0,0 +1,91 @@
+# This file was generated by the `rspec --init` command. Conventionally, all
+# specs live under a `spec` directory, which RSpec adds to the `$LOAD_PATH`.
+# The generated .rspec file contains `--require spec_helper` which will cause this

This comment has been minimized.

@xaviershay

xaviershay Dec 15, 2013

Member

.rspec needs backticks around it.

@xaviershay

xaviershay Dec 15, 2013

Member

.rspec needs backticks around it.

+# light-weight as possible. Requiring heavyweight dependencies from this file
+# (such as loading up an entire rails app) will add to the boot time of your
+# test suite on EVERY test run, even for an individual file that may not need
+# all of that loaded.

This comment has been minimized.

@xaviershay

xaviershay Dec 15, 2013

Member

I approve of this paragraph.

@xaviershay

xaviershay Dec 15, 2013

Member

I approve of this paragraph.

This comment has been minimized.

@myronmarston

myronmarston Dec 16, 2013

Member

Thanks. Unfortunately, the rspec-rails generator makes spec_helper.rb load the entire rails app, which is directly against the advice here. We should do something to solve this apparent contradiction. Two ideas:

  • Keep this, but have rspec-rails generate a rails_spec_helper.rb file that requires spec_helper.rb and adds the rails-specific stuff. Then users can require rails_spec_helper from their files that are meant to integrate with rails and spec_helper for stand-alone ruby objects.
  • Rename this file to unit_spec_helper (or base_spec_helper).

@alindeman -- any thoughts? I think it'd be good to have new projects start with defaults that encourage folks to be more intentional about integrating with rails.

@myronmarston

myronmarston Dec 16, 2013

Member

Thanks. Unfortunately, the rspec-rails generator makes spec_helper.rb load the entire rails app, which is directly against the advice here. We should do something to solve this apparent contradiction. Two ideas:

  • Keep this, but have rspec-rails generate a rails_spec_helper.rb file that requires spec_helper.rb and adds the rails-specific stuff. Then users can require rails_spec_helper from their files that are meant to integrate with rails and spec_helper for stand-alone ruby objects.
  • Rename this file to unit_spec_helper (or base_spec_helper).

@alindeman -- any thoughts? I think it'd be good to have new projects start with defaults that encourage folks to be more intentional about integrating with rails.

This comment has been minimized.

@soulcutter

soulcutter Dec 16, 2013

Member

I think @myronmarston 's suggestion is a brilliant idea. A good default like that should encourage people to write in a style that results in faster-running tests. In the worst-case scenario they will just replace all require 'spec_helper' lines to require 'rails_spec_helper', but this would still give them a lever to improve test startup time.

@soulcutter

soulcutter Dec 16, 2013

Member

I think @myronmarston 's suggestion is a brilliant idea. A good default like that should encourage people to write in a style that results in faster-running tests. In the worst-case scenario they will just replace all require 'spec_helper' lines to require 'rails_spec_helper', but this would still give them a lever to improve test startup time.

This comment has been minimized.

@alindeman

alindeman Dec 23, 2013

Contributor

Ah, I just saw this comment.

The idea of a rails_spec_helper.rb intrigues me quite a bit. I was initially put off by it, thinking that loading the Rails app is consistent with Rails' defaults (even if we want to debate whether it's a good default 馃榾) and inconsistent with a low barrier to entry for new users (advanced users are going to figure out how to do this anyway).

That said, rails_spec_helper.rb is growing on me. We could change all of the generators in -rails to load rails_spec_helper.rb by default, though noting somewhere (docs? comment at the top of spec_helper.rb?) that you can load just spec_helper.rb to get a speed boost for objects that don't depend on Rails or only depend on a subset of Rails.

I think we could have a big positive impact on how people write Rails tests by doing this. Are there any cons I'm missing?

I'm mildly 馃憥 to rename this file to unit or base, because I think those terms will becoming confusing in the future.

@alindeman

alindeman Dec 23, 2013

Contributor

Ah, I just saw this comment.

The idea of a rails_spec_helper.rb intrigues me quite a bit. I was initially put off by it, thinking that loading the Rails app is consistent with Rails' defaults (even if we want to debate whether it's a good default 馃榾) and inconsistent with a low barrier to entry for new users (advanced users are going to figure out how to do this anyway).

That said, rails_spec_helper.rb is growing on me. We could change all of the generators in -rails to load rails_spec_helper.rb by default, though noting somewhere (docs? comment at the top of spec_helper.rb?) that you can load just spec_helper.rb to get a speed boost for objects that don't depend on Rails or only depend on a subset of Rails.

I think we could have a big positive impact on how people write Rails tests by doing this. Are there any cons I'm missing?

I'm mildly 馃憥 to rename this file to unit or base, because I think those terms will becoming confusing in the future.

+# test suite on EVERY test run, even for an individual file that may not need
+# all of that loaded.
+#
+# The .rspec file also contains a few flags that are not defaults but that

This comment has been minimized.

@xaviershay

xaviershay Dec 15, 2013

Member

ditto re backticks.

@xaviershay

xaviershay Dec 15, 2013

Member

ditto re backticks.

This comment has been minimized.

@myronmarston

myronmarston Dec 17, 2013

Member

Done.

+=begin
+ # Limit the spec run to only specs with the focus metadata. If no specs have
+ # the filtering metadata and `run_all_when_everything_filtered = true` then
+ # all specs will run.

This comment has been minimized.

@xaviershay

xaviershay Dec 15, 2013

Member

pretty sure this comment will make zero sense to a new rspec user.

@xaviershay

xaviershay Dec 15, 2013

Member

pretty sure this comment will make zero sense to a new rspec user.

This comment has been minimized.

@myronmarston

myronmarston Dec 16, 2013

Member

FWIW, this is already there. This is copied from what we originally had. I'll think about wording it better, though.

@myronmarston

myronmarston Dec 16, 2013

Member

FWIW, this is already there. This is copied from what we originally had. I'll think about wording it better, though.

+
+ # Run all specs when none match the provided filter. This works well in
+ # conjunction with `config.filter_run :focus`, as it will run the entire
+ # suite when no specs have `:focus` metadata.

This comment has been minimized.

@xaviershay

xaviershay Dec 15, 2013

Member

This comment is better, maybe expand it to apply to both this setting and the one above?

@xaviershay

xaviershay Dec 15, 2013

Member

This comment is better, maybe expand it to apply to both this setting and the one above?

This comment has been minimized.

@myronmarston

myronmarston Dec 16, 2013

Member

Do you think that these two options should be on consecutive lines with one paragraph to explain both?

@myronmarston

myronmarston Dec 16, 2013

Member

Do you think that these two options should be on consecutive lines with one paragraph to explain both?

This comment has been minimized.

@xaviershay

xaviershay Dec 16, 2013

Member

I think that could make it clearer.

@xaviershay

xaviershay Dec 16, 2013

Member

I think that could make it clearer.

This comment has been minimized.

@JonRowe

JonRowe Dec 16, 2013

Member

How about something like:

# By default filter (ignore) specs that don't have the `:focus => *` metadata
# (or an alias of such), but run all specs when there are no specs matching
# a filter.
config.filter_run :focus
config.run_all_when_everything_filtered = true
@JonRowe

JonRowe Dec 16, 2013

Member

How about something like:

# By default filter (ignore) specs that don't have the `:focus => *` metadata
# (or an alias of such), but run all specs when there are no specs matching
# a filter.
config.filter_run :focus
config.run_all_when_everything_filtered = true
+ # top-level DSL methods like `describe`, `shared_examples_for`, etc.
+ # Instead, you can access these methods off of `RSpec`
+ # (e.g. `RSpec.describe`).
+ config.expose_dsl_globally = false

This comment has been minimized.

@xaviershay

xaviershay Dec 15, 2013

Member

This should probably be true, unless we're going to make a big push to get people to adopt RSpec.describe syntax (which I don't think would be a good idea.)

Probably just leave it out.

@xaviershay

xaviershay Dec 15, 2013

Member

This should probably be true, unless we're going to make a big push to get people to adopt RSpec.describe syntax (which I don't think would be a good idea.)

Probably just leave it out.

This comment has been minimized.

@myronmarston

myronmarston Dec 16, 2013

Member

Why do you think it's a bad idea to encourage the adoption of RSpec.describe? I've been hoping that we could make RSpec 4.0 default to zero monkey patching, and encouraging the use of RSpec.describe is necessary to get there.

That said, it's brand new, so whereas the expect syntax has been there a while and we've gotten very positive feedback on it, the same can't be said for this. So maybe it's something that we should wait to add until we get user feedback about it? OTOH, I want folks to be aware that they can run RSpec w/o monkey patching, and having this here helps raise awareness.

@myronmarston

myronmarston Dec 16, 2013

Member

Why do you think it's a bad idea to encourage the adoption of RSpec.describe? I've been hoping that we could make RSpec 4.0 default to zero monkey patching, and encouraging the use of RSpec.describe is necessary to get there.

That said, it's brand new, so whereas the expect syntax has been there a while and we've gotten very positive feedback on it, the same can't be said for this. So maybe it's something that we should wait to add until we get user feedback about it? OTOH, I want folks to be aware that they can run RSpec w/o monkey patching, and having this here helps raise awareness.

This comment has been minimized.

@xaviershay

xaviershay Dec 16, 2013

Member

my gut tells me that no monkey-patching wouldn't catch on and we'd be pushing up hill.

for most people the monkey patching has zero practical negative affect.

Raising awareness that it is an option is good though.

@xaviershay

xaviershay Dec 16, 2013

Member

my gut tells me that no monkey-patching wouldn't catch on and we'd be pushing up hill.

for most people the monkey patching has zero practical negative affect.

Raising awareness that it is an option is good though.

This comment has been minimized.

@JonRowe

JonRowe Dec 16, 2013

Member

What about adding this, but then commenting the actual option out...

@JonRowe

JonRowe Dec 16, 2013

Member

What about adding this, but then commenting the actual option out...

This comment has been minimized.

@myronmarston

myronmarston Dec 16, 2013

Member

What about adding this, but then commenting the actual option out...

All of the config options here are commented out (via the =begin and =end markers). A while ago in #796 we had a big discussion and came to the conclusion that generally speaking, the only things in spec_helper that should be uncommented-out are things that will become defaults in future rspec releases (e.g. the treat_metadata_keys... option fro 2.x). We use commented out options in the generated spec_helper.rb to indicate settings we recommend.

@myronmarston

myronmarston Dec 16, 2013

Member

What about adding this, but then commenting the actual option out...

All of the config options here are commented out (via the =begin and =end markers). A while ago in #796 we had a big discussion and came to the conclusion that generally speaking, the only things in spec_helper that should be uncommented-out are things that will become defaults in future rspec releases (e.g. the treat_metadata_keys... option fro 2.x). We use commented out options in the generated spec_helper.rb to indicate settings we recommend.

This comment has been minimized.

@xaviershay

xaviershay Dec 17, 2013

Member

Two suggestions that both might be terrible:

  • double comment them
  • s/# config.//
@xaviershay

xaviershay Dec 17, 2013

Member

Two suggestions that both might be terrible:

  • double comment them
  • s/# config.//

This comment has been minimized.

@JonRowe

JonRowe Dec 17, 2013

Member

How about:

=begin
  config.individual_setting
=end
@JonRowe

JonRowe Dec 17, 2013

Member

How about:

=begin
  config.individual_setting
=end

This comment has been minimized.

@myronmarston

myronmarston Dec 17, 2013

Member

Wrapping each individual config option in =begin and =end feels like the worst possible setup :(. I chose =begin/=end specifically to wrap the entire set of config options so that they are all commented out and can easily be uncommented.

How about this:

  • Keep the "wrap everything with =begin/=end" setup we have now, as that makes it easy to accept the suggested settings (and makes the cuke I added easy to write).
  • Remove this setting from the generated spec_helper.rb for now, with the plan to consider adding it back in some 3.x release if it looks like users are adopting it.
  • Update all the docs (including READMEs and cukes) to use RSpec.describe rather than bare describe to make that form more visible to end users. I'll make sure to mention this in the 3.0 release blog post when that time comes.
@myronmarston

myronmarston Dec 17, 2013

Member

Wrapping each individual config option in =begin and =end feels like the worst possible setup :(. I chose =begin/=end specifically to wrap the entire set of config options so that they are all commented out and can easily be uncommented.

How about this:

  • Keep the "wrap everything with =begin/=end" setup we have now, as that makes it easy to accept the suggested settings (and makes the cuke I added easy to write).
  • Remove this setting from the generated spec_helper.rb for now, with the plan to consider adding it back in some 3.x release if it looks like users are adopting it.
  • Update all the docs (including READMEs and cukes) to use RSpec.describe rather than bare describe to make that form more visible to end users. I'll make sure to mention this in the 3.0 release blog post when that time comes.

This comment has been minimized.

@myronmarston

myronmarston Dec 17, 2013

Member

For now I went ahead and removed this.

@myronmarston

myronmarston Dec 17, 2013

Member

For now I went ahead and removed this.

This comment has been minimized.

@soulcutter

soulcutter Dec 20, 2013

Member

The comments could have a space after # and the settings could have no whitespace after the #

@soulcutter

soulcutter Dec 20, 2013

Member

The comments could have a space after # and the settings could have no whitespace after the #

+ Kernel.srand config.seed
+
+ # rspec-mocks config goes here. You can use an alternate test double
+ # library (such as bogus or mocha) by changing the `mock_with` option here.

This comment has been minimized.

@xaviershay

xaviershay Dec 15, 2013

Member

alternate revenue stream: sponsored product placement. :P

@xaviershay

xaviershay Dec 15, 2013

Member

alternate revenue stream: sponsored product placement. :P

This comment has been minimized.

+ # Enable only the newer, non-monkey-patching expect syntax.
+ # For more details, see:
+ # - http://myronmars.to/n/dev-blog/2012/06/rspecs-new-expectation-syntax
+ # - http://teaisaweso.me/blog/2013/05/27/rspecs-new-message-expectation-syntax/

This comment has been minimized.

@xaviershay

xaviershay Dec 15, 2013

Member

It'd be nice to link to an official RSpec page here (which initially just links to these two posts).

Or maybe just https://relishapp.com/rspec/rspec-mocks/v/3-0/docs/message-expectations/expect-message-using-expect ?

I thought this was enabled by default anyways?

@xaviershay

xaviershay Dec 15, 2013

Member

It'd be nice to link to an official RSpec page here (which initially just links to these two posts).

Or maybe just https://relishapp.com/rspec/rspec-mocks/v/3-0/docs/message-expectations/expect-message-using-expect ?

I thought this was enabled by default anyways?

This comment has been minimized.

@myronmarston

myronmarston Dec 16, 2013

Member

Yeah, once we get a new rspec.info site up we can update these. For now I'm just going to link to the resources we have.

The relish page shows usage, but doesn't explain any of the rationale or background, which I think is important.

I thought this was enabled by default anyways?

expect is enabled by default. But so is the old should syntax. The option here is disabling the :should syntax by not including it in what's enabled.

@myronmarston

myronmarston Dec 16, 2013

Member

Yeah, once we get a new rspec.info site up we can update these. For now I'm just going to link to the resources we have.

The relish page shows usage, but doesn't explain any of the rationale or background, which I think is important.

I thought this was enabled by default anyways?

expect is enabled by default. But so is the old should syntax. The option here is disabling the :should syntax by not including it in what's enabled.

This comment has been minimized.

@JonRowe

JonRowe Dec 16, 2013

Member

馃憤 for turning off should in these generated helpers

@JonRowe

JonRowe Dec 16, 2013

Member

馃憤 for turning off should in these generated helpers

This comment has been minimized.

@myronmarston

myronmarston Dec 16, 2013

Member

馃憤 for turning off should in these generated helpers

Bear in mind that these settings are commented out, so the user still has to choose to disable :should by uncommenting these settings.

@myronmarston

myronmarston Dec 16, 2013

Member

馃憤 for turning off should in these generated helpers

Bear in mind that these settings are commented out, so the user still has to choose to disable :should by uncommenting these settings.

This comment has been minimized.

@JonRowe

JonRowe Dec 16, 2013

Member

Yes but it helps encourage them :)

@JonRowe

JonRowe Dec 16, 2013

Member

Yes but it helps encourage them :)

+ # Enable only the newer, non-monkey-patching expect syntax.
+ # For more details, see:
+ # - http://myronmars.to/n/dev-blog/2012/06/rspecs-new-expectation-syntax
+ expectations.syntax = :expect

This comment has been minimized.

@xaviershay

xaviershay Dec 15, 2013

Member

ditto

This comment has been minimized.

@myronmarston

myronmarston Dec 16, 2013

Member

ditto

@xaviershay

This comment has been minimized.

Show comment
Hide comment
@xaviershay

xaviershay Dec 15, 2013

Member

Fantastic change overall, left some specific opinions on individual settings.

Member

xaviershay commented Dec 15, 2013

Fantastic change overall, left some specific opinions on individual settings.

@myronmarston

This comment has been minimized.

Show comment
Hide comment
@myronmarston

myronmarston Dec 16, 2013

Member

BTW, @xaviershay, thanks for the detailed feedback.

Member

myronmarston commented Dec 16, 2013

BTW, @xaviershay, thanks for the detailed feedback.

Address feedback.
- Add missing back ticks.
- Combine `filter_run`/`run_all_when_everything_filtered`
  configs and explanations.
- Remove the `expose_dsl_globally = false` config for now,
  with the plan to potentially add it back in a later
  release if user feedback about it is positive.
- Re-order things a bit.
@myronmarston

This comment has been minimized.

Show comment
Hide comment
@myronmarston

myronmarston Dec 22, 2013

Member

I'd love to merge this soon, but given that the recommendations that show up in the generated spec_helper.rb go directly against what rspec-rails generates, it would be good to have a definitive plan to resolve that (even if we don't implement it yet). @alindeman -- can you weigh in, please?

Member

myronmarston commented Dec 22, 2013

I'd love to merge this soon, but given that the recommendations that show up in the generated spec_helper.rb go directly against what rspec-rails generates, it would be good to have a definitive plan to resolve that (even if we don't implement it yet). @alindeman -- can you weigh in, please?

@JonRowe

This comment has been minimized.

Show comment
Hide comment
@JonRowe

JonRowe Dec 23, 2013

Member

rspec-rails is great for newbies getting into rspec and or rails. Personally I'd advocate against using it if you're a seasoned Ruby dev, it's only ever added pain for me. Note that this is isn't RSpec's fault, but Rails fault. As such I'm ok with it being slightly at odds with pure rspec.

I've always seen rspec-rails as being "rspec the rails way"

Member

JonRowe commented Dec 23, 2013

rspec-rails is great for newbies getting into rspec and or rails. Personally I'd advocate against using it if you're a seasoned Ruby dev, it's only ever added pain for me. Note that this is isn't RSpec's fault, but Rails fault. As such I'm ok with it being slightly at odds with pure rspec.

I've always seen rspec-rails as being "rspec the rails way"

@alindeman

This comment has been minimized.

Show comment
Hide comment
@alindeman

alindeman Dec 23, 2013

Contributor

I'd love to merge this soon, but given that the recommendations that show up in the generated spec_helper.rb go directly against what rspec-rails generates, it would be good to have a definitive plan to resolve that (even if we don't implement it yet). @alindeman -- can you weigh in, please?

Is there anything in this improved spec_helper.rb that will cause issues in -rails by default? If not, I'm 馃憤 to synchronize them.

Contributor

alindeman commented Dec 23, 2013

I'd love to merge this soon, but given that the recommendations that show up in the generated spec_helper.rb go directly against what rspec-rails generates, it would be good to have a definitive plan to resolve that (even if we don't implement it yet). @alindeman -- can you weigh in, please?

Is there anything in this improved spec_helper.rb that will cause issues in -rails by default? If not, I'm 馃憤 to synchronize them.

@alindeman

This comment has been minimized.

Show comment
Hide comment
@alindeman

alindeman Dec 23, 2013

Contributor

I've always seen rspec-rails as being "rspec the rails way"

I definitely agree with this. I think RSpec has the potential to be offputting to new Rails users if there is a significant barrier to entry, even if it's an objectively superior way of writing tests.

That said, compromises like a rails_spec_helper.rb that default to the slow, coupled, though "rails" way, alongside another file that allows you to iterate toward a faster, less coupled, "modular" way is really appealing to me.

馃憤 to a spec_helper.rb that matches here (can we just invoke it directly in rspec-rails?), though with a rails_spec_helper.rb that has more rails-y defaults.

Contributor

alindeman commented Dec 23, 2013

I've always seen rspec-rails as being "rspec the rails way"

I definitely agree with this. I think RSpec has the potential to be offputting to new Rails users if there is a significant barrier to entry, even if it's an objectively superior way of writing tests.

That said, compromises like a rails_spec_helper.rb that default to the slow, coupled, though "rails" way, alongside another file that allows you to iterate toward a faster, less coupled, "modular" way is really appealing to me.

馃憤 to a spec_helper.rb that matches here (can we just invoke it directly in rspec-rails?), though with a rails_spec_helper.rb that has more rails-y defaults.

@myronmarston

This comment has been minimized.

Show comment
Hide comment
@myronmarston

myronmarston Dec 23, 2013

Member

Is there anything in this improved spec_helper.rb that will cause issues in -rails by default? If not, I'm 馃憤 to synchronize them.

Not that I can think of. Note that all the settings in this file are commented out by default since they are all just suggestions. The defaults of disabling the :should syntax for rspec-mocks and rspec-expectations could potentially cause issues if anything in rspec-rails assumes the :should syntax is available....that's the one possible issue I can think of.

Member

myronmarston commented Dec 23, 2013

Is there anything in this improved spec_helper.rb that will cause issues in -rails by default? If not, I'm 馃憤 to synchronize them.

Not that I can think of. Note that all the settings in this file are commented out by default since they are all just suggestions. The defaults of disabling the :should syntax for rspec-mocks and rspec-expectations could potentially cause issues if anything in rspec-rails assumes the :should syntax is available....that's the one possible issue I can think of.

@myronmarston

This comment has been minimized.

Show comment
Hide comment
@myronmarston

myronmarston Dec 23, 2013

Member

I definitely agree with this. I think RSpec has the potential to be offputting to new Rails users if there is a significant barrier to entry, even if it's an objectively superior way of writing tests.

That said, compromises like a rails_spec_helper.rb that default to the slow, coupled, though "rails" way, alongside another file that allows you to iterate toward a faster, less coupled, "modular" way is really appealing to me.

Right...using require 'rails_spec_helper' rather than require 'spec_helper' in your spec files doesn't seem like much of a barrier to entry.

馃憤 to a spec_helper.rb that matches here (can we just invoke it directly in rspec-rails?), though with a rails_spec_helper.rb that has more rails-y defaults.

I don't know much about the rails generators. If you can run arbitrary code during a rails generator you can have it run RSpec::Core::ProjectInitializer.new.run. Alternately, if you want it to emit your own output to match the typical generator output, you can just use the dot_rspec and spec_helper files that this PR adds, and write them to disk from your generator w/ whatever output you need. Or we can improve ProjectInitializer to make it usable by rspec-rails if there is a simple change we can do to help. Let us know what you need.

Anyhow, I'm going to merge since we all seem to be on the same page about this :).

Member

myronmarston commented Dec 23, 2013

I definitely agree with this. I think RSpec has the potential to be offputting to new Rails users if there is a significant barrier to entry, even if it's an objectively superior way of writing tests.

That said, compromises like a rails_spec_helper.rb that default to the slow, coupled, though "rails" way, alongside another file that allows you to iterate toward a faster, less coupled, "modular" way is really appealing to me.

Right...using require 'rails_spec_helper' rather than require 'spec_helper' in your spec files doesn't seem like much of a barrier to entry.

馃憤 to a spec_helper.rb that matches here (can we just invoke it directly in rspec-rails?), though with a rails_spec_helper.rb that has more rails-y defaults.

I don't know much about the rails generators. If you can run arbitrary code during a rails generator you can have it run RSpec::Core::ProjectInitializer.new.run. Alternately, if you want it to emit your own output to match the typical generator output, you can just use the dot_rspec and spec_helper files that this PR adds, and write them to disk from your generator w/ whatever output you need. Or we can improve ProjectInitializer to make it usable by rspec-rails if there is a simple change we can do to help. Let us know what you need.

Anyhow, I'm going to merge since we all seem to be on the same page about this :).

myronmarston added a commit that referenced this pull request Dec 23, 2013

@myronmarston myronmarston merged commit 98c6ad4 into master Dec 23, 2013

1 check passed

default The Travis CI build passed
Details

@myronmarston myronmarston deleted the rspec-init branch Dec 23, 2013

@alindeman

This comment has been minimized.

Show comment
Hide comment
@alindeman

alindeman Dec 23, 2013

Contributor

Or we can improve ProjectInitializer to make it usable by rspec-rails if there is a simple change we can do to help. Let us know what you need.

Got it! I've added it to the list鈩 and will try to get to it this week.

Contributor

alindeman commented Dec 23, 2013

Or we can improve ProjectInitializer to make it usable by rspec-rails if there is a simple change we can do to help. Let us know what you need.

Got it! I've added it to the list鈩 and will try to get to it this week.

@slowjack2k

This comment has been minimized.

Show comment
Hide comment
@slowjack2k

slowjack2k Sep 24, 2015

Hi,

I have a question to those 2 helpers.

Say I have a spec a_spec.rb which requires 'rails_spec_helper'
and a spec b_spec.rb which only requires 'spec_helper' .

When I run

rspec a_spec.rb b_spec.rb

will rspec clean every thing up or will b_spec.rb run with every thing loaded?

Further more when I configure additional settings, for instance

within rails_spec_helper

config.use_transactional_fixtures = true

within spec_helper

config.use_transactional_fixtures = false

will they mess up?

regards
dieter

Hi,

I have a question to those 2 helpers.

Say I have a spec a_spec.rb which requires 'rails_spec_helper'
and a spec b_spec.rb which only requires 'spec_helper' .

When I run

rspec a_spec.rb b_spec.rb

will rspec clean every thing up or will b_spec.rb run with every thing loaded?

Further more when I configure additional settings, for instance

within rails_spec_helper

config.use_transactional_fixtures = true

within spec_helper

config.use_transactional_fixtures = false

will they mess up?

regards
dieter

@myronmarston

This comment has been minimized.

Show comment
Hide comment
@myronmarston

myronmarston Sep 24, 2015

Member

will rspec clean every thing up or will b_spec.rb run with every thing loaded?

I'm not sure what you mean by "will rspec clean every thing up" but ruby does not provide a way to "unrequire" a file and it would be confusing/surprising for RSpec to do that to a file you have required -- so for the command you provided, the specs will run with both helper files loaded (assuming you are requiring the helper files at file load time rather than within an individual spec).

will they mess up?

Yes. config.use_transactional_fixtures is global state. If you set it to one value, then change it to a new value, it'll be set to the new value. I wouldn't recommend setting it in different files.

Besides, use_transaction_fixtures is not an rspec-core config option -- it's one added by rspec-rails to support rails testing and as such, it doesn't really make sense to set it in spec_helper.rb as spec_helper.rb is intended for "base" RSpec options and is meant to be usable w/o rails or rspec-rails (but also with rails and rspec-rails).

Member

myronmarston commented Sep 24, 2015

will rspec clean every thing up or will b_spec.rb run with every thing loaded?

I'm not sure what you mean by "will rspec clean every thing up" but ruby does not provide a way to "unrequire" a file and it would be confusing/surprising for RSpec to do that to a file you have required -- so for the command you provided, the specs will run with both helper files loaded (assuming you are requiring the helper files at file load time rather than within an individual spec).

will they mess up?

Yes. config.use_transactional_fixtures is global state. If you set it to one value, then change it to a new value, it'll be set to the new value. I wouldn't recommend setting it in different files.

Besides, use_transaction_fixtures is not an rspec-core config option -- it's one added by rspec-rails to support rails testing and as such, it doesn't really make sense to set it in spec_helper.rb as spec_helper.rb is intended for "base" RSpec options and is meant to be usable w/o rails or rspec-rails (but also with rails and rspec-rails).

@slowjack2k

This comment has been minimized.

Show comment
Hide comment
@slowjack2k

slowjack2k Sep 24, 2015

Thanks for the explanation @myronmarston . It answers my question. With 'clean every thing up' I meant reset Rspec config and unload every thing accordingly. Until now I thought rspec will fork new processes or call system or some thing else. So that every spec is run in a clean environment.

config.use_transactional_fixtures was only an example but I should have been more accurate
and call the 2 helpers unit_spec_helper.rb and acceptance_spec_helper.rb.

So when I set

# acceptance_spec_helper.rb
RSpec.configure do |config|
config.before(:suite) do
    DatabaseCleaner.strategy = :truncation
  end
# unit_spec_helper.rb
RSpec.configure do |config|
config.before(:suite) do
    DatabaseCleaner.strategy = :transaction
    DatabaseCleaner.clean_with(:truncation)
  end

It will screw depending on the execution order of my specs.

So whats the best way to deal with such different rspec configurations?

Thanks for the explanation @myronmarston . It answers my question. With 'clean every thing up' I meant reset Rspec config and unload every thing accordingly. Until now I thought rspec will fork new processes or call system or some thing else. So that every spec is run in a clean environment.

config.use_transactional_fixtures was only an example but I should have been more accurate
and call the 2 helpers unit_spec_helper.rb and acceptance_spec_helper.rb.

So when I set

# acceptance_spec_helper.rb
RSpec.configure do |config|
config.before(:suite) do
    DatabaseCleaner.strategy = :truncation
  end
# unit_spec_helper.rb
RSpec.configure do |config|
config.before(:suite) do
    DatabaseCleaner.strategy = :transaction
    DatabaseCleaner.clean_with(:truncation)
  end

It will screw depending on the execution order of my specs.

So whats the best way to deal with such different rspec configurations?

@myronmarston

This comment has been minimized.

Show comment
Hide comment
@myronmarston

myronmarston Sep 25, 2015

Member

I'm not totally sure how you want your configuration to behave, but my best guess at what you want is this:

# unit_spec_helper.rb
RSpec.configure do |config|
  config.before(:suite) do
    DatabaseCleaner.strategy = :transaction
    DatabaseCleaner.clean_with(:truncation)
  end
end
# acceptance_spec_helper.rb
RSpec.configure do |config|
  original_strategy = nil

  config.before(:context, :file_path => /spec\/acceptance/) do
    original_strategy = DatabaseCleaner.strategy
    DatabaseCleaner.strategy = :truncation
  end

  config.after(:context, :file_path => /spec\/acceptance/) do
    DatabaseCleaner.strategy = original_strategy
  end
end

This sets the default global strategy to :transaction but then in acceptance_spec_helper.rb sets things up so that before specs in spec/acceptance get run, the strategy is temporarily changed to :truncation and then changed back afterwards.

Is that the behavior you're trying to achieve?

Member

myronmarston commented Sep 25, 2015

I'm not totally sure how you want your configuration to behave, but my best guess at what you want is this:

# unit_spec_helper.rb
RSpec.configure do |config|
  config.before(:suite) do
    DatabaseCleaner.strategy = :transaction
    DatabaseCleaner.clean_with(:truncation)
  end
end
# acceptance_spec_helper.rb
RSpec.configure do |config|
  original_strategy = nil

  config.before(:context, :file_path => /spec\/acceptance/) do
    original_strategy = DatabaseCleaner.strategy
    DatabaseCleaner.strategy = :truncation
  end

  config.after(:context, :file_path => /spec\/acceptance/) do
    DatabaseCleaner.strategy = original_strategy
  end
end

This sets the default global strategy to :transaction but then in acceptance_spec_helper.rb sets things up so that before specs in spec/acceptance get run, the strategy is temporarily changed to :truncation and then changed back afterwards.

Is that the behavior you're trying to achieve?

@slowjack2k

This comment has been minimized.

Show comment
Hide comment
@slowjack2k

slowjack2k Sep 25, 2015

@myronmarston I think your solution is the nearest as I come to my goal and I appreciate your help. To explain it a little furthor my folder layout:

  • spec/
    • support
    • acceptance
    • support
    • integration
    • support
    • unit
      • support
    • spec_helper.rb
    • rails_helper.rb
    • acceptance_spec_helper.rb
    • integration_spec_helper.rb
    • unit_spec_helper.rb

Unit Specs are without external deps (as far as you can achieve it within rails), so they require only spec_helper.rb & global support & unit support folder.

Integration specs are with external deps, so they require only rails_helper.rb & global support & integration support folder. They need the DB-Cleaner settings.

Acceptance are end to end tests so they require only rails_helper.rb & global support & acceptance support folder. Because these tests use for instance capybara so I can't use transactional fixtures. But furthor more I need some more around filters for instance:

 config.after(:each) do
    wait_for_ajax if respond_to? :wait_for_ajax
  end

But this is only a starting point. The configurations will differ more and more over time I think.

When I get you right I have to ensure that I use config.before(:context, :file_path => /spec\/acceptance/)
and must ensure that my support methods, modules, classes don't interfer each other.

Edit: Can't rspec fork before a spec file is loaded and executed? This would solve my issues ;).

Edit2: I tried your approach but it didn't work. config.use_transactional_fixtures can't be changed within a before hook and DatabaseCleaner has no getter for strategy.

@myronmarston I think your solution is the nearest as I come to my goal and I appreciate your help. To explain it a little furthor my folder layout:

  • spec/
    • support
    • acceptance
    • support
    • integration
    • support
    • unit
      • support
    • spec_helper.rb
    • rails_helper.rb
    • acceptance_spec_helper.rb
    • integration_spec_helper.rb
    • unit_spec_helper.rb

Unit Specs are without external deps (as far as you can achieve it within rails), so they require only spec_helper.rb & global support & unit support folder.

Integration specs are with external deps, so they require only rails_helper.rb & global support & integration support folder. They need the DB-Cleaner settings.

Acceptance are end to end tests so they require only rails_helper.rb & global support & acceptance support folder. Because these tests use for instance capybara so I can't use transactional fixtures. But furthor more I need some more around filters for instance:

 config.after(:each) do
    wait_for_ajax if respond_to? :wait_for_ajax
  end

But this is only a starting point. The configurations will differ more and more over time I think.

When I get you right I have to ensure that I use config.before(:context, :file_path => /spec\/acceptance/)
and must ensure that my support methods, modules, classes don't interfer each other.

Edit: Can't rspec fork before a spec file is loaded and executed? This would solve my issues ;).

Edit2: I tried your approach but it didn't work. config.use_transactional_fixtures can't be changed within a before hook and DatabaseCleaner has no getter for strategy.

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