Skip to content

Commit

Permalink
test: Change test isolation model to 'database per test' (#220)
Browse files Browse the repository at this point in the history 
Improve the reliability and performance of the test suite by moving the
test isolation model from 'container per test' to 'database per test'.

The current test suite works well locally but is [very flaky
](https://github.com/xataio/pgroll/actions) when run on Github Actions.
The cause of the flakiness is the 'container per test' isolation model,
under which each testcase in each test starts its own Postgres
container. This model worked well initially but as the number of tests
has increased the actions runner often fails to make available the
required number of containers for such a large number of parallel tests.

This PR changes the isolation model to 'database per test'. Each package
creates one Postgres container and then each testcase in each test
creates a database within that container. This greatly reduces the
number of simultaneous containers required, making the test suite faster
and more reliable.

Each job in the test matrix sees a 60-70% reduction in duration and
(anecdotally) far fewer failures with no failures observed in ~20 runs.
  • Loading branch information
@andrew-farries
andrew-farries authored Jan 8, 2024
1 parent c10dabf commit f4c37b8
Show file tree
Hide file tree
Showing 5 changed files with 234 additions and 237 deletions.
85 changes: 6 additions & 79 deletions pkg/migrations/op_common_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,25 +7,16 @@ import (
"database/sql"
"errors"
"fmt"
"os"
"testing"
"time"

"github.com/lib/pq"
"github.com/testcontainers/testcontainers-go"
"github.com/testcontainers/testcontainers-go/modules/postgres"
"github.com/testcontainers/testcontainers-go/wait"
"github.com/xataio/pgroll/pkg/migrations"
"github.com/xataio/pgroll/pkg/roll"
"github.com/xataio/pgroll/pkg/state"
"github.com/xataio/pgroll/pkg/testutils"
"golang.org/x/exp/maps"
"golang.org/x/exp/slices"
)

// The version of postgres against which the tests are run
// if the POSTGRES_VERSION environment variable is not set.
const defaultPostgresVersion = "15.3"

type TestCase struct {
name string
migrations []migrations.Migration
Expand All @@ -37,10 +28,14 @@ type TestCase struct {

type TestCases []TestCase

func TestMain(m *testing.M) {
testutils.SharedTestMain(m)
}

func ExecuteTests(t *testing.T, tests TestCases) {
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
withMigratorAndConnectionToContainer(t, func(mig *roll.Roll, db *sql.DB) {
testutils.WithMigratorAndConnectionToContainer(t, func(mig *roll.Roll, db *sql.DB) {
ctx := context.Background()

// run all migrations except the last one
Expand Down Expand Up @@ -100,74 +95,6 @@ func ExecuteTests(t *testing.T, tests TestCases) {
}
}

func withMigratorAndConnectionToContainer(t *testing.T, fn func(mig *roll.Roll, db *sql.DB)) {
t.Helper()
ctx := context.Background()

waitForLogs := wait.
ForLog("database system is ready to accept connections").
WithOccurrence(2).
WithStartupTimeout(5 * time.Second)

pgVersion := os.Getenv("POSTGRES_VERSION")
if pgVersion == "" {
pgVersion = defaultPostgresVersion
}

ctr, err := postgres.RunContainer(ctx,
testcontainers.WithImage("postgres:"+pgVersion),
testcontainers.WithWaitStrategy(waitForLogs),
)
if err != nil {
t.Fatal(err)
}

t.Cleanup(func() {
if err := ctr.Terminate(ctx); err != nil {
t.Fatalf("Failed to terminate container: %v", err)
}
})

cStr, err := ctr.ConnectionString(ctx, "sslmode=disable")
if err != nil {
t.Fatal(err)
}

st, err := state.New(ctx, cStr, "pgroll")
if err != nil {
t.Fatal(err)
}
err = st.Init(ctx)
if err != nil {
t.Fatal(err)
}

const lockTimeoutMs = 500
mig, err := roll.New(ctx, cStr, "public", lockTimeoutMs, st)
if err != nil {
t.Fatal(err)
}

t.Cleanup(func() {
if err := mig.Close(); err != nil {
t.Fatalf("Failed to close migrator connection: %v", err)
}
})

db, err := sql.Open("postgres", cStr)
if err != nil {
t.Fatal(err)
}

t.Cleanup(func() {
if err := db.Close(); err != nil {
t.Fatalf("Failed to close database connection: %v", err)
}
})

fn(mig, db)
}

// Common assertions

func ViewMustExist(t *testing.T, db *sql.DB, schema, version, view string) {
Expand Down
109 changes: 13 additions & 96 deletions pkg/roll/execute_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,31 +7,28 @@ import (
"database/sql"
"errors"
"fmt"
"os"
"testing"
"time"

"github.com/lib/pq"
"github.com/stretchr/testify/assert"
"github.com/testcontainers/testcontainers-go"
"github.com/testcontainers/testcontainers-go/modules/postgres"
"github.com/testcontainers/testcontainers-go/wait"
"github.com/xataio/pgroll/pkg/migrations"
"github.com/xataio/pgroll/pkg/roll"
"github.com/xataio/pgroll/pkg/state"
"github.com/xataio/pgroll/pkg/testutils"
)

const (
schema = "public"
// The version of postgres against which the tests are run
// if the POSTGRES_VERSION environment variable is not set.
defaultPostgresVersion = "15.3"
)

func TestMain(m *testing.M) {
testutils.SharedTestMain(m)
}

func TestSchemaIsCreatedfterMigrationStart(t *testing.T) {
t.Parallel()

withMigratorAndConnectionToContainer(t, func(mig *roll.Roll, db *sql.DB) {
testutils.WithMigratorAndConnectionToContainer(t, func(mig *roll.Roll, db *sql.DB) {
ctx := context.Background()
version := "1_create_table"

Expand Down Expand Up @@ -63,7 +60,7 @@ func TestPreviousVersionIsDroppedAfterMigrationCompletion(t *testing.T) {
t.Parallel()

t.Run("when the previous version is a pgroll migration", func(t *testing.T) {
withMigratorAndConnectionToContainer(t, func(mig *roll.Roll, db *sql.DB) {
testutils.WithMigratorAndConnectionToContainer(t, func(mig *roll.Roll, db *sql.DB) {
ctx := context.Background()
const (
firstVersion = "1_create_table"
Expand Down Expand Up @@ -104,7 +101,7 @@ func TestPreviousVersionIsDroppedAfterMigrationCompletion(t *testing.T) {
})

t.Run("when the previous version is an inferred DDL migration", func(t *testing.T) {
withMigratorAndConnectionToContainer(t, func(mig *roll.Roll, db *sql.DB) {
testutils.WithMigratorAndConnectionToContainer(t, func(mig *roll.Roll, db *sql.DB) {
ctx := context.Background()
const (
firstVersion = "1_create_table"
Expand Down Expand Up @@ -157,7 +154,7 @@ func TestPreviousVersionIsDroppedAfterMigrationCompletion(t *testing.T) {
func TestSchemaIsDroppedAfterMigrationRollback(t *testing.T) {
t.Parallel()

withMigratorAndConnectionToContainer(t, func(mig *roll.Roll, db *sql.DB) {
testutils.WithMigratorAndConnectionToContainer(t, func(mig *roll.Roll, db *sql.DB) {
ctx := context.Background()
version := "1_create_table"

Expand Down Expand Up @@ -191,7 +188,7 @@ func TestSchemaIsDroppedAfterMigrationRollback(t *testing.T) {
func TestSchemaOptionIsRespected(t *testing.T) {
t.Parallel()

withMigratorInSchemaAndConnectionToContainer(t, "schema1", func(mig *roll.Roll, db *sql.DB) {
testutils.WithMigratorInSchemaAndConnectionToContainer(t, "schema1", func(mig *roll.Roll, db *sql.DB) {
ctx := context.Background()
const version1 = "1_create_table"
const version2 = "2_create_another_table"
Expand Down Expand Up @@ -256,7 +253,7 @@ func TestSchemaOptionIsRespected(t *testing.T) {
func TestLockTimeoutIsEnforced(t *testing.T) {
t.Parallel()

withMigratorInSchemaWithLockTimeoutAndConnectionToContainer(t, "public", 100, func(mig *roll.Roll, db *sql.DB) {
testutils.WithMigratorInSchemaWithLockTimeoutAndConnectionToContainer(t, "public", 100, func(mig *roll.Roll, db *sql.DB) {
ctx := context.Background()

// Start a create table migration
Expand Down Expand Up @@ -310,7 +307,7 @@ func TestLockTimeoutIsEnforced(t *testing.T) {
func TestViewsAreCreatedWithSecurityInvokerTrue(t *testing.T) {
t.Parallel()

withMigratorAndConnectionToContainer(t, func(mig *roll.Roll, db *sql.DB) {
testutils.WithMigratorAndConnectionToContainer(t, func(mig *roll.Roll, db *sql.DB) {
ctx := context.Background()
version := "1_create_table"

Expand Down Expand Up @@ -392,7 +389,7 @@ func TestViewsAreCreatedWithSecurityInvokerTrue(t *testing.T) {
func TestStatusMethodReturnsCorrectStatus(t *testing.T) {
t.Parallel()

withMigratorAndConnectionToContainer(t, func(mig *roll.Roll, db *sql.DB) {
testutils.WithMigratorAndConnectionToContainer(t, func(mig *roll.Roll, db *sql.DB) {
ctx := context.Background()

// Get the initial migration status before any migrations are run
Expand Down Expand Up @@ -490,86 +487,6 @@ func addColumnOp(tableName string) *migrations.OpAddColumn {
}
}

func withMigratorInSchemaWithLockTimeoutAndConnectionToContainer(t *testing.T, schema string, lockTimeoutMs int, fn func(mig *roll.Roll, db *sql.DB)) {
t.Helper()
ctx := context.Background()

waitForLogs := wait.
ForLog("database system is ready to accept connections").
WithOccurrence(2).
WithStartupTimeout(5 * time.Second)

pgVersion := os.Getenv("POSTGRES_VERSION")
if pgVersion == "" {
pgVersion = defaultPostgresVersion
}

ctr, err := postgres.RunContainer(ctx,
testcontainers.WithImage("postgres:"+pgVersion),
testcontainers.WithWaitStrategy(waitForLogs),
)
if err != nil {
t.Fatal(err)
}

t.Cleanup(func() {
if err := ctr.Terminate(ctx); err != nil {
t.Fatalf("Failed to terminate container: %v", err)
}
})

cStr, err := ctr.ConnectionString(ctx, "sslmode=disable")
if err != nil {
t.Fatal(err)
}

st, err := state.New(ctx, cStr, "pgroll")
if err != nil {
t.Fatal(err)
}
err = st.Init(ctx)
if err != nil {
t.Fatal(err)
}

mig, err := roll.New(ctx, cStr, schema, lockTimeoutMs, st)
if err != nil {
t.Fatal(err)
}

t.Cleanup(func() {
if err := mig.Close(); err != nil {
t.Fatalf("Failed to close migrator connection: %v", err)
}
})

db, err := sql.Open("postgres", cStr)
if err != nil {
t.Fatal(err)
}

t.Cleanup(func() {
if err := db.Close(); err != nil {
t.Fatalf("Failed to close database connection: %v", err)
}
})

_, err = db.ExecContext(ctx, fmt.Sprintf("CREATE SCHEMA IF NOT EXISTS %s", schema))
if err != nil {
t.Fatal(err)
}

fn(mig, db)
}

func withMigratorInSchemaAndConnectionToContainer(t *testing.T, schema string, fn func(mig *roll.Roll, db *sql.DB)) {
withMigratorInSchemaWithLockTimeoutAndConnectionToContainer(t, schema, 500, fn)
}

func withMigratorAndConnectionToContainer(t *testing.T, fn func(mig *roll.Roll, db *sql.DB)) {
withMigratorInSchemaWithLockTimeoutAndConnectionToContainer(t, "public", 500, fn)
}

func MustSelect(t *testing.T, db *sql.DB, schema, version, table string) []map[string]any {
t.Helper()
versionSchema := roll.VersionedSchemaName(schema, version)
Expand Down
Loading

0 comments on commit f4c37b8

Please sign in to comment.