Run before(:all) hooks for rails magic example groups #829

Closed
wants to merge 7 commits into from

3 participants

@JonRowe
RSpec member

We automagically configure ExampleGroup types for the various sets of Rails
helpers, however in 2.14 and master ATM these incorrectly pickup configured
helpers. E.g. a before(:all, :type => :model) will not run for a spec in
spec/models unless the type is set manually.

This is because rspec-core configures hooks based on their current metadata
and not on any metadata set later on. The alternative approach would be to
reload the global hooks from config when the metadata is changed in rspec-core
but this fixes #825 for now.

Note that manually specifying type is not affected, nor are any child groups
or examples. It is only the top level group that is affected.

Thoughts? /cc @alindeman @myronmarston

@myronmarston
RSpec member

Thanks for tracking this down, @JonRowe. A few thoughts:

  • I consider register_globals to be a private API to me. I would never recommend that extension gems use it. We should treat rspec-rails the same...so I'm not sure that this is the right way to fix it.
  • You mention that 2.14 has this behavior. Do you know if it's a regression in 2.14 (or some other recent version) or has this always been a problem? If it's a regression it would be good to know what PR or commit caused it (git bisect will help track that down).
  • It seems like there's a bug (or at least an inconsistency) in rspec-core: either rspec-core should support included modules mutating the metadata, and fully support that properly, or it shouldn't support that and should prevent or print warnings when included modules do that. Currently rspec-rails allows it (and I've always thought it was a bit odd...) but I don't know if anyone else does. How hard would it be to fix rspec-core so it properly supports it? One idea is to move the hooks.register_globals call out of set_it_up and into run, so that the hooks are registered at the last possible moment. If we could fix it that way, I think the changes in rspec-rails here would become unnecessary (although it's probably worth keeping the specs to prevent future regressions).
@myronmarston myronmarston and 1 other commented on an outdated diff Sep 30, 2013
lib/rspec/rails/example/rails_example_group.rb
@@ -10,6 +10,12 @@ module RailsExampleGroup
include RSpec::Rails::MinitestLifecycleAdapter if ::Rails::VERSION::STRING >= '4'
include RSpec::Rails::MinitestAssertionAdapter
include RSpec::Rails::Matchers
+
+ def set_metadata_type(type)
+ metadata[:type] = type
+ hooks.register_globals(self, RSpec.configuration.hooks)
+ end
@myronmarston
RSpec member

Looks like this isn't used anywhere? Looks like a useful abstraction, though; if we stick with this solution for some reason it would be good to leverage this. (Although I suspect the final fix will be in rspec-core).

@JonRowe
RSpec member
JonRowe added a note Sep 30, 2013

Oops yeah, I tried to DRY it up (couldn't get it to work cause of the included scoping)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@myronmarston myronmarston commented on the diff Sep 30, 2013
.../rspec/rails/example/controller_example_group_spec.rb
@@ -1,6 +1,6 @@
require "spec_helper"
-class ::ApplicationController
+class ::ApplicationController < ActionController::Base
@myronmarston
RSpec member

Why the change here?

@JonRowe
RSpec member
JonRowe added a note Sep 30, 2013

Because I run the group of specs to check that they work and the hooks are invoked. Controller specs require an actual controller to work. (And the build's still failing)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@myronmarston myronmarston and 1 other commented on an outdated diff Sep 30, 2013
spec/support/helpers.rb
@@ -16,5 +16,11 @@ def metadata_with(additional_metadata)
m
end
+ def with_isolated_config
+ config = RSpec.configuration
+ yield config.dup if block_given?
@myronmarston
RSpec member

dup only produces a shallow duplicate, which means that any mutation on referenced objects will not be isolated by this. To get a deep dup for full isolation you can use Marshal.load(Marshal.dump config)

Also, it's not clear to me how this even works: you are yielding a dup of config, but it's not what RSpec.configuration references, so it seems like within the block, mutations to the yielded config argument won't affect anything that tries to use RSpec.configuration.

@JonRowe
RSpec member
JonRowe added a note Sep 30, 2013

hmm, good point, I need to set the config. I'm trying not to pollute the global config with my before hooks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@JonRowe
RSpec member
  • I consider register_globals to be a private API to me. I would never recommend that extension gems use it. We should treat rspec-rails the same...so I'm not sure that this is the right way to fix it.

So do I, but this seemed the best way to start a discussion about it. The problem is that rspec-rails does something I wouldn't recommend already (magically mixing in metadata like this).

  • You mention that 2.14 has this behavior. Do you know if it's a regression in 2.14 (or some other recent version) or has this always been a problem? If it's a regression it would be good to know what PR or commit caused it (git bisect will help track that down).

I haven't tried, I don't remember us changing anything that would make it a regression, I will try at some point with 2.13.

  • It seems like there's a bug (or at least an inconsistency) in rspec-core: either rspec-core should support included modules mutating the metadata, and fully support that properly, or it shouldn't support that and should prevent or print warnings when included modules do that. Currently rspec-rails allows it (and I've always thought it was a bit odd...) but I don't know if anyone else does. How hard would it be to fix rspec-core so it properly supports it? One idea is to move the hooks.register_globals call out of set_it_up and into run, so that the hooks are registered at the last possible moment. If we could fix it that way, I think the changes in rspec-rails here would become unnecessary (although it's probably worth keeping the specs to prevent future regressions).

I think this is a "sort of" bug in core, this is why I initially started poking at that to try to fix this. One way to solve this would be to make the metadata to reload global hooks when it's mutated. Or move the hook setup to be lazy and only configure from the global configuration when run (which is what your suggestion is I guess)

@JonRowe JonRowe referenced this pull request in rspec/rspec-core Sep 30, 2013
Closed

Reload hooks when metadata changes #1089

@JonRowe
RSpec member

Had to update this so it fails correctly, it's awaiting rspec/rspec-core#1089 before it'll work.

@alindeman

Hi all--

What's the status on this PR and rspec/rspec-core#1089? Do we need it before 3.0? Can I help?

@JonRowe
RSpec member

So I'm still in favour of rspec/rspec-core#1089 but the alternative is we work something out that doesn't involve mutating metadata for rspec-rails integration

@myronmarston myronmarston added a commit that referenced this pull request Apr 21, 2014
@myronmarston myronmarston 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.
e135076
@myronmarston
RSpec member

Closing in favor of #1002.

@myronmarston myronmarston added a commit that referenced this pull request Apr 22, 2014
@myronmarston myronmarston 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.
0387ff2
@myronmarston myronmarston added a commit to rspec/rspec-core that referenced this pull request Dec 17, 2014
@myronmarston myronmarston Remove spec for behavior we will no longer be supporting.
Fully supporting metadata mutation is fraught with difficulties
see (rspec/rspec-rails#829 and the attempted fix in #1089).
For RSpec 3, we decided to expose `define_derived_metadata`
as an officially supported way to accomplish the sorts of
things metadata mutation was used for in RSpec 2. This spec
got left behind (since it was passing at the time, it wasn't
noticed) but isn’t something we want to support going forward
since module inclusion shouldn't mutate metadata. It’s also
getting in the way of a perf optimization I'm implementing.
620269e
@myronmarston myronmarston added a commit to rspec/rspec-core that referenced this pull request Dec 18, 2014
@myronmarston myronmarston Remove spec for behavior we will no longer be supporting.
Fully supporting metadata mutation is fraught with difficulties
see (rspec/rspec-rails#829 and the attempted fix in #1089).
For RSpec 3, we decided to expose `define_derived_metadata`
as an officially supported way to accomplish the sorts of
things metadata mutation was used for in RSpec 2. This spec
got left behind (since it was passing at the time, it wasn't
noticed) but isn’t something we want to support going forward
since module inclusion shouldn't mutate metadata. It’s also
getting in the way of a perf optimization I'm implementing.
9062f3a
@myronmarston myronmarston added a commit to rspec/rspec-core that referenced this pull request Dec 19, 2014
@myronmarston myronmarston Remove spec for behavior we will no longer be supporting.
Fully supporting metadata mutation is fraught with difficulties
see (rspec/rspec-rails#829 and the attempted fix in #1089).
For RSpec 3, we decided to expose `define_derived_metadata`
as an officially supported way to accomplish the sorts of
things metadata mutation was used for in RSpec 2. This spec
got left behind (since it was passing at the time, it wasn't
noticed) but isn’t something we want to support going forward
since module inclusion shouldn't mutate metadata. It’s also
getting in the way of a perf optimization I'm implementing.
fa61873
@cupakromer cupakromer deleted the run_request_before_all_hooks branch Dec 28, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment