Use In-Memory Database for Tests

[funnyboy-roks]

Switch to using an in-memory database for testing. This makes tests use a bit more memory (though not significantly), but seems to fix the spurious errors that we have been getting in CI.

I am able to force the errors on main locally by using this fish command:

for x in (seq 1 20)
    cargo t -q -j 1 -- --test-threads=24 &
end
wait

I think the bash equivalent is this, but I haven't tested it:

for x in $(seq 1 20); do
    cargo t -q -j 1 -- --test-threads=24 &
done
wait

The issues do not seem to persist locally when using an in-memory database, but this PR should be a better indicator as to whether it persists in CI.

Resolve #62

[funnyboy-roks]

After a bit of thought, I think the specific issue we were having was a data race when SqliteLayer::from_path is called on the same path from different threads. The RNG used for the temp file seems to be able to create duplicate paths (maybe based on system time?).

I think the race happens as such:

Thread A creates the file
Thread B checks if the file exists
Thread B sees the file exists, so doesn't attempt to populate the DB
Thread B's test runs
Thread A populates the database

[jackjohn7]

After a bit of thought, I think the specific issue we were having was a data race when SqliteLayer::from_path is called on the same path from different threads. The RNG used for the temp file seems to be able to create duplicate paths (maybe based on system time?).

I think the race happens as such:

Thread A creates the file Thread B checks if the file exists Thread B sees the file exists, so doesn't attempt to populate the DB Thread B's test runs Thread A populates the database

Any thoughts on just giving the tempfiles different names? async_tempfile::TempFile::new_with_name allows us to give the tempfile a name. Above memory concerns, the file-based approach allows us to verify that our database is being generated correctly. That same database is what's going to be used in the actual runtime.

[jackjohn7]

Looks good. I like how it simplifies the test code. However, since it's the generated DB file that we end up using at runtime, it's nice that we get to test against it. Since we're no longer doing that, could you add a test that simply produces the mock database in a tempfile like we do ordinarily and then just produce some dummy data? I want to make sure we still have a mechanism for catching regressions on that front.

[funnyboy-roks]

After a bit of thought, I think the specific issue we were having was a data race when SqliteLayer::from_path is called on the same path from different threads. The RNG used for the temp file seems to be able to create duplicate paths (maybe based on system time?).
I think the race happens as such:
Thread A creates the file Thread B checks if the file exists Thread B sees the file exists, so doesn't attempt to populate the DB Thread B's test runs Thread A populates the database

Any thoughts on just giving the tempfiles different names? async_tempfile::TempFile::new_with_name allows us to give the tempfile a name. Above memory concerns, the file-based approach allows us to verify that our database is being generated correctly. That same database is what's going to be used in the actual runtime.

I changed the initialisation of the database to no longer use the compile-time built database (I'm honestly not sure why we did that to begin with). I also added a test to ensure that the database persists between creation of the SqliteLayer.

[jackjohn7]

Looks good. I think we had done that for a tiny perf optimization that really isn't worth having. Also, be sure that the merge commit message is updated to contain this information as well

[funnyboy-roks]

I think we had done that for a tiny perf optimization that really isn't worth having.

Yeah that seems likely. Any potential performance gain would definitely be very minor.