Fix: nextest cleanup race condition #3334

bonega · 2024-07-07T15:28:25Z

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.

sqlx-postgres/src/testing/mod.rs

sqlx-core/src/testing/mod.rs

sqlx-postgres/src/testing/mod.rs

abonander

Looks decent so far. Requesting changes so when you hit "request review" I know to check back.

abonander · 2024-07-19T21:30:20Z

@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.

bonega · 2024-07-22T21:01:57Z

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.

abonander · 2024-07-30T00:31:00Z

@bonega do you have time to look at my last comment?

bonega · 2024-07-30T06:41:49Z

@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

bonega · 2024-08-01T19:51:25Z

@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.

abonander · 2024-08-01T21:10:50Z

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.

bonega · 2024-08-11T18:35:22Z

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.

abonander · 2024-08-11T20:57:14Z

@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.

bonega · 2024-09-01T20:02:55Z

@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

Some nits, otherwise it looks all right.

Cargo.toml

sqlx-core/src/testing/mod.rs

abonander · 2024-09-02T18:21:23Z

sqlx-mysql/src/testing/mod.rs

-            Ok(Some(num_deleted))
-        })
-    }
-


Reminder to check this trait implementation for proper quoting of the database name. It doesn't look like it does it.

@bonega don't forget this. Even with URL_SAFE the database name still needs to be quoted because it may contain dashes.

I checked and it seems to work.

A test in examples/postgres/axum-social-with-tests has the test_path: comment::test_create_comment
Which becomes the database name of _sqlx_test_hsAQWz-87IRR7sjnVDeMcHtoDLU3QyT6yWizSbWKFvNwoBt6Q60I

It is deleted correctly even though it contains dashes.

I see now that you're referencing the mysql implementation which I haven't started yet.
I will try and do it and the sqlite today.

Just to update that I am fighting with running the tests on (both my branch and main)

bonega · 2024-09-15T19:42:45Z

@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.

abonander

One blocking change ({db_name:?}), one non-blocking (cleanup_test_dbs()).

Also, what failure are you getting locally? I can't help without any details.

sqlx-mysql/src/testing/mod.rs

sqlx-core/src/testing/mod.rs

abonander · 2024-09-18T03:42:47Z

sqlx-mysql/src/testing/mod.rs


-    query("insert into _sqlx_test_databases(test_path) values (?)")
+    query("insert into _sqlx_test_databases(test_path) values ($1, $2)")


This isn't valid syntax in MySQL.

You're totally right that it isn't the right syntax, also I didn't name the column 😞

For the general tests I can't run them because I get this error:
> ./x.py

# unit test core $ cargo test --no-default-features --manifest-path sqlx-core/Cargo.toml --features json,offline,migrate,_rt-async-std,_tls-rustls Compiling sqlx-core v0.8.1 (/home/andreasliljeqvist/git/sqlx/sqlx-core) error[E0425]: cannot find value `provider` in this scope --> sqlx-core/src/net/tls/tls_rustls.rs:97:54 | 97 | let config = ClientConfig::builder_with_provider(provider.clone()) | ^^^^^^^^ not found in this scope

I pushed another commit that I hope will fix your comment.

I really need to just get rid of x.py. I don't use it and it's not tested in CI so there's no reason for it to get fixed or kept up to date unless someone else bothers to.

@abonander , sorry to tag, but wanted to ask: will you have time to see the corrected changes?

I'm currently on vacation and don't have access to a computer to cut a release. I will set aside time for SQLx next week.

thanks for the quick reply. have a great vacation!

I tried to integrate the changes as we have a lot of tests (kamu-data/kamu-cli#919) and many of them use sqlx + nextest combination.

In the case of PostgreSQL, everything works as expected, but in the case of MySQL, the changes are not finalized:

failed to connect to setup test database: Database(MySqlDatabaseError { code: Some("42000"), number: 1064, message: "You have an error in your SQL syntax; check the manual that corresponds to your MariaDB server version for the right syntax to use near '\"_sqlx_test_b4OPm0XBsXli4d_oBPTB7Qy_7snq3Wiwey-n0eGShY0FeWK3ecaW\"' at line 1" }) note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

Based on the PostgreSQL implementation, I finished MySQL one: bonega#1 -- @bonega , please take a look and if you wish, you can use my changes (paste into your branch).

We have a large number of tests (sqlx + nextest), so in general I think it's fair to call it successful change testing

Addendum: during testing, a regression on not deleting SQLite databases was identified -- it is also fixed in bonega#1

bonega · 2024-10-27T19:32:49Z

@s373r Thank you.
I imported what I think was the gist of your changes.

s373r

Added comments

s373r · 2024-10-29T11:27:35Z

sqlx-core/src/testing/mod.rs

+        let mut hasher = Sha512::new();
+        hasher.update(args.test_path.as_bytes());
+        let hash = hasher.finalize();
+        let hash = URL_SAFE.encode(&hash[..39]);


I think it's important to use - to _ here, since - is not a valid character for SQL identifiers.

let db_name = format!("_sqlx_test_{}", hash).replace("-", "_");

I've had errors related to this

s373r · 2024-10-29T11:30:30Z

sqlx-sqlite/src/testing/mod.rs

@@ -15,6 +15,7 @@ impl TestSupport for Sqlite {
    }

    fn cleanup_test(db_name: &str) -> BoxFuture<'_, Result<(), Error>> {
+        let db_name = convert_path(db_name);


It looks like there will be no file deletion:

in run_test() (sqlx-core/src/testing/mod.rs) calls the database name transformation

if let Err(e) = DB::cleanup_test(&DB::db_name(&args)).await {

and here, we're trying to delete it with a new name that won't be found

bonega commented Jul 7, 2024

View reviewed changes

sqlx-postgres/src/testing/mod.rs Outdated Show resolved Hide resolved

bonega changed the title ~~Work around nextest cleanup race condition~~ Fix: nextest cleanup race condition Jul 8, 2024

abonander reviewed Jul 16, 2024

View reviewed changes

sqlx-core/src/testing/mod.rs Outdated Show resolved Hide resolved

abonander reviewed Jul 16, 2024

View reviewed changes

sqlx-postgres/src/testing/mod.rs Outdated Show resolved Hide resolved

abonander requested changes Jul 16, 2024

View reviewed changes

bonega force-pushed the nextest-race-condition branch from 9e20181 to 1a9845f Compare August 12, 2024 07:54

s373r mentioned this pull request Aug 13, 2024

E2E: Flaky test errors for MySQL implementation kamu-data/kamu-cli#718

Closed

bonega force-pushed the nextest-race-condition branch from 1a9845f to ca1c710 Compare September 1, 2024 20:00

bonega added 3 commits September 2, 2024 09:55

remove unused trait fn cleanup_test_dbs

62c3065

*wip* solve test cleanup race condition

910820e

check for exactly 63 chars in database name

ba296be

bonega force-pushed the nextest-race-condition branch from b27a056 to ba296be Compare September 2, 2024 09:12

abonander requested changes Sep 2, 2024

View reviewed changes

bonega added 8 commits September 3, 2024 09:47

move base64 dependency

8ab335d

change

8ba1033

Use url_safe base64 encoding

04686a6

Assert quoting for database name

bdd42dd

refactor

4179629

add mysql support?

cd4abf8

borrow

9b1e831

fix borrows

44c3b26

bonega requested a review from abonander September 15, 2024 19:42

abonander requested changes Sep 15, 2024

View reviewed changes

sqlx-mysql/src/testing/mod.rs Outdated Show resolved Hide resolved

sqlx-core/src/testing/mod.rs Outdated Show resolved Hide resolved

bonega added 2 commits September 16, 2024 09:41

ensure quoting

e54bd93

re-add trait cleanup_test_dbs

2a52c3a

bonega requested a review from abonander September 16, 2024 10:53

abonander reviewed Sep 18, 2024

View reviewed changes

fix mysql insert

c5d4702

bonega requested a review from abonander September 20, 2024 10:40

Merge branch 'main' into nextest-race-condition

87b6b98

s373r mentioned this pull request Oct 23, 2024

sqlx + nextest: stabilization kamu-data/kamu-cli#919

Merged

16 tasks

bonega added 3 commits October 27, 2024 13:38

cargo lock

31b72e6

use actual field

9a2714a

cleanup converted path in sqlite

8319512

abonander approved these changes Oct 28, 2024

View reviewed changes

s373r suggested changes Oct 29, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: nextest cleanup race condition #3334

Fix: nextest cleanup race condition #3334

bonega commented Jul 7, 2024

abonander left a comment

abonander commented Jul 19, 2024 •

edited

Loading

bonega commented Jul 22, 2024

abonander commented Jul 30, 2024

bonega commented Jul 30, 2024

bonega commented Aug 1, 2024

abonander commented Aug 1, 2024

bonega commented Aug 11, 2024

abonander commented Aug 11, 2024

bonega commented Sep 1, 2024 •

edited

Loading

abonander left a comment •

edited

Loading

abonander Sep 2, 2024

abonander Sep 4, 2024

bonega Sep 4, 2024

bonega Sep 7, 2024

bonega commented Sep 15, 2024

abonander left a comment

abonander Sep 18, 2024

bonega Sep 18, 2024 •

edited

Loading

abonander Oct 2, 2024

s373r Oct 15, 2024

abonander Oct 15, 2024

s373r Oct 16, 2024

s373r Oct 23, 2024

s373r Oct 24, 2024

bonega commented Oct 27, 2024

s373r left a comment

s373r Oct 29, 2024

s373r Oct 29, 2024


		query("insert into _sqlx_test_databases(test_path) values (?)")
		query("insert into _sqlx_test_databases(test_path) values ($1, $2)")

Fix: nextest cleanup race condition #3334

Are you sure you want to change the base?

Fix: nextest cleanup race condition #3334

Conversation

bonega commented Jul 7, 2024

abonander left a comment

Choose a reason for hiding this comment

abonander commented Jul 19, 2024 • edited Loading

bonega commented Jul 22, 2024

abonander commented Jul 30, 2024

bonega commented Jul 30, 2024

bonega commented Aug 1, 2024

abonander commented Aug 1, 2024

bonega commented Aug 11, 2024

abonander commented Aug 11, 2024

bonega commented Sep 1, 2024 • edited Loading

abonander left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bonega commented Sep 15, 2024

abonander left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bonega Sep 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bonega commented Oct 27, 2024

s373r left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abonander commented Jul 19, 2024 •

edited

Loading

bonega commented Sep 1, 2024 •

edited

Loading

abonander left a comment •

edited

Loading

bonega Sep 18, 2024 •

edited

Loading