-
-
Notifications
You must be signed in to change notification settings - Fork 753
Add RSpec.current_scope method to replace currently_executing_a_context_hook?
and self.inspect
hack
#2895
Conversation
Nice. RSpec.describe Thing, :type => :model do
# scope: example_group, context: example group class
fixtures :things
things(:one) # BOOM
let_it_be(:stuff) { ... }
stuff # BOOM
before(:context) do
# scope: example_group_hook, context: example group instance?
things(:one) # BOOM
stuff # BOOM
end
it "fixture method defined" do
# scope: example, context: example group instance
# same as inside `before(:example)` for the needs of this PR
things(:one) # OK
stuff # OK
end
end We need to differentiate between the latter two, is that correct? |
d783bfc
to
dce099d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The correct names for hooks are :example
:context
and :suite
, they are aliased to :each
and :all
but are not the main names, so that should reflect that.
I also feel we shouldn't be restoring previous states but explicitly setting the state we are moving to, otherwise a bug will persist through that stack
Fixed and fixed. Turns out the prev_scope thing was unnecessary anyway. |
Did another iteration on this and I'm starting to feel satisfied with the implementation. Let me know if I missed anything else. |
I'm having some real trouble with rubocop here. Pipeline says "74 files inspected, 1 offense detected", but my local install says "74 files inspected, 94 offenses detected". I can't figure out the difference between our setups. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great!
A couple of cosmetic notes.
This comment has been minimized.
This comment has been minimized.
Ruby 2.7 on Windows failure is unrelated, fails in other builds, too:
PS JRuby 1.7 failure is unrelated, fails in other builds as well. Otherwise looks green. @JonRowe, what do you think? |
d28f76c
to
b47d954
Compare
55fb304
to
789d48b
Compare
Is there anything else you want me to do? Should I fixup the |
@odinhb CI is green. Would you like to send a PR against That might be the last drop needed to convince @JonRowe Good job! |
I can do that. I can't start until after the weekend however. |
This is fine. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
@JonRowe This is the only open PR I can think of that it makes sense to merge before we proceed with 4.0 release plan.
It was introduced in rspec/rspec-core#2895
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some documentation changes mainly, and a request to remove a deprecation
My reasoning is: rspec-rails needs to do the following condition: `if RSpec.current_scope == :before_context_hook` test_prof needs to do the following condition: `if RSpec.current_scope == :before_context_hook` amd my little helper needs the following `unless [:before_example_hook, :example].include?(RSpec.current_scope)`
All green. Since there were no objections to the approach, and all amendments were considered, I'll go ahead and merge. @odinhb Thanks for the contribution! |
Add RSpec.current_scope method to replace `currently_executing_a_context_hook?` and `self.inspect` hack
…cope Add RSpec.current_scope method to replace `currently_executing_a_context_hook?` and `self.inspect` hack --- This commit was imported from rspec/rspec-core@07db8f8.
…cope Add RSpec.current_scope method to replace `currently_executing_a_context_hook?` and `self.inspect` hack --- This commit was imported from rspec/rspec-core@fe497cc.
fixes #2893