New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make filetype infer opt-in #970

Merged
merged 1 commit into from Apr 21, 2014

Conversation

Projects
None yet
4 participants
@xaviershay
Member

xaviershay commented Mar 23, 2014

(Re-issue of #967 against master)

Begins to address #662

  • Configuration is a bang method rather than a setting, because we don't have a public API for "un-including" modules.
  • As a result there is no way to make this "opt-out" without requiring a breaking change to config.
  • Newly generated spec_helper.rb will opt-in, with an explanatory comment.

DO NOT MERGE: Needs a 2.99 deprecation.

@alindeman @myronmarston

Show outdated Hide outdated lib/generators/rspec/install/templates/spec/spec_helper.rb.tt
# You can disable this behaviour by removing the line below, and instead
# explictly tag your specs with their type, e.g.:
#
# describe UsersController, :controller do

This comment has been minimized.

@myronmarston

myronmarston Mar 23, 2014

Member

Shouldn't this metadata be :type => :controller? :controller is just the short form for :controller => true.

@myronmarston

myronmarston Mar 23, 2014

Member

Shouldn't this metadata be :type => :controller? :controller is just the short form for :controller => true.

This comment has been minimized.

@xaviershay

xaviershay Mar 23, 2014

Member

oh whoops, yeah.

@xaviershay

xaviershay Mar 23, 2014

Member

oh whoops, yeah.

Show outdated Hide outdated lib/rspec/rails/configuration.rb
}
end
end
module_function :infer_spec_type_from_file_location!

This comment has been minimized.

@myronmarston

myronmarston Mar 23, 2014

Member

Can this be defined as a config option on RSpec.configuration instead? rspec-rails adds other configs that are exposed via RSpec.configure and this feels like it should be the same to me.

@myronmarston

myronmarston Mar 23, 2014

Member

Can this be defined as a config option on RSpec.configuration instead? rspec-rails adds other configs that are exposed via RSpec.configure and this feels like it should be the same to me.

This comment has been minimized.

@xaviershay

xaviershay Mar 23, 2014

Member

Yeah I wasn't sure about that. I'm used to mocks where we have our own separate configuration.

@xaviershay

xaviershay Mar 23, 2014

Member

Yeah I wasn't sure about that. I'm used to mocks where we have our own separate configuration.

This comment has been minimized.

@cupakromer

cupakromer Mar 27, 2014

Member

I agree we should make this part of RSpec.configure.

@cupakromer

cupakromer Mar 27, 2014

Member

I agree we should make this part of RSpec.configure.

This comment has been minimized.

@myronmarston

myronmarston Mar 27, 2014

Member

The reason mocks has its own configuration (and expectations) is because they are designed to be usable on their own, apart from rspec-core. (For example, you can use them with minitest or cucumber).

rspec-rails, on the other hand, requires rspec-core, and so it's safe to assume the rspec-core config object is available, and makes sense to piggy back on that to present the config interface.

@myronmarston

myronmarston Mar 27, 2014

Member

The reason mocks has its own configuration (and expectations) is because they are designed to be usable on their own, apart from rspec-core. (For example, you can use them with minitest or cucumber).

rspec-rails, on the other hand, requires rspec-core, and so it's safe to assume the rspec-core config object is available, and makes sense to piggy back on that to present the config interface.

This comment has been minimized.

@xaviershay

xaviershay Mar 27, 2014

Member

aha, that makes sense.

@xaviershay

xaviershay Mar 27, 2014

Member

aha, that makes sense.

@myronmarston

This comment has been minimized.

Show comment
Hide comment
@myronmarston

myronmarston Mar 23, 2014

Member

👍 I'm a fan of this. I think that making it opt-in with the generated file turning it on is the right approach to take.

Member

myronmarston commented Mar 23, 2014

👍 I'm a fan of this. I think that making it opt-in with the generated file turning it on is the right approach to take.

@myronmarston

This comment has been minimized.

Show comment
Hide comment
@myronmarston

myronmarston Mar 23, 2014

Member

Hmm...one thought though: if we move towards having separate spec_helper (being rspec-core's file, which is meant to be very light weight) and rails_spec_helper files (being the extra rails stuff), would this belong in rails_spec_helper?

Member

myronmarston commented Mar 23, 2014

Hmm...one thought though: if we move towards having separate spec_helper (being rspec-core's file, which is meant to be very light weight) and rails_spec_helper files (being the extra rails stuff), would this belong in rails_spec_helper?

@xaviershay

This comment has been minimized.

Show comment
Hide comment
@xaviershay

xaviershay Mar 23, 2014

Member

So I just started using the other PR we merged (require 'rspec/rails/without_filetype_infer) and that's really easy and solves all my problems.

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

Member

xaviershay commented Mar 23, 2014

So I just started using the other PR we merged (require 'rspec/rails/without_filetype_infer) and that's really easy and solves all my problems.

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

@JonRowe

This comment has been minimized.

Show comment
Hide comment
@JonRowe

JonRowe Mar 23, 2014

Member

Personally. I feel we should remove filetype inferring and update the generators to set the specific tags rather than dealing with this. We can obviously only do this at major versions like this, but it would be beneficial to the Rails testing community.

Member

JonRowe commented Mar 23, 2014

Personally. I feel we should remove filetype inferring and update the generators to set the specific tags rather than dealing with this. We can obviously only do this at major versions like this, but it would be beneficial to the Rails testing community.

@xaviershay

This comment has been minimized.

Show comment
Hide comment
@xaviershay

xaviershay Mar 27, 2014

Member

So after getting that other PR in, I'm feeling zero inclination to work on this. I don't think it should be merged as is (even after addressing local feedback), not useful enough.

Member

xaviershay commented Mar 27, 2014

So after getting that other PR in, I'm feeling zero inclination to work on this. I don't think it should be merged as is (even after addressing local feedback), not useful enough.

@xaviershay xaviershay closed this Mar 27, 2014

@cupakromer

This comment has been minimized.

Show comment
Hide comment
@cupakromer

cupakromer Mar 27, 2014

Member

Personally, I'm not a fan of completely removing the filetype inferring. I agree it can be confusing for people first encountering it. Though I'm not sure forcing metadata tags makes that any less magical; it feels like swapping one magic convention for another. I can see the flexibility/freedom it will allow with spec organization; though I'm not entirely convinced that's all good.

My other major concern is large existing projects which wish to upgrade to RSpec 3. Without either a tool to automatically tag all of the existing specs, or a way to opt-in to the well established behavior, upgrading will be come a much more time consuming task.

Member

cupakromer commented Mar 27, 2014

Personally, I'm not a fan of completely removing the filetype inferring. I agree it can be confusing for people first encountering it. Though I'm not sure forcing metadata tags makes that any less magical; it feels like swapping one magic convention for another. I can see the flexibility/freedom it will allow with spec organization; though I'm not entirely convinced that's all good.

My other major concern is large existing projects which wish to upgrade to RSpec 3. Without either a tool to automatically tag all of the existing specs, or a way to opt-in to the well established behavior, upgrading will be come a much more time consuming task.

@JonRowe

This comment has been minimized.

Show comment
Hide comment
@JonRowe

JonRowe Mar 27, 2014

Member

Transpec would surely be able to upgrade the specs, it's not a hard identification task.

I completely disagree that tagging them is as magic as inferring from directory structure. The former is explicit the later is completely hidden from the developer, note that Rails does not force folder conventions on developers (you can happily have active_record models in app/controllers if you so choose) so why should we assume that too?

Member

JonRowe commented Mar 27, 2014

Transpec would surely be able to upgrade the specs, it's not a hard identification task.

I completely disagree that tagging them is as magic as inferring from directory structure. The former is explicit the later is completely hidden from the developer, note that Rails does not force folder conventions on developers (you can happily have active_record models in app/controllers if you so choose) so why should we assume that too?

@cupakromer

This comment has been minimized.

Show comment
Hide comment
@cupakromer

cupakromer Mar 27, 2014

Member

After sleeping on it, I agree that metadata is less magical.

When I first used RSpec, metadata was something that never came up. So as I first started hearing about it, and saw it for the first time, it seemed a bit magical. At the time, "just add this tag" and "put the spec file there" held the same level of "magic under the hood". This feeling was compounded when the metadata definitions were in a gem.

I agree that having transpec upgrade the specs will go a very long way. Also, I think most of my concerns at this point are simply resolved by updating the docs explaining the different ways to mark a spec. I'm happy to work on the doc changes.

Member

cupakromer commented Mar 27, 2014

After sleeping on it, I agree that metadata is less magical.

When I first used RSpec, metadata was something that never came up. So as I first started hearing about it, and saw it for the first time, it seemed a bit magical. At the time, "just add this tag" and "put the spec file there" held the same level of "magic under the hood". This feeling was compounded when the metadata definitions were in a gem.

I agree that having transpec upgrade the specs will go a very long way. Also, I think most of my concerns at this point are simply resolved by updating the docs explaining the different ways to mark a spec. I'm happy to work on the doc changes.

@xaviershay

This comment has been minimized.

Show comment
Hide comment
@xaviershay

xaviershay Apr 20, 2014

Member

I'm looking at this again.

Member

xaviershay commented Apr 20, 2014

I'm looking at this again.

@xaviershay xaviershay reopened this Apr 20, 2014

@xaviershay

This comment has been minimized.

Show comment
Hide comment
@xaviershay

xaviershay Apr 20, 2014

Member

Addressed comments.

Completely removing file type inferring is too drastic a step without at least going through an opt-in period first, because it would break basically every tutorial on the internet. We should consider it for 4 though, if opt-out becomes popular.

Member

xaviershay commented Apr 20, 2014

Addressed comments.

Completely removing file type inferring is too drastic a step without at least going through an opt-in period first, because it would break basically every tutorial on the internet. We should consider it for 4 though, if opt-out becomes popular.

@myronmarston

This comment has been minimized.

Show comment
Hide comment
@myronmarston

myronmarston Apr 21, 2014

Member

Completely removing file type inferring is too drastic a step without at least going through an opt-in period first, because it would break basically every tutorial on the internet. We should consider it for 4 though, if opt-out becomes popular.

I completely agree.

Anyhow, this LGTM. I think we need a deprecation warning in 2.99 instructing people to add the new config setting (and the setting API should be backported but be a no-op).

Also, with this in place does it still make sense to have the without_filetype_infer file as we now have a proper config API for that? (not a merge blocker, of course).

Member

myronmarston commented Apr 21, 2014

Completely removing file type inferring is too drastic a step without at least going through an opt-in period first, because it would break basically every tutorial on the internet. We should consider it for 4 though, if opt-out becomes popular.

I completely agree.

Anyhow, this LGTM. I think we need a deprecation warning in 2.99 instructing people to add the new config setting (and the setting API should be backported but be a no-op).

Also, with this in place does it still make sense to have the without_filetype_infer file as we now have a proper config API for that? (not a merge blocker, of course).

@JonRowe

This comment has been minimized.

Show comment
Hide comment
@JonRowe

JonRowe Apr 21, 2014

Member

If this works I think we should :shipit:

Member

JonRowe commented Apr 21, 2014

If this works I think we should :shipit:

@myronmarston

This comment has been minimized.

Show comment
Hide comment
@myronmarston

myronmarston Apr 21, 2014

Member

I'm going to merge this since I'm happy with it and the thing I'm working on (which resolves rspec/rspec-core#1089 / #829 / rspec/rspec-core#969 all in one pretty simple change) will build off of this.

Member

myronmarston commented Apr 21, 2014

I'm going to merge this since I'm happy with it and the thing I'm working on (which resolves rspec/rspec-core#1089 / #829 / rspec/rspec-core#969 all in one pretty simple change) will build off of this.

myronmarston added a commit that referenced this pull request Apr 21, 2014

Merge pull request #970 from rspec/issue-662
Make filetype infer opt-in

@myronmarston myronmarston merged commit 041f542 into master Apr 21, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details

@myronmarston myronmarston deleted the issue-662 branch Apr 21, 2014

myronmarston added a commit that referenced this pull request Apr 21, 2014

Fix spec type inferrence.
- In #970 we added `infer_spec_type_from_file_location!` but forgot
  to remove the code in `lib/rspec/rails/configuration.rb` that
  made it always infer - so the opt-in API was there but it
  was always enabled. Here I've made it truly opt-in.
- The logic of `infer_spec_type_from_file_location!` also setup
  the helper module inclusion via explicit `:type` metadata but
  was only intended to setup the implicit mapping of spec file
  location to type.  Here I've made the module inclusion via
  `:type` always work w/o an explicit config option needed to
  enable it.
- We used to mutate the `:type` metadata on inclusion of the helper
  module, but this caused bugs such as #825. The problem is that
  rspec-core uses a raw hash for the metadata and doesn't re-apply
  filtering/inclusion logic when the metadata hash is mutated.
  Instead, we use a new explicit `define_derived_metadata` API.

Fixes #825 and closes #829.

myronmarston added a commit that referenced this pull request Apr 22, 2014

Fix spec type inference.
- In #970 we added `infer_spec_type_from_file_location!` but forgot
  to remove the code in `lib/rspec/rails/configuration.rb` that
  made it always infer - so the opt-in API was there but it
  was always enabled. Here I've made it truly opt-in.
- The logic of `infer_spec_type_from_file_location!` also setup
  the helper module inclusion via explicit `:type` metadata but
  was only intended to setup the implicit mapping of spec file
  location to type.  Here I've made the module inclusion via
  `:type` always work w/o an explicit config option needed to
  enable it.
- We used to mutate the `:type` metadata on inclusion of the helper
  module, but this caused bugs such as #825. The problem is that
  rspec-core uses a raw hash for the metadata and doesn't re-apply
  filtering/inclusion logic when the metadata hash is mutated.
  Instead, we use a new explicit `define_derived_metadata` API.

Fixes #825 and closes #829.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment