Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Default spec_helper configuration generated by 'rspec --init' is non-intuitive #796

Closed
wants to merge 2 commits into from
@jasonkarns

The default rspec configuration block generated by rspec --init in /spec/spec_helper.rb is currently (with comments removed):

RSpec.configure do |config|
  config.treat_symbols_as_metadata_keys_with_true_values = true
  config.run_all_when_everything_filtered = true
  config.filter_run :focus

  config.order = 'random'
end

I take issue with the defaults for two of these options: run_all_when_everything_filtered and filter_run.

Consider the scenario of someone fairly new to rspec, who runs rspec --init and begins writing their specs. As the default configuration gets them up and going, they do not modify the defaults. Everything works. They write hundreds or thousands of specs over the course of the next few months. Then, one day, they learn about tags and begin to use them to selectively run specs. Until they run the specs with a typo'd tag and the entire suite runs. WAT? No idea why that happened, but oh well. The suite still works. They continue on and keep adding various tags. Then they add a :focus tag. (Perhaps it means something special in their domain.) Now when they run rspec without arguments, only the :focus specs run. That's a serious WAT!?

Two instances where rspec takes one completely by surprise, solely because of the defaults chosen. In general, we should strive for the Principle of Least Surprise, especially with regards to default options.

I understand the utility of these two options in combination. It's great to be able to simply add :focus as a WIP tag while writing specs. However, I think they should be off by default (or simply left commented-out in the generated spec_helper).

@jasonkarns jasonkarns Make the generated spec_helper less surprising
Having the two options 'run_all_when_everything_filtered' and
'filter_run' default to true and :focus (respecively) does not follow
the principle of least surprise. It is a more suitable default that
these options remain unset.

Fixes #796
45bfe2b
@soulcutter
Collaborator

I like this default -- anybody who writes hundreds or thousands of specs is going to have looked at spec_helper.rb once or twice. I don't find this behavior particularly surprising.

@jasonkarns

It's my opinion that default settings should be set with beginners in mind. Experienced users know what settings they actually want from past experience and can configure to their own taste. That's why I think leaving the settings in there but commented out is a good compromise. Experienced users can simply uncomment them. Beginners, on the other hand, having no idea what those settings mean, would be left with a clean slate that doesn't presuppose advanced usage.

The scenario I outlined was not hypothetical. I'm not a beginner with rspec but this was my first time ever using rspec --init to scaffold a new project. I actually did go months without investigating the settings in question (even though I was routinely in the spec_helper). And I actually did need a :focus tag that I used in a domain context, unrelated to its usage per this setting.

@dchelimsky
Owner

Another way to look at this is that :focus is a feature of rspec that happens to be implemented using a more general feature of rspec (metadata). Rather than not making it a default, I'd advocate making it more visible and helping (programmatically) users to understand the scenario you ran into.

When you run rspec it prints out:

Run options:
  include {:focus=>true}

What else can we do to make the meaning of this more obvious?

@myronmarston

Until they run the specs with a typo'd tag and the entire suite runs. WAT? No idea why that happened, but oh well. The suite still works.

I don't see why this is confusing. RSpec prints out a very clear message explaining the tags are being ignored:

All examples were filtered out; ignoring {:focus=>true, :abc=>true}

Now when they run rspec without arguments, only the :focus specs run. That's a serious WAT!?

Again, I'm not convinced this is particularly confusing. :focus is a pretty clear term for what it does, I think, and the output (Run options: include {:focus=>true}) explains it. The user is a very quick ack or grep away from finding it in the spec helper file.

However, I think they should be off by default

I agree; that's why they are off by default. They have to be explicitly listed in spec_helper or some such file. The generated spec_helper.rb file contains recommended settings, and these are recommended settings that most rspec users find useful.

We can't please everyone with every decision, and my instinct here is that you've got a preference for one way but many more people have the a preference for the way it is now. I'll consider changing it if enough other rspec users vote for it to be changed, though.

@sferik

IMHO, the defaults should be the defaults. rspec --init should not generate an RSpec.configure block. RSpec.configure should only be necessary for overriding the recommended configuration.

For example, if order = 'random' is a best practice, it should be the default ordering. If you want to run your specs in file system order, that should require configuration.

@samphippen
Collaborator

I'm weakly against this change.

I think the way it works now is surprisingly effective: when we filter down to empty the two behaviours that make most sense are to run everything or run nothing. Running everything might show the user that they've broken something else. We're also pretty loud about telling the user that we're going to run everything because we filter down to nothing.

Regarding the focus tagging I'm not sure of the exact behaviour when you run rspec with no arguments and specs tagged with focus. IMO the stuff where it prints :focus is probably good enough for now.

@andrewpthorp

I completely agree with @sferik on this one. Configure blocks should be used for overriding default behaviour. The community should decide on what 'default' means, and clearly document how to override these settings.

@samphippen
Collaborator

Regarding that I also find this compelling. My thoughts are we should generate a configure block with all the stuff inside it commented out. Thoughts?

@JoshTGreenwood

I agree with @samphippen. Set the defaults that most beginning users will find useful and comment out common overrides. It seems like a win-win to me.

@myronmarston

@sferik -- we had a long discussion about making --random the default in #635. I think it's a good setting for virtually any project, but as pointed out in that thread, it also has the potential to be very confusing for folks that are trying ruby/rspec/TDD for the first time, and it's easier for folks who know RSpec well and want this setting to turn it on than it is for newbies who are just getting their feet wet to turn it off.

In theory, I agree that all the defaults should be the defaults, without a generated spec_helper.rb providing further defaults. However, there's the issue of SemVer: if there's a new config setting that we think most users will want (and that would have been the default if the setting had been around at the last major release), but would break existing spec suites, we can't simply make it the default, as it would violate SemVer. The best we can do is add it to the generated spec_helper.rb so that it becomes the default for new projects, and eases the transition if/when we decide to make it a true default in the future.

On top of that, there are some common settings (such as using the :focus tag to filter examples) that are essentially arbitrarily chosen names (it could be :focused or :foc or :f instead...). I would never want to make this kind of setting a true default (since the tag name is more or less arbitrary), and making it explicit in RSpec.configure is a good thing, IMO. There's also value to the community in encouraging a set of conventions around these settings (so that it's easier when folks contribute to a project that uses these settings as they may already be familiar with these conventions), and having these settings generated in spec_helper.rb by rspec --init helps here.

@sferik

@samphippen:

Regarding that I also find this compelling. My thoughts are we should generate a configure block with all the stuff inside it commented out. Thoughts?

This is will lead to something like the default config/routes.rb file in Rails where 47 of the 49 lines are comments. The first thing I do whenever I create a new Rails app is delete those 47 lines.

There happens to be a fantastic guide to Rails routes. IMHO, the default routes file should contain a single comment with a link to that guide.

There also happens to be a fantastic guide to RSpec configuration. Let's just link to that, please.

If you think offline access is important, it shouldn’t be that hard to generate an offline equivalent. This problem has already been solved for rdoc. I don't know if it's already supported by relish but we could certainly request that feature. /cc @mattwynne @justinko.

@sferik

@myronmarston:

@sferik -- we had a long discussion about making --random the default in #635. I think it's a good setting for virtually any project, but as pointed out in that thread, it also has the potential to be very confusing for folks that are trying ruby/rspec/TDD for the first time, and it's easier for folks who know RSpec well and want this setting to turn it on than it is for newbies who are just getting their feet wet to turn it off.

If it's not a good default then it shouldn't be generated by rspec --init. I'm not really arguing for you to change the defaults. That was just an example. I'm arguing for consistency.

In theory, I agree that all the defaults should be the defaults, without a generated spec_helper.rb providing further defaults. However, there's the issue of SemVer: if there's a new config setting that we think most users will want (and that would have been the default if the setting had been around at the last major release), but would break existing spec suites, we can't simply make it the default, as it would violate SemVer. The best we can do is add it to the generated spec_helper.rb so that it becomes the default for new projects, and eases the transition if/when we decide to make it a true default in the future.

If changing the defaults requires a major version bump, so be it. RSpec 2 was released in 2010. There have been 13 minor releases since then. I think it’s about time for a major release that requires less configuration.

On top of that, there are some common settings (such as using the :focus tag to filter examples) that are essentially arbitrarily chosen names (it could be :focused or :foc or :f instead...). I would never want to make this kind of setting a true default (since the tag name is more or less arbitrary), and making it explicit in RSpec.configure is a good thing, IMO. There's also value to the community in encouraging a set of conventions around these settings (so that it's easier when folks contribute to a project that uses these settings as they may already be familiar with these conventions), and having these settings generated in spec_helper.rb by rspec --init helps here.

See my previous comment about linking to documentation. Without documentation, users probably won't understand what these non-default default settings mean, anyway. This leads to the type of confusion that caused this issue to be opened in the first place. It's better to provide canonical examples in the documentation. Furthermore, many users prefer not to use the features/workflows (e.g. filters), so these configuration lines are just noise, to be deleted.

@samphippen
Collaborator

@sferik I'm not sure if that's necessary for RSpec. Most people don't look inside their spec configuration every day. People do look at their routes every day. Having examples inlined in the configuration file would also server as some pretty nice documentation. Going to a webpage is great and all, but there's nothing that makes me happier than when useful documentation sits right next to the relevant piece of code.

@sferik

@samphippen:

@sferik I'm not sure if that's necessary for RSpec. Most people don't look inside their spec configuration every day. People do look at their routes every day.

Personally, I look at my spec/spec_helper.rb about as frequently as I look at config/routes.rb in a Rails app (especially in more mature Rails apps).

Maybe I'm alone, but I don't use my spec/spec_helper.rb solely for RSpec configuration. I also use it to configure test-related libraries such as SimpleCov, VCR, WebMock, and Timecop, as well as a place to define helper methods that are used across multiple specs. The less crap RSpec adds to this file by default, the better.

Having examples inlined in the configuration file would also server as some pretty nice documentation. Going to a webpage is great and all, but there's nothing that makes me happier than when useful documentation sits right next to the relevant piece of code.

Providing non-default default example code is not the same thing as providing good documentation. In many cases, it can be confusing because there's a tension between keeping it short and sweet (so not to muck up your spec/spec_helper.rb) and making it clear and descriptive. With documentation on the web, there is no such tension.

@cupakromer
Collaborator

I'm with @myronmarston on this. RSpec is an opinionated testing suite. However, due to SemVer, changing the behavior of how things currently work on a dot release would violate the principle of least surprise. Since the drive is for these settings to be the defaults, new project should use them. Enforcing them with a custom init setup makes this clear.

Perhaps, a small comment explaining this forward direction, with a link to the documentation is warranted in the generated file.

@samphippen
Collaborator

I agree 100% with everything @cupakromer said.

@jasonkarns

The discussion in the thread is slowly softening my position that these settings should be commented out in the generated spec_helper. However, @myronmarston's comment is really the core of my feeling that these particular options should be generated commented out.

there are some common settings (such as using the :focus tag to filter examples) that are essentially arbitrarily chosen names (it could be :focused or :foc or :f instead...).

Settings that provide clear, measurable benefit (order=random being one) that will likely become actual defaults but are currently not (due to SemVer) should be generated as uncommented in spec_helper.

However, settings that are very subjective to user taste and personal preference (or personal development flow) and are admittedly arbitrary (filter_run and it's tightly coupled companion run_all_when_everything_filtered) should be generated commented-out.

If the generated spec_helper is to be treated somewhat like a transitional phase wherein new options are called out and eventually become default options in a major release, then options that are quite clearly subjective and personal (which would never become actual defaults) should remain commented out in the generated file.

This draws a nice distinction between would-be/will-be defaults and 'recommended practice'.

@sorentwo

There seem to be four differing opinions on how to proceed. Listed without any attribution:

  1. Leave the config as is
  2. Put explanatory comments above the filtering config settings and leave them in commented.
  3. Comment the lines out, but with explanatory comments.
  4. Remove all comments save the documentation link at the top (which may be better served as a relish link than rdoc)

I think option 2 is the least dramatic change, and also softens confusion over spec meaning the most. @jasonkarns , if I (or anybody else) provided a patch documenting the config would that suit you?

@myronmarston

If the generated spec_helper is to be treated somewhat like a transitional phase wherein new options are called out and eventually become default options in a major release, then options that are quite clearly subjective and personal (which would never become actual defaults) should remain commented out in the generated file.

This draws a nice distinction between would-be/will-be defaults and 'recommended practice'.

Circling back around on this...I like this. In addition, I think it would be great to add more explanatory comments to the recommended settings in the commented-out section.

Anyone want to take a stab at it?

@sorentwo

@myronmarston I'll have a go at it, I essentially volunteered for just such a task earlier.

@jasonkarns

I've added comments to the spec_helper (comments taken from @sorentwo's PR #877). However, I don't think we want to comment out the order=random option because this will eventually become a default, no?

I also left treat_symbols_as_metadata_keys_with_true_values=true uncommented because this also seems like a likely future default. If I understand correctly, I think run_all_when_everything_filtered and filter_run are the only options that are not likely to become defaults.

@JonRowe
Owner

I'm in favour of the idea of generating a config file that explains the options and shows the defaults commented out, similar to how Rails does it, (even if the first thing I tend to do one a new project is delete all those comments ;) ) but we shouldn't remove the current defaults until we're working on RSpec 3.

@parasquid parasquid referenced this pull request from a commit in parasquid/namecheap
@dmyers dmyers Renamed spec helper file 870f680
@myronmarston
Owner

Closed by #877.

This was referenced
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Feb 19, 2013
  1. @jasonkarns

    Make the generated spec_helper less surprising

    jasonkarns authored
    Having the two options 'run_all_when_everything_filtered' and
    'filter_run' default to true and :focus (respecively) does not follow
    the principle of least surprise. It is a more suitable default that
    these options remain unset.
    
    Fixes #796
Commits on Apr 18, 2013
  1. @jasonkarns
This page is out of date. Refresh to see the latest.
View
21 lib/rspec/core/project_initializer.rb
@@ -48,9 +48,26 @@ def create_spec_helper_file
#
# See http://rubydoc.info/gems/rspec-core/RSpec/Core/Configuration
RSpec.configure do |config|
+ # Specify metadata using just symbols. Each symbol used will effectively be
+ # treated as a `true` value, allowing easier filtering when running specs.
+ # it 'can be filtered using simple metadata', :fast, :simple do
+ # end
+ #
+ # rspec -t fast
+ #
config.treat_symbols_as_metadata_keys_with_true_values = true
- config.run_all_when_everything_filtered = true
- config.filter_run :focus
+
+ # 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 `:filter` metadata.
+ #
+ # config.run_all_when_everything_filtered = true
+
+ # 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.
+ #
+ # config.filter_run :focus
# Run specs in random order to surface order dependencies. If you find an
# order dependency and want to debug it, you can fix the order by providing
View
10 spec/rspec/core/project_initializer_spec.rb
@@ -49,6 +49,16 @@ module RSpec::Core
command_line_config.run
expect(File.read('spec/spec_helper.rb')).to match /RSpec\.configure do \|config\|/m
end
+
+ it "leaves 'run_all_when_everything_filtered' commented out" do
+ command_line_config.run
+ expect(File.read('spec/spec_helper.rb')).to match /^\s\s# config\.run_all_when_everything_filtered = true/m
+ end
+
+ it "leaves 'filter_run' commented out" do
+ command_line_config.run
+ expect(File.read('spec/spec_helper.rb')).to match /^\s\s# config\.filter_run :focus/m
+ end
end
context "with a spec/spec_helper.rb file" do
Something went wrong with that request. Please try again.