Skip to content
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

Remove translation of exception on reconnect #49

Merged
merged 1 commit into from
Jul 7, 2023

Conversation

dbussink
Copy link
Contributor

@dbussink dbussink commented Jul 6, 2023

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.

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.
@dbussink
Copy link
Contributor Author

dbussink commented Jul 6, 2023

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

Not entirely true, we also hit trilogy-libraries/trilogy#97 but we're getting there 😄.

@bensheldon
Copy link
Contributor

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

@dbussink
Copy link
Contributor Author

dbussink commented Jul 7, 2023

@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 bensheldon merged commit 017a171 into trilogy-libraries:main Jul 7, 2023
@bensheldon
Copy link
Contributor

@dbussink thank you for the explanation! 🙏🏻

@dbussink
Copy link
Contributor Author

dbussink commented Jul 7, 2023

@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?

@dbussink dbussink deleted the fix-reconnect-logic branch July 7, 2023 18:23
@bensheldon
Copy link
Contributor

@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!)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants