ory/x

sqlcon: Add a way to purge test databases individually

Closed this issue · 3 comments

hperl commented

Preflight checklist

Describe your problem

Tests using the test helpers from sqlcon like dockertest.RunTestMySQL(t) could be trivially paralellizable, where it not for the fact that there is no way to clean up these containers apart from the global dockertest.KillAllTestDatabases. The problem with dockertest.KillAllTestDatabases is that there is no good place to put it. Within a t.Cleanup means that the tests can't run in parallel, since any cleanup purges all containers, also from other tests. This is also the case with a TestMain, if multiple different packages rely on the dockertest helpers.

Initial tests show that e.g. paralellizing the tests in github.com/ory/keto/internal/persistence/sql more than halves the test duration (from 84s 33s).

Describe your ideal solution

There are two options, both introducing a new dockertest.RunTest{DB}(*testing.T) variant.

Option 1: dockertest.RunTest{DB}WithCleanup(*testing.T) (dsn string, cleanupFn func()) returns an additional cleanup function that the user can pass into t.Cleanup.

Option 2: dockertest.RunTest{DB}AndCleanup(*testing.T) (dsn string) returns no extra value, but adds a cleanup to t.Cleanup automatically.

Additionally, the RunTest{DB} functions could add the t.Cleanup call without breaking the API, but it would still change the behaviour of the API.


I would prefer either directly modifying the RunTest{DB} functions to add t.Cleanup or adding a new function that uses t.Cleanup, as I assue that when you want a test database, you want it to be removed after the test. This way, all tests that use these helpers benefit from a more sound cleanup mechanism immediately.

Workarounds or alternatives

  • Continue using KillAllTestDatabases and live with the tests not running in parallel.

Version

master

Additional Context

No response

hperl commented

@zepatrik WDYT? Running the DB tests in parallel would be quite an improvement.

Makes sense for local setups, but in the CI we start the database containers separately (e.g. https://github.com/ory/keto/blob/932035d6e835691d1c15d15d6a0d2fc7b5b9727c/.circleci/config.yml#L15-L31). So in that case, we don't have an easy way to parallelize stuff. We could try to not do that and start the databases as required, which will add the startup time to tests (significant only for mysql) and migration time.
A better alternative would be to use different databases on the one running instance, so one test would use DSN=cockroach://root:root@::1/$UNIQUE_NAME_HERE. This would reduce the startup time, but still need to run migrations for each test. We could add snapshot backups of migrated databases to remedy that 🤔
So I think it is a bit more work to parallelize those tests, but it would definitely be worth it.

hperl commented

My concern was primarily the local setup, so I would only parallelize it there (checking for env CI).