-
Notifications
You must be signed in to change notification settings - Fork 125
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
Add a STI disabled spec #245
Conversation
So, one issue I see is that this uses the current schema to check "old" migrations, which is invalid. You need to use the schema at the time of the migration to determine if the inheritance disablement is necessary. |
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.
☝️
efafe43
to
ed03d47
Compare
@Fryguy Updated, please take a look |
spec/support/migration_helper.rb
Outdated
@@ -15,6 +15,19 @@ def migration_context(direction, &block) | |||
|
|||
it("with empty tables") { migrate } | |||
|
|||
it "STI is disabled for all involved ActiveRecord::Base descendants" do | |||
classes = ActiveRecord::Base.descendants.select { |klass| klass.namespace.first == described_class.to_s }.compact |
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.
Might be better to flip this around to something like
described_class.constants.select { |c| c < ActiveRecord::Base }
ed03d47
to
f69f8b6
Compare
Checked commits bdunne/manageiq-schema@8aaeae5~...f69f8b6 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0 |
Adding this as an automated code review to address #4 (comment)
Also fixing the current failures