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

Also test trilogy adapter through activerecord #487

Merged
merged 1 commit into from
Apr 28, 2023

Conversation

etiennebarrie
Copy link
Member

Follow-up to #486

Since the adapter is in Active Record now, we can test Trilogy through Active Record too. So I just extracted the rails tests and used them both for the mysql2 and trilogy adapter.

Actually at first, I thought maybe the semian/rails code isn't too related to the specific adapter, so maybe there's no need to test it for each adapter/client semian supports, but then I saw the code from #395 which is specific to mysql2.

By adding a share test we could see the special case made for Mysql2 and reproduce it for Trilogy, but looking more into it, I don't think it's ever used:

>> ActiveRecord::ConnectionAdapters::Mysql2Adapter.method(:new_client)
=> #<Method: ActiveRecord::ConnectionAdapters::Mysql2Adapter.new_client(config) /Users/etienne/.gem/ruby/3.2.2/gems/activerecord-7.0.4.3/lib/active_record/connection_adapters/mysql2_adapter.rb:43>
>> ActiveRecord::ConnectionAdapters::Mysql2Adapter.method(:new_client).super_method
=> #<Method: Semian::Rails::ClassMethods#new_client(config) /Users/etienne/.gem/ruby/3.2.2/gems/semian-0.18.0/lib/semian/rails.rb:13>

and ActiveRecord::ConnectionAdapters::Mysql2Adapter.new_client doesn't call super so those exceptions are never rescued/re-raised.

Anyway, I can look into this later but this may still be worth it. Let me know what you think.

Copy link
Contributor

@casperisfine casperisfine left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@etiennebarrie etiennebarrie force-pushed the test-trilogy-with-rails branch 2 times, most recently from 6ef3181 to 9305574 Compare April 28, 2023 10:31
@etiennebarrie etiennebarrie merged commit fe3395a into master Apr 28, 2023
43 checks passed
@etiennebarrie etiennebarrie deleted the test-trilogy-with-rails branch April 28, 2023 12:10
@adrianna-chang-shopify
Copy link
Contributor

❤️

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.

None yet

4 participants