-
Notifications
You must be signed in to change notification settings - Fork 679
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 migrations in tests for rails 5.2 #409
Fix migrations in tests for rails 5.2 #409
Conversation
dd0955a
to
5df7c3b
Compare
spec/audited_spec_helpers.rb
Outdated
@@ -17,4 +17,12 @@ def create_versions(n = 2) | |||
end | |||
end | |||
|
|||
def run_migrations(direction, migrations_paths, target_version = nil) | |||
if Rails.version >= '5.2.0.0' |
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 will fail once 5.2.0 is out, since '5.2.0' >= '5.2.0.0' => false
.
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.
ouch, extra zero.
Such Rails.version >= '5.2.0'
will not work for .beta2
, after which change was introduced. Should I bother of exactly greater than .beta2
or is Rails.version >= '5.2.0'
enough?
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.
perhaps Rails.version.start_with? '5.2'
for now, and change it later if needed for 6?
5df7c3b
to
4c61d32
Compare
Updated this to use exact version:
|
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 @fatkodima !
Rails 5.2.rc1 introduced some breaking changes for
ActiveRecord::Migrator
, so this PR fixes tests according to new changes.