set rspec type to :feature instead #809

Merged
merged 1 commit into from Sep 9, 2012

Conversation

Projects
None yet
6 participants
Collaborator

jnicklas commented Sep 7, 2012

Hey @josevalim, @dchelimsky, @sobrinho and anyone else who has had an opinion on this. When applied, this commit changes Capybara to use :feature as the type for its own feature helper, as well as only including Capybara in those steps with type :feature. This follows José's suggestion. I feel that this is a very sensible approach.

Though there is obviously a big issue: It breaks compatibility with rspec, especially with rspec-rails.

I'd like to get this into Capybara 2.0 so that we don't have to break the API post release. Can you guys let me know what you think? Should I just release this and hope for the other tools to catch up as soon as possible?

Collaborator

jnicklas commented Sep 7, 2012

Just a note that I change the type to :feature, instead of :features (plural) as suggested by José. It seems like that follows the naming convention better, right?

Contributor

josevalim commented Sep 7, 2012

@jnicklas The naming convention is a bit tricky. We have spec/models, spec/views, spec/helpers, so I would vote for spec/features. But I will leave the final decision for @dchelimsky.

I think adding support for these in rspec-rails will be trivial as well. So rspec-rails should catch up quickly. My $.02. :)

Contributor

sobrinho commented Sep 7, 2012

I think that's a great move and won't be a big deal for other tools.

This will make the suite much more concise if we follow the convention of spec/features, using :feature type, for capybara specs and spec/acceptance, using :acceptance type, for turnip specs.

You didn't said anything about the latest one.

Are you planning to change this on turnip either?

Contributor

dchelimsky commented Sep 7, 2012

@josevalim @jnicklas if capybara attaches itself to feature (or features) what would rspec-rails even need to do?

Contributor

josevalim commented Sep 7, 2012

@dchelimsky I think we need to attach the basic rspec-rails stuff there, like:

https://github.com/rspec/rspec-rails/blob/master/lib/rspec/rails/example/rails_example_group.rb

Or this is attached by default to all example groups? If so, I guess rspec-rails doesn't have to do anything.

Contributor

dchelimsky commented Sep 7, 2012

@josevalim not automatic for all groups, so that would need a fix in rspec-rails - need to coordinate that with a minor release of rspec-rails (eg 2.12.0)

Collaborator

jnicklas commented Sep 7, 2012

@sobrinho: yes, this is also in part to clear up jnicklas/turnip#66, I would personally prefer if Turnip used :feature as its type, and the recommendation is to use spec/features. After all, Gherkin files do start with the word Feature, and I would like to have Turnip behave as close to regular RSpec as possible.

@josevalim: It seems that the folders are plural, but the :type is singular, from what I can tell. At least for :model, :controller, :request, :acceptance, etc... Maybe @dchelimsky can clear this up.

@dchelimsky: I don't know the internals of rspec-rails at all, so I don't know if it does anything special at all. Maybe not. In that case, even better :)

Contributor

josevalim commented Sep 7, 2012

It seems that the folders are plural, but the :type is singular, from what I can tell. At least for :model, :controller, :request, :acceptance, etc..

Ah, right. @dchelimsky so don't we need to change rspec-rails to also map the type :feature to the folder spec/features?

Contributor

dchelimsky commented Sep 7, 2012

@josevalim if we don't care about :type => xxx, capybara can do it by directory the way rspec-rails does it:

RSpec::configure do |c|
  def c.escaped_path(*parts)
    Regexp.compile(parts.join('[\\\/]'))
  end

  c.include Capybara::Whatever, :example_group => {
    :file_path => c.escaped_path(%w[spec features])
  }
end
Collaborator

jnicklas commented Sep 9, 2012

Okay, seems the general consensus is that this is a good idea.

jnicklas merged commit 9c0093a into master Sep 9, 2012

1 check passed

default The Travis build passed
Details
Contributor

dchelimsky commented Sep 9, 2012

@jnicklas just so we're on the same page: the change you just merged will break integration with rspec-2.11. Is that your understanding/intent?

jnicklas referenced this pull request in jnicklas/turnip Sep 9, 2012

Closed

Choose acceptance type instead of turnip #66

Collaborator

jnicklas commented Sep 9, 2012

@dchelimsky yes, I'm aware of that. I hope that doesn't screw up your release cycle too much. It'll be at least a month or two before Capybara 2.0 hits, maybe more, so you guys do have some time to prepare.

Contributor

dchelimsky commented Sep 9, 2012

Any reason for you to not just use my suggestion above (using :file_path rather than `:type)?

Contributor

dchelimsky commented Sep 9, 2012

BTW - it's less about the release cycle than it is about forcing end-users to upgrade two libs at the same time (and some of them not understanding that that is necessary).

/cc @myronmarston, @alindeman

Collaborator

jnicklas commented Sep 9, 2012

It seems strange to me that Capybara would make assumptions about the file layout of the users' test suites. It makes sense for rspec-rails to do that, but not really for Capybara, imo.

Contributor

dchelimsky commented Sep 9, 2012

I disagree in this case. "features" is a capybara concept, not an rspec-rails concept. Also, I think it's simpler and easier for end-users to grok "capy will attach its behavior to files in spec/features" than "capy will attach its behavior to example groups tagged :feature => true" and then have to go look at rspec-rails' docs to see that "rspec-rails adds :feature => true to example groups declared in the spec/features directory".

Contributor

sobrinho commented Sep 9, 2012

Assuming spec/features for capybara makes sense for me.

But what about the developers that want to change this structure, @dchelimsky?

Contributor

dchelimsky commented Sep 9, 2012

@sobrinho as things stand today in rspec-rails, integration, model, view, controller, and routing behavior are all added based on directory structure. It also supports :type => 'xxx', but I don't think any end users write describe X, :type => 'y' do in their specs.

Contributor

dchelimsky commented Sep 9, 2012

@sobrinho to align well w/ how rspec-rails works for the other types, Capy could add itself based on either :type => :feature or dir => spec/features.

Collaborator

jnicklas commented Sep 9, 2012

I certainly wouldn't have a problem with adding the folder based behaviour too. How does rspec-rails handle this? I can see the metadata :type => :model for model specs, but I don't explicitly specify it, so rspec-rails is adding this somewhere? Can we do the same thing? In that case, I think that would be good, and maybe easier to explain.

It's worth mentioning that Capybara's global feature method (described here) adds the type explicitly, so it doesn't matter what directory it's in, it will always get the correct type. The issue is mainly with behaviour that people have attached to Capybara specs themselves, and limited to only :request specs, that's where compatibility will most likely be affected most.

As @josevalim mentioned however, we still need to attach the base Rails behaviour stuff (routing etc.) to :type => :feature specs, and that is something that will require a new rspec release, from what I can tell.

Contributor

dchelimsky commented Sep 9, 2012

rspec-rails includes modules based on :type and :file_path (https://github.com/rspec/rspec-rails/blob/69a63cef280310c1164dda457bdecfd7700b10f0/lib/rspec/rails/example.rb#L10-38) and then adds :type from the extensions: https://github.com/rspec/rspec-rails/blob/69a63cef280310c1164dda457bdecfd7700b10f0/lib/rspec/rails/example/controller_example_group.rb#L120

This way end-users are able to use conventional directory structure or add :type => xxx in a non-conventional directory structure, and then extensions are able to attach their behavior based on :type.

In the case of capy, it's really adding a completely new type of example group, not extending an existing one, so I'd recommend treating it the same way as rspec-rails treats its own ControllerExampleGroup, for example.

As for including other support such as routing, all of that is in Rails' test framework, not RSpec. I wonder if this could all be managed from Capybara. I'm not trying to push extra work on Capy, just trying to reduce its dependency on RSpec to including itself via existing APIs. Thoughts?

Contributor

dchelimsky commented Sep 9, 2012

Correction in my last comment - it's ":type or :file_path" (not and).

Collaborator

jnicklas commented Sep 9, 2012

There should be no problem with emulating what rspec-rails does in Capybara, and then we can just tell people to move their specs to spec/features and change any includes which reference request or acceptance and it doesn't have to concern rspec-rails at all.

However, there is one hiccup. Currently, we have capybara/rspec, which isn't Rails specific, and capybara/rails, which isn't RSpec specific. I think it would make more sense to do this stuff in capybara/rspec, but then we'd have to do some if defined?(Rails) stuff, which I don't like at all. Any way we can get around that?

I guess we could have capybara/rspec_rails or something, but that seems ugly as well.

Contributor

sobrinho commented Sep 9, 2012

I don't remember to use capybara/rails on my rails projects.

I always required capybara/rspec using a spec/support/capybara.rb like this one:

require 'capybara/rspec'
require 'capybara/poltergeist'

Capybara.configure do |config|
  config.default_driver = :poltergeist
end

Is this required for rails 2 apps?

Contributor

dchelimsky commented Sep 9, 2012

I guess we could have capybara/rspec_rails or something, but that seems ugly as well.

Why would that be ugly? I don't see any conflict or order-dependency between https://github.com/jnicklas/capybara/blob/master/lib/capybara/rspec.rb and https://github.com/jnicklas/capybara/blob/master/lib/capybara/rails.rb, so I think it's reasonable to either tell end-users to require both, or add capybara/rspec-rails.rb that simply requires the other two.

Collaborator

jnicklas commented Sep 9, 2012

The more I think about it, the more I feel that this should be in rspec-rails. We already integrate with Capybara from rspec-rails, so that direction is already set. From within Capybara, we can do a lot of stuff, but it's very tricky to do stuff only when both RSpec and Rails are loaded. Whereas that's completely natural and obvious in rspec-rails. I'd be happy to provide a pull request to rspec-rails, if you're worried about the extra work.

So yes, this would require users to upgrade RSpec, but if we just explain this clearly, then it shouldn't be a problem. Maybe we can even check the RSpec version and print a warning if it's too low, or something. What do you think?

Collaborator

joliss commented Sep 9, 2012

We could just bump the rspec-rails dependency - no harm in that, especially for a major release, no?

Collaborator

joliss commented Sep 9, 2012

Wait, of course we don't depend on rspec-rails. Ignore me please.

Contributor

dchelimsky commented Sep 12, 2012

I'm OK w/ an rspec-rails release that deals with the directory paths, but it would be good if Capy could check for the rspec version in capybara/rspec.rb (will need to be >= 2.12, but that doesn't exist yet, so you probably want to wait for that release before making this change).

/cc @alindeman

Contributor

alindeman commented Sep 13, 2012

I'd be happy to review the changes for rspec-rails if you make a pull, @jnicklas.

Collaborator

jnicklas commented Sep 13, 2012

Okay, I'll try to throw together a pull request for rspec, I probably won't have time for that until next week though.

Contributor

alindeman commented Sep 25, 2012

Any news? Can I do anything to make this easier?

Contributor

dchelimsky commented Sep 26, 2012

@jnicklas @alindeman I'll have time after Monday (conf preso) and happy to work w/ either one or both of you on this.

Contributor

alindeman commented Sep 26, 2012

That'd work for me; I don't feel entirely comfortable approaching this alone.

Collaborator

jnicklas commented Sep 26, 2012

I started a little on a pull request to rspec-rails, but it's a bit daunting and I don't have a lot of time at the moment.

Contributor

dchelimsky commented Sep 27, 2012

@jnicklas don't sweat it. @alindeman and I will work on it after Monday. When are you trying to get your release out? I'd like the rspec-rails release that supports this to be out well in advance of the capy release.

Collaborator

jnicklas commented Sep 27, 2012

@dchelimsky we're pretty much ready to go with Capybara. I want to do another beta release, and then wait for a month before I do the final release, but aside from this issue there isn't anything stopping us. So the ideal for us would be if we do the beta release immediately after you guys do your release, and then wait a month, then final 2.0 release. Does that sound sensible?

Contributor

dchelimsky commented Sep 27, 2012

@jnicklas sure, but it might be a couple of weeks before we're ready to go. This will be part of a minor release of the entire rspec suite so there are some things that need to be coordinated. Most likely we'll be able to get it out the door by Monday, Oct 8, but I can't promise that at this point. We'll keep you posted.

Contributor

dchelimsky commented Oct 6, 2012

OK - I got started on this. I've added one commit that includes Capybara::DSL and Capybara::RSpecMatchers in example groups in the spec/features directory: rspec/rspec-rails@9332167

There is no automated testing for this but there was none before either. I've verified that it does the right thing by ensuring that the following runs correctly (in spec/features/thing_list_spec.rb):

require 'spec_helper'

describe "things"
  before do
    Thing.create!(:name => "Widget")
  end

  example "view the list" do
    visit "/things"
    page.body.should include("Widget")
  end
end

feature "things" do
  background do
    Thing.create!(:name => "Widget")
  end

  scenario "view the list" do
    visit "/things"
    page.body.should include("Widget")
  end
end

This solves for the "features" directory, but not the "api" directory. I see that as separate from this specific issue even though it's part of the wider discussion spawned by @josevalim. Agree?

Collaborator

jnicklas commented Oct 6, 2012

Yes agreed! Best we get the minimal solution out the door as soon as possible, then we can tweak it.

Contributor

dchelimsky commented Oct 6, 2012

@jnicklas I'm not sure I'll be able to get a minor release out this weekend, but I can do a patch release of rspec-rails. This would be rspec-rails-2.11.1. Are you OK explaining in your release notes that capy-2 requires rspec-rails >= 2.11.1 or would you rather wait until we do the 2.12 release (which is unlikely to happen before next weekend).

Collaborator

jnicklas commented Oct 6, 2012

Whatever you prefer, I'm not in a rush.

Contributor

dchelimsky commented Oct 7, 2012

Hey @jnicklas looking this over a bit more, right now capybara takes the responsibility of adding itself if type == :feature, but rspec-rails is responsible to add capybara if directory == spec/features. This feels odd to me. I'm thinking that either rspec-rails or capybara should own both. rspec-rails would still include Capybara::RSpecMatchers in other types of specs (but not DSL) either way. Thoughts?

/cc @alindeman

Contributor

dchelimsky commented Oct 7, 2012

Actually, if we do it from rspec-rails, there's no reason to use :type => :feature or :type => :capybara_feature. rspec-rails would include Capybara::DSL, Capybara::RSpecMatchers, and Capybara::Features when directory => spec/features.

Contributor

dchelimsky commented Oct 14, 2012

@jnicklas I released rspec-rails-2.11.4 today with support for including Capybara::DSL and Capybara::RSpecMatchers in spec/features. I've tested this building a new app using capybara-head and rspec-rails-2.11.4 and files added spec/features work as expected (visit/page are available and get|post|put|delete|head are not), and spec/requests also work as expected (get|post|put|delete|head work but visit/page do not).

I was under the false impression that the capybara-2.0.0.beta release already removed support for spec/acceptance and spec/requests, so I documented that you can use rspec-rails-2.11.4 + capybara-2.0.0.beta2. This does work in the sense that you can now use visit/page in spec/features, but it does not solve the "get and visit are both available" problem. Any chance you can drop a capybara-2.0.0.beta3 release sometime soon?

Collaborator

jnicklas commented Oct 14, 2012

Yes, I'll do a release either today or tomorrow, need to take a look at the backlog, but I think we should be good to go.

Contributor

josevalim commented Nov 1, 2012

I am using this in a new app and it is working amazingly well.
I just want to thank everyone, you all have a special place in my ❤️.

Collaborator

jnicklas commented Nov 2, 2012

Awesome :)

Collaborator

joliss commented Nov 7, 2012

/cc @dchelimsky

After @alindeman's rspec/rspec-rails@fd25838 I think we're getting closer.

One (hopefully last) issue is that previously we'd have a :type set on the examples, e.g. you could do

RSpec.configure do |config|
  config.include RequestHelpers, type: :request
end

Looking at https://github.com/rspec/rspec-rails/blob/master/lib/rspec/rails/vendor/capybara.rb and grepping the rspec-rails source for :feature, it seems that we don't really have a corresponding :feature type now, but instead we just go off of the spec path. Should we perhaps change that?

Collaborator

jnicklas commented Nov 7, 2012

Collaborator

joliss commented Nov 7, 2012

Oh I'd missed that. Thanks!

Contributor

alindeman commented Nov 11, 2012

I just pushed some new docs for feature specs. Reviews welcomed: rspec/rspec-rails@6e68bdd

@jperkin jperkin pushed a commit to joyent/pkgsrc that referenced this pull request Dec 9, 2013

taca Update ruby-rspec-rails to 2.12.0.
### 2.12.0 / 2012-11-12
[full changelog](http://github.com/rspec/rspec-rails/compare/v2.11.4...2.12.0)

Enhancements

* Support validation contexts when using `#errors_on` (Woody Peterson)
* Include RequestExampleGroup in groups in spec/api

Bug fixes

* Add `should` and `should_not` to `CollectionProxy` (Rails 3.1+) and
  `AssociationProxy` (Rails 3.0).  (Myron Marston)
* `controller.controller_path` is set correctly for view specs in Rails 3.1+.
  (Andy Lindeman)
* Generated specs support module namespacing (e.g., in a Rails engine).
  (Andy Lindeman)
* `render` properly infers the view to be rendered in Rails 3.0 and 3.1
  (John Firebaugh)
* AutoTest mappings involving config/ work correctly (Brent J. Nordquist)
* Failures message for `be_new_record` are more useful (Andy Lindeman)

### 2.11.4 / 2012-10-14
[full changelog](http://github.com/rspec/rspec-rails/compare/v2.11.0...v2.11.4)

Capybara-2.0 integration support:

* include RailsExampleGroup in spec/features (necessary when there is no AR)
* include Capybara::DSL and Capybara::RSpecMatchers in spec/features

See [teamcapybara/capybara#809
and [http://rubydoc.info/gems/rspec-rails/file/Capybara.md](http://rubydoc.info/gems/rspec-rails/file/Capybara.md)
for background.

2.11.1, .2, .3 were yanked due to errant documentation.
9bb7c11

@jsonn jsonn pushed a commit to jsonn/pkgsrc that referenced this pull request Mar 12, 2014

taca Update ruby-rspec-rails to 2.12.0.
### 2.12.0 / 2012-11-12
[full changelog](http://github.com/rspec/rspec-rails/compare/v2.11.4...2.12.0)

Enhancements

* Support validation contexts when using `#errors_on` (Woody Peterson)
* Include RequestExampleGroup in groups in spec/api

Bug fixes

* Add `should` and `should_not` to `CollectionProxy` (Rails 3.1+) and
  `AssociationProxy` (Rails 3.0).  (Myron Marston)
* `controller.controller_path` is set correctly for view specs in Rails 3.1+.
  (Andy Lindeman)
* Generated specs support module namespacing (e.g., in a Rails engine).
  (Andy Lindeman)
* `render` properly infers the view to be rendered in Rails 3.0 and 3.1
  (John Firebaugh)
* AutoTest mappings involving config/ work correctly (Brent J. Nordquist)
* Failures message for `be_new_record` are more useful (Andy Lindeman)

### 2.11.4 / 2012-10-14
[full changelog](http://github.com/rspec/rspec-rails/compare/v2.11.0...v2.11.4)

Capybara-2.0 integration support:

* include RailsExampleGroup in spec/features (necessary when there is no AR)
* include Capybara::DSL and Capybara::RSpecMatchers in spec/features

See [teamcapybara/capybara#809
and [http://rubydoc.info/gems/rspec-rails/file/Capybara.md](http://rubydoc.info/gems/rspec-rails/file/Capybara.md)
for background.

2.11.1, .2, .3 were yanked due to errant documentation.
789e033

@alexrothenberg alexrothenberg pushed a commit to alexrothenberg/rspec-rails that referenced this pull request Apr 7, 2014

@dchelimsky dchelimsky include Capybara::DSL and Capybara::RSpecMatchers in spec/features 9332167

@jsonn jsonn pushed a commit to jsonn/pkgsrc that referenced this pull request Oct 11, 2014

taca Update ruby-rspec-rails to 2.12.0.
### 2.12.0 / 2012-11-12
[full changelog](http://github.com/rspec/rspec-rails/compare/v2.11.4...2.12.0)

Enhancements

* Support validation contexts when using `#errors_on` (Woody Peterson)
* Include RequestExampleGroup in groups in spec/api

Bug fixes

* Add `should` and `should_not` to `CollectionProxy` (Rails 3.1+) and
  `AssociationProxy` (Rails 3.0).  (Myron Marston)
* `controller.controller_path` is set correctly for view specs in Rails 3.1+.
  (Andy Lindeman)
* Generated specs support module namespacing (e.g., in a Rails engine).
  (Andy Lindeman)
* `render` properly infers the view to be rendered in Rails 3.0 and 3.1
  (John Firebaugh)
* AutoTest mappings involving config/ work correctly (Brent J. Nordquist)
* Failures message for `be_new_record` are more useful (Andy Lindeman)

### 2.11.4 / 2012-10-14
[full changelog](http://github.com/rspec/rspec-rails/compare/v2.11.0...v2.11.4)

Capybara-2.0 integration support:

* include RailsExampleGroup in spec/features (necessary when there is no AR)
* include Capybara::DSL and Capybara::RSpecMatchers in spec/features

See [teamcapybara/capybara#809
and [http://rubydoc.info/gems/rspec-rails/file/Capybara.md](http://rubydoc.info/gems/rspec-rails/file/Capybara.md)
for background.

2.11.1, .2, .3 were yanked due to errant documentation.
60ca0b7
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment