chore: add postgres template caching for tests #15336
Conversation
There was a problem hiding this comment.
Overall I like the direction for this.
The container staying up though I am more hesitant of. It just feels strange to leave open a container for running tests. But I also see the value in keeping it open 🤷♂️.
I won't block on that. I am ok with keeping it, and letting people complain if it becomes an issue.
| // If there's no database running on the default port, we'll start a | ||
| // postgres container. We won't be cleaning it up so it can be reused | ||
| // by subsequent tests. It'll keep on running until the user terminates | ||
| // it manually. |
There was a problem hiding this comment.
I know this is a big change. It used to cleanup, and people might be averse to having it stay open?
Is it still slow if we reuse the same docker volume on subsequent docker starts?
There was a problem hiding this comment.
The container actually uses ramdisk so there's no volume. Cleaning it up would be also hard when you're running more than 1 test. You'd like to share it among the tests in that run, but clean up only once they all finish. And if you're running 2 go test processes at once, you'd need to make sure one doesn't kill the container while the other is running.
| }, func() { | ||
| _ = pool.Purge(resource) | ||
| _ = os.RemoveAll(tempDir) | ||
| }, nil |
There was a problem hiding this comment.
If another process is reusing the same container, does this purge kill that second container?
Is the resource existence check for when you run go test in 2 different terminals?
There was a problem hiding this comment.
If another process is reusing the container there'd be only one container, and the purge would kill it.
Is the resource existence check for when you run go test in 2 different terminals?
Yes.
There was a problem hiding this comment.
If test 1 calls purge while test 2 is running. That breaks test 2 right?
There was a problem hiding this comment.
That's correct. It would have to be a named container though, and the name would have to be the same in both runs. The test could work around this by generating a unique name per test run.
There was a problem hiding this comment.
Note: no tests currently use named containers because I introduced them in this PR :)
| err = resource.Expire(120) | ||
| err = container.Resource.Expire(120) | ||
| if err != nil { | ||
| return "", nil, xerrors.Errorf("expire resource: %w", err) | ||
| } | ||
|
|
||
| pool.MaxWait = 120 * time.Second | ||
| container.Pool.MaxWait = 120 * time.Second |
There was a problem hiding this comment.
Might be useful on the other docker create. If a test is ctl+c the cleanup will not run.
Although I do recall you mentioning the db should stay up after tests are done.
There was a problem hiding this comment.
Yeah, I intentionally did not set up expiry times on the other openContainer call.
|
@Emyrk I addressed your comments. I don't think leaving a testing container open is an issue - that's how |
This PR is the first in a series aimed at closing #15109.
Changes
Template Database Creation:
dbtestutil.Opennow has the ability to create a template database if none is provided viaDB_FROM. The template database’s name is derived from a hash of the migration files, ensuring that it can be reused across tests and is automatically updated whenever migrations change.Optimized Database Handling:
Previously,
dbtestutil.Openwould spin up a new container for each test whenDB_FROMwas unset. Now, it first checks for an active PostgreSQL instance onlocalhost:5432. If none is found, it creates a single container that remains available for subsequent tests, eliminating repeated container startups.These changes address the long individual test times (10+ seconds) reported by some users, likely due to the time Docker took to start and complete migrations.