Consider making example group mixins more explicit #662

Closed
myronmarston opened this Issue Jan 1, 2013 · 25 comments

Comments

Projects
None yet
@myronmarston
Member

myronmarston commented Jan 1, 2013

There's a lot of implicit file-location-based configuration done by rspec-rails--e.g. spec files in "spec/{models/controllers/requests/mailers/etc}" get a special mixin just by being in that directory. This fits well with rails' "convention over configuration" philosophy, but often creates confusion, as it's not obvious from looking at the code where particular helper methods are coming from. In addition, this setup can actively get in your way if you decide that you want to use a different directory structure. For example, @xaviershay blogged about the way he tests rails apps, and he uses a top level unit/integration/acceptance directory structure. I haven't worked on a rails app in a while, but I've been using kind of top-level structure in my ruby applications as well, and I'd probably use it the next time I build a rails app.

For rspec-rails, I was thinking we could make things a bit more explicit:

  • By default, don't mixin the various example group modules based on file location.
  • Use simple, explicit metadata tagging to mixin example groups instead (e.g. describe User, :model do... or describe UsersController, :controller do...). This is the same mechanism that shared contexts can use, and in fact, the existing mixins could easily be rewritten as shared contexts (which would decouple them from ActiveSupport::Concern).
  • Provide a config option so that users who want to use the file-location mechanism still can, e.g. RSpec.configuration.infer_spec_type_from_file_location = true.
  • The generated spec_helper.rb file could include this config line by default, with a comment explaining it--that would keep the current behavior the same for new apps.
  • If we move in this direction, we'll probably want to provide the config option in the last 2.x release, and maybe have it generate a warning if it's not set (so that users can use that release to help get their apps ready to upgrade, then upgrade to rspec-rails 3 and have it work).
@mehulkar

This comment has been minimized.

Show comment
Hide comment
@mehulkar

mehulkar May 23, 2013

👍

Or maybe the ability to write a top level yaml file that defines what our directory structure looks like? I'm returning to rspec after a year and I'm having trouble figuring out exactly what methods are available where.

👍

Or maybe the ability to write a top level yaml file that defines what our directory structure looks like? I'm returning to rspec after a year and I'm having trouble figuring out exactly what methods are available where.

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Aug 16, 2013

I don't think this makes anything more transparent in terms of seeing what modules are mixed in or what modules are available but I'm fan of not enforcing a directory layout on people.

ghost commented Aug 16, 2013

I don't think this makes anything more transparent in terms of seeing what modules are mixed in or what modules are available but I'm fan of not enforcing a directory layout on people.

@xaviershay

This comment has been minimized.

Show comment
Hide comment
@xaviershay

xaviershay Aug 16, 2013

Member

Obviously I am in favour of this :) I've always found the magic directories confusing, even before I started doing it my own way.

Member

xaviershay commented Aug 16, 2013

Obviously I am in favour of this :) I've always found the magic directories confusing, even before I started doing it my own way.

@cupakromer

This comment has been minimized.

Show comment
Hide comment
@cupakromer

cupakromer Aug 16, 2013

Member

👉 (mixed on this one)

I personally, just my 2¢, really like the auto-inferring based on the file path. This very much falls in line with the Rails convention over configuration. Rails has specific locations for code, it just makes sense that the tests mirror that (to a degree). It also makes sense that the related type of tests auto-includes the relevant setup (yes I realize that MiniTest and Rails force class inheritance and don't mix stuff in magically).

IMHO, I think RSpec.configuration.infer_spec_type_from_file_location should default to true. The comment added should then instead state set it to false (or add uncomment the following line, which sets false) - opt-out not in. It is always a bit frustrating to have long time explicit convention defaults reversed.

With that said, there are times I do want to customize it. Having looked at the code, I know how to do this (see setup file). Though, duplicating those lines for different file paths is not a fun approach. It would be great to see:

  • The ability to add directories to these defaults. Maybe something like: RSpec.configuration.integration_specs_path << "acceptance"?
  • Better Relish/Readme documentation stating the metadata required for each spec type
  • Better Relish/Readme documentation stating how to customize the different spec paths

❤️

Member

cupakromer commented Aug 16, 2013

👉 (mixed on this one)

I personally, just my 2¢, really like the auto-inferring based on the file path. This very much falls in line with the Rails convention over configuration. Rails has specific locations for code, it just makes sense that the tests mirror that (to a degree). It also makes sense that the related type of tests auto-includes the relevant setup (yes I realize that MiniTest and Rails force class inheritance and don't mix stuff in magically).

IMHO, I think RSpec.configuration.infer_spec_type_from_file_location should default to true. The comment added should then instead state set it to false (or add uncomment the following line, which sets false) - opt-out not in. It is always a bit frustrating to have long time explicit convention defaults reversed.

With that said, there are times I do want to customize it. Having looked at the code, I know how to do this (see setup file). Though, duplicating those lines for different file paths is not a fun approach. It would be great to see:

  • The ability to add directories to these defaults. Maybe something like: RSpec.configuration.integration_specs_path << "acceptance"?
  • Better Relish/Readme documentation stating the metadata required for each spec type
  • Better Relish/Readme documentation stating how to customize the different spec paths

❤️

@chris-teague

This comment has been minimized.

Show comment
Hide comment
@chris-teague

chris-teague Aug 16, 2013

RSpec.configuration.integration_specs_path << "acceptance" etc included in config out the box feels like a nice layer of visibility for those wanting to break from the norm.

RSpec.configuration.integration_specs_path << "acceptance" etc included in config out the box feels like a nice layer of visibility for those wanting to break from the norm.

@DanielHeath

This comment has been minimized.

Show comment
Hide comment
@DanielHeath

DanielHeath Aug 16, 2013

What about RSpec.configuration.auto_mixins["models"] = ReplacementModelMixin?

Maintain the existing magic whilst making overriding/reconfiguring easy.

What about RSpec.configuration.auto_mixins["models"] = ReplacementModelMixin?

Maintain the existing magic whilst making overriding/reconfiguring easy.

@todd

This comment has been minimized.

Show comment
Hide comment
@todd

todd Aug 16, 2013

I can go either way on this one, but I think providing this as a configuration option instead of the default might be a better path to go down. Just my $0.02.

Either way, I think it's worth implementing in some form.

todd commented Aug 16, 2013

I can go either way on this one, but I think providing this as a configuration option instead of the default might be a better path to go down. Just my $0.02.

Either way, I think it's worth implementing in some form.

@cupakromer

This comment has been minimized.

Show comment
Hide comment
@cupakromer

cupakromer Aug 16, 2013

Member

I'll take a stab at adding the [current version of the] docs I proposed tomorrow ❤️ ❤️

Member

cupakromer commented Aug 16, 2013

I'll take a stab at adding the [current version of the] docs I proposed tomorrow ❤️ ❤️

@fatgeekuk

This comment has been minimized.

Show comment
Hide comment
@fatgeekuk

fatgeekuk Aug 16, 2013

possibly interesting, but how about infering the mixin from the class of the top level describe subject.

So if it is a kind_of? ActiveRecord::Base, it gets the models, if a Controller it gets them. thats ok I suppose for Model and Controller mixings, but when you get to Request... hmmm, then fall back to explicitly mentioning again, and allow explicit for when you have a bare Object subclass as a model...

possibly interesting, but how about infering the mixin from the class of the top level describe subject.

So if it is a kind_of? ActiveRecord::Base, it gets the models, if a Controller it gets them. thats ok I suppose for Model and Controller mixings, but when you get to Request... hmmm, then fall back to explicitly mentioning again, and allow explicit for when you have a bare Object subclass as a model...

@jimmycuadra

This comment has been minimized.

Show comment
Hide comment
@jimmycuadra

jimmycuadra Aug 16, 2013

Great idea. I don't see this as being the standard use case for most Rails developers, so I agree that it should remain the way it is now with a configuration option to disable the auto-mixins. However, adding it as a configuration option in the generated spec_helper.rb implies that the intent is to eventually change the default, which I don't think is a good idea. This is a situation where the Rails-ism of "sensible defaults with a way to choose something else if you really want to" applies perfectly. I would suggest having the option set to true by default, and developers who want more control can explicitly set it to false.

Great idea. I don't see this as being the standard use case for most Rails developers, so I agree that it should remain the way it is now with a configuration option to disable the auto-mixins. However, adding it as a configuration option in the generated spec_helper.rb implies that the intent is to eventually change the default, which I don't think is a good idea. This is a situation where the Rails-ism of "sensible defaults with a way to choose something else if you really want to" applies perfectly. I would suggest having the option set to true by default, and developers who want more control can explicitly set it to false.

@dchelimsky

This comment has been minimized.

Show comment
Hide comment
@dchelimsky

dchelimsky Aug 16, 2013

Member

rspec-rails uses public APIs from rspec-core to do this auto-inclusion. There is one additional helper in that file, which is there to support windows and *nix paths. We could probably clarify that helper a bit and expose it as an API, but I don't think we need to expose any new API on Configuration beyond the infer_spec_type_from_file_locationoption which, IMO, should default to true for rspec-rails-3 and only default to false in rspec-rails-4 or later if community pushes it that way.

Member

dchelimsky commented Aug 16, 2013

rspec-rails uses public APIs from rspec-core to do this auto-inclusion. There is one additional helper in that file, which is there to support windows and *nix paths. We could probably clarify that helper a bit and expose it as an API, but I don't think we need to expose any new API on Configuration beyond the infer_spec_type_from_file_locationoption which, IMO, should default to true for rspec-rails-3 and only default to false in rspec-rails-4 or later if community pushes it that way.

@tadast

This comment has been minimized.

Show comment
Hide comment
@tadast

tadast Aug 16, 2013

I personally never had a need to change the inferred defaults and think that making it explicit would be a step back. Overridible defaults > configuration.

tadast commented Aug 16, 2013

I personally never had a need to change the inferred defaults and think that making it explicit would be a step back. Overridible defaults > configuration.

@dasch

This comment has been minimized.

Show comment
Hide comment
@dasch

dasch Aug 16, 2013

Contributor

👍 it will make it easier to avoid getting the Active Record stuff mixed into specs of non-AR models.

Contributor

dasch commented Aug 16, 2013

👍 it will make it easier to avoid getting the Active Record stuff mixed into specs of non-AR models.

@jeremywrowe

This comment has been minimized.

Show comment
Hide comment
@jeremywrowe

jeremywrowe Aug 18, 2013

I think being explicit when it comes to test environment loading is a fantastic idea. My vote would be to turn the RSpec.configuration.infer_spec_type_from_file_location option on by default (with warnings) for new 2.* releases and to remove the default in 3.* releases. Backwards compatibility is nice, but the overall goal of reducing confusion wins in this case I believe. As far as tagging each spec with the "type" of spec it is to load a proper test environment seems pretty heavy handed.

consider

├── lib
├── mailers
├── models
├── observers
├── presenters
├── requests
├── routing
├── scripts
├── services
└── spec_helper.rb

What I would like to see is a vanilla RSpec environment for all specs referencing spec/spec_helper.rb and for each sub-directory for example spec/routing an overridable spec_helper.rb spec/routing/spec_helper.rb that does two things. 1) loads the original spec helper spec/spec_helper.rb 2) allows for additional configuration for the given directory. i.e. specify the rspec modules to load.

I like this approach for a couple reasons. It localizes configuration for spec types (change little instead of a lot) And it also is very clear what the spec_helper.rb file in the sub-directory is intended for.

I think being explicit when it comes to test environment loading is a fantastic idea. My vote would be to turn the RSpec.configuration.infer_spec_type_from_file_location option on by default (with warnings) for new 2.* releases and to remove the default in 3.* releases. Backwards compatibility is nice, but the overall goal of reducing confusion wins in this case I believe. As far as tagging each spec with the "type" of spec it is to load a proper test environment seems pretty heavy handed.

consider

├── lib
├── mailers
├── models
├── observers
├── presenters
├── requests
├── routing
├── scripts
├── services
└── spec_helper.rb

What I would like to see is a vanilla RSpec environment for all specs referencing spec/spec_helper.rb and for each sub-directory for example spec/routing an overridable spec_helper.rb spec/routing/spec_helper.rb that does two things. 1) loads the original spec helper spec/spec_helper.rb 2) allows for additional configuration for the given directory. i.e. specify the rspec modules to load.

I like this approach for a couple reasons. It localizes configuration for spec types (change little instead of a lot) And it also is very clear what the spec_helper.rb file in the sub-directory is intended for.

@ndelage

This comment has been minimized.

Show comment
Hide comment
@ndelage

ndelage Oct 22, 2013

As someone who teaches beginning developers to test Rails apps using RSpec this feature often causes a lot of confusion. Mostly because the spec/[models/controllers/features/etc] aren't automatically created when running the installer script. Creating those folders via the install script would really help beginners (even though it might upset devs that prefer an alternative structure).

Really though, I'd like to auto generate the folder structure and require explicit mixin inclusion (if that's the only magic done at the moment).

ndelage commented Oct 22, 2013

As someone who teaches beginning developers to test Rails apps using RSpec this feature often causes a lot of confusion. Mostly because the spec/[models/controllers/features/etc] aren't automatically created when running the installer script. Creating those folders via the install script would really help beginners (even though it might upset devs that prefer an alternative structure).

Really though, I'd like to auto generate the folder structure and require explicit mixin inclusion (if that's the only magic done at the moment).

@eignerchris

This comment has been minimized.

Show comment
Hide comment
@eignerchris

eignerchris Oct 23, 2013

Just out of curiosity, how many different types of directories are supported? models, unit, features, routing, requests, views, controllers, acceptance, integration, helpers, support, api? Seems we support multiple conventions, which breaks the convention argument. Let's pick one.

For instance, from http://rubydoc.info/gems/rspec-rails/file/README.md, "Request specs live in spec/requests, spec/api and spec/integration" ... "Note that Capybara's DSL as shown is, by default, only available in specs in the spec/features"

This type of caveating shouldn't be necessary. I think the support for multiple conventions is what makes it difficult to use.

My vote is to reduce the directory support and as @ndelage said, generate the directories when the generator is run.

Just out of curiosity, how many different types of directories are supported? models, unit, features, routing, requests, views, controllers, acceptance, integration, helpers, support, api? Seems we support multiple conventions, which breaks the convention argument. Let's pick one.

For instance, from http://rubydoc.info/gems/rspec-rails/file/README.md, "Request specs live in spec/requests, spec/api and spec/integration" ... "Note that Capybara's DSL as shown is, by default, only available in specs in the spec/features"

This type of caveating shouldn't be necessary. I think the support for multiple conventions is what makes it difficult to use.

My vote is to reduce the directory support and as @ndelage said, generate the directories when the generator is run.

@JonRowe

This comment has been minimized.

Show comment
Hide comment
@JonRowe

JonRowe Oct 23, 2013

Member

generate the directories when the generator is run

They should be generated when the specific generator for a type is invoked. It's not a necessity for any of them to be used so creating a tonne of empty directories will just confuse people.

The read me could do with updating, request specs live in request specs. The other two are for integration tests, they just happen to use the same functionality.

Member

JonRowe commented Oct 23, 2013

generate the directories when the generator is run

They should be generated when the specific generator for a type is invoked. It's not a necessity for any of them to be used so creating a tonne of empty directories will just confuse people.

The read me could do with updating, request specs live in request specs. The other two are for integration tests, they just happen to use the same functionality.

@xaviershay

This comment has been minimized.

Show comment
Hide comment
@xaviershay

xaviershay Mar 23, 2014

Member

PR to bikeshed on: #970
PR that we should just merge: #969

(they conflict with each other, but the second one can be reverted once we decide to apply the first)

Member

xaviershay commented Mar 23, 2014

PR to bikeshed on: #970
PR that we should just merge: #969

(they conflict with each other, but the second one can be reverted once we decide to apply the first)

@xaviershay

This comment has been minimized.

Show comment
Hide comment
@xaviershay

xaviershay Apr 20, 2014

Member

I don't think we should move on this right now. With #969 merged, we enable people who care about this to try a number of different ideas. I'd like to do that to see if anything ends up being naturally popular. If nothing takes hold we can circle back and lead the charge.

Member

xaviershay commented Apr 20, 2014

I don't think we should move on this right now. With #969 merged, we enable people who care about this to try a number of different ideas. I'd like to do that to see if anything ends up being naturally popular. If nothing takes hold we can circle back and lead the charge.

@xaviershay xaviershay changed the title from Consider making example group mixins more explicit in rspec-rails 3 to Consider making example group mixins more explicit Apr 20, 2014

@myronmarston

This comment has been minimized.

Show comment
Hide comment
@myronmarston

myronmarston Apr 20, 2014

Member

With #969 merged, we enable people who care about this to try a number of different ideas.

#969 seems like a pretty hidden feature -- you have to know about rspec/rails/without_filetype_infer to benefit from it, and I suspect it won't be widely known or used simply because you have to dig to find it.

I'd still like to see the directory-based inference changed from opt-out to opt-in as it's not a way I would recommend using RSpec, and it seems odd for the defaults of a new major version not to align with the lead maintainer's recommendation. That said, I don't use rails and the fact that no one has stepped forward to implement this suggests it's not super important to those who use rspec-rails. Also, I don't consider this a release blocker.

Member

myronmarston commented Apr 20, 2014

With #969 merged, we enable people who care about this to try a number of different ideas.

#969 seems like a pretty hidden feature -- you have to know about rspec/rails/without_filetype_infer to benefit from it, and I suspect it won't be widely known or used simply because you have to dig to find it.

I'd still like to see the directory-based inference changed from opt-out to opt-in as it's not a way I would recommend using RSpec, and it seems odd for the defaults of a new major version not to align with the lead maintainer's recommendation. That said, I don't use rails and the fact that no one has stepped forward to implement this suggests it's not super important to those who use rspec-rails. Also, I don't consider this a release blocker.

@xaviershay

This comment has been minimized.

Show comment
Hide comment
@xaviershay

xaviershay Apr 20, 2014

Member

you have to know about rspec/rails/without_filetype_infer to benefit from it

My point being that the few people who really care about this issue now have non-hack way to experiment with different ideas and hopefully propose something that works in the real world.

An example of this happening (perhaps not how we wanted!): with #969 I personally don't feel any need to change anything else, since I can just disable the entire feature I dislike ;)

Member

xaviershay commented Apr 20, 2014

you have to know about rspec/rails/without_filetype_infer to benefit from it

My point being that the few people who really care about this issue now have non-hack way to experiment with different ideas and hopefully propose something that works in the real world.

An example of this happening (perhaps not how we wanted!): with #969 I personally don't feel any need to change anything else, since I can just disable the entire feature I dislike ;)

@myronmarston

This comment has been minimized.

Show comment
Hide comment
@myronmarston

myronmarston Apr 20, 2014

Member

I've been mulling this over some more. I'm still definitely in the "this is not a release blocker" camp but I think I'd like to see #970 resurrected and merged in. In #970, @xaviershay had said:

We probably want to hold off this PR until it's more comprehensive (i.e. allows explicit configuration of directories maybe?)

IMO, explicit configuration of directories would be a new feature and could easily go in any future 3.x release, but changing from opt-out to opt-in is a "now or never" (or at least not until 4.0) thing -- and given how close #970 is to changing this to opt-in, I think we should take it across the finish line. Having the line in the generated spec_helper about the opt-in flag will give this much more visibility and make many more people aware that there's even the option to do something different.

Besides, I think an explicit RSpec.configure API is much better (and more inline with the rest of rspec) than a "require this special file rather than rspec/rails" API.

I may even work on resurrecting #970 myself.

Member

myronmarston commented Apr 20, 2014

I've been mulling this over some more. I'm still definitely in the "this is not a release blocker" camp but I think I'd like to see #970 resurrected and merged in. In #970, @xaviershay had said:

We probably want to hold off this PR until it's more comprehensive (i.e. allows explicit configuration of directories maybe?)

IMO, explicit configuration of directories would be a new feature and could easily go in any future 3.x release, but changing from opt-out to opt-in is a "now or never" (or at least not until 4.0) thing -- and given how close #970 is to changing this to opt-in, I think we should take it across the finish line. Having the line in the generated spec_helper about the opt-in flag will give this much more visibility and make many more people aware that there's even the option to do something different.

Besides, I think an explicit RSpec.configure API is much better (and more inline with the rest of rspec) than a "require this special file rather than rspec/rails" API.

I may even work on resurrecting #970 myself.

@xaviershay

This comment has been minimized.

Show comment
Hide comment
@xaviershay

xaviershay Apr 20, 2014

Member

I resurrected #970

Member

xaviershay commented Apr 20, 2014

I resurrected #970

@cupakromer

This comment has been minimized.

Show comment
Hide comment
@cupakromer

cupakromer Apr 28, 2014

Member

Do we have anything left to complete for this one?

Member

cupakromer commented Apr 28, 2014

Do we have anything left to complete for this one?

@JonRowe

This comment has been minimized.

Show comment
Hide comment
@JonRowe

JonRowe Apr 28, 2014

Member

Not as far as I know.

Member

JonRowe commented Apr 28, 2014

Not as far as I know.

@JonRowe JonRowe closed this Apr 28, 2014

amatriain added a commit to amatriain/feedbunch that referenced this issue Jun 3, 2014

Reorganized specs into specs/unit_tests and specs/acceptance_tests di…
…rectories. Added metadata to all tests to let rspec-rails know the type of each test suite (controller, model, feature etc).

Older rspec-rails versions demanded a particular directory structure for the specs, so that e.g. controllers tests (which had to be in /specs/controllers) had a module mixed in during testing, to enable helper methods necessary for each type of test (e.g. accessing response headers directly during controller tests).

This is being deprecated and it will become opt-in in rspec-rails 3. A particular directory structure will no longer be enforced, each test suite will have to specify the type of test by using metadata (a :type named argument to the "describe" method at the top of each file).

I've gone ahead and, in preparation for rspec-rails 3, I've added metadata to all tests which make use of rspec-rails helper methods. Now that a particular directory structure is no longer enforced, I've reorganized tests in a "unit_tests" and "acceptance_tests" structure which is more confortable for me.

For details about the old directory structure and the new metadata-based spect type specification, see:

https://relishapp.com/rspec/rspec-rails/v/3-0/docs/directory-structure

For interesting discussions about this topic see:

rspec/rspec-rails#662
https://github.com/rspec/rspec-rails/pull/970/files
rspec/rspec-rails#1002
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment