sqlx
sqlx copied to clipboard
launchbadge
Fix: nextest cleanup race condition
fixes #2123 #[sqlx::test] should play nicely with nextest
Nextest provides a NEXTEST_RUN_ID that we can use to clear up tests belonging to the same cargo invocation.
See for a more general fix for cargo test runners.
Currently I haven't implemented support for mysql and sqlite, I'm looking for feedback before going ahead and doing that.
@bonega another thought is, instead of generating an arbitrary database name, we could generate one based on the path of the test function that's running. If the database name is always the same, then there's no real extra cleanup step necessary besides dropping and recreating the database before running the test and dropping it afterward. And it would make it a lot easier to find and inspect the database in question.
I would honestly make it always clean up the test database afterward unless an environment variable is set; the existing behavior seems to just confuse and annoy people more than anything. Bikeshedding: SQLX_TEST_KEEP_DB?
Do you know of any test runners that execute the same function multiple times concurrently? I think we could even handle that by holding a lock on the row in the test-databases table as a mutex. Arguably though, that table wouldn't even need to exist.
I think the main reason I didn't just do this originally was because I didn't want to have to think of a way to convert a path to a database name. However, as it turns out, MySQL and Postgres both allow (quoted) database names to contain any character besides NUL (\0) so we don't really have to come up with any special mapping scheme; we could just use the fully qualified path of the test function itself.
The biggest potential issue is that both databases have a length limit of 64 and 63 characters, respectively, so we'd have to either abbreviate or just truncate. I guess truncating makes the most sense. 63 characters is quite a lot for a path, it's unlikely to collide in most situations. If there is a collision, the mutual exclusion would just make the tests execute serially. And we could always make the attribute take an argument to override the database name.
Hey, just giving a heads-up that I am super busy with moving apartment, will take a closer look on this in a couple of days.
@bonega do you have time to look at my last comment?
It is starting to wind down now, think I will have time today or tomorrow
@abonander I think it seems to be a generally good idea. Though I worry a bit about the truncating/locking. I have seen some very long test names before.
I will do some prototyping and see if it seems workable.
To get the module path you'll probably need to make #[sqlx::test] emit a recursive invocation of itself with module_path!() in its arguments. It's very fortunate that arguments to attributes are expanded eagerly now.
Hey, Sorry for being unresponsive.
I have checked around a little. The test path can easily get long since it also includes the modules. Also I don't want to impose naming requirements on the end-users.
By doing a truncate and lock I fear that we might regress the performance for some of the current users.
@bonega we could instead hash the path. A SHA-256 hash is incredibly unlikely to collide and can be encoded in 64 hex characters, so in Base64 (with dash and underscore) would be more like... 40?
I think deterministic naming is the right answer here, because then each test function can be wholly responsible for its own database and doesn't need any sort of memoization.
@abonander Hey, I am getting back into shape now.
Is this in the direction of what you wanted? Will do some more refactoring and fixing up if it looks alright.
@abonander I still can't run the tests locally but I see that this is passing in CI. Mysql is finished and from what I can see sqlite already does a database per test path.
Hey! We are looking forward to use this fix. Do you know when we will have a new release of sqlx? Thanks!
