-
-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Fix WrongScopeError
being thrown on Rails master:
#2215
Conversation
Appears the fix triggered an existing rubocop error. |
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.
A minor change suggested, and is there a test that fails without this?
5bfbfff
to
12b0e5d
Compare
Thanks for the quick review. I made the suggested changes |
lib/rspec/rails/fixture_support.rb
Outdated
@@ -50,6 +50,12 @@ def self.proxy_method_warning_if_called_in_before_context_scope(method_name) | |||
end | |||
end | |||
|
|||
if ::ActiveRecord.version >= Gem::Version.new('6.1.0') |
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.
Sorry you can't use Gem::Version
, as we don't have a dependency on rubygems being available. A simple
if ::ActiveRecord.version >= Gem::Version.new('6.1.0') | |
if Rails.version.to_f > 6.0 |
is preferred.
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.
Rubygem is part of the standard library since ruby 1.9. I can use Rails.version.to_f
if you really want to but I don't see any real reason
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.
We don't load any part of the standard library unless 100% necessary as it causes environment poisoning (where someone might not have required the right file but we do, tests pass, but it fails outside of tests). There is also a speed benefit to not using rubygems which we ourselves utilise in our tests by disabling it (which gives us the speed benefit and tests we're not poisoning the env).
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.
e.g Its simpler to not use it so please don't :)
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.
Ok I'll use Rails.version.to_f
, thanks for quick answer :)
Not trying to argue on such a small detail but Rails.version
already loads rubygem 🤷♂ . But anyway having the syntax you proposed will make things consistent since that's what it's used in this library.
@@ -13,5 +13,15 @@ module RSpec::Rails | |||
expect(group).to respond_to(:fixture_path=) | |||
end | |||
end | |||
|
|||
it "doesn't raise a WrongScopeError on Rails 6.1" do | |||
allow(ActiveRecord).to receive(:version) { Gem::Version.new('6.1.0') } |
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.
Proposed change removes this necessity
If you rebase this the rubocop build should be fixed |
@Edouard-chin - Looking forward to this fixed version. |
+1 |
a428956
to
4f980ff
Compare
Rails::VERSION::STRING.to_f |
4f980ff
to
0507fc2
Compare
Not too sure what's up with the jruby builds, otherwise it's 🍏 |
Thanks @pirj. I just reran the 2 builds, the issue on json seems to be fixed now. |
@Edouard-chin thanks a lot for the last commit. Do you think you can handle @JonRowe review? Thanks :) |
Sure happy to, but which comment did I not address ? If you are referring to #2215 (comment) I added a test to load fixtures |
Sorry @Edouard-chin. I just noticed. @JonRowe are you ok with the last changes? Cheers :) |
end | ||
|
||
expect { group.new.name }.to_not raise_error | ||
end |
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.
This test can be removed, the other 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.
Nearly there, thanks for working on this!
a187433
to
921e67d
Compare
Rebased and CI is 🍏 . Thanks for your patience |
Fix `WrongScopeError` being thrown on Rails master:
Related errors below. == Test WrongScopeError Error: ``` Failures: 1) Posts API (Custom Controller) GET /posts returns a list of posts Failure/Error: raise WrongScopeError, "`#{name}` is not available from within an example (e.g. an " \ "`it` block) or from constructs that run in the scope of an " \ "example (e.g. `before`, `let`, etc). It is only available " \ "on an example group (e.g. a `describe` or `context` block)." `name` is not available from within an example (e.g. an `it` block) or from constructs that run in the scope of an example (e.g. `before`, `let`, etc). It is only available on an example group (e.g. a `describe` or `context` block). # /Users/alexkitchens/.rbenv/versions/2.6.1/lib/ruby/gems/2.6.0/gems/rspec-core-3.8.0/lib/rspec/core/example_group.rb:742:in `method_missing' # /Users/alexkitchens/fun/journey/dependencies/rails/actionpack/lib/action_dispatch/testing/assertions/routing.rb:188:in `method_missing' # /Users/alexkitchens/fun/journey/dependencies/rails/actionpack/lib/action_dispatch/testing/integration.rb:422:in `method_missing' # /Users/alexkitchens/fun/journey/dependencies/rails/activerecord/lib/active_record/test_fixtures.rb:101:in `run_in_transaction?' # /Users/alexkitchens/fun/journey/dependencies/rails/activerecord/lib/active_record/test_fixtures.rb:115:in `setup_fixtures' # /Users/alexkitchens/fun/journey/dependencies/rails/activerecord/lib/active_record/test_fixtures.rb:8:in `before_setup' # /Users/alexkitchens/fun/journey/dependencies/rails/actionpack/lib/action_dispatch/testing/integration.rb:331:in `before_setup' # /Users/alexkitchens/.rbenv/versions/2.6.1/lib/ruby/gems/2.6.0/gems/rspec-rails-3.8.2/lib/rspec/rails/adapters.rb:126:in `block (2 levels) in <module:MinitestLifecycleAdapter>' # /Users/alexkitchens/.rbenv/versions/2.6.1/lib/ruby/gems/2.6.0/gems/rspec-core-3.8.0/lib/rspec/core/example.rb:447:in `instance_exec' # /Users/alexkitchens/.rbenv/versions/2.6.1/lib/ruby/gems/2.6.0/gems/rspec-core-3.8.0/lib/rspec/core/example.rb:447:in `instance_exec' # /Users/alexkitchens/.rbenv/versions/2.6.1/lib/ruby/gems/2.6.0/gems/rspec-core-3.8.0/lib/rspec/core/hooks.rb:373:in `execute_with' # # Showing full backtrace because every line was filtered out. # See docs for RSpec::Configuration#backtrace_exclusion_patterns and # RSpec::Configuration#backtrace_inclusion_patterns for more information. ``` Bisect led to d4367eb72601f6e5bba3206443e9d141e9753af8 Issue opened here: rails/rails#37783 Fix in Rspec Rails here: rspec/rspec-rails#2215 The fix is not in a full release yet, so I've pinned to the latest beta release. ``` commit d4367eb72601f6e5bba3206443e9d141e9753af8 Author: Edouard CHIN <[email protected]> Date: Thu Nov 21 18:39:54 2019 +0100 Modify ActiveRecord::TestFixtures to not rely on AS::TestCase: - ### Problem If one wants to use ActiveRecord::TestFixtures it is mandatory for the test suite to inherit from `ActiveSupport::TestCase`. TestFixtures makes use of specific method from AS::TestCase (`file_fixture_path` and `method_name`). ### Solution This PR fixes that by not making use of method_name and file_fixture_path. ``` == Time Issue Error: ``` Failure/Error: require File.expand_path('../../config/environment', __FILE__) NoMethodError: undefined method `hour' for 1:Integer Run options: include {:locations=>{"./spec/requests/api/posts_spec.rb"=>[10]}} ``` Bisect led to commit 12959dee0f19a8e050dd1236b031c2c690729905 Issue open at rails/rails#37391
Rails 6.1 requires an upgrade to rspec-rails 4.0.0 which fixes a WrongScopeError: rspec/rspec-rails#2215 Rspec-rails 4.0.0 dropped support for Rails below 5.0, so Rails 4 has been removed from the appraisals.
Rails 6.1 requires an upgrade to rspec-rails 4.0.0 which fixes a WrongScopeError: rspec/rspec-rails#2215 Rspec-rails 4.0.0 dropped support for Rails below 5.0, so Rails 4 has been removed from the appraisals.
Rails 6.1 requires an upgrade to rspec-rails 4.0.0 which fixes a WrongScopeError: rspec/rspec-rails#2215 Rspec-rails 4.0.0 dropped support for Rails below 5.0, so Rails 4 has been removed from the appraisals.
Rails 6.1 requires an upgrade to rspec-rails 4.0.0 which fixes a WrongScopeError: rspec/rspec-rails#2215 Rspec-rails 4.0.0 dropped support for Rails below 5.0, so Rails 4 has been removed from the appraisals.
It seems like rails 6.1 broke rspec <4.0 until it was fixed with rspec/rspec-rails#2215 then fixed differently with rspec/rspec-rails#2461 so we skip rspec 3x with rails 6.1 in CI
It seems like rails 6.1 broke rspec <4.0 until it was fixed with rspec/rspec-rails#2215 then fixed differently with rspec/rspec-rails#2461 so we skip rspec 3x with rails 6.1 in CI
It seems like rails 6.1 broke rspec-rails. RSpec-rails 4.0 fixed it with rspec/rspec-rails#2215 then fixed differently with rspec/rspec-rails#2461 so we skip rspec-rails >4 with rails 6.1 in CI
It seems like rails 6.1 broke rspec-rails. RSpec-rails 4.0 fixed it with rspec/rspec-rails#2215 then fixed differently with rspec/rspec-rails#2461 so we skip rspec-rails >4 with rails 6.1 in CI
It seems like rails 6.1 broke rspec-rails. RSpec-rails 4.0 fixed it with rspec/rspec-rails#2215 then fixed differently with rspec/rspec-rails#2461 so we skip rspec-rails >4 with rails 6.1 in CI
It seems like rails 6.1 broke rspec-rails. RSpec-rails 4.0 fixed it with rspec/rspec-rails#2215 then fixed differently with rspec/rspec-rails#2461 so we skip rspec-rails >4 with rails 6.1 in CI
It seems like rails 6.1 broke rspec-rails. RSpec-rails 4.0 fixed it with rspec/rspec-rails#2215 then fixed differently with rspec/rspec-rails#2461 so we skip rspec-rails >4 with rails 6.1 in CI
It seems like rails 6.1 broke rspec-rails. RSpec-rails 4.0 fixed it with rspec/rspec-rails#2215 then fixed differently with rspec/rspec-rails#2461 so we skip rspec-rails >4 with rails 6.1 in CI
Some of the missing combinations in the matrix: * Ruby 3 with Rails 5.2 are skipped because my tests error when trying to start rails * Rspec-rails 3 with Rails 6.1 because of a rails change rspec-rails needs rspec/rspec-rails#2215 or the more recent rspec/rspec-rails#2461
Some of the missing combinations in the matrix: * Ruby 3 with Rails 5.2 are skipped because my tests error when trying to start rails * Rspec-rails 3 with Rails 6.1 because of a rails change rspec-rails needs rspec/rspec-rails#2215 or the more recent rspec/rspec-rails#2461
Some of the missing combinations in the matrix: * Ruby 3 with Rails 5.2 are skipped because my tests error when trying to start rails * Rspec-rails 3 with Rails 6.1 because of a rails change rspec-rails needs rspec/rspec-rails#2215 or the more recent rspec/rspec-rails#2461
There's an issue with Rails 6.1 that caused error `WrongScopeError`, which was only fixed in rspec-rails 4.0 by rspec#2215.
Fix
WrongScopeError
being thrown on Rails master:Problem
Rails made a change in rails/rails@d4367eb
and TestFixtures don't make use of
method_name
anymore, but simplyname
.name
on RSpec raises aWrongScopeError
when called within anexample.
This patch overrides
name
to return the example name instead.