-
-
Notifications
You must be signed in to change notification settings - Fork 1k
Fix WrongScopeError #2423
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 #2423
Conversation
e0232e9
to
1ccfe5c
Compare
This comment has been minimized.
This comment has been minimized.
This is a standalone reproduction that can be run in a rails app generated with
|
I will try to look at it if I have time around Christmas. |
When people are using To reproduce the same behavior as in #2417. I had to de-comment I think the patch should be validated with another specs, closer to file_fixtures. I will keep searching for another proposal for your patch @pirj, if you have any idea. Feel free. :) We could also change the no-ar-app but I think we may simply lack of specs on file_fixtures? To reproduce the issue easily: require "bundler/inline"
gemfile(true) do
source "https://rubygems.org"
git_source(:github) { |repo| "https://github.com/#{repo}.git" }
gem "rails", "~> 6.1.0"
gem "rspec-rails", "4.0.1"
gem "sqlite3"
end
require "active_record/railtie"
require "logger"
require 'rspec/autorun'
require 'rspec/rails'
class Command
end
RSpec.configure do |config|
config.use_active_record = false
end
RSpec.describe Command do
it 'foo' do
Command.new
end
end The commit responsible of the change is this one. rails/rails@d4367eb |
Awesome find, @benoittgt ! |
Hello @pirj
Yes, I can confirm it is working. I am wondering if adding a new rails app isn't too much. 🤔 Trying to write a non regressive test similar to rails/rails@d4367eb#diff-76f5025b3bc6be04d08bb7df8f22ee2f0c69afe58aa4ba4933e6d6b9c328dc21R35-R38 could be an idea? What do you think? |
I'd like us to be able to run snippets like @benoittgt's above within our build, I think they are easier to grok than the whole app builds for one spec; we shouldn't get ride of the app builds, they are important to demonstrate working generators etc but for one off issues like this it's easier if we can do |
I love the idea. I wrote something similar recently : The spec: https://github.com/WeTransfer/format_parser/blob/master/spec/active_storage/rails_app_spec.rb Do you think about something similar? If yes I can import similar code to rspec-rails code base. |
Curious about your point of view on my last comment. For the initial PR from @pirj I think we should try to at least add a spec on fixture_support ? |
Awesome idea, @benoittgt ! If we reuse already installed gems and not re-download them, that should also work pretty quickly. |
6486fa6
to
6167064
Compare
@JonRowe @benoittgt Implemented the concept of testing snippets, added them to the build. To test locally:
(fails, without the fix for
|
Snippets seem to be reusing Dir.chdir('snippets') do
Dir['*.rb'].each do |snippet|
sh "echo Running #{snippet}"
sh "ruby #{snippet}"
end
end failed with:
Adding an It also seems that |
Great proposal. I love the idea to easily test behavior separately. I had the similar issue with the |
4ab959b
to
09dc769
Compare
I push a small change. @pirj fill free to remove/squash it if you have better ideas. 😊 |
Rakefile
Outdated
task :snippets do | ||
sh "echo Running snippets" | ||
sh <<-SH | ||
ruby snippets/use_active_record_false.rb |
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.
Does this definitely fail if it er... fails?
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.
It does, yes.
snippets/use_active_record_false.rb
Outdated
require "bundler/inline" | ||
|
||
gemfile(true) do | ||
source "https://rubygems.org" | ||
|
||
git_source(:github) { |repo| "https://github.com/#{repo}.git" } | ||
|
||
# Those Gemfiles carefully pick the right versions depending on | ||
# settings in the ENV, `.rails-version` and `maintenance-branch`. | ||
eval_gemfile './Gemfile-sqlite-dependencies' | ||
eval_gemfile './Gemfile-rspec-dependencies' | ||
eval_gemfile './Gemfile-rails-dependencies' | ||
|
||
gem "ammeter" | ||
gem "rspec-rails", path: "./" | ||
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.
Do we want to eventually extract this to snipets/gemfile.rb
or similar? And require that?
@benoittgt Thanks a lot for fixing this, it seems to work! There was a glitch,
but it resolved after:
It seems that if any of the dependencies with flexible version constraints update, we have to keep the repo's I'll play around with it, maybe something like require 'bundle/setup' helps. |
Fixes #2417
Related: #2215, rails/rails#37770
To test locally:
(pass)
(fails, without the fix for
WrongScopeError
)See #2417 (comment) for an explanation of at least one of the possible causes