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

Simplify the sanity check in ARTest.connect #48872

Merged

Conversation

aidanharan
Copy link
Contributor

Motivation / Background

This Pull Request has been created to allow Active Record SQL Server Adapter tests to run against the Rails main branch.

Detail

In PR Fix trilogy builds, a check was included to prevent the tests running against the wrong adapter. The fix handled the special case of SQLite which uses different connection (sqlite3_mem) and adapter (sqlite3) names.

This Pull Request extends that check to also handle the SQL Server adapter. The adapter's name is sqlserver but the connection's name is dblib. I can't find an alternative method in the adapter's codebase of fixing this issue.

Checklist

Before submitting the PR make sure the following are checked:

  • This Pull Request is related to one change. Changes that are unrelated should be opened in separate PRs.
  • Commit message has a detailed description of what changed and why. If this PR fixes a related issue include it in the commit message. Ex: [Fix #issue-number]
  • Tests are added or updated if you fix a bug or add a feature.
  • CHANGELOG files are updated for the changed libraries if there is a behavior change or additional feature. Minor bug fixes and documentation changes should not be included.

@eileencodes
Copy link
Member

eileencodes commented Aug 2, 2023

Hey @aidanharan - thanks for the PR. I'm a bit confused though. We don't run the sql server tests on rails/rails CI and I don't see a Rails main build with our checkout running in the sql server repo. Where would this change be used?

@aidanharan
Copy link
Contributor Author

@eileencodes Thank you for your reply. The Rails ActiveRecord tests are run as part of the SQL Server test suite. So we pull in the Rails repos and run the AR tests on our CI.

In rails-sqlserver/activerecord-sqlserver-adapter#1065 I am working on getting the SQL Server tests to run against Rails main. In the CI run https://github.com/rails-sqlserver/activerecord-sqlserver-adapter/actions/runs/5740688165/job/15559083287?pr=1065#step:4:24 you can see the The connection name did not match the adapter name. Connection name is 'dblib' and the adapter name is 'sqlserver'. exception caused by https://github.com/rails/rails/blob/main/activerecord/test/support/connection.rb#L33

I know it would be best not to mix SQL Server adapter requirements into the Rails codebase but I'm not sure what alternative I have. Thanks.

@casperisfine
Copy link
Contributor

What if we add some alternate_names method on Adapter allowing each adapter to define this.

That would also allow to get rid of this hard coded special case for sqlite3_mem.

@aidanharan
Copy link
Contributor Author

aidanharan commented Aug 3, 2023

Apologies after looking into the issue further I found that my issue was just caused by the arbitrary name of dblib that we gave to the connection. Changing it to sqlserver fixes the issue in the SQL Server CI.

The purpose of the exception was as a sanity check to prevent the tests running against the wrong database (initially the Trinity tests were running against a MySQL2 database). Since the test connection names are set in

the hard coded special case for sqlite3_mem can be removed easily by just checking that the connection name includes the adapter name. I've updated the PR for do this.

@aidanharan aidanharan force-pushed the sqlserver-test-connection-and-adapter-naming branch from 9eb81c5 to db3578a Compare August 3, 2023 09:55
@byroot byroot changed the title [SQL Server] During testing do not raise error for connection and adapter name mismatch Simplify the sanity check in ARTest.connect Aug 3, 2023
@byroot byroot merged commit 318c0fe into rails:main Aug 3, 2023
8 of 9 checks passed
@byroot
Copy link
Member

byroot commented Aug 3, 2023

Makes sense to me. Thanks!

@aidanharan aidanharan deleted the sqlserver-test-connection-and-adapter-naming branch August 3, 2023 12:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants