-
-
Notifications
You must be signed in to change notification settings - Fork 1k
Avoid loading active record fixtures #2869
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
base: main
Are you sure you want to change the base?
Avoid loading active record fixtures #2869
Conversation
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.
We have a couple of “test snippets” in the snippets/ directory related to the No-AR case.
Does any of them reflect your scenario? Can you create an additional one for your case?
There are quite some changes in the PR, and some of the docs you moved are fated (eg we have a non-ambiguous use_transactional_specs config setting now).
Can you narrow this down to cover just your case?
I’m away from my computer to see what broke on CI, most
certainly related to the recent Rails 8.1 release. Please ignore.
| require 'rails_helper' | ||
|
|
||
| RSpec.describe 'Example App', :use_fixtures, type: :model do | ||
| RSpec.describe 'Example App', type: :model do |
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 you revert this spec change, will the old spec still work?
If it won’t, why?
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'm not sure that using :use_fixtures changes anything at all.
Please look at this code:
rspec-rails/lib/rspec/rails/configuration.rb
Lines 73 to 82 in ef12dbc
| config.include RSpec::Rails::FixtureSupport, :use_fixtures | |
| # We'll need to create a deprecated module in order to properly report to | |
| # gems / projects which are relying on this being loaded globally. | |
| # | |
| # See rspec/rspec-rails#1355 for history | |
| # | |
| # @deprecated Include `RSpec::Rails::RailsExampleGroup` or | |
| # `RSpec::Rails::FixtureSupport` directly instead | |
| config.include RSpec::Rails::FixtureSupport |
It includes RSpec::Rails::FixtureSupport unconditionally, even if :use_fixtures is not provided.
So the purpose of this meta-tag is a little vague.
Thanks for the suggestion. I'll revert this change.
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.
The global config.include with metadata is a conditional include as far as I recall
|
|
||
| RSpec.describe 'Example App', :use_fixtures, type: :model do | ||
| RSpec.describe 'Example App', type: :model do | ||
| it "does not set up fixtures" do |
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.
This means “we don’t set up fixtures even if AR is loaded and there’s a metadata tag”, right? I hat changed?
| # If you're not using ActiveRecord, or you'd prefer not to run each of your | ||
| # examples within a transaction, remove the following line or assign false | ||
| # instead of true. | ||
| # If you'd prefer not to run each of your examples within a transaction, |
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.
Why remove the mention of AR here?
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.
Well, originally this statement is not quite correct, specifically the If you're not using ActiveRecord part.
The use_transactional_fixtures setting doesn't make any sense without the ActiveRecord, as it's a part of its configuration in specs. But you are right, it needs to mention the setting is related to the ActiveRecord.
Thanks
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'd prefer it if this was reverted, you should delete this setting if you're not using ActiveRecord, thats why it's written that way.
Will do, thanks.
Alright, I'll add some explicit usage notes for |
| config.use_transactional_fixtures = true | ||
| # You can uncomment this line to turn off ActiveRecord support entirely. | ||
| # You can uncomment this line to turn off ActiveRecord support entirely |
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.
Revert this change please.
| # If you're not using ActiveRecord, or you'd prefer not to run each of your | ||
| # examples within a transaction, remove the following line or assign false | ||
| # instead of true. | ||
| # If you'd prefer not to run each of your examples within a transaction, |
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'd prefer it if this was reverted, you should delete this setting if you're not using ActiveRecord, thats why it's written that way.
| @@ -0,0 +1,181 @@ | |||
| # Active Record | |||
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'd prefer it if you reverted this change, if you want to add to the documentation in another PR or in the original file thats fine, but this is really hard to review what if anything you've changed
| end | ||
| end | ||
| end | ||
| 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.
This needs to remain, see the documentation comment, it exists to test that stuff doesn't happen when the flag is set to false and the module is defined, this is because even though we can exclude active record, it often still exists due to dependencies, but hasn't been configured...
Even when
use_active_record = falseis set, rspec-rails still includedActiveRecord::TestFixturesand interceptedbeforeandafterhooks, attempting to access the databases associated with active record models. This caused issues in non-standard Rails projects where ActiveRecord is present but not using the Rails testing framework directly.This PR allows projects with ActiveRecord present to cleanly opt-out of fixture-based testing without database hook interference.
Changes
ActiveRecord::TestFixturesinclusion inside theuse_active_record?conditional checkActiveRecord.mdguide covering fixtures, transactions, and configuration options. MergedTransactions.mdto the new documentrails_helper.rbcomments to be aligned with updated documentation.