From c2610bb939685fdfd00972bef229de62083d76fd Mon Sep 17 00:00:00 2001 From: Alexander Adam Date: Mon, 11 Aug 2025 01:05:00 +0200 Subject: [PATCH 01/11] Add internal test exports and improve golangci-lint configuration - Introduced `exports_internal_test.go` for internal testing of private methods. - Updated `.golangci.yaml` with additional linter settings and exclusions. - Fixed typos and improved documentation consistency across files. - Namespaced test imports properly for clarity. --- .github/workflows/codeql.yml | 2 +- .golangci.yaml | 91 +++++++++++--- dialects.go | 4 +- dialects_test.go | 31 ++--- exports_internal_test.go | 15 +++ file_migration.go | 9 +- file_migration_test.go | 25 ++-- migration.go | 35 ++++-- migration_test.go | 228 ++++++++++++++++++----------------- 9 files changed, 269 insertions(+), 171 deletions(-) create mode 100644 exports_internal_test.go diff --git a/.github/workflows/codeql.yml b/.github/workflows/codeql.yml index c892699..0a786a1 100644 --- a/.github/workflows/codeql.yml +++ b/.github/workflows/codeql.yml @@ -82,7 +82,7 @@ jobs: # For more details on CodeQL's query packs, refer to: https://docs.github.com/en/code-security/code-scanning/automatically-scanning-your-code-for-vulnerabilities-and-errors/configuring-code-scanning#using-queries-in-ql-packs # queries: security-extended,security-and-quality - # If the analyze step fails for one of the languages you are analyzing with + # If the analyze-step fails for one of the languages you are analyzing with # "We were unable to automatically build your code", modify the matrix above # to set the build mode to "manual" for that language. Then modify this step # to build your code. diff --git a/.golangci.yaml b/.golangci.yaml index fed4424..43fadae 100644 --- a/.golangci.yaml +++ b/.golangci.yaml @@ -1,3 +1,6 @@ +# Copyright the dmorph contributors. +# SPDX-License-Identifier: MPL-2.0 + # Configuration file for golangci-lint # See https://golangci-lint.run/usage/configuration/ for more information @@ -9,25 +12,83 @@ run: linters: default: all - #enable: - # # Default linters - # - errcheck - # - govet - # - ineffassign - # - staticcheck - # - unused - # # Additional linters - # - gosec - # - misspell - # - revive - # - bodyclose - # - noctx + + disable: + - exhaustruct + - forbidigo + - noinlineerr + - nonamedreturns + - wsl + - wsl_v5 exclusions: + warn-unused: true + rules: - - path: main_test.go + - path: _test\.go linters: - - gosec + - cyclop + - dupl + - dupword + - err113 + - funlen + - gocognit + - maintidx + - nestif + + - path: examples/ + linters: + - err113 + + settings: + cyclop: + max-complexity: 25 + + depguard: + rules: + main: + files: + - $all + - "!$test" + - "!**/examples/**/*" + allow: + - $gostd + - gopkg.in/yaml.v3 + - github.com/Masterminds/sprig/v3 + test: + files: + - $test + - "**/examples/**/*" + allow: + - $gostd + - gopkg.in/yaml.v3 + - github.com/Masterminds/sprig/v3 + - github.com/stretchr/testify/assert + - github.com/AlphaOne1/dmorph + - modernc.org/sqlite + + exhaustive: + default-signifies-exhaustive: true + + mnd: + ignored-numbers: + - "2" + + paralleltest: + ignore-missing: true + + perfsprint: + errorf: false + + testpackage: + skip-regexp: internal_test\.go + + varnamelen: + max-distance: 10 + + whitespace: + multi-if: true + multi-func: true issues: max-issues-per-linter: 0 diff --git a/dialects.go b/dialects.go index 038a523..08b1bb4 100644 --- a/dialects.go +++ b/dialects.go @@ -11,7 +11,7 @@ import ( // BaseDialect is a convenience type for databases that manage the necessary operations solely using // queries. Defining the CreateTemplate, AppliedTemplate and RegisterTemplate enables the BaseDialect to -// perform all the necessary operation to fulfill the Dialect interface. +// perform all the necessary operations to fulfill the Dialect interface. type BaseDialect struct { CreateTemplate string // statement ensuring the existence of the migration table AppliedTemplate string // statement getting applied migrations ordered by application date @@ -32,11 +32,13 @@ func (b BaseDialect) EnsureMigrationTableExists(db *sql.DB, tableName string) er if execErr != nil { rollbackErr := tx.Rollback() + return errors.Join(execErr, rollbackErr) } if err := tx.Commit(); err != nil { rollbackErr := tx.Rollback() + return errors.Join(err, rollbackErr) } diff --git a/dialects_test.go b/dialects_test.go index 279a874..c3206d9 100644 --- a/dialects_test.go +++ b/dialects_test.go @@ -1,7 +1,7 @@ // Copyright the DMorph contributors. // SPDX-License-Identifier: MPL-2.0 -package dmorph +package dmorph_test import ( "database/sql" @@ -10,6 +10,8 @@ import ( "testing" "github.com/stretchr/testify/assert" + + "github.com/AlphaOne1/dmorph" ) // TestDialectStatements verifies that each database dialect has valid and @@ -19,15 +21,15 @@ func TestDialectStatements(t *testing.T) { // that the statements for the databases are somehow filled tests := []struct { name string - caller func() BaseDialect + caller func() dmorph.BaseDialect }{ - {name: "CSVQ", caller: DialectCSVQ}, - {name: "DB2", caller: DialectDB2}, - {name: "MSSQL", caller: DialectMSSQL}, - {name: "MySQL", caller: DialectMySQL}, - {name: "Oracle", caller: DialectOracle}, - {name: "Postgres", caller: DialectPostgres}, - {name: "SQLite", caller: DialectSQLite}, + {name: "CSVQ", caller: dmorph.DialectCSVQ}, + {name: "DB2", caller: dmorph.DialectDB2}, + {name: "MSSQL", caller: dmorph.DialectMSSQL}, + {name: "MySQL", caller: dmorph.DialectMySQL}, + {name: "Oracle", caller: dmorph.DialectOracle}, + {name: "Postgres", caller: dmorph.DialectPostgres}, + {name: "SQLite", caller: dmorph.DialectSQLite}, } re := regexp.MustCompile("%s") @@ -77,17 +79,17 @@ func TestCallsOnClosedDB(t *testing.T) { } assert.Error(t, - DialectSQLite().EnsureMigrationTableExists(db, "irrelevant"), + dmorph.DialectSQLite().EnsureMigrationTableExists(db, "irrelevant"), "expected error on closed database") - _, err := DialectSQLite().AppliedMigrations(db, "irrelevant") + _, err := dmorph.DialectSQLite().AppliedMigrations(db, "irrelevant") assert.Error(t, err, "expected error on closed database") } // TestEnsureMigrationTableExistsSQLError tests the EnsureMigrationTableExists function // for handling SQL errors during execution. func TestEnsureMigrationTableExistsSQLError(t *testing.T) { - d := BaseDialect{ + d := dmorph.BaseDialect{ CreateTemplate: ` CRATE TABLE test ( id VARCHAR(255) PRIMARY KEY, @@ -114,9 +116,10 @@ func TestEnsureMigrationTableExistsSQLError(t *testing.T) { assert.Error(t, d.EnsureMigrationTableExists(db, "test"), "expected error") } -// TestEnsureMigrationTableExistsCommitError tests the behavior of EnsureMigrationTableExists when a commit error occurs. +// TestEnsureMigrationTableExistsCommitError tests the behavior of EnsureMigrationTableExists +// when a commit error occurs. func TestEnsureMigrationTableExistsCommitError(t *testing.T) { - d := BaseDialect{ + d := dmorph.BaseDialect{ CreateTemplate: ` CREATE TABLE t0 ( id INTEGER PRIMARY KEY diff --git a/exports_internal_test.go b/exports_internal_test.go new file mode 100644 index 0000000..0946320 --- /dev/null +++ b/exports_internal_test.go @@ -0,0 +1,15 @@ +package dmorph + +import "database/sql" + +// The exported names in this file are only used for internal testing and are not part of the public API. + +var ( + TapplyStepsStream = applyStepsStream + TmigrationFromFileFS = migrationFromFileFS + TmigrationOrder = migrationOrder +) + +func (m *Morpher) TapplyMigrations(db *sql.DB, lastMigration string) error { + return m.applyMigrations(db, lastMigration) +} diff --git a/file_migration.go b/file_migration.go index a3b732f..aad97fc 100644 --- a/file_migration.go +++ b/file_migration.go @@ -72,6 +72,7 @@ func WithMigrationsFromFS(d fs.ReadDirFS) MorphOption { if err == nil { for _, entry := range dirEntry { morpher.Log.Info("entry", slog.String("name", entry.Name())) + if entry.Type().IsRegular() { morpher.Migrations = append(morpher.Migrations, migrationFromFileFS(entry.Name(), d, morpher.Log)) @@ -83,7 +84,7 @@ func WithMigrationsFromFS(d fs.ReadDirFS) MorphOption { } } -// migrationFromFileFS creates a FileMigration instance for a specific migration file from an fs.FS directory. +// migrationFromFileFS creates a FileMigration instance for a specific migration file from a fs.FS directory. func migrationFromFileFS(name string, dir fs.FS, log *slog.Logger) FileMigration { return FileMigration{ Name: name, @@ -105,8 +106,8 @@ func migrationFromFileFS(name string, dir fs.FS, log *slog.Logger) FileMigration // applyStepsStream executes database migration steps read from an io.Reader, separated by semicolons, in a transaction. // Returns the corresponding error if any step execution fails. Also, as some database drivers or engines seem to not // support comments, leading comments are removed. This function does not undertake efforts to scan the SQL to find -// other comments. So leading comments telling what a step is going to do, work. But comments in the middle of a -// statement will not be removed. At least with SQLite this will lead to hard to find errors. +// other comments. Such leading comments telling what a step is going to do, work. But comments in the middle of a +// statement will not be removed. At least with SQLite this will lead to hard-to-find errors. func applyStepsStream(tx *sql.Tx, r io.Reader, id string, log *slog.Logger) error { buf := bytes.Buffer{} @@ -138,7 +139,7 @@ func applyStepsStream(tx *sql.Tx, r io.Reader, id string, log *slog.Logger) erro } } - // cleanup after, for final statement without the closing ; on a new line + // cleanup after, for the final statement without the closing `;` on a new line if buf.Len() > 0 { log.Info("migration step", slog.String("id", id), diff --git a/file_migration_test.go b/file_migration_test.go index 3140dbb..3f53610 100644 --- a/file_migration_test.go +++ b/file_migration_test.go @@ -1,7 +1,7 @@ // Copyright the DMorph contributors. // SPDX-License-Identifier: MPL-2.0 -package dmorph +package dmorph_test import ( "bytes" @@ -12,6 +12,8 @@ import ( "testing" "github.com/stretchr/testify/assert" + + "github.com/AlphaOne1/dmorph" ) func TestWithMigrationFromFile(t *testing.T) { @@ -31,9 +33,9 @@ func TestWithMigrationFromFile(t *testing.T) { defer func() { _ = db.Close() }() } - runErr := Run(db, - WithDialect(DialectSQLite()), - WithMigrationFromFile("testData/01_base_table.sql")) + runErr := dmorph.Run(db, + dmorph.WithDialect(dmorph.DialectSQLite()), + dmorph.WithMigrationFromFile("testData/01_base_table.sql")) assert.NoError(t, runErr, "did not expect an error") } @@ -55,21 +57,22 @@ func TestWithMigrationFromFileError(t *testing.T) { defer func() { _ = db.Close() }() } - runErr := Run(db, - WithDialect(DialectSQLite()), - WithMigrationFromFile("testData/00_non_existent.sql")) + runErr := dmorph.Run(db, + dmorph.WithDialect(dmorph.DialectSQLite()), + dmorph.WithMigrationFromFile("testData/00_non_existent.sql")) var pathErr *fs.PathError assert.ErrorAs(t, runErr, &pathErr, "unexpected error") } -// TestMigrationFromFileFSError validates that migrationFromFileFS returns an error when the specified file does not exist. +// TestMigrationFromFileFSError validates that migrationFromFileFS returns an error +// when the specified file does not exist. func TestMigrationFromFileFSError(t *testing.T) { dir, dirErr := os.OpenRoot("testData") assert.NoError(t, dirErr, "could not open test data directory") - mig := migrationFromFileFS("nonexistent", dir.FS(), slog.Default()) + mig := dmorph.TmigrationFromFileFS("nonexistent", dir.FS(), slog.Default()) err := mig.Migrate(nil) @@ -101,7 +104,7 @@ func TestApplyStepsStreamError(t *testing.T) { assert.NoError(t, txErr, "expected no tx error") - err := applyStepsStream(tx, &buf, "test", slog.Default()) + err := dmorph.TapplyStepsStream(tx, &buf, "test", slog.Default()) assert.Error(t, err, "expected error") @@ -114,7 +117,7 @@ func TestApplyStepsStreamError(t *testing.T) { buf.Reset() buf.WriteString("utter nonsense\n;") - err = applyStepsStream(tx, &buf, "test", slog.Default()) + err = dmorph.TapplyStepsStream(tx, &buf, "test", slog.Default()) assert.Error(t, err, "expected error") diff --git a/migration.go b/migration.go index 6d085a2..5e6c401 100644 --- a/migration.go +++ b/migration.go @@ -19,29 +19,29 @@ const MigrationTableName = "migrations" // ValidTableNameRex is the regular expression used to check if a given migration table name is valid. var ValidTableNameRex = regexp.MustCompile("^[a-zA-Z0-9_]+$") -// ErrMigrationsUnrelated signalizes, that the set of migrations to apply and the already applied set do not have the +// ErrMigrationsUnrelated signalizes that the set of migrations to apply and the already applied set do not have the // same (order of) applied migrations. Applying unrelated migrations could severely harm the database. var ErrMigrationsUnrelated = errors.New("migrations unrelated") -// ErrMigrationsUnsorted tells that the already applied migrations were not registered in the order (using the timestamp) -// that they should have been registered (using their id) +// ErrMigrationsUnsorted tells that the already applied migrations were not registered in the order +// (using the timestamp) that they should have been registered (using their id). var ErrMigrationsUnsorted = errors.New("migrations unsorted") // ErrNoDialect signalizes that no dialect for the database operations was chosen. var ErrNoDialect = errors.New("no dialect") -// ErrNoMigrations signalizes that no migrations were chosen to be applied +// ErrNoMigrations signalizes that no migrations were chosen to be applied. var ErrNoMigrations = errors.New("no migrations") -// ErrNoMigrationTable occurs if there is not migration table present +// ErrNoMigrationTable occurs if there is no migration table present. var ErrNoMigrationTable = errors.New("no migration table") -// ErrMigrationTableNameInvalid occurs if the migration table does not adhere to ValidTableNameRex +// ErrMigrationTableNameInvalid occurs if the migration table does not adhere to ValidTableNameRex. var ErrMigrationTableNameInvalid = errors.New("invalid migration table name") // ErrMigrationsTooOld signalizes that the migrations to be applied are older than the migrations that are already // present in the database. This error can occur when an older version of the application is started using a database -// that was used already by a newer version of the application. +// used already by a newer version of the application. var ErrMigrationsTooOld = errors.New("migrations too old") // Dialect is an interface describing the functionalities needed to manage migrations inside a database. @@ -80,10 +80,11 @@ type Morpher struct { // MorphOption is the type used for functional options. type MorphOption func(*Morpher) error -// WithDialect sets the vendor specific database dialect to be used. +// WithDialect sets the vendor-specific database dialect to be used. func WithDialect(dialect Dialect) MorphOption { return func(m *Morpher) error { m.Dialect = dialect + return nil } } @@ -92,6 +93,7 @@ func WithDialect(dialect Dialect) MorphOption { func WithMigrations(migrations ...Migration) MorphOption { return func(m *Morpher) error { m.Migrations = append(m.Migrations, migrations...) + return nil } } @@ -101,6 +103,7 @@ func WithMigrations(migrations ...Migration) MorphOption { func WithLog(log *slog.Logger) MorphOption { return func(m *Morpher) error { m.Log = log + return nil } } @@ -118,6 +121,7 @@ func WithTableName(tableName string) func(*Morpher) error { } m.TableName = tableName + return nil } } @@ -213,6 +217,7 @@ func (m *Morpher) applyMigrations(db *sql.DB, lastMigration string) error { for _, migration := range m.Migrations { if lastMigration >= migration.Key() { m.Log.Info("migration already applied", slog.String("file", migration.Key())) + continue } @@ -224,23 +229,26 @@ func (m *Morpher) applyMigrations(db *sql.DB, lastMigration string) error { return txBeginErr } - // even if we are sure to catch all possibilities, we use this as a safeguard that also with later - // modifications, if a successful commit cannot be done, at least the rollback is executed freeing + // Even if we are sure to catch all possibilities, we use this as a safeguard that also with later + // modifications. When a successful commit cannot be done, at least the rollback is executed, freeing // allocated resources of the transaction. defer func() { _ = tx.Rollback() }() if err := migration.Migrate(tx); err != nil { rollbackErr := tx.Rollback() + return errors.Join(err, rollbackErr) } if registerErr := m.Dialect.RegisterMigration(tx, migration.Key(), m.TableName); registerErr != nil { rollbackErr := tx.Rollback() + return errors.Join(registerErr, rollbackErr) } if commitErr := tx.Commit(); commitErr != nil { rollbackErr := tx.Rollback() + return errors.Join(commitErr, rollbackErr) } m.Log.Info("migration applied", @@ -248,6 +256,7 @@ func (m *Morpher) applyMigrations(db *sql.DB, lastMigration string) error { slog.Duration("duration", time.Since(startMigration)), ) } + return nil } @@ -256,6 +265,7 @@ func (m *Morpher) applyMigrations(db *sql.DB, lastMigration string) error { func (m *Morpher) checkAppliedMigrations(appliedMigrations []string) error { if !slices.IsSorted(appliedMigrations) { m.Log.Error("migrations not applied in order") + return ErrMigrationsUnsorted } @@ -265,16 +275,17 @@ func (m *Morpher) checkAppliedMigrations(appliedMigrations []string) error { if len(m.Migrations) < len(appliedMigrations) { // it is impossible to have a migration newer than the one already applied - // without having at least the same amount of previous migrations + // without having at least the same number of previous migrations return ErrMigrationsUnrelated } - // we know here, that there are at least as many migrations applied as we got to apply + // we know here that there are at least as many migrations applied as we got to apply for i := 0; i < len(appliedMigrations); i++ { if appliedMigrations[i] != m.Migrations[i].Key() { return ErrMigrationsUnrelated } } + return nil } diff --git a/migration_test.go b/migration_test.go index 5dfe7aa..a9eac47 100644 --- a/migration_test.go +++ b/migration_test.go @@ -1,7 +1,7 @@ // Copyright the DMorph contributors. // SPDX-License-Identifier: MPL-2.0 -package dmorph +package dmorph_test import ( "database/sql" @@ -13,6 +13,8 @@ import ( "github.com/stretchr/testify/assert" _ "modernc.org/sqlite" + + "github.com/AlphaOne1/dmorph" ) //go:embed testData @@ -23,7 +25,6 @@ func prepareDB() (string, error) { var result string dbFile, dbFileErr := os.CreateTemp("", "") - // dbFile, dbFileErr := os.Create("testdb.sqlite") if dbFileErr != nil { return "", dbFileErr @@ -58,9 +59,9 @@ func TestMigration(t *testing.T) { assert.NoError(t, migrationsDirErr, "migrations directory could not be opened") - runErr := Run(db, - WithDialect(DialectSQLite()), - WithMigrationsFromFS(migrationsDir.(fs.ReadDirFS))) + runErr := dmorph.Run(db, + dmorph.WithDialect(dmorph.DialectSQLite()), + dmorph.WithMigrationsFromFS(migrationsDir.(fs.ReadDirFS))) assert.NoError(t, runErr, "migrations could not be run") } @@ -87,15 +88,15 @@ func TestMigrationUpdate(t *testing.T) { assert.NoError(t, migrationsDirErr, "migrations directory could not be opened") - runErr := Run(db, - WithDialect(DialectSQLite()), - WithMigrationFromFileFS("01_base_table.sql", migrationsDir)) + runErr := dmorph.Run(db, + dmorph.WithDialect(dmorph.DialectSQLite()), + dmorph.WithMigrationFromFileFS("01_base_table.sql", migrationsDir)) assert.NoError(t, runErr, "preparation migrations could not be run") - runErr = Run(db, - WithDialect(DialectSQLite()), - WithMigrationsFromFS(migrationsDir.(fs.ReadDirFS))) + runErr = dmorph.Run(db, + dmorph.WithDialect(dmorph.DialectSQLite()), + dmorph.WithMigrationsFromFS(migrationsDir.(fs.ReadDirFS))) assert.NoError(t, runErr, "migrations could not be run") } @@ -105,6 +106,7 @@ type TestMigrationImpl struct{} func (m TestMigrationImpl) Key() string { return "TestMigration" } func (m TestMigrationImpl) Migrate(tx *sql.Tx) error { _, err := tx.Exec("CREATE TABLE t0 (id INTEGER PRIMARY KEY)") + return err } @@ -126,9 +128,9 @@ func TestWithMigrations(t *testing.T) { defer func() { _ = db.Close() }() } - runErr := Run(db, - WithDialect(DialectSQLite()), - WithMigrations(TestMigrationImpl{})) + runErr := dmorph.Run(db, + dmorph.WithDialect(dmorph.DialectSQLite()), + dmorph.WithMigrations(TestMigrationImpl{})) assert.NoError(t, runErr, "did not expect error") } @@ -136,12 +138,12 @@ func TestWithMigrations(t *testing.T) { // TestMigrationUnableToCreateMorpher tests to use the Run function without any // useful parameter. func TestMigrationUnableToCreateMorpher(t *testing.T) { - runErr := Run(nil) + runErr := dmorph.Run(nil) assert.Error(t, runErr, "morpher should not have run") } -// TestMigration tests what happens, if the applied migrations are too old. +// TestMigrationTooOld tests what happens if the applied migrations are too old. func TestMigrationTooOld(t *testing.T) { dbFile, dbFileErr := prepareDB() @@ -163,20 +165,20 @@ func TestMigrationTooOld(t *testing.T) { assert.NoError(t, migrationsDirErr, "migrations directory could not be opened") - runErr := Run(db, - WithDialect(DialectSQLite()), - WithMigrationsFromFS(migrationsDir.(fs.ReadDirFS))) + runErr := dmorph.Run(db, + dmorph.WithDialect(dmorph.DialectSQLite()), + dmorph.WithMigrationsFromFS(migrationsDir.(fs.ReadDirFS))) assert.NoError(t, runErr, "preparation migrations could not be run") - runErr = Run(db, - WithDialect(DialectSQLite()), - WithMigrationFromFileFS("01_base_table.sql", migrationsDir)) + runErr = dmorph.Run(db, + dmorph.WithDialect(dmorph.DialectSQLite()), + dmorph.WithMigrationFromFileFS("01_base_table.sql", migrationsDir)) - assert.ErrorIs(t, runErr, ErrMigrationsTooOld, "migrations did not give expected error") + assert.ErrorIs(t, runErr, dmorph.ErrMigrationsTooOld, "migrations did not give expected error") } -// TestMigrationUnrelated0 tests what happens, if the applied migrations are unrelated to existing ones. +// TestMigrationUnrelated0 tests what happens if the applied migrations are unrelated to existing ones. func TestMigrationUnrelated0(t *testing.T) { dbFile, dbFileErr := prepareDB() @@ -198,20 +200,20 @@ func TestMigrationUnrelated0(t *testing.T) { assert.NoError(t, migrationsDirErr, "migrations directory could not be opened") - runErr := Run(db, - WithDialect(DialectSQLite()), - WithMigrationsFromFS(migrationsDir.(fs.ReadDirFS))) + runErr := dmorph.Run(db, + dmorph.WithDialect(dmorph.DialectSQLite()), + dmorph.WithMigrationsFromFS(migrationsDir.(fs.ReadDirFS))) assert.NoError(t, runErr, "preparation migrations could not be run") - runErr = Run(db, - WithDialect(DialectSQLite()), - WithMigrationFromFileFS("02_addon_table.sql", migrationsDir)) + runErr = dmorph.Run(db, + dmorph.WithDialect(dmorph.DialectSQLite()), + dmorph.WithMigrationFromFileFS("02_addon_table.sql", migrationsDir)) - assert.ErrorIs(t, runErr, ErrMigrationsUnrelated, "migrations did not give expected error") + assert.ErrorIs(t, runErr, dmorph.ErrMigrationsUnrelated, "migrations did not give expected error") } -// TestMigrationUnrelated1 tests what happens, if the applied migrations are unrelated to existing ones. +// TestMigrationUnrelated1 tests what happens if the applied migrations are unrelated to existing ones. func TestMigrationUnrelated1(t *testing.T) { dbFile, dbFileErr := prepareDB() @@ -233,17 +235,17 @@ func TestMigrationUnrelated1(t *testing.T) { assert.NoError(t, migrationsDirErr, "migrations directory could not be opened") - runErr := Run(db, - WithDialect(DialectSQLite()), - WithMigrationFromFileFS("01_base_table.sql", migrationsDir)) + runErr := dmorph.Run(db, + dmorph.WithDialect(dmorph.DialectSQLite()), + dmorph.WithMigrationFromFileFS("01_base_table.sql", migrationsDir)) assert.NoError(t, runErr, "preparation migrations could not be run") - runErr = Run(db, - WithDialect(DialectSQLite()), - WithMigrationFromFileFS("02_addon_table.sql", migrationsDir)) + runErr = dmorph.Run(db, + dmorph.WithDialect(dmorph.DialectSQLite()), + dmorph.WithMigrationFromFileFS("02_addon_table.sql", migrationsDir)) - assert.ErrorIs(t, runErr, ErrMigrationsUnrelated, "migrations did not give expected error") + assert.ErrorIs(t, runErr, dmorph.ErrMigrationsUnrelated, "migrations did not give expected error") } // TestMigrationAppliedUnordered tests the case, that somehow the migrations in the @@ -269,7 +271,7 @@ func TestMigrationAppliedUnordered(t *testing.T) { assert.NoError(t, migrationsDirErr, "migrations directory could not be opened") - assert.NoError(t, DialectSQLite().EnsureMigrationTableExists(db, "migrations")) + assert.NoError(t, dmorph.DialectSQLite().EnsureMigrationTableExists(db, "migrations")) _, execErr := db.Exec(` INSERT INTO migrations (id, create_ts) VALUES ('01_base_table', '2021-01-02 00:00:00'); @@ -278,42 +280,42 @@ func TestMigrationAppliedUnordered(t *testing.T) { assert.NoError(t, execErr, "unordered test could not be prepared") - runErr := Run(db, - WithDialect(DialectSQLite()), - WithMigrationsFromFS(migrationsDir.(fs.ReadDirFS))) + runErr := dmorph.Run(db, + dmorph.WithDialect(dmorph.DialectSQLite()), + dmorph.WithMigrationsFromFS(migrationsDir.(fs.ReadDirFS))) assert.ErrorIs(t, runErr, - ErrMigrationsUnsorted, + dmorph.ErrMigrationsUnsorted, "migrations did not give expected error") } // TestMigrationOrder checks that the migrations ordering function works as expected. func TestMigrationOrder(t *testing.T) { tests := []struct { - m0 Migration - m1 Migration + m0 dmorph.Migration + m1 dmorph.Migration order int }{ { - m0: FileMigration{Name: "01"}, - m1: FileMigration{Name: "01"}, + m0: dmorph.FileMigration{Name: "01"}, + m1: dmorph.FileMigration{Name: "01"}, order: 0, }, { - m0: FileMigration{Name: "01"}, - m1: FileMigration{Name: "02"}, + m0: dmorph.FileMigration{Name: "01"}, + m1: dmorph.FileMigration{Name: "02"}, order: -1, }, { - m0: FileMigration{Name: "02"}, - m1: FileMigration{Name: "01"}, + m0: dmorph.FileMigration{Name: "02"}, + m1: dmorph.FileMigration{Name: "01"}, order: 1, }, } for k, v := range tests { - res := migrationOrder(v.m0, v.m1) + res := dmorph.TmigrationOrder(v.m0, v.m1) assert.Equal(t, v.order, res, "order of migrations is wrong for test %v", k) } @@ -322,48 +324,48 @@ func TestMigrationOrder(t *testing.T) { // TestMigrationIsValid checks the validity checks for migrations. func TestMigrationIsValid(t *testing.T) { tests := []struct { - m Morpher + m dmorph.Morpher err error }{ { - m: Morpher{ - Dialect: DialectSQLite(), - Migrations: []Migration{FileMigration{Name: "01"}}, + m: dmorph.Morpher{ + Dialect: dmorph.DialectSQLite(), + Migrations: []dmorph.Migration{dmorph.FileMigration{Name: "01"}}, TableName: "migrations", }, err: nil, }, { - m: Morpher{ + m: dmorph.Morpher{ Dialect: nil, - Migrations: []Migration{FileMigration{Name: "01"}}, + Migrations: []dmorph.Migration{dmorph.FileMigration{Name: "01"}}, TableName: "migrations", }, - err: ErrNoDialect, + err: dmorph.ErrNoDialect, }, { - m: Morpher{ - Dialect: DialectSQLite(), + m: dmorph.Morpher{ + Dialect: dmorph.DialectSQLite(), Migrations: nil, TableName: "migrations", }, - err: ErrNoMigrations, + err: dmorph.ErrNoMigrations, }, { - m: Morpher{ - Dialect: DialectSQLite(), - Migrations: []Migration{FileMigration{Name: "01"}}, + m: dmorph.Morpher{ + Dialect: dmorph.DialectSQLite(), + Migrations: []dmorph.Migration{dmorph.FileMigration{Name: "01"}}, TableName: "", }, - err: ErrNoMigrationTable, + err: dmorph.ErrNoMigrationTable, }, { - m: Morpher{ - Dialect: DialectSQLite(), - Migrations: []Migration{FileMigration{Name: "01"}}, + m: dmorph.Morpher{ + Dialect: dmorph.DialectSQLite(), + Migrations: []dmorph.Migration{dmorph.FileMigration{Name: "01"}}, TableName: "blah(); DROP TABLE blah;", }, - err: ErrMigrationTableNameInvalid, + err: dmorph.ErrMigrationTableNameInvalid, }, } @@ -381,10 +383,10 @@ func TestMigrationWithLogger(t *testing.T) { Level: slog.LevelWarn, })) - morpher, err := NewMorpher( - WithDialect(DialectSQLite()), - WithMigrationFromFile("testData/01_base_table.sql"), - WithLog(l), + morpher, err := dmorph.NewMorpher( + dmorph.WithDialect(dmorph.DialectSQLite()), + dmorph.WithMigrationFromFile("testData/01_base_table.sql"), + dmorph.WithLog(l), ) assert.NoError(t, err, "morpher could not be created") @@ -393,8 +395,8 @@ func TestMigrationWithLogger(t *testing.T) { // TestMigrationWithoutMigrations ensures that creating a Morpher instance without migrations results in an error. func TestMigrationWithoutMigrations(t *testing.T) { - _, err := NewMorpher( - WithDialect(DialectSQLite()), + _, err := dmorph.NewMorpher( + dmorph.WithDialect(dmorph.DialectSQLite()), ) assert.Error(t, err, "morpher created without migrations") @@ -403,10 +405,10 @@ func TestMigrationWithoutMigrations(t *testing.T) { // TestMigrationWithTableNameValid verifies the correct creation of a Morpher // with a valid custom table name configuration. func TestMigrationWithTableNameValid(t *testing.T) { - morpher, err := NewMorpher( - WithDialect(DialectSQLite()), - WithMigrationFromFile("testData/01_base_table.sql"), - WithTableName("dimorphodon"), + morpher, err := dmorph.NewMorpher( + dmorph.WithDialect(dmorph.DialectSQLite()), + dmorph.WithMigrationFromFile("testData/01_base_table.sql"), + dmorph.WithTableName("dimorphodon"), ) assert.NoError(t, err, "morpher could not be created") @@ -416,10 +418,10 @@ func TestMigrationWithTableNameValid(t *testing.T) { // TestMigrationWithTableNameInvalidSize verifies that creating a Morpher // with an invalid table name size produces an error. func TestMigrationWithTableNameInvalidSize(t *testing.T) { - _, err := NewMorpher( - WithDialect(DialectSQLite()), - WithMigrationFromFile("testData/01_base_table.sql"), - WithTableName(""), + _, err := dmorph.NewMorpher( + dmorph.WithDialect(dmorph.DialectSQLite()), + dmorph.WithMigrationFromFile("testData/01_base_table.sql"), + dmorph.WithTableName(""), ) assert.Error(t, err, "morpher could created with empty table name") @@ -428,10 +430,10 @@ func TestMigrationWithTableNameInvalidSize(t *testing.T) { // TestMigrationWithTableNameInvalidChars ensures that creating a Morpher // fails when the table name contains invalid characters. func TestMigrationWithTableNameInvalidChars(t *testing.T) { - _, err := NewMorpher( - WithDialect(DialectSQLite()), - WithMigrationFromFile("testData/01_base_table.sql"), - WithTableName("di/mor/pho/don"), + _, err := dmorph.NewMorpher( + dmorph.WithDialect(dmorph.DialectSQLite()), + dmorph.WithMigrationFromFile("testData/01_base_table.sql"), + dmorph.WithTableName("di/mor/pho/don"), ) assert.Error(t, err, "morpher could created with invalid table name") @@ -439,7 +441,7 @@ func TestMigrationWithTableNameInvalidChars(t *testing.T) { // TestMigrationRunInvalid verifies that running a Morpher with invalid configuration results in an error. func TestMigrationRunInvalid(t *testing.T) { - morpher := Morpher{} + morpher := dmorph.Morpher{} runErr := morpher.Run(nil) @@ -465,12 +467,12 @@ func TestMigrationRunInvalidCreate(t *testing.T) { defer func() { _ = db.Close() }() } - dialect := DialectSQLite() + dialect := dmorph.DialectSQLite() dialect.CreateTemplate = "utter nonsense" - morpher, morpherErr := NewMorpher( - WithDialect(dialect), - WithMigrationFromFile("testData/01_base_table.sql")) + morpher, morpherErr := dmorph.NewMorpher( + dmorph.WithDialect(dialect), + dmorph.WithMigrationFromFile("testData/01_base_table.sql")) assert.NoError(t, morpherErr, "morpher could not be created") @@ -497,12 +499,12 @@ func TestMigrationRunInvalidApplied(t *testing.T) { defer func() { _ = db.Close() }() } - dialect := DialectSQLite() + dialect := dmorph.DialectSQLite() dialect.AppliedTemplate = "utter nonsense" - morpher, morpherErr := NewMorpher( - WithDialect(dialect), - WithMigrationFromFile("testData/01_base_table.sql")) + morpher, morpherErr := dmorph.NewMorpher( + dmorph.WithDialect(dialect), + dmorph.WithMigrationFromFile("testData/01_base_table.sql")) assert.NoError(t, morpherErr, "morpher could not be created") @@ -529,14 +531,14 @@ func TestMigrationApplyInvalidDB(t *testing.T) { _ = db.Close() } - morpher, morpherErr := NewMorpher( - WithDialect(DialectSQLite()), - WithMigrationFromFile("testData/01_base_table.sql")) + morpher, morpherErr := dmorph.NewMorpher( + dmorph.WithDialect(dmorph.DialectSQLite()), + dmorph.WithMigrationFromFile("testData/01_base_table.sql")) assert.NoError(t, morpherErr, "morpher could not be created") assert.Error(t, - morpher.applyMigrations(db, "irrelevant"), + morpher.TapplyMigrations(db, "irrelevant"), "morpher should error on invalid DB") } @@ -558,22 +560,22 @@ func TestMigrationApplyUnableRegister(t *testing.T) { defer func() { _ = db.Close() }() } - morpher, morpherErr := NewMorpher( - WithDialect(DialectSQLite()), - WithMigrationFromFile("testData/01_base_table.sql")) + morpher, morpherErr := dmorph.NewMorpher( + dmorph.WithDialect(dmorph.DialectSQLite()), + dmorph.WithMigrationFromFile("testData/01_base_table.sql")) assert.NoError(t, morpherErr, "morpher could not be created") - d, _ := morpher.Dialect.(BaseDialect) + d, _ := morpher.Dialect.(dmorph.BaseDialect) d.RegisterTemplate = "utter nonsense" morpher.Dialect = d assert.Error(t, - morpher.applyMigrations(db, ""), + morpher.TapplyMigrations(db, ""), "morpher should fail to register") } -// TestMigrationApplyUnableCommit tests the scenario where migration application fails +// TestMigrationApplyUnableCommit tests the scenario where a migration application fails // due to inability to commit a transaction. func TestMigrationApplyUnableCommit(t *testing.T) { dbFile, dbFileErr := prepareDB() @@ -592,16 +594,16 @@ func TestMigrationApplyUnableCommit(t *testing.T) { defer func() { _ = db.Close() }() } - morpher, morpherErr := NewMorpher( - WithDialect(DialectSQLite()), - WithMigrationFromFile("testData/01_base_table.sql")) + morpher, morpherErr := dmorph.NewMorpher( + dmorph.WithDialect(dmorph.DialectSQLite()), + dmorph.WithMigrationFromFile("testData/01_base_table.sql")) assert.NoError(t, morpherErr, "morpher could not be created") _, execErr := db.Exec("PRAGMA foreign_keys = ON") assert.NoError(t, execErr, "foreign keys checking could not be enabled") - d, _ := morpher.Dialect.(BaseDialect) + d, _ := morpher.Dialect.(dmorph.BaseDialect) d.RegisterTemplate = ` CREATE TABLE t0 ( id INTEGER PRIMARY KEY @@ -621,6 +623,6 @@ func TestMigrationApplyUnableCommit(t *testing.T) { morpher.Dialect = d assert.Error(t, - morpher.applyMigrations(db, ""), + morpher.TapplyMigrations(db, ""), "morpher should fail to register") } From 663fb3ccda58cbe42d5f16f968ccbfa529cd8605 Mon Sep 17 00:00:00 2001 From: Alexander Adam Date: Tue, 12 Aug 2025 17:34:24 +0200 Subject: [PATCH 02/11] Refactor migration logic, enhance tests, and improve buffer handling - Simplified `for` loop in `migration.go` using `range`. - Replaced `assert` with `require` in tests for stricter checks. - Updated scanner buffer handling in `applyStepsStream` for performance. - Adjusted `.golangci.yaml` linter rules to include `require`. - Minor refactoring in `dialects.go` to inline error handling. - Clarified comments in `codeql.yml`. --- .github/workflows/codeql.yml | 2 +- .golangci.yaml | 5 +---- dialects.go | 6 +----- dialects_test.go | 4 ++-- file_migration.go | 4 ++++ migration.go | 2 +- migration_test.go | 21 +++++++++++++-------- 7 files changed, 23 insertions(+), 21 deletions(-) diff --git a/.github/workflows/codeql.yml b/.github/workflows/codeql.yml index 0a786a1..ccd22ae 100644 --- a/.github/workflows/codeql.yml +++ b/.github/workflows/codeql.yml @@ -82,7 +82,7 @@ jobs: # For more details on CodeQL's query packs, refer to: https://docs.github.com/en/code-security/code-scanning/automatically-scanning-your-code-for-vulnerabilities-and-errors/configuring-code-scanning#using-queries-in-ql-packs # queries: security-extended,security-and-quality - # If the analyze-step fails for one of the languages you are analyzing with + # If the "analyze" step fails for one of the languages you are analyzing with # "We were unable to automatically build your code", modify the matrix above # to set the build mode to "manual" for that language. Then modify this step # to build your code. diff --git a/.golangci.yaml b/.golangci.yaml index 43fadae..0dd0821 100644 --- a/.golangci.yaml +++ b/.golangci.yaml @@ -53,17 +53,14 @@ linters: - "!**/examples/**/*" allow: - $gostd - - gopkg.in/yaml.v3 - - github.com/Masterminds/sprig/v3 test: files: - $test - "**/examples/**/*" allow: - $gostd - - gopkg.in/yaml.v3 - - github.com/Masterminds/sprig/v3 - github.com/stretchr/testify/assert + - github.com/stretchr/testify/require - github.com/AlphaOne1/dmorph - modernc.org/sqlite diff --git a/dialects.go b/dialects.go index 08b1bb4..df6dda8 100644 --- a/dialects.go +++ b/dialects.go @@ -26,11 +26,7 @@ func (b BaseDialect) EnsureMigrationTableExists(db *sql.DB, tableName string) er return err } - defer func() { _ = tx.Rollback() }() - - _, execErr := tx.Exec(fmt.Sprintf(b.CreateTemplate, tableName)) - - if execErr != nil { + if _, execErr := tx.Exec(fmt.Sprintf(b.CreateTemplate, tableName)); execErr != nil { rollbackErr := tx.Rollback() return errors.Join(execErr, rollbackErr) diff --git a/dialects_test.go b/dialects_test.go index c3206d9..8f00aba 100644 --- a/dialects_test.go +++ b/dialects_test.go @@ -89,7 +89,7 @@ func TestCallsOnClosedDB(t *testing.T) { // TestEnsureMigrationTableExistsSQLError tests the EnsureMigrationTableExists function // for handling SQL errors during execution. func TestEnsureMigrationTableExistsSQLError(t *testing.T) { - d := dmorph.BaseDialect{ + dialect := dmorph.BaseDialect{ CreateTemplate: ` CRATE TABLE test ( id VARCHAR(255) PRIMARY KEY, @@ -113,7 +113,7 @@ func TestEnsureMigrationTableExistsSQLError(t *testing.T) { defer func() { _ = db.Close() }() } - assert.Error(t, d.EnsureMigrationTableExists(db, "test"), "expected error") + assert.Error(t, dialect.EnsureMigrationTableExists(db, "test"), "expected error") } // TestEnsureMigrationTableExistsCommitError tests the behavior of EnsureMigrationTableExists diff --git a/file_migration.go b/file_migration.go index aad97fc..f894ed6 100644 --- a/file_migration.go +++ b/file_migration.go @@ -109,9 +109,13 @@ func migrationFromFileFS(name string, dir fs.FS, log *slog.Logger) FileMigration // other comments. Such leading comments telling what a step is going to do, work. But comments in the middle of a // statement will not be removed. At least with SQLite this will lead to hard-to-find errors. func applyStepsStream(tx *sql.Tx, r io.Reader, id string, log *slog.Logger) error { + const InitialScannerBufSize = 64 * 1024 + const MaxScannerBufSize = 1024 * 1024 + buf := bytes.Buffer{} scanner := bufio.NewScanner(r) + scanner.Buffer(make([]byte, 0, InitialScannerBufSize), MaxScannerBufSize) newStep := true var i int diff --git a/migration.go b/migration.go index 5e6c401..51f984f 100644 --- a/migration.go +++ b/migration.go @@ -280,7 +280,7 @@ func (m *Morpher) checkAppliedMigrations(appliedMigrations []string) error { } // we know here that there are at least as many migrations applied as we got to apply - for i := 0; i < len(appliedMigrations); i++ { + for i := range appliedMigrations { if appliedMigrations[i] != m.Migrations[i].Key() { return ErrMigrationsUnrelated } diff --git a/migration_test.go b/migration_test.go index a9eac47..12e0a6a 100644 --- a/migration_test.go +++ b/migration_test.go @@ -12,6 +12,7 @@ import ( "testing" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" _ "modernc.org/sqlite" "github.com/AlphaOne1/dmorph" @@ -163,13 +164,13 @@ func TestMigrationTooOld(t *testing.T) { migrationsDir, migrationsDirErr := fs.Sub(testMigrationsDir, "testData") - assert.NoError(t, migrationsDirErr, "migrations directory could not be opened") + require.NoError(t, migrationsDirErr, "migrations directory could not be opened") runErr := dmorph.Run(db, dmorph.WithDialect(dmorph.DialectSQLite()), dmorph.WithMigrationsFromFS(migrationsDir.(fs.ReadDirFS))) - assert.NoError(t, runErr, "preparation migrations could not be run") + require.NoError(t, runErr, "preparation migrations could not be run") runErr = dmorph.Run(db, dmorph.WithDialect(dmorph.DialectSQLite()), @@ -535,7 +536,7 @@ func TestMigrationApplyInvalidDB(t *testing.T) { dmorph.WithDialect(dmorph.DialectSQLite()), dmorph.WithMigrationFromFile("testData/01_base_table.sql")) - assert.NoError(t, morpherErr, "morpher could not be created") + require.NoError(t, morpherErr, "morpher could not be created") assert.Error(t, morpher.TapplyMigrations(db, "irrelevant"), @@ -564,9 +565,11 @@ func TestMigrationApplyUnableRegister(t *testing.T) { dmorph.WithDialect(dmorph.DialectSQLite()), dmorph.WithMigrationFromFile("testData/01_base_table.sql")) - assert.NoError(t, morpherErr, "morpher could not be created") + require.NoError(t, morpherErr, "morpher could not be created") + + d, dialectOK := morpher.Dialect.(dmorph.BaseDialect) + require.True(t, dialectOK, "dialect is not a BaseDialect") - d, _ := morpher.Dialect.(dmorph.BaseDialect) d.RegisterTemplate = "utter nonsense" morpher.Dialect = d @@ -598,12 +601,14 @@ func TestMigrationApplyUnableCommit(t *testing.T) { dmorph.WithDialect(dmorph.DialectSQLite()), dmorph.WithMigrationFromFile("testData/01_base_table.sql")) - assert.NoError(t, morpherErr, "morpher could not be created") + require.NoError(t, morpherErr, "morpher could not be created") _, execErr := db.Exec("PRAGMA foreign_keys = ON") - assert.NoError(t, execErr, "foreign keys checking could not be enabled") + require.NoError(t, execErr, "foreign keys checking could not be enabled") + + d, dialectOK := morpher.Dialect.(dmorph.BaseDialect) + require.True(t, dialectOK, "dialect is not a BaseDialect") - d, _ := morpher.Dialect.(dmorph.BaseDialect) d.RegisterTemplate = ` CREATE TABLE t0 ( id INTEGER PRIMARY KEY From c63f2745334512a8d733082e0c03b8c29a16eb66 Mon Sep 17 00:00:00 2001 From: Alexander Adam Date: Tue, 12 Aug 2025 22:28:10 +0200 Subject: [PATCH 03/11] Refactor tests and migration logic for safety and clarity - Replaced `assert` with `require` in tests for stricter validation. - Updated migration directory handling with explicit safety comments. - Adjusted buffer writes to fix potential order issues in `applyStepsStream`. - Added safety net with deferred transaction rollback in `dialects.go`. --- dialects.go | 11 +++++++++++ file_migration.go | 4 ++-- migration_test.go | 8 +++++--- 3 files changed, 18 insertions(+), 5 deletions(-) diff --git a/dialects.go b/dialects.go index df6dda8..5aaab36 100644 --- a/dialects.go +++ b/dialects.go @@ -26,18 +26,29 @@ func (b BaseDialect) EnsureMigrationTableExists(db *sql.DB, tableName string) er return err } + // Safety net for unexpected panics + defer func() { + if tx != nil { + _ = tx.Rollback() + } + }() + if _, execErr := tx.Exec(fmt.Sprintf(b.CreateTemplate, tableName)); execErr != nil { rollbackErr := tx.Rollback() + tx = nil return errors.Join(execErr, rollbackErr) } if err := tx.Commit(); err != nil { rollbackErr := tx.Rollback() + tx = nil return errors.Join(err, rollbackErr) } + tx = nil + return nil } diff --git a/file_migration.go b/file_migration.go index f894ed6..6aa004c 100644 --- a/file_migration.go +++ b/file_migration.go @@ -120,8 +120,6 @@ func applyStepsStream(tx *sql.Tx, r io.Reader, id string, log *slog.Logger) erro var i int for i = 0; scanner.Scan(); { - buf.Write(scanner.Bytes()) - if newStep && strings.HasPrefix(scanner.Text(), "--") { // skip leading comments continue @@ -129,6 +127,8 @@ func applyStepsStream(tx *sql.Tx, r io.Reader, id string, log *slog.Logger) erro newStep = false + buf.Write(scanner.Bytes()) + if scanner.Text() == ";" { log.Info("migration step", slog.String("id", id), diff --git a/migration_test.go b/migration_test.go index 12e0a6a..62a4021 100644 --- a/migration_test.go +++ b/migration_test.go @@ -58,11 +58,12 @@ func TestMigration(t *testing.T) { migrationsDir, migrationsDirErr := fs.Sub(testMigrationsDir, "testData") - assert.NoError(t, migrationsDirErr, "migrations directory could not be opened") + require.NoError(t, migrationsDirErr, "migrations directory could not be opened") runErr := dmorph.Run(db, dmorph.WithDialect(dmorph.DialectSQLite()), - dmorph.WithMigrationsFromFS(migrationsDir.(fs.ReadDirFS))) + dmorph.WithMigrationsFromFS( + migrationsDir.(fs.ReadDirFS))) // Safe: migrationDir guaranteed to implement fs.ReadDirFS assert.NoError(t, runErr, "migrations could not be run") } @@ -97,7 +98,8 @@ func TestMigrationUpdate(t *testing.T) { runErr = dmorph.Run(db, dmorph.WithDialect(dmorph.DialectSQLite()), - dmorph.WithMigrationsFromFS(migrationsDir.(fs.ReadDirFS))) + dmorph.WithMigrationsFromFS( + migrationsDir.(fs.ReadDirFS))) // Safe: migrationDir guaranteed to implement fs.ReadDirFS assert.NoError(t, runErr, "migrations could not be run") } From f479d28af0e400f280cf8c5ec486cc4f869a289b Mon Sep 17 00:00:00 2001 From: Alexander Adam Date: Wed, 13 Aug 2025 21:19:29 +0200 Subject: [PATCH 04/11] Add `context.Context` to migration functions and tests for consistency - Updated all migration methods to include `context.Context` as the first argument. - Refactored related tests to use `context.Background()` where necessary. - Adjusted `applyStepsStream` and SQL execution calls to utilize context-aware methods. - Improved function signatures in `dialects.go` and `file_migration.go` for better consistency with Go best practices. --- dialects.go | 15 +++++----- dialects_test.go | 9 +++--- exports_internal_test.go | 9 ++++-- file_migration.go | 47 ++++++++++++++++++------------- file_migration_test.go | 13 +++++---- migration.go | 29 +++++++++---------- migration_test.go | 60 ++++++++++++++++++++++++---------------- 7 files changed, 106 insertions(+), 76 deletions(-) diff --git a/dialects.go b/dialects.go index 5aaab36..b65c0ab 100644 --- a/dialects.go +++ b/dialects.go @@ -4,6 +4,7 @@ package dmorph import ( + "context" "database/sql" "errors" "fmt" @@ -19,8 +20,8 @@ type BaseDialect struct { } // EnsureMigrationTableExists ensures that the migration table, saving the applied migrations ids, exists. -func (b BaseDialect) EnsureMigrationTableExists(db *sql.DB, tableName string) error { - tx, err := db.Begin() +func (b BaseDialect) EnsureMigrationTableExists(ctx context.Context, db *sql.DB, tableName string) error { + tx, err := db.BeginTx(ctx, nil) if err != nil { return err @@ -33,7 +34,7 @@ func (b BaseDialect) EnsureMigrationTableExists(db *sql.DB, tableName string) er } }() - if _, execErr := tx.Exec(fmt.Sprintf(b.CreateTemplate, tableName)); execErr != nil { + if _, execErr := tx.ExecContext(ctx, fmt.Sprintf(b.CreateTemplate, tableName)); execErr != nil { rollbackErr := tx.Rollback() tx = nil @@ -53,8 +54,8 @@ func (b BaseDialect) EnsureMigrationTableExists(db *sql.DB, tableName string) er } // AppliedMigrations gets the already applied migrations from the database, ordered by application date. -func (b BaseDialect) AppliedMigrations(db *sql.DB, tableName string) ([]string, error) { - rows, rowsErr := db.Query(fmt.Sprintf(b.AppliedTemplate, tableName)) +func (b BaseDialect) AppliedMigrations(ctx context.Context, db *sql.DB, tableName string) ([]string, error) { + rows, rowsErr := db.QueryContext(ctx, fmt.Sprintf(b.AppliedTemplate, tableName)) if rowsErr != nil { return nil, rowsErr @@ -76,8 +77,8 @@ func (b BaseDialect) AppliedMigrations(db *sql.DB, tableName string) ([]string, } // RegisterMigration registers a migration in the migration table. -func (b BaseDialect) RegisterMigration(tx *sql.Tx, id string, tableName string) error { - _, err := tx.Exec(fmt.Sprintf(b.RegisterTemplate, tableName), +func (b BaseDialect) RegisterMigration(ctx context.Context, tx *sql.Tx, id string, tableName string) error { + _, err := tx.ExecContext(ctx, fmt.Sprintf(b.RegisterTemplate, tableName), sql.Named("id", id)) return err diff --git a/dialects_test.go b/dialects_test.go index 8f00aba..36dda08 100644 --- a/dialects_test.go +++ b/dialects_test.go @@ -4,6 +4,7 @@ package dmorph_test import ( + "context" "database/sql" "os" "regexp" @@ -79,10 +80,10 @@ func TestCallsOnClosedDB(t *testing.T) { } assert.Error(t, - dmorph.DialectSQLite().EnsureMigrationTableExists(db, "irrelevant"), + dmorph.DialectSQLite().EnsureMigrationTableExists(context.Background(), db, "irrelevant"), "expected error on closed database") - _, err := dmorph.DialectSQLite().AppliedMigrations(db, "irrelevant") + _, err := dmorph.DialectSQLite().AppliedMigrations(context.Background(), db, "irrelevant") assert.Error(t, err, "expected error on closed database") } @@ -113,7 +114,7 @@ func TestEnsureMigrationTableExistsSQLError(t *testing.T) { defer func() { _ = db.Close() }() } - assert.Error(t, dialect.EnsureMigrationTableExists(db, "test"), "expected error") + assert.Error(t, dialect.EnsureMigrationTableExists(context.Background(), db, "test"), "expected error") } // TestEnsureMigrationTableExistsCommitError tests the behavior of EnsureMigrationTableExists @@ -157,5 +158,5 @@ func TestEnsureMigrationTableExistsCommitError(t *testing.T) { assert.NoError(t, execErr, "foreign keys checking could not be enabled") - assert.Error(t, d.EnsureMigrationTableExists(db, "test"), "expected error") + assert.Error(t, d.EnsureMigrationTableExists(context.Background(), db, "test"), "expected error") } diff --git a/exports_internal_test.go b/exports_internal_test.go index 0946320..8893598 100644 --- a/exports_internal_test.go +++ b/exports_internal_test.go @@ -1,6 +1,9 @@ package dmorph -import "database/sql" +import ( + "context" + "database/sql" +) // The exported names in this file are only used for internal testing and are not part of the public API. @@ -10,6 +13,6 @@ var ( TmigrationOrder = migrationOrder ) -func (m *Morpher) TapplyMigrations(db *sql.DB, lastMigration string) error { - return m.applyMigrations(db, lastMigration) +func (m *Morpher) TapplyMigrations(ctx context.Context, db *sql.DB, lastMigration string) error { + return m.applyMigrations(ctx, db, lastMigration) } diff --git a/file_migration.go b/file_migration.go index 6aa004c..55ec013 100644 --- a/file_migration.go +++ b/file_migration.go @@ -6,6 +6,7 @@ package dmorph import ( "bufio" "bytes" + "context" "database/sql" "io" "io/fs" @@ -18,7 +19,7 @@ import ( type FileMigration struct { Name string FS fs.FS - migrationFunc func(tx *sql.Tx, migration string) error + migrationFunc func(ctx context.Context, tx *sql.Tx, migration string) error } // Key returns the key of the migration to register in the migration table. @@ -27,8 +28,8 @@ func (f FileMigration) Key() string { } // Migrate executes the migration on the given transaction. -func (f FileMigration) Migrate(tx *sql.Tx) error { - return f.migrationFunc(tx, f.Name) +func (f FileMigration) Migrate(ctx context.Context, tx *sql.Tx) error { + return f.migrationFunc(ctx, tx, f.Name) } // WithMigrationFromFile generates a FileMigration that will run the content of the given file. @@ -36,7 +37,7 @@ func WithMigrationFromFile(name string) MorphOption { return func(morpher *Morpher) error { morpher.Migrations = append(morpher.Migrations, FileMigration{ Name: name, - migrationFunc: func(tx *sql.Tx, migration string) error { + migrationFunc: func(ctx context.Context, tx *sql.Tx, migration string) error { m, mErr := os.Open(migration) if mErr != nil { @@ -45,7 +46,7 @@ func WithMigrationFromFile(name string) MorphOption { defer func() { _ = m.Close() }() - return applyStepsStream(tx, m, migration, morpher.Log) + return applyStepsStream(ctx, tx, m, migration, morpher.Log) }, }) @@ -89,7 +90,7 @@ func migrationFromFileFS(name string, dir fs.FS, log *slog.Logger) FileMigration return FileMigration{ Name: name, FS: dir, - migrationFunc: func(tx *sql.Tx, migration string) error { + migrationFunc: func(ctx context.Context, tx *sql.Tx, migration string) error { m, mErr := dir.Open(migration) if mErr != nil { @@ -98,7 +99,7 @@ func migrationFromFileFS(name string, dir fs.FS, log *slog.Logger) FileMigration defer func() { _ = m.Close() }() - return applyStepsStream(tx, m, migration, log) + return applyStepsStream(ctx, tx, m, migration, log) }, } } @@ -108,7 +109,7 @@ func migrationFromFileFS(name string, dir fs.FS, log *slog.Logger) FileMigration // support comments, leading comments are removed. This function does not undertake efforts to scan the SQL to find // other comments. Such leading comments telling what a step is going to do, work. But comments in the middle of a // statement will not be removed. At least with SQLite this will lead to hard-to-find errors. -func applyStepsStream(tx *sql.Tx, r io.Reader, id string, log *slog.Logger) error { +func applyStepsStream(ctx context.Context, tx *sql.Tx, r io.Reader, id string, log *slog.Logger) error { const InitialScannerBufSize = 64 * 1024 const MaxScannerBufSize = 1024 * 1024 @@ -117,40 +118,48 @@ func applyStepsStream(tx *sql.Tx, r io.Reader, id string, log *slog.Logger) erro scanner := bufio.NewScanner(r) scanner.Buffer(make([]byte, 0, InitialScannerBufSize), MaxScannerBufSize) newStep := true - var i int + var step int - for i = 0; scanner.Scan(); { + for step = 0; scanner.Scan(); { if newStep && strings.HasPrefix(scanner.Text(), "--") { // skip leading comments continue } - newStep = false - - buf.Write(scanner.Bytes()) - if scanner.Text() == ";" { log.Info("migration step", slog.String("id", id), - slog.Int("step", i), + slog.Int("step", step), ) - if _, err := tx.Exec(buf.String()); err != nil { + + if _, err := tx.ExecContext(ctx, buf.String()); err != nil { return err } buf.Reset() - i++ + newStep = true + step++ + + continue + } + + // Append the current line (preserve formatting by adding a newline between lines) + if buf.Len() > 0 { + buf.WriteByte('\n') } + + buf.Write(scanner.Bytes()) + newStep = false } // cleanup after, for the final statement without the closing `;` on a new line if buf.Len() > 0 { log.Info("migration step", slog.String("id", id), - slog.Int("step", i), + slog.Int("step", step), ) - if _, err := tx.Exec(buf.String()); err != nil { + if _, err := tx.ExecContext(ctx, buf.String()); err != nil { return err } } diff --git a/file_migration_test.go b/file_migration_test.go index 3f53610..fb2ef66 100644 --- a/file_migration_test.go +++ b/file_migration_test.go @@ -5,6 +5,7 @@ package dmorph_test import ( "bytes" + "context" "database/sql" "io/fs" "log/slog" @@ -33,7 +34,8 @@ func TestWithMigrationFromFile(t *testing.T) { defer func() { _ = db.Close() }() } - runErr := dmorph.Run(db, + runErr := dmorph.Run(context.Background(), + db, dmorph.WithDialect(dmorph.DialectSQLite()), dmorph.WithMigrationFromFile("testData/01_base_table.sql")) @@ -57,7 +59,8 @@ func TestWithMigrationFromFileError(t *testing.T) { defer func() { _ = db.Close() }() } - runErr := dmorph.Run(db, + runErr := dmorph.Run(context.Background(), + db, dmorph.WithDialect(dmorph.DialectSQLite()), dmorph.WithMigrationFromFile("testData/00_non_existent.sql")) @@ -74,7 +77,7 @@ func TestMigrationFromFileFSError(t *testing.T) { mig := dmorph.TmigrationFromFileFS("nonexistent", dir.FS(), slog.Default()) - err := mig.Migrate(nil) + err := mig.Migrate(context.Background(), nil) assert.Error(t, err, "expected error") } @@ -104,7 +107,7 @@ func TestApplyStepsStreamError(t *testing.T) { assert.NoError(t, txErr, "expected no tx error") - err := dmorph.TapplyStepsStream(tx, &buf, "test", slog.Default()) + err := dmorph.TapplyStepsStream(context.Background(), tx, &buf, "test", slog.Default()) assert.Error(t, err, "expected error") @@ -117,7 +120,7 @@ func TestApplyStepsStreamError(t *testing.T) { buf.Reset() buf.WriteString("utter nonsense\n;") - err = dmorph.TapplyStepsStream(tx, &buf, "test", slog.Default()) + err = dmorph.TapplyStepsStream(context.Background(), tx, &buf, "test", slog.Default()) assert.Error(t, err, "expected error") diff --git a/migration.go b/migration.go index 51f984f..a5bc07a 100644 --- a/migration.go +++ b/migration.go @@ -4,6 +4,7 @@ package dmorph import ( + "context" "database/sql" "errors" "fmt" @@ -46,15 +47,15 @@ var ErrMigrationsTooOld = errors.New("migrations too old") // Dialect is an interface describing the functionalities needed to manage migrations inside a database. type Dialect interface { - EnsureMigrationTableExists(db *sql.DB, tableName string) error - AppliedMigrations(db *sql.DB, tableName string) ([]string, error) - RegisterMigration(tx *sql.Tx, id string, tableName string) error + EnsureMigrationTableExists(ctx context.Context, db *sql.DB, tableName string) error + AppliedMigrations(ctx context.Context, db *sql.DB, tableName string) ([]string, error) + RegisterMigration(ctx context.Context, tx *sql.Tx, id string, tableName string) error } // Migration is an interface to provide abstract information about the migration at hand. type Migration interface { - Key() string // identifier, used for ordering - Migrate(tx *sql.Tx) error // migration functionality + Key() string // identifier, used for ordering + Migrate(ctx context.Context, tx *sql.Tx) error // migration functionality } // migrationOrder is used to order Migration instances. @@ -174,16 +175,16 @@ func (m *Morpher) IsValid() error { // returned. // Run will run each migration in a separate transaction, with the last step to register the // migration in the migration table. -func (m *Morpher) Run(db *sql.DB) error { +func (m *Morpher) Run(ctx context.Context, db *sql.DB) error { if validErr := m.IsValid(); validErr != nil { return validErr } - if err := m.Dialect.EnsureMigrationTableExists(db, m.TableName); err != nil { + if err := m.Dialect.EnsureMigrationTableExists(ctx, db, m.TableName); err != nil { return fmt.Errorf("could not create migration table: %w", err) } - appliedMigrations, appliedMigrationsErr := m.Dialect.AppliedMigrations(db, m.TableName) + appliedMigrations, appliedMigrationsErr := m.Dialect.AppliedMigrations(ctx, db, m.TableName) if appliedMigrationsErr != nil { return fmt.Errorf("could not get applied migrations: %w", appliedMigrationsErr) @@ -206,12 +207,12 @@ func (m *Morpher) Run(db *sql.DB) error { lastMigration = appliedMigrations[len(appliedMigrations)-1] } - return m.applyMigrations(db, lastMigration) + return m.applyMigrations(ctx, db, lastMigration) } // applyMigrations applies the given migrations to the database. // This method does not check for the validity or consistency of the database. -func (m *Morpher) applyMigrations(db *sql.DB, lastMigration string) error { +func (m *Morpher) applyMigrations(ctx context.Context, db *sql.DB, lastMigration string) error { var startMigration time.Time for _, migration := range m.Migrations { @@ -234,13 +235,13 @@ func (m *Morpher) applyMigrations(db *sql.DB, lastMigration string) error { // allocated resources of the transaction. defer func() { _ = tx.Rollback() }() - if err := migration.Migrate(tx); err != nil { + if err := migration.Migrate(ctx, tx); err != nil { rollbackErr := tx.Rollback() return errors.Join(err, rollbackErr) } - if registerErr := m.Dialect.RegisterMigration(tx, migration.Key(), m.TableName); registerErr != nil { + if registerErr := m.Dialect.RegisterMigration(ctx, tx, migration.Key(), m.TableName); registerErr != nil { rollbackErr := tx.Rollback() return errors.Join(registerErr, rollbackErr) @@ -291,12 +292,12 @@ func (m *Morpher) checkAppliedMigrations(appliedMigrations []string) error { // Run is a convenience function to easily get the migration job done. For more control use the // Morpher directly. -func Run(db *sql.DB, options ...MorphOption) error { +func Run(ctx context.Context, db *sql.DB, options ...MorphOption) error { m, morphErr := NewMorpher(options...) if morphErr != nil { return morphErr } - return m.Run(db) + return m.Run(ctx, db) } diff --git a/migration_test.go b/migration_test.go index 62a4021..5899353 100644 --- a/migration_test.go +++ b/migration_test.go @@ -4,6 +4,7 @@ package dmorph_test import ( + "context" "database/sql" "embed" "io/fs" @@ -60,7 +61,8 @@ func TestMigration(t *testing.T) { require.NoError(t, migrationsDirErr, "migrations directory could not be opened") - runErr := dmorph.Run(db, + runErr := dmorph.Run(context.Background(), + db, dmorph.WithDialect(dmorph.DialectSQLite()), dmorph.WithMigrationsFromFS( migrationsDir.(fs.ReadDirFS))) // Safe: migrationDir guaranteed to implement fs.ReadDirFS @@ -90,13 +92,15 @@ func TestMigrationUpdate(t *testing.T) { assert.NoError(t, migrationsDirErr, "migrations directory could not be opened") - runErr := dmorph.Run(db, + runErr := dmorph.Run(context.Background(), + db, dmorph.WithDialect(dmorph.DialectSQLite()), dmorph.WithMigrationFromFileFS("01_base_table.sql", migrationsDir)) assert.NoError(t, runErr, "preparation migrations could not be run") - runErr = dmorph.Run(db, + runErr = dmorph.Run(context.Background(), + db, dmorph.WithDialect(dmorph.DialectSQLite()), dmorph.WithMigrationsFromFS( migrationsDir.(fs.ReadDirFS))) // Safe: migrationDir guaranteed to implement fs.ReadDirFS @@ -107,8 +111,8 @@ func TestMigrationUpdate(t *testing.T) { type TestMigrationImpl struct{} func (m TestMigrationImpl) Key() string { return "TestMigration" } -func (m TestMigrationImpl) Migrate(tx *sql.Tx) error { - _, err := tx.Exec("CREATE TABLE t0 (id INTEGER PRIMARY KEY)") +func (m TestMigrationImpl) Migrate(ctx context.Context, tx *sql.Tx) error { + _, err := tx.ExecContext(ctx, "CREATE TABLE t0 (id INTEGER PRIMARY KEY)") return err } @@ -131,7 +135,8 @@ func TestWithMigrations(t *testing.T) { defer func() { _ = db.Close() }() } - runErr := dmorph.Run(db, + runErr := dmorph.Run(context.Background(), + db, dmorph.WithDialect(dmorph.DialectSQLite()), dmorph.WithMigrations(TestMigrationImpl{})) @@ -141,7 +146,7 @@ func TestWithMigrations(t *testing.T) { // TestMigrationUnableToCreateMorpher tests to use the Run function without any // useful parameter. func TestMigrationUnableToCreateMorpher(t *testing.T) { - runErr := dmorph.Run(nil) + runErr := dmorph.Run(context.Background(), nil) assert.Error(t, runErr, "morpher should not have run") } @@ -168,13 +173,15 @@ func TestMigrationTooOld(t *testing.T) { require.NoError(t, migrationsDirErr, "migrations directory could not be opened") - runErr := dmorph.Run(db, + runErr := dmorph.Run(context.Background(), + db, dmorph.WithDialect(dmorph.DialectSQLite()), dmorph.WithMigrationsFromFS(migrationsDir.(fs.ReadDirFS))) require.NoError(t, runErr, "preparation migrations could not be run") - runErr = dmorph.Run(db, + runErr = dmorph.Run(context.Background(), + db, dmorph.WithDialect(dmorph.DialectSQLite()), dmorph.WithMigrationFromFileFS("01_base_table.sql", migrationsDir)) @@ -203,13 +210,15 @@ func TestMigrationUnrelated0(t *testing.T) { assert.NoError(t, migrationsDirErr, "migrations directory could not be opened") - runErr := dmorph.Run(db, + runErr := dmorph.Run(context.Background(), + db, dmorph.WithDialect(dmorph.DialectSQLite()), dmorph.WithMigrationsFromFS(migrationsDir.(fs.ReadDirFS))) assert.NoError(t, runErr, "preparation migrations could not be run") - runErr = dmorph.Run(db, + runErr = dmorph.Run(context.Background(), + db, dmorph.WithDialect(dmorph.DialectSQLite()), dmorph.WithMigrationFromFileFS("02_addon_table.sql", migrationsDir)) @@ -238,13 +247,15 @@ func TestMigrationUnrelated1(t *testing.T) { assert.NoError(t, migrationsDirErr, "migrations directory could not be opened") - runErr := dmorph.Run(db, + runErr := dmorph.Run(context.Background(), + db, dmorph.WithDialect(dmorph.DialectSQLite()), dmorph.WithMigrationFromFileFS("01_base_table.sql", migrationsDir)) assert.NoError(t, runErr, "preparation migrations could not be run") - runErr = dmorph.Run(db, + runErr = dmorph.Run(context.Background(), + db, dmorph.WithDialect(dmorph.DialectSQLite()), dmorph.WithMigrationFromFileFS("02_addon_table.sql", migrationsDir)) @@ -274,7 +285,7 @@ func TestMigrationAppliedUnordered(t *testing.T) { assert.NoError(t, migrationsDirErr, "migrations directory could not be opened") - assert.NoError(t, dmorph.DialectSQLite().EnsureMigrationTableExists(db, "migrations")) + assert.NoError(t, dmorph.DialectSQLite().EnsureMigrationTableExists(context.Background(), db, "migrations")) _, execErr := db.Exec(` INSERT INTO migrations (id, create_ts) VALUES ('01_base_table', '2021-01-02 00:00:00'); @@ -283,7 +294,8 @@ func TestMigrationAppliedUnordered(t *testing.T) { assert.NoError(t, execErr, "unordered test could not be prepared") - runErr := dmorph.Run(db, + runErr := dmorph.Run(context.Background(), + db, dmorph.WithDialect(dmorph.DialectSQLite()), dmorph.WithMigrationsFromFS(migrationsDir.(fs.ReadDirFS))) @@ -446,7 +458,7 @@ func TestMigrationWithTableNameInvalidChars(t *testing.T) { func TestMigrationRunInvalid(t *testing.T) { morpher := dmorph.Morpher{} - runErr := morpher.Run(nil) + runErr := morpher.Run(context.Background(), nil) assert.Error(t, runErr, "morpher should run") } @@ -479,7 +491,7 @@ func TestMigrationRunInvalidCreate(t *testing.T) { assert.NoError(t, morpherErr, "morpher could not be created") - runErr := morpher.Run(db) + runErr := morpher.Run(context.Background(), db) assert.Error(t, runErr, "morpher should not run") } @@ -511,7 +523,7 @@ func TestMigrationRunInvalidApplied(t *testing.T) { assert.NoError(t, morpherErr, "morpher could not be created") - runErr := morpher.Run(db) + runErr := morpher.Run(context.Background(), db) assert.Error(t, runErr, "morpher should not run") } @@ -541,7 +553,7 @@ func TestMigrationApplyInvalidDB(t *testing.T) { require.NoError(t, morpherErr, "morpher could not be created") assert.Error(t, - morpher.TapplyMigrations(db, "irrelevant"), + morpher.TapplyMigrations(context.Background(), db, "irrelevant"), "morpher should error on invalid DB") } @@ -576,7 +588,7 @@ func TestMigrationApplyUnableRegister(t *testing.T) { morpher.Dialect = d assert.Error(t, - morpher.TapplyMigrations(db, ""), + morpher.TapplyMigrations(context.Background(), db, ""), "morpher should fail to register") } @@ -608,10 +620,10 @@ func TestMigrationApplyUnableCommit(t *testing.T) { _, execErr := db.Exec("PRAGMA foreign_keys = ON") require.NoError(t, execErr, "foreign keys checking could not be enabled") - d, dialectOK := morpher.Dialect.(dmorph.BaseDialect) + baseDialect, dialectOK := morpher.Dialect.(dmorph.BaseDialect) require.True(t, dialectOK, "dialect is not a BaseDialect") - d.RegisterTemplate = ` + baseDialect.RegisterTemplate = ` CREATE TABLE t0 ( id INTEGER PRIMARY KEY ); @@ -627,9 +639,9 @@ func TestMigrationApplyUnableCommit(t *testing.T) { -- %s catching argument DELETE FROM t0 WHERE id = 1;` - morpher.Dialect = d + morpher.Dialect = baseDialect assert.Error(t, - morpher.TapplyMigrations(db, ""), + morpher.TapplyMigrations(context.Background(), db, ""), "morpher should fail to register") } From 165ba423195f768f71f347111281f9db3cef56ce Mon Sep 17 00:00:00 2001 From: Alexander Adam Date: Mon, 25 Aug 2025 00:08:03 +0200 Subject: [PATCH 05/11] Add context check before migration and refactor tests for stricter validation - Added context cancellation check before starting a database transaction in `migration.go`. - Replaced `assert` with `require` in tests for stricter and immediate validation. - Added safety comments for migration directory handling in tests. --- file_migration_test.go | 3 ++- migration.go | 6 ++++++ migration_test.go | 17 ++++++++++------- 3 files changed, 18 insertions(+), 8 deletions(-) diff --git a/file_migration_test.go b/file_migration_test.go index fb2ef66..80b6dbd 100644 --- a/file_migration_test.go +++ b/file_migration_test.go @@ -13,6 +13,7 @@ import ( "testing" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" "github.com/AlphaOne1/dmorph" ) @@ -73,7 +74,7 @@ func TestWithMigrationFromFileError(t *testing.T) { func TestMigrationFromFileFSError(t *testing.T) { dir, dirErr := os.OpenRoot("testData") - assert.NoError(t, dirErr, "could not open test data directory") + require.NoError(t, dirErr, "could not open test data directory") mig := dmorph.TmigrationFromFileFS("nonexistent", dir.FS(), slog.Default()) diff --git a/migration.go b/migration.go index a5bc07a..75a634e 100644 --- a/migration.go +++ b/migration.go @@ -224,6 +224,12 @@ func (m *Morpher) applyMigrations(ctx context.Context, db *sql.DB, lastMigration m.Log.Info("applying migration", slog.String("file", migration.Key())) startMigration = time.Now() + + // Check context before starting a transaction + if err := ctx.Err(); err != nil { + return fmt.Errorf("context cancelled before migration %s: %w", migration.Key(), err) + } + tx, txBeginErr := db.Begin() if txBeginErr != nil { diff --git a/migration_test.go b/migration_test.go index 5899353..cb6d2bd 100644 --- a/migration_test.go +++ b/migration_test.go @@ -97,7 +97,7 @@ func TestMigrationUpdate(t *testing.T) { dmorph.WithDialect(dmorph.DialectSQLite()), dmorph.WithMigrationFromFileFS("01_base_table.sql", migrationsDir)) - assert.NoError(t, runErr, "preparation migrations could not be run") + require.NoError(t, runErr, "preparation migrations could not be run") runErr = dmorph.Run(context.Background(), db, @@ -176,7 +176,8 @@ func TestMigrationTooOld(t *testing.T) { runErr := dmorph.Run(context.Background(), db, dmorph.WithDialect(dmorph.DialectSQLite()), - dmorph.WithMigrationsFromFS(migrationsDir.(fs.ReadDirFS))) + dmorph.WithMigrationsFromFS( + migrationsDir.(fs.ReadDirFS))) // Safe: migrationDir guaranteed to implement fs.ReadDirFS require.NoError(t, runErr, "preparation migrations could not be run") @@ -213,7 +214,8 @@ func TestMigrationUnrelated0(t *testing.T) { runErr := dmorph.Run(context.Background(), db, dmorph.WithDialect(dmorph.DialectSQLite()), - dmorph.WithMigrationsFromFS(migrationsDir.(fs.ReadDirFS))) + dmorph.WithMigrationsFromFS( + migrationsDir.(fs.ReadDirFS))) // Safe: migrationDir guaranteed to implement fs.ReadDirFS assert.NoError(t, runErr, "preparation migrations could not be run") @@ -252,7 +254,7 @@ func TestMigrationUnrelated1(t *testing.T) { dmorph.WithDialect(dmorph.DialectSQLite()), dmorph.WithMigrationFromFileFS("01_base_table.sql", migrationsDir)) - assert.NoError(t, runErr, "preparation migrations could not be run") + require.NoError(t, runErr, "preparation migrations could not be run") runErr = dmorph.Run(context.Background(), db, @@ -297,7 +299,8 @@ func TestMigrationAppliedUnordered(t *testing.T) { runErr := dmorph.Run(context.Background(), db, dmorph.WithDialect(dmorph.DialectSQLite()), - dmorph.WithMigrationsFromFS(migrationsDir.(fs.ReadDirFS))) + dmorph.WithMigrationsFromFS( + migrationsDir.(fs.ReadDirFS))) // Safe: migrationDir guaranteed to implement fs.ReadDirFS assert.ErrorIs(t, runErr, @@ -489,7 +492,7 @@ func TestMigrationRunInvalidCreate(t *testing.T) { dmorph.WithDialect(dialect), dmorph.WithMigrationFromFile("testData/01_base_table.sql")) - assert.NoError(t, morpherErr, "morpher could not be created") + require.NoError(t, morpherErr, "morpher could not be created") runErr := morpher.Run(context.Background(), db) @@ -521,7 +524,7 @@ func TestMigrationRunInvalidApplied(t *testing.T) { dmorph.WithDialect(dialect), dmorph.WithMigrationFromFile("testData/01_base_table.sql")) - assert.NoError(t, morpherErr, "morpher could not be created") + require.NoError(t, morpherErr, "morpher could not be created") runErr := morpher.Run(context.Background(), db) From 7683c1856401fd18e9fe4b5c635adedcac150e91 Mon Sep 17 00:00:00 2001 From: Alexander Adam Date: Mon, 25 Aug 2025 00:54:04 +0200 Subject: [PATCH 06/11] Refactor error messages, transaction handling, and tests for clarity and precision - Standardized error descriptions by replacing "signalizes" with "signals" for grammatical correctness. - Refactored migration logic to encapsulate single migration execution in a new `runOneMigration` method. - Improved transaction handling with better error propagation and deferred rollback safety. - Simplified `WithMigrationsFromFS` function signature for better consistency. - Updated tests to utilize `require` over `assert` for stricter validation. - Enhanced `.golangci.yaml` configuration to ignore common variable names. --- .golangci.yaml | 5 +++ file_migration.go | 4 +-- file_migration_test.go | 14 +++------ migration.go | 70 ++++++++++++++++++++++++------------------ migration_test.go | 25 ++++++--------- 5 files changed, 62 insertions(+), 56 deletions(-) diff --git a/.golangci.yaml b/.golangci.yaml index 0dd0821..916b64e 100644 --- a/.golangci.yaml +++ b/.golangci.yaml @@ -82,6 +82,11 @@ linters: varnamelen: max-distance: 10 + ignore-names: + - ctx + - db + - err + - tx whitespace: multi-if: true diff --git a/file_migration.go b/file_migration.go index 55ec013..a768f83 100644 --- a/file_migration.go +++ b/file_migration.go @@ -66,9 +66,9 @@ func WithMigrationFromFileFS(name string, dir fs.FS) MorphOption { // WithMigrationsFromFS generates a FileMigration that will run all migration scripts of the files in the given // filesystem. -func WithMigrationsFromFS(d fs.ReadDirFS) MorphOption { +func WithMigrationsFromFS(d fs.FS) MorphOption { return func(morpher *Morpher) error { - dirEntry, err := d.ReadDir(".") + dirEntry, err := fs.ReadDir(d, ".") if err == nil { for _, entry := range dirEntry { diff --git a/file_migration_test.go b/file_migration_test.go index 80b6dbd..c25e23b 100644 --- a/file_migration_test.go +++ b/file_migration_test.go @@ -12,10 +12,8 @@ import ( "os" "testing" - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" - "github.com/AlphaOne1/dmorph" + "github.com/stretchr/testify/assert" ) func TestWithMigrationFromFile(t *testing.T) { @@ -72,11 +70,9 @@ func TestWithMigrationFromFileError(t *testing.T) { // TestMigrationFromFileFSError validates that migrationFromFileFS returns an error // when the specified file does not exist. func TestMigrationFromFileFSError(t *testing.T) { - dir, dirErr := os.OpenRoot("testData") - - require.NoError(t, dirErr, "could not open test data directory") + dir := os.DirFS("testData") - mig := dmorph.TmigrationFromFileFS("nonexistent", dir.FS(), slog.Default()) + mig := dmorph.TmigrationFromFileFS("nonexistent", dir, slog.Default()) err := mig.Migrate(context.Background(), nil) @@ -104,7 +100,7 @@ func TestApplyStepsStreamError(t *testing.T) { buf := bytes.Buffer{} buf.WriteString("utter nonsense") - tx, txErr := db.Begin() + tx, txErr := db.BeginTx(t.Context(), nil) assert.NoError(t, txErr, "expected no tx error") @@ -114,7 +110,7 @@ func TestApplyStepsStreamError(t *testing.T) { _ = tx.Rollback() - tx, txErr = db.Begin() + tx, txErr = db.BeginTx(t.Context(), nil) assert.NoError(t, txErr, "expected no tx error") diff --git a/migration.go b/migration.go index 75a634e..523d076 100644 --- a/migration.go +++ b/migration.go @@ -20,18 +20,18 @@ const MigrationTableName = "migrations" // ValidTableNameRex is the regular expression used to check if a given migration table name is valid. var ValidTableNameRex = regexp.MustCompile("^[a-zA-Z0-9_]+$") -// ErrMigrationsUnrelated signalizes that the set of migrations to apply and the already applied set do not have the +// ErrMigrationsUnrelated signals that the set of migrations to apply and the already applied set do not have the // same (order of) applied migrations. Applying unrelated migrations could severely harm the database. var ErrMigrationsUnrelated = errors.New("migrations unrelated") -// ErrMigrationsUnsorted tells that the already applied migrations were not registered in the order +// ErrMigrationsUnsorted indicates that the already applied migrations were not registered in the order // (using the timestamp) that they should have been registered (using their id). var ErrMigrationsUnsorted = errors.New("migrations unsorted") -// ErrNoDialect signalizes that no dialect for the database operations was chosen. +// ErrNoDialect signals that no dialect for the database operations was chosen. var ErrNoDialect = errors.New("no dialect") -// ErrNoMigrations signalizes that no migrations were chosen to be applied. +// ErrNoMigrations signals that no migrations were chosen to be applied. var ErrNoMigrations = errors.New("no migrations") // ErrNoMigrationTable occurs if there is no migration table present. @@ -40,7 +40,7 @@ var ErrNoMigrationTable = errors.New("no migration table") // ErrMigrationTableNameInvalid occurs if the migration table does not adhere to ValidTableNameRex. var ErrMigrationTableNameInvalid = errors.New("invalid migration table name") -// ErrMigrationsTooOld signalizes that the migrations to be applied are older than the migrations that are already +// ErrMigrationsTooOld signals that the migrations to be applied are older than the migrations that are already // present in the database. This error can occur when an older version of the application is started using a database // used already by a newer version of the application. var ErrMigrationsTooOld = errors.New("migrations too old") @@ -114,7 +114,7 @@ func WithLog(log *slog.Logger) MorphOption { func WithTableName(tableName string) func(*Morpher) error { return func(m *Morpher) error { if len(tableName) < 1 { - return fmt.Errorf("table name empty") + return ErrMigrationTableNameInvalid } if !ValidTableNameRex.MatchString(tableName) { @@ -230,38 +230,48 @@ func (m *Morpher) applyMigrations(ctx context.Context, db *sql.DB, lastMigration return fmt.Errorf("context cancelled before migration %s: %w", migration.Key(), err) } - tx, txBeginErr := db.Begin() - - if txBeginErr != nil { - return txBeginErr + if err := m.runOneMigration(ctx, db, migration); err != nil { + return err } - // Even if we are sure to catch all possibilities, we use this as a safeguard that also with later - // modifications. When a successful commit cannot be done, at least the rollback is executed, freeing - // allocated resources of the transaction. - defer func() { _ = tx.Rollback() }() + m.Log.Info("migration applied", + slog.String("file", migration.Key()), + slog.Duration("duration", time.Since(startMigration)), + ) + } + + return nil +} - if err := migration.Migrate(ctx, tx); err != nil { - rollbackErr := tx.Rollback() +// runOneMigration executes a single migration within a database transaction and logs its completion. +func (m *Morpher) runOneMigration(ctx context.Context, db *sql.DB, mig Migration) error { + tx, err := db.BeginTx(ctx, nil) - return errors.Join(err, rollbackErr) - } + if err != nil { + return fmt.Errorf("begin tx: %w", err) + } - if registerErr := m.Dialect.RegisterMigration(ctx, tx, migration.Key(), m.TableName); registerErr != nil { - rollbackErr := tx.Rollback() + // Even if we are sure to catch all possibilities, we use this as a safeguard that also with later + // modifications. When a successful commit cannot be done, at least the rollback is executed, freeing + // allocated resources of the transaction. + defer func() { _ = tx.Rollback() }() - return errors.Join(registerErr, rollbackErr) - } + if err := mig.Migrate(ctx, tx); err != nil { + rollbackErr := tx.Rollback() - if commitErr := tx.Commit(); commitErr != nil { - rollbackErr := tx.Rollback() + return errors.Join(err, rollbackErr) + } - return errors.Join(commitErr, rollbackErr) - } - m.Log.Info("migration applied", - slog.String("file", migration.Key()), - slog.Duration("duration", time.Since(startMigration)), - ) + if err := m.Dialect.RegisterMigration(ctx, tx, mig.Key(), m.TableName); err != nil { + rollbackErr := tx.Rollback() + + return errors.Join(err, rollbackErr) + } + + if commitErr := tx.Commit(); commitErr != nil { + rollbackErr := tx.Rollback() + + return errors.Join(commitErr, rollbackErr) } return nil diff --git a/migration_test.go b/migration_test.go index cb6d2bd..9494ad2 100644 --- a/migration_test.go +++ b/migration_test.go @@ -64,8 +64,7 @@ func TestMigration(t *testing.T) { runErr := dmorph.Run(context.Background(), db, dmorph.WithDialect(dmorph.DialectSQLite()), - dmorph.WithMigrationsFromFS( - migrationsDir.(fs.ReadDirFS))) // Safe: migrationDir guaranteed to implement fs.ReadDirFS + dmorph.WithMigrationsFromFS(migrationsDir)) assert.NoError(t, runErr, "migrations could not be run") } @@ -102,8 +101,7 @@ func TestMigrationUpdate(t *testing.T) { runErr = dmorph.Run(context.Background(), db, dmorph.WithDialect(dmorph.DialectSQLite()), - dmorph.WithMigrationsFromFS( - migrationsDir.(fs.ReadDirFS))) // Safe: migrationDir guaranteed to implement fs.ReadDirFS + dmorph.WithMigrationsFromFS(migrationsDir)) assert.NoError(t, runErr, "migrations could not be run") } @@ -176,8 +174,7 @@ func TestMigrationTooOld(t *testing.T) { runErr := dmorph.Run(context.Background(), db, dmorph.WithDialect(dmorph.DialectSQLite()), - dmorph.WithMigrationsFromFS( - migrationsDir.(fs.ReadDirFS))) // Safe: migrationDir guaranteed to implement fs.ReadDirFS + dmorph.WithMigrationsFromFS(migrationsDir)) require.NoError(t, runErr, "preparation migrations could not be run") @@ -214,8 +211,7 @@ func TestMigrationUnrelated0(t *testing.T) { runErr := dmorph.Run(context.Background(), db, dmorph.WithDialect(dmorph.DialectSQLite()), - dmorph.WithMigrationsFromFS( - migrationsDir.(fs.ReadDirFS))) // Safe: migrationDir guaranteed to implement fs.ReadDirFS + dmorph.WithMigrationsFromFS(migrationsDir)) assert.NoError(t, runErr, "preparation migrations could not be run") @@ -285,22 +281,21 @@ func TestMigrationAppliedUnordered(t *testing.T) { migrationsDir, migrationsDirErr := fs.Sub(testMigrationsDir, "testData") - assert.NoError(t, migrationsDirErr, "migrations directory could not be opened") + require.NoError(t, migrationsDirErr, "migrations directory could not be opened") - assert.NoError(t, dmorph.DialectSQLite().EnsureMigrationTableExists(context.Background(), db, "migrations")) + require.NoError(t, dmorph.DialectSQLite().EnsureMigrationTableExists(context.Background(), db, "migrations")) - _, execErr := db.Exec(` + _, execErr := db.ExecContext(t.Context(), ` INSERT INTO migrations (id, create_ts) VALUES ('01_base_table', '2021-01-02 00:00:00'); INSERT INTO migrations (id, create_ts) VALUES ('02_addon_table', '2021-01-01 00:00:00'); `) - assert.NoError(t, execErr, "unordered test could not be prepared") + require.NoError(t, execErr, "unordered test could not be prepared") runErr := dmorph.Run(context.Background(), db, dmorph.WithDialect(dmorph.DialectSQLite()), - dmorph.WithMigrationsFromFS( - migrationsDir.(fs.ReadDirFS))) // Safe: migrationDir guaranteed to implement fs.ReadDirFS + dmorph.WithMigrationsFromFS(migrationsDir)) assert.ErrorIs(t, runErr, @@ -620,7 +615,7 @@ func TestMigrationApplyUnableCommit(t *testing.T) { require.NoError(t, morpherErr, "morpher could not be created") - _, execErr := db.Exec("PRAGMA foreign_keys = ON") + _, execErr := db.ExecContext(t.Context(), "PRAGMA foreign_keys = ON") require.NoError(t, execErr, "foreign keys checking could not be enabled") baseDialect, dialectOK := morpher.Dialect.(dmorph.BaseDialect) From 9699cd8c93686ad8f6815b6266841ba9739b70ba Mon Sep 17 00:00:00 2001 From: Alexander Adam Date: Mon, 25 Aug 2025 01:11:02 +0200 Subject: [PATCH 07/11] Refactor tests and migration logic for stricter validation and improved error clarity - Replaced `assert` with `require` in tests for immediate validation. - Added file type check for `.sql` files in `file_migration.go`. - Enhanced error messages in `applyStepsStream` for better debugging. - Renamed `id` to `migrationID` for clarity. - Included `fmt.Errorf` for detailed error propagation. --- file_migration.go | 13 +++++++------ file_migration_test.go | 7 ++++--- migration_test.go | 8 ++++---- 3 files changed, 15 insertions(+), 13 deletions(-) diff --git a/file_migration.go b/file_migration.go index a768f83..69f84f4 100644 --- a/file_migration.go +++ b/file_migration.go @@ -8,6 +8,7 @@ import ( "bytes" "context" "database/sql" + "fmt" "io" "io/fs" "log/slog" @@ -74,7 +75,7 @@ func WithMigrationsFromFS(d fs.FS) MorphOption { for _, entry := range dirEntry { morpher.Log.Info("entry", slog.String("name", entry.Name())) - if entry.Type().IsRegular() { + if entry.Type().IsRegular() && strings.HasSuffix(entry.Name(), ".sql") { morpher.Migrations = append(morpher.Migrations, migrationFromFileFS(entry.Name(), d, morpher.Log)) } @@ -109,7 +110,7 @@ func migrationFromFileFS(name string, dir fs.FS, log *slog.Logger) FileMigration // support comments, leading comments are removed. This function does not undertake efforts to scan the SQL to find // other comments. Such leading comments telling what a step is going to do, work. But comments in the middle of a // statement will not be removed. At least with SQLite this will lead to hard-to-find errors. -func applyStepsStream(ctx context.Context, tx *sql.Tx, r io.Reader, id string, log *slog.Logger) error { +func applyStepsStream(ctx context.Context, tx *sql.Tx, r io.Reader, migrationID string, log *slog.Logger) error { const InitialScannerBufSize = 64 * 1024 const MaxScannerBufSize = 1024 * 1024 @@ -128,12 +129,12 @@ func applyStepsStream(ctx context.Context, tx *sql.Tx, r io.Reader, id string, l if scanner.Text() == ";" { log.Info("migration step", - slog.String("id", id), + slog.String("migrationID", migrationID), slog.Int("step", step), ) if _, err := tx.ExecContext(ctx, buf.String()); err != nil { - return err + return fmt.Errorf("apply migration %q step %d: %w", migrationID, step, err) } buf.Reset() @@ -155,12 +156,12 @@ func applyStepsStream(ctx context.Context, tx *sql.Tx, r io.Reader, id string, l // cleanup after, for the final statement without the closing `;` on a new line if buf.Len() > 0 { log.Info("migration step", - slog.String("id", id), + slog.String("migrationID", migrationID), slog.Int("step", step), ) if _, err := tx.ExecContext(ctx, buf.String()); err != nil { - return err + return fmt.Errorf("apply migration %q step %d (final): %w", migrationID, step, err) } } diff --git a/file_migration_test.go b/file_migration_test.go index c25e23b..912947c 100644 --- a/file_migration_test.go +++ b/file_migration_test.go @@ -14,6 +14,7 @@ import ( "github.com/AlphaOne1/dmorph" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestWithMigrationFromFile(t *testing.T) { @@ -102,17 +103,17 @@ func TestApplyStepsStreamError(t *testing.T) { tx, txErr := db.BeginTx(t.Context(), nil) - assert.NoError(t, txErr, "expected no tx error") + require.NoError(t, txErr, "expected no tx error") err := dmorph.TapplyStepsStream(context.Background(), tx, &buf, "test", slog.Default()) - assert.Error(t, err, "expected error") + require.Error(t, err, "expected error") _ = tx.Rollback() tx, txErr = db.BeginTx(t.Context(), nil) - assert.NoError(t, txErr, "expected no tx error") + require.NoError(t, txErr, "expected no tx error") buf.Reset() buf.WriteString("utter nonsense\n;") diff --git a/migration_test.go b/migration_test.go index 9494ad2..ff82c01 100644 --- a/migration_test.go +++ b/migration_test.go @@ -89,7 +89,7 @@ func TestMigrationUpdate(t *testing.T) { migrationsDir, migrationsDirErr := fs.Sub(testMigrationsDir, "testData") - assert.NoError(t, migrationsDirErr, "migrations directory could not be opened") + require.NoError(t, migrationsDirErr, "migrations directory could not be opened") runErr := dmorph.Run(context.Background(), db, @@ -206,14 +206,14 @@ func TestMigrationUnrelated0(t *testing.T) { migrationsDir, migrationsDirErr := fs.Sub(testMigrationsDir, "testData") - assert.NoError(t, migrationsDirErr, "migrations directory could not be opened") + require.NoError(t, migrationsDirErr, "migrations directory could not be opened") runErr := dmorph.Run(context.Background(), db, dmorph.WithDialect(dmorph.DialectSQLite()), dmorph.WithMigrationsFromFS(migrationsDir)) - assert.NoError(t, runErr, "preparation migrations could not be run") + require.NoError(t, runErr, "preparation migrations could not be run") runErr = dmorph.Run(context.Background(), db, @@ -243,7 +243,7 @@ func TestMigrationUnrelated1(t *testing.T) { migrationsDir, migrationsDirErr := fs.Sub(testMigrationsDir, "testData") - assert.NoError(t, migrationsDirErr, "migrations directory could not be opened") + require.NoError(t, migrationsDirErr, "migrations directory could not be opened") runErr := dmorph.Run(context.Background(), db, From 5b9bccca95399790e4f40d932fa13108cda5d219 Mon Sep 17 00:00:00 2001 From: Alexander Adam Date: Mon, 25 Aug 2025 01:28:27 +0200 Subject: [PATCH 08/11] Refactor tests to use `t.Context()` and improve error messaging - Replaced `context.Background()` with `t.Context()` in tests for better test isolation. - Standardized error messages in `assert` and `require` for improved clarity and consistency. - Minor grammatical corrections in error strings. - Removed unused `context` imports from test files. --- dialects_test.go | 9 +++---- file_migration_test.go | 11 ++++---- migration_test.go | 58 +++++++++++++++++++----------------------- 3 files changed, 35 insertions(+), 43 deletions(-) diff --git a/dialects_test.go b/dialects_test.go index 36dda08..08ad840 100644 --- a/dialects_test.go +++ b/dialects_test.go @@ -4,7 +4,6 @@ package dmorph_test import ( - "context" "database/sql" "os" "regexp" @@ -80,10 +79,10 @@ func TestCallsOnClosedDB(t *testing.T) { } assert.Error(t, - dmorph.DialectSQLite().EnsureMigrationTableExists(context.Background(), db, "irrelevant"), + dmorph.DialectSQLite().EnsureMigrationTableExists(t.Context(), db, "irrelevant"), "expected error on closed database") - _, err := dmorph.DialectSQLite().AppliedMigrations(context.Background(), db, "irrelevant") + _, err := dmorph.DialectSQLite().AppliedMigrations(t.Context(), db, "irrelevant") assert.Error(t, err, "expected error on closed database") } @@ -114,7 +113,7 @@ func TestEnsureMigrationTableExistsSQLError(t *testing.T) { defer func() { _ = db.Close() }() } - assert.Error(t, dialect.EnsureMigrationTableExists(context.Background(), db, "test"), "expected error") + assert.Error(t, dialect.EnsureMigrationTableExists(t.Context(), db, "test"), "expected error") } // TestEnsureMigrationTableExistsCommitError tests the behavior of EnsureMigrationTableExists @@ -158,5 +157,5 @@ func TestEnsureMigrationTableExistsCommitError(t *testing.T) { assert.NoError(t, execErr, "foreign keys checking could not be enabled") - assert.Error(t, d.EnsureMigrationTableExists(context.Background(), db, "test"), "expected error") + assert.Error(t, d.EnsureMigrationTableExists(t.Context(), db, "test"), "expected error") } diff --git a/file_migration_test.go b/file_migration_test.go index 912947c..04bab43 100644 --- a/file_migration_test.go +++ b/file_migration_test.go @@ -5,7 +5,6 @@ package dmorph_test import ( "bytes" - "context" "database/sql" "io/fs" "log/slog" @@ -34,7 +33,7 @@ func TestWithMigrationFromFile(t *testing.T) { defer func() { _ = db.Close() }() } - runErr := dmorph.Run(context.Background(), + runErr := dmorph.Run(t.Context(), db, dmorph.WithDialect(dmorph.DialectSQLite()), dmorph.WithMigrationFromFile("testData/01_base_table.sql")) @@ -59,7 +58,7 @@ func TestWithMigrationFromFileError(t *testing.T) { defer func() { _ = db.Close() }() } - runErr := dmorph.Run(context.Background(), + runErr := dmorph.Run(t.Context(), db, dmorph.WithDialect(dmorph.DialectSQLite()), dmorph.WithMigrationFromFile("testData/00_non_existent.sql")) @@ -75,7 +74,7 @@ func TestMigrationFromFileFSError(t *testing.T) { mig := dmorph.TmigrationFromFileFS("nonexistent", dir, slog.Default()) - err := mig.Migrate(context.Background(), nil) + err := mig.Migrate(t.Context(), nil) assert.Error(t, err, "expected error") } @@ -105,7 +104,7 @@ func TestApplyStepsStreamError(t *testing.T) { require.NoError(t, txErr, "expected no tx error") - err := dmorph.TapplyStepsStream(context.Background(), tx, &buf, "test", slog.Default()) + err := dmorph.TapplyStepsStream(t.Context(), tx, &buf, "test", slog.Default()) require.Error(t, err, "expected error") @@ -118,7 +117,7 @@ func TestApplyStepsStreamError(t *testing.T) { buf.Reset() buf.WriteString("utter nonsense\n;") - err = dmorph.TapplyStepsStream(context.Background(), tx, &buf, "test", slog.Default()) + err = dmorph.TapplyStepsStream(t.Context(), tx, &buf, "test", slog.Default()) assert.Error(t, err, "expected error") diff --git a/migration_test.go b/migration_test.go index ff82c01..da38ef0 100644 --- a/migration_test.go +++ b/migration_test.go @@ -43,25 +43,19 @@ func prepareDB() (string, error) { func TestMigration(t *testing.T) { dbFile, dbFileErr := prepareDB() - if dbFileErr != nil { - t.Errorf("DB file could not be created: %v", dbFileErr) - } else { - defer func() { _ = os.Remove(dbFile) }() - } + require.NoError(t, dbFileErr, "DB file could not be created") + defer func() { _ = os.Remove(dbFile) }() db, dbErr := sql.Open("sqlite", dbFile) - if dbErr != nil { - t.Errorf("DB file could not be created: %v", dbErr) - } else { - defer func() { _ = db.Close() }() - } + require.NoError(t, dbErr, "DB file could not be created") + t.Cleanup(func() { _ = db.Close() }) migrationsDir, migrationsDirErr := fs.Sub(testMigrationsDir, "testData") require.NoError(t, migrationsDirErr, "migrations directory could not be opened") - runErr := dmorph.Run(context.Background(), + runErr := dmorph.Run(t.Context(), db, dmorph.WithDialect(dmorph.DialectSQLite()), dmorph.WithMigrationsFromFS(migrationsDir)) @@ -91,14 +85,14 @@ func TestMigrationUpdate(t *testing.T) { require.NoError(t, migrationsDirErr, "migrations directory could not be opened") - runErr := dmorph.Run(context.Background(), + runErr := dmorph.Run(t.Context(), db, dmorph.WithDialect(dmorph.DialectSQLite()), dmorph.WithMigrationFromFileFS("01_base_table.sql", migrationsDir)) require.NoError(t, runErr, "preparation migrations could not be run") - runErr = dmorph.Run(context.Background(), + runErr = dmorph.Run(t.Context(), db, dmorph.WithDialect(dmorph.DialectSQLite()), dmorph.WithMigrationsFromFS(migrationsDir)) @@ -133,7 +127,7 @@ func TestWithMigrations(t *testing.T) { defer func() { _ = db.Close() }() } - runErr := dmorph.Run(context.Background(), + runErr := dmorph.Run(t.Context(), db, dmorph.WithDialect(dmorph.DialectSQLite()), dmorph.WithMigrations(TestMigrationImpl{})) @@ -144,7 +138,7 @@ func TestWithMigrations(t *testing.T) { // TestMigrationUnableToCreateMorpher tests to use the Run function without any // useful parameter. func TestMigrationUnableToCreateMorpher(t *testing.T) { - runErr := dmorph.Run(context.Background(), nil) + runErr := dmorph.Run(t.Context(), nil) assert.Error(t, runErr, "morpher should not have run") } @@ -171,14 +165,14 @@ func TestMigrationTooOld(t *testing.T) { require.NoError(t, migrationsDirErr, "migrations directory could not be opened") - runErr := dmorph.Run(context.Background(), + runErr := dmorph.Run(t.Context(), db, dmorph.WithDialect(dmorph.DialectSQLite()), dmorph.WithMigrationsFromFS(migrationsDir)) require.NoError(t, runErr, "preparation migrations could not be run") - runErr = dmorph.Run(context.Background(), + runErr = dmorph.Run(t.Context(), db, dmorph.WithDialect(dmorph.DialectSQLite()), dmorph.WithMigrationFromFileFS("01_base_table.sql", migrationsDir)) @@ -208,14 +202,14 @@ func TestMigrationUnrelated0(t *testing.T) { require.NoError(t, migrationsDirErr, "migrations directory could not be opened") - runErr := dmorph.Run(context.Background(), + runErr := dmorph.Run(t.Context(), db, dmorph.WithDialect(dmorph.DialectSQLite()), dmorph.WithMigrationsFromFS(migrationsDir)) require.NoError(t, runErr, "preparation migrations could not be run") - runErr = dmorph.Run(context.Background(), + runErr = dmorph.Run(t.Context(), db, dmorph.WithDialect(dmorph.DialectSQLite()), dmorph.WithMigrationFromFileFS("02_addon_table.sql", migrationsDir)) @@ -245,14 +239,14 @@ func TestMigrationUnrelated1(t *testing.T) { require.NoError(t, migrationsDirErr, "migrations directory could not be opened") - runErr := dmorph.Run(context.Background(), + runErr := dmorph.Run(t.Context(), db, dmorph.WithDialect(dmorph.DialectSQLite()), dmorph.WithMigrationFromFileFS("01_base_table.sql", migrationsDir)) require.NoError(t, runErr, "preparation migrations could not be run") - runErr = dmorph.Run(context.Background(), + runErr = dmorph.Run(t.Context(), db, dmorph.WithDialect(dmorph.DialectSQLite()), dmorph.WithMigrationFromFileFS("02_addon_table.sql", migrationsDir)) @@ -283,7 +277,7 @@ func TestMigrationAppliedUnordered(t *testing.T) { require.NoError(t, migrationsDirErr, "migrations directory could not be opened") - require.NoError(t, dmorph.DialectSQLite().EnsureMigrationTableExists(context.Background(), db, "migrations")) + require.NoError(t, dmorph.DialectSQLite().EnsureMigrationTableExists(t.Context(), db, "migrations")) _, execErr := db.ExecContext(t.Context(), ` INSERT INTO migrations (id, create_ts) VALUES ('01_base_table', '2021-01-02 00:00:00'); @@ -292,7 +286,7 @@ func TestMigrationAppliedUnordered(t *testing.T) { require.NoError(t, execErr, "unordered test could not be prepared") - runErr := dmorph.Run(context.Background(), + runErr := dmorph.Run(t.Context(), db, dmorph.WithDialect(dmorph.DialectSQLite()), dmorph.WithMigrationsFromFS(migrationsDir)) @@ -437,7 +431,7 @@ func TestMigrationWithTableNameInvalidSize(t *testing.T) { dmorph.WithTableName(""), ) - assert.Error(t, err, "morpher could created with empty table name") + assert.Error(t, err, "morpher could be created with empty table name") } // TestMigrationWithTableNameInvalidChars ensures that creating a Morpher @@ -449,16 +443,16 @@ func TestMigrationWithTableNameInvalidChars(t *testing.T) { dmorph.WithTableName("di/mor/pho/don"), ) - assert.Error(t, err, "morpher could created with invalid table name") + assert.Error(t, err, "morpher could be created with invalid table name") } // TestMigrationRunInvalid verifies that running a Morpher with invalid configuration results in an error. func TestMigrationRunInvalid(t *testing.T) { morpher := dmorph.Morpher{} - runErr := morpher.Run(context.Background(), nil) + runErr := morpher.Run(t.Context(), nil) - assert.Error(t, runErr, "morpher should run") + assert.Error(t, runErr, "morpher should not run") } // TestMigrationRunInvalidCreate tests the behavior of running a migration @@ -489,7 +483,7 @@ func TestMigrationRunInvalidCreate(t *testing.T) { require.NoError(t, morpherErr, "morpher could not be created") - runErr := morpher.Run(context.Background(), db) + runErr := morpher.Run(t.Context(), db) assert.Error(t, runErr, "morpher should not run") } @@ -521,7 +515,7 @@ func TestMigrationRunInvalidApplied(t *testing.T) { require.NoError(t, morpherErr, "morpher could not be created") - runErr := morpher.Run(context.Background(), db) + runErr := morpher.Run(t.Context(), db) assert.Error(t, runErr, "morpher should not run") } @@ -551,7 +545,7 @@ func TestMigrationApplyInvalidDB(t *testing.T) { require.NoError(t, morpherErr, "morpher could not be created") assert.Error(t, - morpher.TapplyMigrations(context.Background(), db, "irrelevant"), + morpher.TapplyMigrations(t.Context(), db, "irrelevant"), "morpher should error on invalid DB") } @@ -586,7 +580,7 @@ func TestMigrationApplyUnableRegister(t *testing.T) { morpher.Dialect = d assert.Error(t, - morpher.TapplyMigrations(context.Background(), db, ""), + morpher.TapplyMigrations(t.Context(), db, ""), "morpher should fail to register") } @@ -640,6 +634,6 @@ func TestMigrationApplyUnableCommit(t *testing.T) { morpher.Dialect = baseDialect assert.Error(t, - morpher.TapplyMigrations(context.Background(), db, ""), + morpher.TapplyMigrations(t.Context(), db, ""), "morpher should fail to register") } From 2118ac0ca8387f8efcd954ef838e8bcea347b1e4 Mon Sep 17 00:00:00 2001 From: Alexander Adam Date: Mon, 25 Aug 2025 01:53:56 +0200 Subject: [PATCH 09/11] Refactor tests to use `openTempSQLite` for streamlined database setup and cleanup - Introduced `openTempSQLite` helper to simplify and standardize test database handling. - Replaced repetitive database setup and teardown logic with the new helper across all tests. - Removed redundant error checks and improved code readability in test files. --- dialects_test.go | 56 ++---------- file_migration_test.go | 49 +---------- migration_test.go | 196 +++++------------------------------------ 3 files changed, 33 insertions(+), 268 deletions(-) diff --git a/dialects_test.go b/dialects_test.go index 08ad840..7958c1e 100644 --- a/dialects_test.go +++ b/dialects_test.go @@ -4,8 +4,6 @@ package dmorph_test import ( - "database/sql" - "os" "regexp" "testing" @@ -62,21 +60,7 @@ func TestDialectStatements(t *testing.T) { // TestCallsOnClosedDB verifies that methods fail as expected when called on a closed database connection. func TestCallsOnClosedDB(t *testing.T) { - dbFile, dbFileErr := prepareDB() - - if dbFileErr != nil { - t.Errorf("DB file could not be created: %v", dbFileErr) - } else { - defer func() { _ = os.Remove(dbFile) }() - } - - db, dbErr := sql.Open("sqlite", dbFile) - - if dbErr != nil { - t.Errorf("DB file could not be created: %v", dbErr) - } else { - _ = db.Close() - } + db, _ := openTempSQLite(t) assert.Error(t, dmorph.DialectSQLite().EnsureMigrationTableExists(t.Context(), db, "irrelevant"), @@ -97,21 +81,7 @@ func TestEnsureMigrationTableExistsSQLError(t *testing.T) { )`, } - dbFile, dbFileErr := prepareDB() - - if dbFileErr != nil { - t.Errorf("DB file could not be created: %v", dbFileErr) - } else { - defer func() { _ = os.Remove(dbFile) }() - } - - db, dbErr := sql.Open("sqlite", dbFile) - - if dbErr != nil { - t.Errorf("DB file could not be created: %v", dbErr) - } else { - defer func() { _ = db.Close() }() - } + db, _ := openTempSQLite(t) assert.Error(t, dialect.EnsureMigrationTableExists(t.Context(), db, "test"), "expected error") } @@ -119,7 +89,7 @@ func TestEnsureMigrationTableExistsSQLError(t *testing.T) { // TestEnsureMigrationTableExistsCommitError tests the behavior of EnsureMigrationTableExists // when a commit error occurs. func TestEnsureMigrationTableExistsCommitError(t *testing.T) { - d := dmorph.BaseDialect{ + dialect := dmorph.BaseDialect{ CreateTemplate: ` CREATE TABLE t0 ( id INTEGER PRIMARY KEY @@ -137,25 +107,11 @@ func TestEnsureMigrationTableExistsCommitError(t *testing.T) { DELETE FROM t0 WHERE id = 1;`, } - dbFile, dbFileErr := prepareDB() - - if dbFileErr != nil { - t.Errorf("DB file could not be created: %v", dbFileErr) - } else { - defer func() { _ = os.Remove(dbFile) }() - } - - db, dbErr := sql.Open("sqlite", dbFile) - - if dbErr != nil { - t.Errorf("DB file could not be created: %v", dbErr) - } else { - defer func() { _ = db.Close() }() - } + db, _ := openTempSQLite(t) - _, execErr := db.Exec("PRAGMA foreign_keys = ON") + _, execErr := db.ExecContext(t.Context(), "PRAGMA foreign_keys = ON") assert.NoError(t, execErr, "foreign keys checking could not be enabled") - assert.Error(t, d.EnsureMigrationTableExists(t.Context(), db, "test"), "expected error") + assert.Error(t, dialect.EnsureMigrationTableExists(t.Context(), db, "test"), "expected error") } diff --git a/file_migration_test.go b/file_migration_test.go index 04bab43..e80a5ae 100644 --- a/file_migration_test.go +++ b/file_migration_test.go @@ -5,7 +5,6 @@ package dmorph_test import ( "bytes" - "database/sql" "io/fs" "log/slog" "os" @@ -17,21 +16,7 @@ import ( ) func TestWithMigrationFromFile(t *testing.T) { - dbFile, dbFileErr := prepareDB() - - if dbFileErr != nil { - t.Errorf("DB file could not be created: %v", dbFileErr) - } else { - defer func() { _ = os.Remove(dbFile) }() - } - - db, dbErr := sql.Open("sqlite", dbFile) - - if dbErr != nil { - t.Errorf("DB file could not be created: %v", dbErr) - } else { - defer func() { _ = db.Close() }() - } + db, _ := openTempSQLite(t) runErr := dmorph.Run(t.Context(), db, @@ -42,21 +27,7 @@ func TestWithMigrationFromFile(t *testing.T) { } func TestWithMigrationFromFileError(t *testing.T) { - dbFile, dbFileErr := prepareDB() - - if dbFileErr != nil { - t.Errorf("DB file could not be created: %v", dbFileErr) - } else { - defer func() { _ = os.Remove(dbFile) }() - } - - db, dbErr := sql.Open("sqlite", dbFile) - - if dbErr != nil { - t.Errorf("DB file could not be created: %v", dbErr) - } else { - defer func() { _ = db.Close() }() - } + db, _ := openTempSQLite(t) runErr := dmorph.Run(t.Context(), db, @@ -81,21 +52,7 @@ func TestMigrationFromFileFSError(t *testing.T) { // TestApplyStepsStreamError tests error handling in applyStepsStream. func TestApplyStepsStreamError(t *testing.T) { - dbFile, dbFileErr := prepareDB() - - if dbFileErr != nil { - t.Errorf("DB file could not be created: %v", dbFileErr) - } else { - defer func() { _ = os.Remove(dbFile) }() - } - - db, dbErr := sql.Open("sqlite", dbFile) - - if dbErr != nil { - t.Errorf("DB file could not be created: %v", dbErr) - } else { - defer func() { _ = db.Close() }() - } + db, _ := openTempSQLite(t) buf := bytes.Buffer{} buf.WriteString("utter nonsense") diff --git a/migration_test.go b/migration_test.go index da38ef0..ac3c0e8 100644 --- a/migration_test.go +++ b/migration_test.go @@ -39,18 +39,24 @@ func prepareDB() (string, error) { return result, nil } -// TestMigration tests the happy flow. -func TestMigration(t *testing.T) { - dbFile, dbFileErr := prepareDB() +func openTempSQLite(t *testing.T) (*sql.DB, string) { + t.Helper() - require.NoError(t, dbFileErr, "DB file could not be created") - defer func() { _ = os.Remove(dbFile) }() + dbFile, err := prepareDB() + require.NoError(t, err, "DB file could not be created") + t.Cleanup(func() { _ = os.Remove(dbFile) }) db, dbErr := sql.Open("sqlite", dbFile) - - require.NoError(t, dbErr, "DB file could not be created") + require.NoError(t, dbErr, "DB could not be opened") t.Cleanup(func() { _ = db.Close() }) + return db, dbFile +} + +// TestMigration tests the happy flow. +func TestMigration(t *testing.T) { + db, _ := openTempSQLite(t) + migrationsDir, migrationsDirErr := fs.Sub(testMigrationsDir, "testData") require.NoError(t, migrationsDirErr, "migrations directory could not be opened") @@ -65,21 +71,7 @@ func TestMigration(t *testing.T) { // TestMigrationUpdate tests the happy flow of updating on existing migrations. func TestMigrationUpdate(t *testing.T) { - dbFile, dbFileErr := prepareDB() - - if dbFileErr != nil { - t.Errorf("DB file could not be created: %v", dbFileErr) - } else { - defer func() { _ = os.Remove(dbFile) }() - } - - db, dbErr := sql.Open("sqlite", dbFile) - - if dbErr != nil { - t.Errorf("DB file could not be created: %v", dbErr) - } else { - defer func() { _ = db.Close() }() - } + db, _ := openTempSQLite(t) migrationsDir, migrationsDirErr := fs.Sub(testMigrationsDir, "testData") @@ -111,21 +103,7 @@ func (m TestMigrationImpl) Migrate(ctx context.Context, tx *sql.Tx) error { // TestWithMigrations tests the adding of migrations using WithMigrations. func TestWithMigrations(t *testing.T) { - dbFile, dbFileErr := prepareDB() - - if dbFileErr != nil { - t.Errorf("DB file could not be created: %v", dbFileErr) - } else { - defer func() { _ = os.Remove(dbFile) }() - } - - db, dbErr := sql.Open("sqlite", dbFile) - - if dbErr != nil { - t.Errorf("DB file could not be created: %v", dbErr) - } else { - defer func() { _ = db.Close() }() - } + db, _ := openTempSQLite(t) runErr := dmorph.Run(t.Context(), db, @@ -145,21 +123,7 @@ func TestMigrationUnableToCreateMorpher(t *testing.T) { // TestMigrationTooOld tests what happens if the applied migrations are too old. func TestMigrationTooOld(t *testing.T) { - dbFile, dbFileErr := prepareDB() - - if dbFileErr != nil { - t.Errorf("DB file could not be created: %v", dbFileErr) - } else { - defer func() { _ = os.Remove(dbFile) }() - } - - db, dbErr := sql.Open("sqlite", dbFile) - - if dbErr != nil { - t.Errorf("DB file could not be created: %v", dbErr) - } else { - defer func() { _ = db.Close() }() - } + db, _ := openTempSQLite(t) migrationsDir, migrationsDirErr := fs.Sub(testMigrationsDir, "testData") @@ -182,21 +146,7 @@ func TestMigrationTooOld(t *testing.T) { // TestMigrationUnrelated0 tests what happens if the applied migrations are unrelated to existing ones. func TestMigrationUnrelated0(t *testing.T) { - dbFile, dbFileErr := prepareDB() - - if dbFileErr != nil { - t.Errorf("DB file could not be created: %v", dbFileErr) - } else { - defer func() { _ = os.Remove(dbFile) }() - } - - db, dbErr := sql.Open("sqlite", dbFile) - - if dbErr != nil { - t.Errorf("DB file could not be created: %v", dbErr) - } else { - defer func() { _ = db.Close() }() - } + db, _ := openTempSQLite(t) migrationsDir, migrationsDirErr := fs.Sub(testMigrationsDir, "testData") @@ -219,21 +169,7 @@ func TestMigrationUnrelated0(t *testing.T) { // TestMigrationUnrelated1 tests what happens if the applied migrations are unrelated to existing ones. func TestMigrationUnrelated1(t *testing.T) { - dbFile, dbFileErr := prepareDB() - - if dbFileErr != nil { - t.Errorf("DB file could not be created: %v", dbFileErr) - } else { - defer func() { _ = os.Remove(dbFile) }() - } - - db, dbErr := sql.Open("sqlite", dbFile) - - if dbErr != nil { - t.Errorf("DB file could not be created: %v", dbErr) - } else { - defer func() { _ = db.Close() }() - } + db, _ := openTempSQLite(t) migrationsDir, migrationsDirErr := fs.Sub(testMigrationsDir, "testData") @@ -257,21 +193,7 @@ func TestMigrationUnrelated1(t *testing.T) { // TestMigrationAppliedUnordered tests the case, that somehow the migrations in the // database are registered not in the order of their keys. func TestMigrationAppliedUnordered(t *testing.T) { - dbFile, dbFileErr := prepareDB() - - if dbFileErr != nil { - t.Errorf("DB file could not be created: %v", dbFileErr) - } else { - defer func() { _ = os.Remove(dbFile) }() - } - - db, dbErr := sql.Open("sqlite", dbFile) - - if dbErr != nil { - t.Errorf("DB file could not be created: %v", dbErr) - } else { - defer func() { _ = db.Close() }() - } + db, _ := openTempSQLite(t) migrationsDir, migrationsDirErr := fs.Sub(testMigrationsDir, "testData") @@ -458,21 +380,7 @@ func TestMigrationRunInvalid(t *testing.T) { // TestMigrationRunInvalidCreate tests the behavior of running a migration // with an invalid CreateTemplate in the dialect. func TestMigrationRunInvalidCreate(t *testing.T) { - dbFile, dbFileErr := prepareDB() - - if dbFileErr != nil { - t.Errorf("DB file could not be created: %v", dbFileErr) - } else { - defer func() { _ = os.Remove(dbFile) }() - } - - db, dbErr := sql.Open("sqlite", dbFile) - - if dbErr != nil { - t.Errorf("DB file could not be created: %v", dbErr) - } else { - defer func() { _ = db.Close() }() - } + db, _ := openTempSQLite(t) dialect := dmorph.DialectSQLite() dialect.CreateTemplate = "utter nonsense" @@ -490,21 +398,7 @@ func TestMigrationRunInvalidCreate(t *testing.T) { // TestMigrationRunInvalidApplied tests the failure scenario where the AppliedTemplate of the dialect is invalid. func TestMigrationRunInvalidApplied(t *testing.T) { - dbFile, dbFileErr := prepareDB() - - if dbFileErr != nil { - t.Errorf("DB file could not be created: %v", dbFileErr) - } else { - defer func() { _ = os.Remove(dbFile) }() - } - - db, dbErr := sql.Open("sqlite", dbFile) - - if dbErr != nil { - t.Errorf("DB file could not be created: %v", dbErr) - } else { - defer func() { _ = db.Close() }() - } + db, _ := openTempSQLite(t) dialect := dmorph.DialectSQLite() dialect.AppliedTemplate = "utter nonsense" @@ -522,21 +416,7 @@ func TestMigrationRunInvalidApplied(t *testing.T) { // TestMigrationApplyInvalidDB verifies that applying migrations to an invalid or closed database results in an error. func TestMigrationApplyInvalidDB(t *testing.T) { - dbFile, dbFileErr := prepareDB() - - if dbFileErr != nil { - t.Errorf("DB file could not be created: %v", dbFileErr) - } else { - defer func() { _ = os.Remove(dbFile) }() - } - - db, dbErr := sql.Open("sqlite", dbFile) - - if dbErr != nil { - t.Errorf("DB file could not be created: %v", dbErr) - } else { - _ = db.Close() - } + db, _ := openTempSQLite(t) morpher, morpherErr := dmorph.NewMorpher( dmorph.WithDialect(dmorph.DialectSQLite()), @@ -551,21 +431,7 @@ func TestMigrationApplyInvalidDB(t *testing.T) { // TestMigrationApplyUnableRegister tests the behavior when the migration registration fails due to an invalid template. func TestMigrationApplyUnableRegister(t *testing.T) { - dbFile, dbFileErr := prepareDB() - - if dbFileErr != nil { - t.Errorf("DB file could not be created: %v", dbFileErr) - } else { - defer func() { _ = os.Remove(dbFile) }() - } - - db, dbErr := sql.Open("sqlite", dbFile) - - if dbErr != nil { - t.Errorf("DB file could not be created: %v", dbErr) - } else { - defer func() { _ = db.Close() }() - } + db, _ := openTempSQLite(t) morpher, morpherErr := dmorph.NewMorpher( dmorph.WithDialect(dmorph.DialectSQLite()), @@ -587,21 +453,7 @@ func TestMigrationApplyUnableRegister(t *testing.T) { // TestMigrationApplyUnableCommit tests the scenario where a migration application fails // due to inability to commit a transaction. func TestMigrationApplyUnableCommit(t *testing.T) { - dbFile, dbFileErr := prepareDB() - - if dbFileErr != nil { - t.Errorf("DB file could not be created: %v", dbFileErr) - } else { - defer func() { _ = os.Remove(dbFile) }() - } - - db, dbErr := sql.Open("sqlite", dbFile) - - if dbErr != nil { - t.Errorf("DB file could not be created: %v", dbErr) - } else { - defer func() { _ = db.Close() }() - } + db, _ := openTempSQLite(t) morpher, morpherErr := dmorph.NewMorpher( dmorph.WithDialect(dmorph.DialectSQLite()), From dece3c824c8783027ae1b5a7c0a7c22bf2072d61 Mon Sep 17 00:00:00 2001 From: Alexander Adam Date: Mon, 25 Aug 2025 01:55:20 +0200 Subject: [PATCH 10/11] Add `db.Close()` call in `TestCallsOnClosedDB` to properly simulate closed database scenario --- dialects_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/dialects_test.go b/dialects_test.go index 7958c1e..62d6b9b 100644 --- a/dialects_test.go +++ b/dialects_test.go @@ -61,6 +61,7 @@ func TestDialectStatements(t *testing.T) { // TestCallsOnClosedDB verifies that methods fail as expected when called on a closed database connection. func TestCallsOnClosedDB(t *testing.T) { db, _ := openTempSQLite(t) + db.Close() assert.Error(t, dmorph.DialectSQLite().EnsureMigrationTableExists(t.Context(), db, "irrelevant"), From 3d9c549786d4ffcf95f56f883297beb4ee523ffd Mon Sep 17 00:00:00 2001 From: Alexander Adam Date: Mon, 25 Aug 2025 02:19:45 +0200 Subject: [PATCH 11/11] Refactor `openTempSQLite` to return only `*sql.DB` and update all test cases accordingly --- dialects_test.go | 17 ++++++----------- file_migration_test.go | 6 +++--- migration_test.go | 28 ++++++++++++++-------------- 3 files changed, 23 insertions(+), 28 deletions(-) diff --git a/dialects_test.go b/dialects_test.go index 62d6b9b..2055759 100644 --- a/dialects_test.go +++ b/dialects_test.go @@ -4,10 +4,10 @@ package dmorph_test import ( - "regexp" "testing" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" "github.com/AlphaOne1/dmorph" ) @@ -30,8 +30,6 @@ func TestDialectStatements(t *testing.T) { {name: "SQLite", caller: dmorph.DialectSQLite}, } - re := regexp.MustCompile("%s") - for k, v := range tests { d := v.caller() @@ -40,28 +38,25 @@ func TestDialectStatements(t *testing.T) { } assert.Contains(t, d.CreateTemplate, "%s", "no table name placeholder in create template for", v.name) - assert.Regexp(t, re, d.CreateTemplate) if len(d.AppliedTemplate) < 10 { t.Errorf("%v: applied template is too short for %v", k, v.name) } assert.Contains(t, d.AppliedTemplate, "%s", "no table name placeholder in applied template for", v.name) - assert.Regexp(t, re, d.AppliedTemplate) if len(d.RegisterTemplate) < 10 { t.Errorf("%v: register template is too short for %v", k, v.name) } assert.Contains(t, d.RegisterTemplate, "%s", "no table name placeholder in register template for", v.name) - assert.Regexp(t, re, d.RegisterTemplate) } } // TestCallsOnClosedDB verifies that methods fail as expected when called on a closed database connection. func TestCallsOnClosedDB(t *testing.T) { - db, _ := openTempSQLite(t) - db.Close() + db := openTempSQLite(t) + require.NoError(t, db.Close()) assert.Error(t, dmorph.DialectSQLite().EnsureMigrationTableExists(t.Context(), db, "irrelevant"), @@ -82,7 +77,7 @@ func TestEnsureMigrationTableExistsSQLError(t *testing.T) { )`, } - db, _ := openTempSQLite(t) + db := openTempSQLite(t) assert.Error(t, dialect.EnsureMigrationTableExists(t.Context(), db, "test"), "expected error") } @@ -108,11 +103,11 @@ func TestEnsureMigrationTableExistsCommitError(t *testing.T) { DELETE FROM t0 WHERE id = 1;`, } - db, _ := openTempSQLite(t) + db := openTempSQLite(t) _, execErr := db.ExecContext(t.Context(), "PRAGMA foreign_keys = ON") - assert.NoError(t, execErr, "foreign keys checking could not be enabled") + require.NoError(t, execErr, "foreign keys checking could not be enabled") assert.Error(t, dialect.EnsureMigrationTableExists(t.Context(), db, "test"), "expected error") } diff --git a/file_migration_test.go b/file_migration_test.go index e80a5ae..e9de046 100644 --- a/file_migration_test.go +++ b/file_migration_test.go @@ -16,7 +16,7 @@ import ( ) func TestWithMigrationFromFile(t *testing.T) { - db, _ := openTempSQLite(t) + db := openTempSQLite(t) runErr := dmorph.Run(t.Context(), db, @@ -27,7 +27,7 @@ func TestWithMigrationFromFile(t *testing.T) { } func TestWithMigrationFromFileError(t *testing.T) { - db, _ := openTempSQLite(t) + db := openTempSQLite(t) runErr := dmorph.Run(t.Context(), db, @@ -52,7 +52,7 @@ func TestMigrationFromFileFSError(t *testing.T) { // TestApplyStepsStreamError tests error handling in applyStepsStream. func TestApplyStepsStreamError(t *testing.T) { - db, _ := openTempSQLite(t) + db := openTempSQLite(t) buf := bytes.Buffer{} buf.WriteString("utter nonsense") diff --git a/migration_test.go b/migration_test.go index ac3c0e8..8bc25bb 100644 --- a/migration_test.go +++ b/migration_test.go @@ -39,7 +39,7 @@ func prepareDB() (string, error) { return result, nil } -func openTempSQLite(t *testing.T) (*sql.DB, string) { +func openTempSQLite(t *testing.T) *sql.DB { t.Helper() dbFile, err := prepareDB() @@ -50,12 +50,12 @@ func openTempSQLite(t *testing.T) (*sql.DB, string) { require.NoError(t, dbErr, "DB could not be opened") t.Cleanup(func() { _ = db.Close() }) - return db, dbFile + return db } // TestMigration tests the happy flow. func TestMigration(t *testing.T) { - db, _ := openTempSQLite(t) + db := openTempSQLite(t) migrationsDir, migrationsDirErr := fs.Sub(testMigrationsDir, "testData") @@ -71,7 +71,7 @@ func TestMigration(t *testing.T) { // TestMigrationUpdate tests the happy flow of updating on existing migrations. func TestMigrationUpdate(t *testing.T) { - db, _ := openTempSQLite(t) + db := openTempSQLite(t) migrationsDir, migrationsDirErr := fs.Sub(testMigrationsDir, "testData") @@ -103,7 +103,7 @@ func (m TestMigrationImpl) Migrate(ctx context.Context, tx *sql.Tx) error { // TestWithMigrations tests the adding of migrations using WithMigrations. func TestWithMigrations(t *testing.T) { - db, _ := openTempSQLite(t) + db := openTempSQLite(t) runErr := dmorph.Run(t.Context(), db, @@ -123,7 +123,7 @@ func TestMigrationUnableToCreateMorpher(t *testing.T) { // TestMigrationTooOld tests what happens if the applied migrations are too old. func TestMigrationTooOld(t *testing.T) { - db, _ := openTempSQLite(t) + db := openTempSQLite(t) migrationsDir, migrationsDirErr := fs.Sub(testMigrationsDir, "testData") @@ -146,7 +146,7 @@ func TestMigrationTooOld(t *testing.T) { // TestMigrationUnrelated0 tests what happens if the applied migrations are unrelated to existing ones. func TestMigrationUnrelated0(t *testing.T) { - db, _ := openTempSQLite(t) + db := openTempSQLite(t) migrationsDir, migrationsDirErr := fs.Sub(testMigrationsDir, "testData") @@ -169,7 +169,7 @@ func TestMigrationUnrelated0(t *testing.T) { // TestMigrationUnrelated1 tests what happens if the applied migrations are unrelated to existing ones. func TestMigrationUnrelated1(t *testing.T) { - db, _ := openTempSQLite(t) + db := openTempSQLite(t) migrationsDir, migrationsDirErr := fs.Sub(testMigrationsDir, "testData") @@ -193,7 +193,7 @@ func TestMigrationUnrelated1(t *testing.T) { // TestMigrationAppliedUnordered tests the case, that somehow the migrations in the // database are registered not in the order of their keys. func TestMigrationAppliedUnordered(t *testing.T) { - db, _ := openTempSQLite(t) + db := openTempSQLite(t) migrationsDir, migrationsDirErr := fs.Sub(testMigrationsDir, "testData") @@ -380,7 +380,7 @@ func TestMigrationRunInvalid(t *testing.T) { // TestMigrationRunInvalidCreate tests the behavior of running a migration // with an invalid CreateTemplate in the dialect. func TestMigrationRunInvalidCreate(t *testing.T) { - db, _ := openTempSQLite(t) + db := openTempSQLite(t) dialect := dmorph.DialectSQLite() dialect.CreateTemplate = "utter nonsense" @@ -398,7 +398,7 @@ func TestMigrationRunInvalidCreate(t *testing.T) { // TestMigrationRunInvalidApplied tests the failure scenario where the AppliedTemplate of the dialect is invalid. func TestMigrationRunInvalidApplied(t *testing.T) { - db, _ := openTempSQLite(t) + db := openTempSQLite(t) dialect := dmorph.DialectSQLite() dialect.AppliedTemplate = "utter nonsense" @@ -416,7 +416,7 @@ func TestMigrationRunInvalidApplied(t *testing.T) { // TestMigrationApplyInvalidDB verifies that applying migrations to an invalid or closed database results in an error. func TestMigrationApplyInvalidDB(t *testing.T) { - db, _ := openTempSQLite(t) + db := openTempSQLite(t) morpher, morpherErr := dmorph.NewMorpher( dmorph.WithDialect(dmorph.DialectSQLite()), @@ -431,7 +431,7 @@ func TestMigrationApplyInvalidDB(t *testing.T) { // TestMigrationApplyUnableRegister tests the behavior when the migration registration fails due to an invalid template. func TestMigrationApplyUnableRegister(t *testing.T) { - db, _ := openTempSQLite(t) + db := openTempSQLite(t) morpher, morpherErr := dmorph.NewMorpher( dmorph.WithDialect(dmorph.DialectSQLite()), @@ -453,7 +453,7 @@ func TestMigrationApplyUnableRegister(t *testing.T) { // TestMigrationApplyUnableCommit tests the scenario where a migration application fails // due to inability to commit a transaction. func TestMigrationApplyUnableCommit(t *testing.T) { - db, _ := openTempSQLite(t) + db := openTempSQLite(t) morpher, morpherErr := dmorph.NewMorpher( dmorph.WithDialect(dmorph.DialectSQLite()),