syncstorage-rs
syncstorage-rs copied to clipboard
[spanner] Use explicit transactions in db-tests
We still have duplicate sync
versions of the SpannerDb::commit/rollback/get_transaction
methods methods vs their _async
(e.g. commit_async
) counterparts. This seems due to the sql_request
request method's usage, which can implicitly begin a transaction, yet would be inconvenient if every invocation required an .await
.
The implicit beginning of a transaction is only used by db-tests ~~(delete_all
also uses it! #615)~~, so we should be able to solve this in a different way (maybe the db::tests::support::db
function could call begin for the tests? at least under Spanner)
The db-tests could call begin
themselves, but it probably make sense to provide the test w/ a test_transaction
method that does this and rolls everything back at the end, similarl to the http_transaction
method now used by the handlers.
Yes, we absolutely should do this, it will make the code so much simpler and the tests will test things that are closer to reality.
~~More than just the tests use implicit (sync) transactions, all the /info/* endpoints do too because they lack a collection. See #768~~ (#768 now fixed)
Another avenue to investigate w/ this issue: would this remove the need for RUST_TEST_THREADS=1
under diesel?
It's not clear why this is required for testing w/ diesel (I know the diesel code base uses it and they've recommended it). I have a theory: begin_test_transaction
doesn't rollback. Diesel does have a test_transaction
method that takes a closure that does a rollback, however -- would switching to it (or doing the equivalent ourselves) negate the need for RUST_TEST_THREADS
?
We might want to ping diesel's gitter about this.