Share a single database across tests

[djeebus]

Should speed up tests quite a lot 🤞

Update: shaved about 10-15 seconds. Not nothing!

[cursor]

PR Summary

Medium Risk
Moderate risk because tests now share a single database instance, which can introduce cross-test state coupling/flakiness and changes cleanup/lifecycle behavior under parallel execution.

Overview
Speeds up integration tests by changing testutils.SetupDatabase to lazily create and then reuse a single migrated Postgres test database across the whole test run (thread-safe with an RW lock and using a non-cancelled context), and updates the reverse-proxy Validate tests to reuse that shared DB by moving one-time auth/team/token setup out of per-subtest setup and making per-case inserts idempotent.

^{Reviewed by Cursor Bugbot for commit 0b441d8. Bugbot is set up for automated code reviews on this repo. Configure here.}

[cursor]

Cursor Bugbot has reviewed your changes and found 2 potential issues.

^{❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.}

^{Reviewed by Cursor Bugbot for commit 7b5021d. Configure here.}

[claude]

Review posted via gh api

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 0b441d8cdb

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Restore teardown for singleton test Postgres container

The new singleton path starts a postgres.Run container but never registers any termination hook, so once connStr is read the container lifecycle is unmanaged for the rest of the process. This leaks Docker resources whenever testcontainers' reaper is unavailable/disabled (a common CI hardening setting), and repeated package test runs can accumulate orphaned DB containers and fail due to exhausted Docker capacity.

Useful? React with 👍 / 👎.

[claude]

The issues I flagged in my previous review (data race, context lifetime) have been addressed in subsequent commits — the -guarded check is now properly synchronized and prevents premature cancellation.

Extended reasoning...

Overview

Changes touch two files: (shared DB singleton) and (test refactor to use it). These are test-only files with no production code impact.

Previous concerns addressed

My earlier review flagged two bugs on commit :

1. Data race — the outer check was not under a lock. Fixed: the current code reads under first, then re-checks under . This is correct double-checked locking in Go.
2. Context lifetime — all pool/client constructors used which would be cancelled when the first test finished. Fixed: strips cancellation, keeping the shared connections alive for the full test run.

Security risks

None — this is purely test infrastructure. No production code paths are modified.

Level of scrutiny

Low-to-medium. The implementation is test-only and the changes correctly handle concurrency (RWMutex double-checked locking) and resource lifetime (WithoutCancel). The bug hunter finds no remaining issues and all previous inline review comments are resolved.

Other factors

The sync.Once approach would be marginally more idiomatic, but the current RWMutex pattern is correct. The absence of cleanup for the container/pool is intentional — the shared database is meant to live for the entire test process.

[DevAsign-Review-Agent]

Merge Score: 90/100

🟢 ██████████████████░░ 90%

The PR significantly improves test execution time by reusing a single PostgreSQL container and database connections across tests within the same package. It correctly uses sync.RWMutex for thread-safe initialization and context.WithoutCancel to prevent the shared database clients from being closed when the first test finishes.

Key Strengths: Excellent use of Go concurrency primitives and context management to achieve the performance gain without leaking contexts.

Key Concerns: Sharing the database means tests running in parallel can now interfere with each other if they use hardcoded IDs (like envID in validate_test.go). This can lead to flaky tests and should be addressed by generating unique IDs per test case.

Code Suggestions (3)

Medium Priority (2)

1. packages/docker-reverse-proxy/internal/auth/validate_test.go (Line 26)
   Use unique environment IDs for each test case to prevent race conditions in parallel tests.

Reasoning: Since the database is now shared across all tests, running subtests in parallel with the same hardcoded envID ("test-env-id") can cause unique constraint violations or flaky tests when concurrently inserting into env_build_assignments or querying builds. Generating a unique ID for each test case ensures isolation.

Suggested Code:

	userID := uuid.New()
	teamID := uuid.New()

	validEnvID := uuid.NewString()
	randomTokenEnvID := uuid.NewString()
	wrongEnvID := uuid.NewString()
	noEnvID := uuid.NewString()
	invalidTokenEnvID := uuid.NewString()

	dbClient := testutils.SetupDatabase(t)

2. packages/docker-reverse-proxy/internal/auth/validate_test.go (Line 66)
   Update the testcases struct to use the unique environment IDs.

Reasoning: Applies the unique environment IDs generated above to each specific test case to guarantee test isolation in the shared database.

Suggested Code:

	testcases := []struct {
		name             string
		valid            bool
		createdEnvId     string
		createdEnvStatus string
		validateEnvId    string
		accessTokenUsed  string
		error            bool
	}{
		{
			name:             "valid token",
			valid:            true,
			createdEnvId:     validEnvID,
			createdEnvStatus: "waiting",
			validateEnvId:    validEnvID,
			accessTokenUsed:  accessToken.PrefixedRawValue,
			error:            false,
		},
		{
			name:             "random access token",
			valid:            false,
			createdEnvId:     randomTokenEnvID,
			createdEnvStatus: "waiting",
			validateEnvId:    randomTokenEnvID,
			accessTokenUsed:  fmt.Sprintf("%s123abc", keys.AccessTokenPrefix),
			error:            false,
		},
		{
			name:             "wrong env ID",
			valid:            false,
			createdEnvId:     wrongEnvID,
			createdEnvStatus: "waiting",
			validateEnvId:    "non-existent-env-id",
			accessTokenUsed:  accessToken.PrefixedRawValue,
			error:            false,
		},
		{
			name:             "no env ID",
			valid:            false,
			createdEnvId:     noEnvID,
			createdEnvStatus: "waiting",
			validateEnvId:    "",
			accessTokenUsed:  accessToken.PrefixedRawValue,
			error:            false,
		},
		{
			name:             "invalid access token",
			valid:            false,
			createdEnvId:     invalidTokenEnvID,
			createdEnvStatus: "waiting",
			validateEnvId:    invalidTokenEnvID,
			accessTokenUsed:  "invalid-access-token",
			error:            true,
		},
	}

Low Priority (1)

1. packages/db/pkg/testutils/db.go (Line 139)
   Use defer instead of t.Cleanup for closing the goose migration database connection.

Reasoning: Since SetupDatabase is now only called once and shared, using t.Cleanup ties the closing of the migration database connection to the completion of the first test. Using defer closes it immediately after migrations finish, which is cleaner and frees up the connection sooner.

Suggested Code:

	gooseDB, err := goose.OpenDBWithDriver("pgx", connStr)
	require.NoError(t, err)
	defer func() {
		err := gooseDB.Close()
		assert.NoError(t, err)
	}()

📊 Review Metadata

• Processing Time: 131s
• Analysis Date: 4/10/2026, 12:26:35 PM

🤖 This review was generated by AI. While we strive for accuracy, please use your judgment when applying suggestions.

💬 Questions about this review? Open an issue or contact support.