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

Fix WrongScopeError #2423

Merged
merged 2 commits into from Jan 20, 2021
Merged

Fix WrongScopeError #2423

merged 2 commits into from Jan 20, 2021

Conversation

pirj
Copy link
Member

@pirj pirj commented Dec 22, 2020

Fixes #2417
Related: #2215, rails/rails#37770

To test locally:
(pass)

bundle exec rake snippets

(fails, without the fix for WrongScopeError)

git checkout HEAD~1
bundle exec rake snippets

See #2417 (comment) for an explanation of at least one of the possible causes

@pirj pirj self-assigned this Dec 22, 2020
@pirj

This comment has been minimized.

@JonRowe
Copy link
Member

JonRowe commented Dec 23, 2020

This is a standalone reproduction that can be run in a rails app generated with rails new --minimal

require File.expand_path('../config/environment', __dir__)
ENV['RAILS_ENV'] ||= 'test'
require 'rspec/rails'

RSpec.configure do |config|
  config.use_active_record = false
end

class MyClass; end

RSpec.describe MyClass do
  it 'foo' do
    MyClass.new
  end
end

@benoittgt
Copy link
Member

I will try to look at it if I have time around Christmas.

@pirj pirj added this to the 4.1 milestone Dec 24, 2020
@pirj pirj linked an issue Dec 28, 2020 that may be closed by this pull request
@benoittgt
Copy link
Member

benoittgt commented Dec 28, 2020

When people are using use_active_record = false they can have a rails app with still lot's of active_record loaded.

To reproduce the same behavior as in #2417. I had to de-comment require "active_record/railtie" in application.rb, then re-ad a database.yml then re-add sqlite in the Gemfile of the tmp/no-ar app.

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

@pirj
Copy link
Member Author

pirj commented Dec 28, 2020

Awesome find, @benoittgt !
I was stuck on adding a failing spec for that fix. Sounds like we could use yet another example_app_generator app, with AR (accidentally left) but use_active_record = false in our tests.
Did the fix above work for this reproduction snippet?

@benoittgt
Copy link
Member

Hello @pirj

Did the fix above work for this reproduction snippet?

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?

@JonRowe
Copy link
Member

JonRowe commented Dec 29, 2020

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 ruby some_file.rb to test something

@benoittgt
Copy link
Member

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
The rails app: https://github.com/WeTransfer/format_parser/blob/master/spec/integration/active_storage/rails_app.rb

Do you think about something similar? If yes I can import similar code to rspec-rails code base.

@benoittgt
Copy link
Member

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 ?

@pirj
Copy link
Member Author

pirj commented Jan 8, 2021

Awesome idea, @benoittgt ! If we reuse already installed gems and not re-download them, that should also work pretty quickly.
Not to say that otherwise our example_app_generator is getting pretty crowded.

@pirj
Copy link
Member Author

pirj commented Jan 14, 2021

@JonRowe @benoittgt Implemented the concept of testing snippets, added them to the build.

To test locally:
(pass)

bundle exec rake snippets

(fails, without the fix for WrongScopeError)

git checkout HEAD~1
bundle exec rake snippets

@pirj pirj marked this pull request as ready for review January 14, 2021 07:13
@pirj
Copy link
Member Author

pirj commented Jan 14, 2021

Snippets seem to be reusing spec/spec_helper.rb from our repo, and we don't want that.
A quick attempt to isolate it

   Dir.chdir('snippets') do
     Dir['*.rb'].each do |snippet|
       sh "echo Running #{snippet}"
       sh "ruby #{snippet}"
     end
    end

failed with:

[!] There was an error parsing `Gemfile`:
[!] There was an error parsing `Gemfile-rspec-dependencies`: No such file or directory @ rb_sysopen - /home/pirj/source/rspec-rails/snippets/maintenance-branch. Bundler cannot continue.

 #  from Gemfile-rspec-dependencies:1
 #  -------------------------------------------
 >  branch = File.read(File.expand_path("../maintenance-branch", __FILE__)).chomp

Adding an .rspec that doesn't --require spec_helper to snippets/ doesn't help, neither does adding a snippets/spec_helper.rb.

It also seems that sh ruby <snippet> from Rake suffers from the cached BUNDLE_GEMFILE issue, we have to reset it, otherwise, an attempt to load Gemfile is made. unset BUNDLE_GEMFILE doesn't help.

@benoittgt
Copy link
Member

Great proposal. I love the idea to easily test behavior separately.

I had the similar issue with the spec_helper but in my case it was related to ammeter and loading issue. I will try to play tonight with it if you didn't find a solution yet.

@benoittgt benoittgt force-pushed the fix-wrong-scope-error branch 2 times, most recently from 4ab959b to 09dc769 Compare January 14, 2021 22:17
@benoittgt
Copy link
Member

benoittgt commented Jan 14, 2021

I push a small change. I have some duplication on SQLite because we do not eval Gemfile. This could be moved somewhere I think, or in a dedicated file that we we could eval in both scenarios.

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does, yes.

Comment on lines 1 to 32
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
Copy link
Member

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?

@pirj
Copy link
Member Author

pirj commented Jan 15, 2021

@benoittgt Thanks a lot for fixing this, it seems to work!

There was a glitch,

/Users/pirj/.rvm/rubies/ruby-2.7.1/lib/ruby/2.7.0/bundler/runtime.rb:312:in `check_for_activated_spec!': You have already activated loofah 2.8.0, but your Gemfile requires loofah 2.9.0. Prepending `bundle exec` to your command may solve this. (Gem::LoadError)

but it resolved after:

$ bundle update --conservative loofah
Using loofah 2.9.0 (was 2.8.0)

It seems that if any of the dependencies with flexible version constraints update, we have to keep the repo's Gemfile.lock up to date to keep them in sync to avoid version mismatches.

I'll play around with it, maybe something like

require 'bundle/setup'

helps.

This was referenced Mar 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

WrongScopeError: name when use_active_record = false
3 participants