chore: replace ory/dockertest with testcontainers #2782
Conversation
github-actions
bot
added
area/cli
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2782 +/- ##
==========================================
- Coverage 77.72% 75.25% -2.47%
==========================================
Files 472 472
Lines 49707 49727 +20
==========================================
- Hits 38631 37416 -1215
- Misses 8228 9494 +1266
+ Partials 2848 2817 -31 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
miparnisari
changed the title
mdelapenya
left a comment
mdelapenya
left a comment
There was a problem hiding this comment.
This looks really promising! I added some comments regarding best practices when using the library. Feel free to ping me for anything you need, I'm more than happy to help you with this 😊
| require.NoError(t, err) | ||
| t.Cleanup(func() { | ||
| _ = pool.Client.RemoveNetwork(network.ID) | ||
| _ = net.Remove(ctx) |
There was a problem hiding this comment.
suggestion: use CleanupNetwork, and call it before require, as it handle nils
nw, err := network.New(ctx, network.WithDriver("bridge"), network.WithLabels(map[string]string{"name": bridgeNetworkName}))
testcontainers.CleanupNetwork(t, nw)
require.NoError(t, err)
| config.RestartPolicy = docker.RestartPolicy{ | ||
| Name: "no", | ||
| } | ||
| container, err := testcontainers.GenericContainer(ctx, testcontainers.GenericContainerRequest{ |
There was a problem hiding this comment.
question: why not using the module? https://github.com/Mariscal6/testcontainers-spicedb-go
If it does not provide the APIs need by these tests, we can contribute them to the upstream. I'm pretty sure @Mariscal6 will be happy to review that
| require.NoError(t, err) | ||
| t.Cleanup(func() { | ||
| _ = pool.Purge(resource) | ||
| _ = container.Terminate(ctx) | ||
| }) |
There was a problem hiding this comment.
suggestion: use CleanupContainer, as for networks.
ctr, err := ... // module's Run function OR the GenericContainer() one.
testcontainers.CleanupContainer(t, ctr)
require.NoError(t, err)| require.NoError(t, err) | ||
|
|
||
| if status != 0 { | ||
| if state.ExitCode != 0 { |
There was a problem hiding this comment.
suggestion: you could define a log consumer at container creation, and print it here. Please see https://golang.testcontainers.org/features/follow_logs/#following-container-logs
| config.RestartPolicy = docker.RestartPolicy{ | ||
| Name: "no", | ||
| } | ||
| migrateContainer, err := testcontainers.GenericContainer(ctx, testcontainers.GenericContainerRequest{ |
There was a problem hiding this comment.
suggestion: given there are a few of this, you could abstract the container creation building your own test "framework", something like this:
// pseudocode
func NewTestSpiceDB(ctx Context, opts ...ContainerOpts) {...}| } | ||
|
|
||
| func createNetworkBridge(t testing.TB, pool *dockertest.Pool) string { | ||
| func createNetworkBridge(t testing.TB) (string, *testcontainers.DockerNetwork) { |
There was a problem hiding this comment.
suggestion: you could expose this in the "framework" and use it everywhere else that you create a network. Adding the CleanupNetwork here would automatically clean up resources with every call.
| "MAX_CLIENT_CONN=" + PostgresTestMaxConnections, | ||
| }, | ||
| req := testcontainers.ContainerRequest{ | ||
| Name: pgbouncerContainerHostname, |
There was a problem hiding this comment.
suggestion: remember the advice against names :)
| WithStartupTimeout(dockerBootTimeout), | ||
| } | ||
|
|
||
| if bridgeNetworkName != "" { |
There was a problem hiding this comment.
suggestion: if moving to the Run function with container customisers as functional options, then you could be passing any of the WithNetworkXXX options. See https://golang.testcontainers.org/features/creating_container/#networking-options
| }, | ||
| req := testcontainers.ContainerRequest{ | ||
| Name: pgbouncerContainerHostname, | ||
| Image: "mirror.gcr.io/edoburu/pgbouncer:latest", |
There was a problem hiding this comment.
question: is this a postgres db instance? Try using the Postgres module with your pgBouncer image, so you get the Postgres module's APIs. Then you could remove all the wait code below
| name := "spanner-" + uuid.New().String() | ||
| resource, err := pool.RunWithOptions(&dockertest.RunOptions{ | ||
|
|
||
| req := testcontainers.ContainerRequest{ |
There was a problem hiding this comment.
suggestion: we have a module for Spanner :)
There was a problem hiding this comment.
Yeah, the issue we've found is that the spanner emulator doesn't replicate the behaviors that we need faithfully enough to be of much use.
There was a problem hiding this comment.
Oh that's good to know! Is this something we can add to the upstream module?
No description provided.