-
-
Notifications
You must be signed in to change notification settings - Fork 1k
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Fix WrongScopeError #2423
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