syncstorage-rs icon indicating copy to clipboard operation
syncstorage-rs copied to clipboard

[spanner] Use explicit transactions in db-tests

Open pjenvey opened this issue 4 years ago • 4 comments

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)

pjenvey avatar Apr 08 '20 22:04 pjenvey

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.

pjenvey avatar Jul 21 '20 22:07 pjenvey

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.

fzzzy avatar Jul 25 '20 00:07 fzzzy

~~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)

pjenvey avatar Aug 06 '20 23:08 pjenvey

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.

pjenvey avatar Sep 24 '20 21:09 pjenvey