-
-
Notifications
You must be signed in to change notification settings - Fork 910
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 exception raised by ActiveSupport Object#in? #1405
Merged
vsppedro
merged 1 commit into
master
from
fix/exception-raised-by-active_support-object_in
Feb 5, 2021
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,89 @@ | ||
require 'acceptance_spec_helper' | ||
|
||
describe 'shoulda-matchers integrates with active record' do | ||
before do | ||
create_active_record_project | ||
|
||
write_file 'Rakefile', <<-FILE | ||
require 'active_record' | ||
require 'sqlite3' | ||
|
||
namespace :db do | ||
desc 'Create the database' | ||
task :create do | ||
File.unlink 'test.sqlite3' if File.exist?('test.sqlite3') | ||
db = SQLite3::Database.new('test.sqlite3') | ||
db.execute("CREATE TABLE users (id integer)") | ||
db.execute("CREATE TABLE profiles (id integer, user_id integer)") | ||
end | ||
end | ||
FILE | ||
|
||
run_rake_tasks!('db:create') | ||
|
||
write_file 'lib/user.rb', <<-FILE | ||
require 'active_record' | ||
|
||
class User < ActiveRecord::Base | ||
end | ||
FILE | ||
|
||
write_file 'lib/profile.rb', <<-FILE | ||
require 'active_record' | ||
require 'user' | ||
|
||
class Profile < ActiveRecord::Base | ||
belongs_to :user | ||
validates_presence_of :user | ||
end | ||
FILE | ||
|
||
write_file 'spec/profile_spec.rb', <<-FILE | ||
require 'spec_helper' | ||
require 'profile' | ||
|
||
describe Profile, type: :model do | ||
it { should validate_presence_of(:user) } | ||
end | ||
FILE | ||
|
||
updating_bundle do | ||
add_rspec_to_project | ||
add_shoulda_matchers_to_project( | ||
manually: true, | ||
with_configuration: false, | ||
) | ||
|
||
write_file 'spec/spec_helper.rb', <<-FILE | ||
require 'active_record' | ||
require 'shoulda-matchers' | ||
|
||
RSpec.configure do |config| | ||
config.before(:suite) do | ||
ActiveRecord::Base.establish_connection(adapter: 'sqlite3', database: 'test.sqlite3') | ||
end | ||
end | ||
|
||
Shoulda::Matchers.configure do |config| | ||
config.integrate do |with| | ||
with.test_framework :rspec | ||
|
||
with.library :active_record | ||
with.library :active_model | ||
end | ||
end | ||
FILE | ||
end | ||
end | ||
|
||
context 'when using both active_record and active_model libraries' do | ||
it 'allows the use of matchers from both libraries' do | ||
result = run_rspec_tests('spec/profile_spec.rb') | ||
|
||
expect(result).to have_output('1 example, 0 failures') | ||
expect(result).to have_output( | ||
'is expected to validate that :user cannot be empty/falsy', | ||
) | ||
end | ||
end | ||
end |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
require_relative 'gem_helpers' | ||
|
||
module AcceptanceTests | ||
module ActiveRecordHelpers | ||
include GemHelpers | ||
|
||
def active_record_version | ||
bundle_version_of('activerecord') | ||
end | ||
end | ||
end |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
I feel like if people are wanting to integrate with ActiveRecord, that we should support/document using
Shoulda::Matchers.configure
(which will addinclude
s for the right libraries) to set this up. What do you think about changing this to:Alternatively, when you call
add_shoulda_matchers_to_project
you can specify the configuration which will add theShoulda::Matchers.configure
line automatically, so what do you think about: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.
I agree.
I tried to make it work with the second option, but the problem is that the shoulda matches configuration is being added to the
rails_helper.rb
instead ofspec_helper.rb
.I think I found the problem, but I'm not sure how to fix it. I'll keep trying.
FWIW, the problem is that is finding
rspec-rails
in the Gemfile because it's checking on the wrong Gemfile - gemfiles/rails_6_0.shoulda-matchers/spec/support/acceptance/adds_shoulda_matchers_to_project.rb
Lines 101 to 108 in bf850fa
Values obtained in the file above using pry-byebug:
Correct me if I'm wrong, please, does the value of BUNDLE_GEMFILE need to be equal to/ tmp / shoulda-matchers-accept / test-project / Gemfile
at that point in the code, or not?EDIT: I tried to change the environment variable, but it did not work. I'll keep looking.
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.
In a "normal" app, yes, it would make sense for BUNDLE_GEMFILE to be the same as the application's Gemfile. For the tests we do a sort of hack where we force it to be the Appraisal gemfile instead. The reason we do that is because we want the Appraisal to dictate the list of gems that are loaded, as that is something we can control. As far as how BUNDLE_GEMFILE gets set, that comes from Appraisal. The 6.0 gemfile is the default because that's the latest one, but you You can override the Gemfile being used by using the
appraisal
command: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.
I get it. Thanks!
I am thinking of using the first approach you suggested. I couldn't get it to work with the second.
What do you think?
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.
Oh that's fine to use the first option! I didn't realize it only added to
rails_helper
.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.
Thanks! Merging!