sqlcon: Add a way to purge test databases individually
Closed this issue · 3 comments
Preflight checklist
- I could not find a solution in the existing issues, docs, nor discussions.
- I agree to follow this project's Code of Conduct.
- I have read and am following this repository's Contribution Guidelines.
- This issue affects my Ory Cloud project.
- I have joined the Ory Community Slack.
- I am signed up to the Ory Security Patch Newsletter.
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
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.
My concern was primarily the local setup, so I would only parallelize it there (checking for env CI
).