search

trilogy-libraries / activerecord-trilogy-adapter

Active Record adapter for the Trilogy database client for Rails v6.0 - v7.0.
https://github.com/trilogy-libraries/trilogy
MIT License
171 stars 17 forks source link

Remove translation of exception on reconnect #49

Closed dbussink closed 1 year ago

dbussink commented 1 year ago

This translation logic breaks Rails parallel tests, because that explicitly rescues the underlying ActiveRecord::NoDatabaseError. This means it fails all tests when running with parallel tests.

Based on Rails upstream, this translation is done there either so this drops it here as well. One test had to be slightly updated for this. But that test is also deleted already on upstream Rails.

WIth this change, we can successfully run tests against our Rails app with Trilogy instead of mysql2.

dbussink commented 1 year ago

WIth this change, we can successfully run tests against our Rails app with Trilogy instead of mysql2.

Not entirely true, we also hit https://github.com/trilogy-libraries/trilogy/pull/97 but we're getting there :smile:.

bensheldon commented 1 year ago

@dbussink thank you! can you share what version of Rails you are using?

dbussink commented 1 year ago

@dbussink thank you! can you share what version of Rails you are using?

This is happening on the latest release, 7.0.6. See also the code here:

https://github.com/rails/rails/blob/7-0-stable/activerecord/lib/active_record/tasks/database_tasks.rb#L405-L421

This explicitly only rescues ActiveRecord::NoDatabaseError which happens with parallel tests since it runs against N different test databases. It then creates the database and tries again.

The wrapping code called here ends up turning the ActiveRecord::NoDatabaseError into a generic ActiveRecord::StatementInvalid error and doesn't get rescued, no database creation happens and all tests fail.

The upstream Trilogy adapter doesn't do this translation either so that's why I'm proposing to remove it here too.

bensheldon commented 1 year ago

@dbussink thank you for the explanation! 🙏🏻

dbussink commented 1 year ago

@bensheldon With this and all the other fixes, would a new release be coming soon? I can also further test against latest git to see if there's any other issues we hit before a release?

bensheldon commented 1 year ago

@dbussink I have a few more PRs I'd like to get merged in before I cut a release. Please use git in the meantime (but soon!)