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 exception raised by ActiveSupport Object#in? #1405

Merged
merged 1 commit into from Feb 5, 2021

Conversation

vsppedro
Copy link
Collaborator

Closes #1291

The part you quoted was the next thing I was going to say :) ActiveSupport is loaded whether you're explicitly using it or not (because both ActiveModel and ActiveRecord depend on it), but it's designed to be quasi-modular, so the exact file that provides .in? may not be required yet.

In any case, you've convinced me on the consistency argument. The only thing I'd like to see here is a test that exercises the scenario you're in so that we don't run into this problem again. We already some some high-level tests which set up an app and double-check that certain matchers are able to be used. We have a scenario which creates a Rails app and plugs into :active_record and :active_model, and we also have a non-Rails app scenario which only plugs into :active_model, but what we don't have is a scenario for a non-Rails app which plugs into :active_record and :active_model. So, what if you did this:

* Comment out the change you made.

* Add a new file `spec/support/acceptance/helpers/active_record_helpers.rb` that's similar to [`active_model_helpers`](https://github.com/thoughtbot/shoulda-matchers/blob/master/spec/support/acceptance/helpers/active_model_helpers.rb) but adds an `active_record_version` helper method.

* Require and include the new module in [this helpers file](https://github.com/thoughtbot/shoulda-matchers/blob/master/spec/support/acceptance/helpers/active_model_helpers.rb).

* Add a new [step helper](https://github.com/thoughtbot/shoulda-matchers/blob/master/spec/support/acceptance/helpers/step_helpers.rb#L15) called `create_active_record_project` that is similar to `create_active_model_project` but also adds the active_record gem in addition to active_model.

* Make a new file in `spec/acceptance` that's like `multiple_libraries_integration_spec.rb`, only instead of `create_rails_application` on [line 5](https://github.com/thoughtbot/shoulda-matchers/blob/master/spec/acceptance/multiple_libraries_integration_spec.rb#L5) you use your new `create_active_record_application` helper.

* Run the new test with: `bundle exec appraisal rails_6_0 rspec spec/acceptance/name_of_your_file_spec.rb`. Watch it fail with the same error you posted.

* Uncomment your change from earlier and re-run the test. It should now pass.

Thanks for this comment on the PR #1291, @mcmire! Helped a lot.

@vsppedro
Copy link
Collaborator Author

Oops, it seems that the command that I used to run the migration in this new acceptance test is not working when the Rails version is below 5_2.

I'll take a look at that ASAP.

@vsppedro vsppedro force-pushed the fix/exception-raised-by-active_support-object_in branch 3 times, most recently from 6b08b2a to 158f6d5 Compare January 29, 2021 02:07
Copy link
Collaborator

@mcmire mcmire left a comment

Choose a reason for hiding this comment

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

This looks good, nice job figuring this out! Aside from the test-related thing you noticed, I just had two suggestions.

write_file 'spec/spec_helper.rb', <<-FILE
require 'shoulda/matchers'

RSpec.configure do |config|
Copy link
Collaborator

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 add includes for the right libraries) to set this up. What do you think about changing this to:

Shoulda::Matchers.configure do |config|
  config.integrate do |with|
    with.library :active_record
    with.test_framework :rspec
  end
end

Alternatively, when you call add_shoulda_matchers_to_project you can specify the configuration which will add the Shoulda::Matchers.configure line automatically, so what do you think about:

add_shoulda_matchers_to_project(
  libraries: [:active_record]
)

Copy link
Collaborator Author

@vsppedro vsppedro Jan 30, 2021

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 of spec_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.

if integrates_with_rspec?(test_framework)
files <<
if bundle.includes?('rspec-rails')
'spec/rails_helper.rb'
else
'spec/spec_helper.rb'
end
end

Values obtained in the file above using pry-byebug:

[1] pry(#<AcceptanceTests::AddsShouldaMatchersToProject>)> bundle
=> #<Tests::Bundle:0x0000555b8b6bf800 @already_updating=false, @fs=#<Tests::Filesystem:0x0000555b8b6bf7d8>>
[2] pry(#<AcceptanceTests::AddsShouldaMatchersToProject>)> bundle.includes?('rspec-rails')
=> true
[3] pry(#<AcceptanceTests::AddsShouldaMatchersToProject>)> ENV['BUNDLE_GEMFILE']
=> "<path-to>/shoulda-matchers/gemfiles/rails_6_0.gemfile"
[4] pry(#<AcceptanceTests::AddsShouldaMatchersToProject>)> 

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.

Copy link
Collaborator

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:

bundle exec appraisal rails_5_2 rspec ...

Copy link
Collaborator Author

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?

Copy link
Collaborator

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks! Merging!

spec/acceptance/active_record_integration_spec.rb Outdated Show resolved Hide resolved
@vsppedro vsppedro force-pushed the fix/exception-raised-by-active_support-object_in branch from 158f6d5 to 183ac64 Compare January 30, 2021 13:21
@vsppedro vsppedro merged commit cd71a95 into master Feb 5, 2021
@vsppedro vsppedro deleted the fix/exception-raised-by-active_support-object_in branch February 5, 2021 12:11
@vsppedro vsppedro mentioned this pull request Jun 4, 2021
@vsppedro vsppedro mentioned this pull request Jul 10, 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.

None yet

2 participants