-
Notifications
You must be signed in to change notification settings - Fork 47
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
Split spec/support/active_record_classes.rb
#342
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #342 +/- ##
=======================================
Coverage 90.22% 90.22%
=======================================
Files 13 13
Lines 757 757
=======================================
Hits 683 683
Misses 74 74 ☔ View full report in Codecov by Sentry. |
fbe9e48
to
a4c1d13
Compare
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 was intending to also move the rest of integration_spec
in this PR but that looked like it was going to involve a lot more edits.
There aren't too many changes in this one despite what the diff says xD
ActiveJob::Base.queue_adapter = :test | ||
end | ||
|
||
def ar_schema |
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 method is responsible for decentralizing ActiveRecord::Schema.define
.
Since the old define
method mainly instance_exec
'ed the block provided to it, this should be fine (define
also updated some metadata tables but they should not be necessary for us).
@@ -0,0 +1,33 @@ | |||
require 'support/active_record_schema' |
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.
These requires I thought would make it obvious where ar_schema
comes from, so it's easier to follow the logic in an integration spec from
require 'support/models/<model>
to
require 'support/active_record_schema
in a few clicks and get a grasp of what is happening.
spec/support/active_record_classes.rb
Debated removing this model 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.
LGTM! Very good work @ellnix
That's the perfect balance between the size of the PR and the changes itself. Very easy to grasp what's going on.
Keep going 🤘
bors merge |
A first step toward #332