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

fix(tests): add test for transaction isolation level on mssql #5054

Conversation

LucianBuzzo
Copy link
Contributor

@LucianBuzzo LucianBuzzo commented Nov 25, 2024

This PR adds a test for transaction isolation levels on Mssql.
On Mssql, isolation levels persist for the duration of a connection, and so the Mssql connection overrides the isolation_level transaction option, setting it to a default value, by providing it's own implementation.
See https://learn.microsoft.com/en-us/sql/t-sql/statements/set-transaction-isolation-level-transact-sql?view=sql-server-ver16#remarks
This test fails for me when running against mssql 2022 with:

make dev-mssql2022 
cargo test --lib --features=all-native  mssql_transaction_isolation_level -- --nocapture

From what I can see the default TransactionCapable::start_transaction() method is called instead of the Mssql implementation. It's also possible that my test case is incorrect!
I'm not sure why this happens, so I am opening this PR to bring attention to this issue and hopefully resolve the bug.

@LucianBuzzo LucianBuzzo requested a review from a team as a code owner November 25, 2024 14:43
@LucianBuzzo LucianBuzzo requested review from jkomyno and removed request for a team November 25, 2024 14:43
@LucianBuzzo LucianBuzzo force-pushed the lucianbuzzo/mssql-isolation-level-test branch from 6063412 to d90ca61 Compare November 25, 2024 14:45
Copy link

codspeed-hq bot commented Nov 25, 2024

CodSpeed Performance Report

Merging #5054 will not alter performance

Comparing LucianBuzzo:lucianbuzzo/mssql-isolation-level-test (d90ca61) with main (5e70d19)

Summary

✅ 11 untouched benchmarks

@aqrln aqrln assigned aqrln and jacek-prisma and unassigned aqrln Nov 26, 2024
@aqrln
Copy link
Member

aqrln commented Nov 26, 2024

Closing in favour of #5056

@aqrln aqrln closed this Nov 26, 2024
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.

3 participants