-
Notifications
You must be signed in to change notification settings - Fork 23
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 tests to verify the behavior of disable_ddl_transaction #101
Conversation
67fcb65
to
abeb752
Compare
|
||
subclass.suppress_messages do | ||
expect(subclass.new.unsafe_pg_ha_migrations_test_method).to eq("sentinel_value") | ||
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.
I just noticed a bunch of this in the test output:
-- pg_ha_migrations_test_method(nil)
-> 0.0001s
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.
Can you squash into the two meaningful commits after applying the one suggestion?
aggregate_failures do | ||
expect do | ||
migration_klass.suppress_messages do | ||
ActiveRecord::MigrationContext.new( |
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.
Can we add comments explaining that our usually way of testing doesn't invoke ActiveRecord's transaction handingling?
d9091a3
to
b36942f
Compare
Done and done |
b36942f
to
51402cb
Compare
With recent PRs / conversations around transactional migrations, I figured it would be a good idea to write some basic tests to verify existing behavior. In other tests, we execute test migrations directly which bypasses the transaction logic in AR.