Skip to content
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

Add RSpec.current_scope method to replace currently_executing_a_context_hook? and self.inspect hack #2895

Merged
merged 1 commit into from
Aug 14, 2021

Conversation

odinhb
Copy link
Contributor

@odinhb odinhb commented Jun 15, 2021

fixes #2893

lib/rspec/core/example.rb Outdated Show resolved Hide resolved
spec/rspec/core_spec.rb Outdated Show resolved Hide resolved
@pirj
Copy link
Member

pirj commented Jun 15, 2021

Nice.
Actually, now when I look at it, it seems to me there is yet another scope, but I don't have a good term to describe it.

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?

lib/rspec/core/example_group.rb Outdated Show resolved Hide resolved
lib/rspec/core.rb Outdated Show resolved Hide resolved
@odinhb odinhb force-pushed the current_scope branch 2 times, most recently from d783bfc to dce099d Compare June 20, 2021 11:35
lib/rspec/core.rb Outdated Show resolved Hide resolved
Copy link
Member

@JonRowe JonRowe left a 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

lib/rspec/core/example.rb Outdated Show resolved Hide resolved
lib/rspec/core.rb Outdated Show resolved Hide resolved
lib/rspec/core.rb Outdated Show resolved Hide resolved
lib/rspec/core/example.rb Outdated Show resolved Hide resolved
lib/rspec/core/example_group.rb Outdated Show resolved Hide resolved
spec/rspec/core_spec.rb Outdated Show resolved Hide resolved
spec/rspec/core_spec.rb Outdated Show resolved Hide resolved
spec/rspec/core_spec.rb Outdated Show resolved Hide resolved
spec/rspec/core_spec.rb Outdated Show resolved Hide resolved
spec/rspec/core_spec.rb Outdated Show resolved Hide resolved
@odinhb odinhb marked this pull request as ready for review June 20, 2021 19:25
@odinhb
Copy link
Contributor Author

odinhb commented Jun 20, 2021

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.

@odinhb
Copy link
Contributor Author

odinhb commented Jun 20, 2021

Did another iteration on this and I'm starting to feel satisfied with the implementation. Let me know if I missed anything else.

@odinhb
Copy link
Contributor Author

odinhb commented Jun 20, 2021

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.

spec/rspec/core_spec.rb Outdated Show resolved Hide resolved
spec/rspec/core_spec.rb Outdated Show resolved Hide resolved
spec/rspec/core_spec.rb Outdated Show resolved Hide resolved
Copy link
Member

@pirj pirj left a 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.

features/metadata/current_scope.feature Outdated Show resolved Hide resolved
lib/rspec/core.rb Outdated Show resolved Hide resolved
lib/rspec/core.rb Outdated Show resolved Hide resolved
lib/rspec/core.rb Outdated Show resolved Hide resolved
lib/rspec/core/example_group.rb Outdated Show resolved Hide resolved
spec/rspec/core_spec.rb Outdated Show resolved Hide resolved
spec/rspec/core_spec.rb Outdated Show resolved Hide resolved
@pirj

This comment has been minimized.

@pirj
Copy link
Member

pirj commented Jun 21, 2021

Ruby 2.7 on Windows failure is unrelated, fails in other builds, too:

An error occurred while loading spec_helper.
Failure/Error: require 'aruba/api'

TypeError:
  unable to resolve type 'intptr_t'
# ./vendor/bundle/ruby/2.7.0/gems/ffi-1.12.2-x64-mingw32/lib/ffi/types.rb:69:in `find_type'

ruby-head fails with RuboCop/psych arity mismatch, seems completely unrelated:

bin/rubocop lib
wrong number of arguments (given 5, expected 1)
/home/runner/.rubies/ruby-head/lib/ruby/3.1.0/psych.rb:323:in `safe_load'

PS JRuby 1.7 failure is unrelated, fails in other builds as well.

Otherwise looks green. @JonRowe, what do you think?

@pirj pirj force-pushed the current_scope branch 2 times, most recently from d28f76c to b47d954 Compare June 22, 2021 10:45
Changelog.md Outdated Show resolved Hide resolved
features/metadata/current_scope.feature Outdated Show resolved Hide resolved
lib/rspec/core.rb Outdated Show resolved Hide resolved
features/metadata/current_scope.feature Outdated Show resolved Hide resolved
@odinhb odinhb force-pushed the current_scope branch 2 times, most recently from 55fb304 to 789d48b Compare June 24, 2021 16:07
@odinhb
Copy link
Contributor Author

odinhb commented Jun 24, 2021

Is there anything else you want me to do?

Should I fixup the fixup! Adjust feature title commit into Update features/metadata/current_scope.feature?

@pirj
Copy link
Member

pirj commented Jun 24, 2021

@odinhb No, it's all good. Thanks a lot for your contribution. 🙌

@JonRowe Do you want to take another look?

@pirj
Copy link
Member

pirj commented Jul 16, 2021

@odinhb CI is green. Would you like to send a PR against rspec-rails's main that would make use of this new method?
main tracks the ongoing development of 5.x of rspec-rails, and it's set to depend on 4.x rspec-core that would include this new method.
I suggest you to tweak those dependency declarations to point to this PR's branch.

That might be the last drop needed to convince @JonRowe

Good job!

@odinhb
Copy link
Contributor Author

odinhb commented Jul 16, 2021

@odinhb CI is green. Would you like to send a PR against rspec-rails's main that would make use of this new method?

I can do that. I can't start until after the weekend however.

@pirj
Copy link
Member

pirj commented Jul 16, 2021

until after the weekend however

This is fine.
I was going to wrap up a few remaining things for 3.99 this weekend, and it may take a while to code review and bring them to a desired shape. There's still time before 4.0 is cut.

@pirj pirj requested a review from JonRowe July 18, 2021 14:13
Copy link
Member

@pirj pirj left a 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.

lib/rspec/core.rb Outdated Show resolved Hide resolved
Copy link
Member

@JonRowe JonRowe left a 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

features/metadata/current_scope.feature Outdated Show resolved Hide resolved
@pirj pirj requested a review from JonRowe August 14, 2021 14:56
@pirj pirj dismissed JonRowe’s stale review August 14, 2021 14:57

Proposed changes accepted. Thanks!

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)`
@pirj
Copy link
Member

pirj commented Aug 14, 2021

All green. Since there were no objections to the approach, and all amendments were considered, I'll go ahead and merge.
I intend to cherry-pick this to 4-0-dev and also remove currently_executing_a_context_hook?. For 3.99 a deprecation warning has been added.

@odinhb Thanks for the contribution!

@pirj pirj merged commit 07db8f8 into rspec:main Aug 14, 2021
pirj added a commit that referenced this pull request Aug 14, 2021
Add RSpec.current_scope method to replace `currently_executing_a_context_hook?` and `self.inspect` hack
yujinakayama pushed a commit to yujinakayama/rspec-monorepo that referenced this pull request Oct 6, 2021
…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.
yujinakayama pushed a commit to yujinakayama/rspec-monorepo that referenced this pull request Oct 19, 2021
…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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Determining the rspec context in which a helper method is run
3 participants