Warn if a fixture method is called from a before(:context) block.
#1501
Conversation
| def self.proxy_method_warning_if_called_in_before_context_scope(method_name) | ||
| orig_implementation = instance_method(method_name) | ||
| define_method(method_name) do |*args, &blk| | ||
| if self.inspect.include?("before(:context)") |
There was a problem hiding this comment.
Is this really the only way to detect this?
There was a problem hiding this comment.
I could not think of a better one after staring at the source... maybe @myronmarston has thoughts?
There was a problem hiding this comment.
Yeah, this is pretty gross and brittle. Also, I think we should also raise errors in after(:context) hooks, right?
Maybe we can make a change to rspec-core so this can be:
if RSpec::Core::Hooks::ContextHook === self
# ...
endThere was a problem hiding this comment.
Actually, if we're going to switch on type like that...could we just include a module with an appropriate method definition in a class like RSpec::Core::Hooks::ContextHook? Seems much more straightforward.
There was a problem hiding this comment.
OK. It seems like the resolution here is to patch something into RSpec core. I'll open an issue over there.
|
Overall this LGTM (I'd rather we had a better way of knowing we were in a hook but if thats the only way then c'est la vie) but the rubocop has a few words to say :) |
|
blocked on rspec/rspec-core#2130 |
This is useful because at the moment the only way to determine if an example is currently executing a context hook is to look at it's inspect string. Here, we add a simple boolean flag which is triggered in before and after context hook execution. This is needed by rspec/rspec-rails#1501.
fables-tales
force-pushed
the
samphippen/bugfix/1442
branch
from
9d0738f
to
f6f6039
Compare
This is useful because at the moment the only way to determine if an example is currently executing a context hook is to look at it's inspect string. Here, we add a simple boolean flag which is triggered in before and after context hook execution. This is needed by rspec/rspec-rails#1501.
| new_methods.each do |method_name| | ||
| proxy_method_warning_if_called_in_before_context_scope(method_name) | ||
| end | ||
| end |
There was a problem hiding this comment.
Could we make this a name module (I'm ok with dynamically creating it) so that it's clear from the ancestor chain we've inserted something?
|
We need to take into account possible |
|
Can you push a failing test that demonstrates that ? Sent from my phone please excuse my brevity.
|
This definitely works for the :all case... we're just capturing all the added methods |
This is useful because at the moment the only way to determine if an example is currently executing a context hook is to look at it's inspect string. Here, we add a simple boolean flag which is triggered in before and after context hook execution. This is needed by rspec/rspec-rails#1501.
|
This just needs @cupakromer's final change made right? |
|
@JonRowe I think I understand enough to deal with this myself... I'll get to it a little later today if @cupakromer can't. He's a busy dude and so I'll try to not create any more work for him if I don't need to ^_^ |
|
Nah I mean his final suggested change ;) |
|
@JonRowe totally. I'm planning on getting to that sometime today. |
|
👍 :) |
fables-tales
force-pushed
the
samphippen/bugfix/1442
branch
from
f6f6039
to
33aa831
Compare
|
@cupakromer @JonRowe done the module and the global_fixtures. Can I get a review one more time :) ? |
fables-tales
force-pushed
the
samphippen/bugfix/1442
branch
from
e49f830
to
33304dd
Compare
|
@JonRowe @cupakromer I need an opinion here: Using the module has broken on rails 3.0.20 only because it doesn't use a module, but instead redefines methods on the object proper. We could:
I personally prefer 1 because it will be less code, but makes it slightly harder to debug. Thoughts please. |
Fixes #1442. The basic approach here is to capture the addition of the fixture methods to the example group instance and then monkeypatch them. The monkeypatch checks to see if we're currently in a before(:context) hook and if we are then it prints a warning and doesn't invoke the method. The warning here is a little sparse at the moment, and I'd like to make it more clear. One thing that's a little gross about this implementation is that it uses the inspect string of the example group to determine if we're in a before(:context) hook. As far as I can tell there isn't a better way to make that determination, but maybe someone's got a clever trick.
fables-tales
force-pushed
the
samphippen/bugfix/1442
branch
from
33304dd
to
7cb796d
Compare
Closes #1442 [ci skip]
|
@JonRowe @cupakromer this is finished. Can one of you give it one last once over. |
|
I'm happy with this given the reasoning above. |
Warn if a fixture method is called from a `before(:context)` block.
Closes rspec#1442 [ci skip]
This is useful because at the moment the only way to determine if an example is currently executing a context hook is to look at it's inspect string. Here, we add a simple boolean flag which is triggered in before and after context hook execution. This is needed by rspec/rspec-rails#1501.
Fixes #1442.
The basic approach here is to capture the addition of the fixture
methods to the example group instance and then monkeypatch them. The
monkeypatch checks to see if we're currently in a
before(:context)hookand if we are then it prints a warning and doesn't invoke the method.
The warning here is a little sparse at the moment, and I'd like to make
it more clear. One thing that's a little gross about this implementation
is that it uses the inspect string of the example group to determine if
we're in a
before(:context)hook. As far as I can tell there isn't abetter way to make that determination, but maybe someone's got a clever
trick.