From b4af752f57daec3b65ea1ff471ad3c0582ed76a8 Mon Sep 17 00:00:00 2001 From: Michael Fridman Date: Sat, 28 Jan 2023 10:41:44 -0500 Subject: [PATCH] fix test race condition and remove verbose global in parser (#457) --- internal/sqlparser/parser.go | 131 +++++++++++++++++------------- internal/sqlparser/parser_test.go | 20 +++-- migration.go | 3 +- tests/e2e/allow_missing_test.go | 2 - tests/e2e/main_test.go | 6 ++ tests/e2e/migrations_test.go | 5 -- tests/e2e/no_versioning_test.go | 1 - 7 files changed, 96 insertions(+), 72 deletions(-) diff --git a/internal/sqlparser/parser.go b/internal/sqlparser/parser.go index 50fc62546..9bb42632b 100644 --- a/internal/sqlparser/parser.go +++ b/internal/sqlparser/parser.go @@ -11,6 +11,20 @@ import ( "sync" ) +type Direction string + +const ( + DirectionUp Direction = "up" + DirectionDown Direction = "down" +) + +func FromBool(b bool) Direction { + if b { + return DirectionUp + } + return DirectionDown +} + type parserState int const ( @@ -23,15 +37,37 @@ const ( gooseStatementEndDown // 6 ) -type stateMachine parserState +type stateMachine struct { + state parserState + verbose bool +} -func (s *stateMachine) Get() parserState { - return parserState(*s) +func newStateMachine(begin parserState, verbose bool) *stateMachine { + return &stateMachine{ + state: begin, + verbose: verbose, + } +} + +func (s *stateMachine) get() parserState { + return s.state } -func (s *stateMachine) Set(new parserState) { - verboseInfo("StateMachine: %v => %v", *s, new) - *s = stateMachine(new) +func (s *stateMachine) set(new parserState) { + s.print("set %d => %d", s.state, new) + s.state = new +} + +const ( + grayColor = "\033[90m" + resetColor = "\033[00m" +) + +func (s *stateMachine) print(msg string, args ...interface{}) { + msg = "StateMachine: " + msg + if s.verbose { + log.Printf(grayColor+msg+resetColor, args...) + } } const scanBufSize = 4 * 1024 * 1024 @@ -53,7 +89,7 @@ var bufferPool = sync.Pool{ // within a statement. For these cases, we provide the explicit annotations // 'StatementBegin' and 'StatementEnd' to allow the script to // tell us to ignore semicolons. -func ParseSQLMigration(r io.Reader, direction bool) (stmts []string, useTx bool, err error) { +func ParseSQLMigration(r io.Reader, direction Direction, verbose bool) (stmts []string, useTx bool, err error) { scanBufPtr := bufferPool.Get().(*[]byte) scanBuf := *scanBufPtr defer bufferPool.Put(scanBufPtr) @@ -61,7 +97,7 @@ func ParseSQLMigration(r io.Reader, direction bool) (stmts []string, useTx bool, scanner := bufio.NewScanner(r) scanner.Buffer(scanBuf, scanBufSize) - stateMachine := stateMachine(start) + stateMachine := newStateMachine(start, verbose) useTx = true var buf bytes.Buffer @@ -70,7 +106,7 @@ func ParseSQLMigration(r io.Reader, direction bool) (stmts []string, useTx bool, if verbose { log.Println(line) } - if stateMachine.Get() == start && strings.TrimSpace(line) == "" { + if stateMachine.get() == start && strings.TrimSpace(line) == "" { continue } // TODO(mf): validate annotations to avoid common user errors: @@ -80,40 +116,40 @@ func ParseSQLMigration(r io.Reader, direction bool) (stmts []string, useTx bool, switch cmd { case "+goose Up": - switch stateMachine.Get() { + switch stateMachine.get() { case start: - stateMachine.Set(gooseUp) + stateMachine.set(gooseUp) default: - return nil, false, fmt.Errorf("duplicate '-- +goose Up' annotations; stateMachine=%v, see https://github.com/pressly/goose#sql-migrations", stateMachine) + return nil, false, fmt.Errorf("duplicate '-- +goose Up' annotations; stateMachine=%d, see https://github.com/pressly/goose#sql-migrations", stateMachine.state) } continue case "+goose Down": - switch stateMachine.Get() { + switch stateMachine.get() { case gooseUp, gooseStatementEndUp: - stateMachine.Set(gooseDown) + stateMachine.set(gooseDown) default: - return nil, false, fmt.Errorf("must start with '-- +goose Up' annotation, stateMachine=%v, see https://github.com/pressly/goose#sql-migrations", stateMachine) + return nil, false, fmt.Errorf("must start with '-- +goose Up' annotation, stateMachine=%d, see https://github.com/pressly/goose#sql-migrations", stateMachine.state) } continue case "+goose StatementBegin": - switch stateMachine.Get() { + switch stateMachine.get() { case gooseUp, gooseStatementEndUp: - stateMachine.Set(gooseStatementBeginUp) + stateMachine.set(gooseStatementBeginUp) case gooseDown, gooseStatementEndDown: - stateMachine.Set(gooseStatementBeginDown) + stateMachine.set(gooseStatementBeginDown) default: - return nil, false, fmt.Errorf("'-- +goose StatementBegin' must be defined after '-- +goose Up' or '-- +goose Down' annotation, stateMachine=%v, see https://github.com/pressly/goose#sql-migrations", stateMachine) + return nil, false, fmt.Errorf("'-- +goose StatementBegin' must be defined after '-- +goose Up' or '-- +goose Down' annotation, stateMachine=%d, see https://github.com/pressly/goose#sql-migrations", stateMachine.state) } continue case "+goose StatementEnd": - switch stateMachine.Get() { + switch stateMachine.get() { case gooseStatementBeginUp: - stateMachine.Set(gooseStatementEndUp) + stateMachine.set(gooseStatementEndUp) case gooseStatementBeginDown: - stateMachine.Set(gooseStatementEndDown) + stateMachine.set(gooseStatementEndDown) default: return nil, false, errors.New("'-- +goose StatementEnd' must be defined after '-- +goose StatementBegin', see https://github.com/pressly/goose#sql-migrations") } @@ -129,11 +165,11 @@ func ParseSQLMigration(r io.Reader, direction bool) (stmts []string, useTx bool, if buf.Len() == 0 { // This check ensures leading comments and empty lines prior to a statement are ignored. if strings.HasPrefix(strings.TrimSpace(line), "--") || line == "" { - verboseInfo("StateMachine: ignore comment") + stateMachine.print("ignore comment") continue } } - switch stateMachine.Get() { + switch stateMachine.get() { case gooseStatementEndDown, gooseStatementEndUp: // Do not include the "+goose StatementEnd" annotation in the final statement. default: @@ -147,46 +183,46 @@ func ParseSQLMigration(r io.Reader, direction bool) (stmts []string, useTx bool, // 1) basic query with semicolon; 2) psql statement // // Export statement once we hit end of statement. - switch stateMachine.Get() { + switch stateMachine.get() { case gooseUp, gooseStatementBeginUp, gooseStatementEndUp: - if !direction /*down*/ { + if direction == DirectionDown { buf.Reset() - verboseInfo("StateMachine: ignore down") + stateMachine.print("ignore down") continue } case gooseDown, gooseStatementBeginDown, gooseStatementEndDown: - if direction /*up*/ { + if direction == DirectionUp { buf.Reset() - verboseInfo("StateMachine: ignore up") + stateMachine.print("ignore up") continue } default: - return nil, false, fmt.Errorf("failed to parse migration: unexpected state %d on line %q, see https://github.com/pressly/goose#sql-migrations", stateMachine, line) + return nil, false, fmt.Errorf("failed to parse migration: unexpected state %d on line %q, see https://github.com/pressly/goose#sql-migrations", stateMachine.state, line) } - switch stateMachine.Get() { + switch stateMachine.get() { case gooseUp: if endsWithSemicolon(line) { stmts = append(stmts, cleanupStatement(buf.String())) buf.Reset() - verboseInfo("StateMachine: store simple Up query") + stateMachine.print("store simple Up query") } case gooseDown: if endsWithSemicolon(line) { stmts = append(stmts, cleanupStatement(buf.String())) buf.Reset() - verboseInfo("StateMachine: store simple Down query") + stateMachine.print("store simple Down query") } case gooseStatementEndUp: stmts = append(stmts, cleanupStatement(buf.String())) buf.Reset() - verboseInfo("StateMachine: store Up statement") - stateMachine.Set(gooseUp) + stateMachine.print("store Up statement") + stateMachine.set(gooseUp) case gooseStatementEndDown: stmts = append(stmts, cleanupStatement(buf.String())) buf.Reset() - verboseInfo("StateMachine: store Down statement") - stateMachine.Set(gooseDown) + stateMachine.print("store Down statement") + stateMachine.set(gooseDown) } } if err := scanner.Err(); err != nil { @@ -194,7 +230,7 @@ func ParseSQLMigration(r io.Reader, direction bool) (stmts []string, useTx bool, } // EOF - switch stateMachine.Get() { + switch stateMachine.get() { case start: return nil, false, errors.New("failed to parse migration: must start with '-- +goose Up' annotation, see https://github.com/pressly/goose#sql-migrations") case gooseStatementBeginUp, gooseStatementBeginDown: @@ -202,7 +238,7 @@ func ParseSQLMigration(r io.Reader, direction bool) (stmts []string, useTx bool, } if bufferRemaining := strings.TrimSpace(buf.String()); len(bufferRemaining) > 0 { - return nil, false, fmt.Errorf("failed to parse migration: state %d, direction: %v: unexpected unfinished SQL query: %q: missing semicolon?", stateMachine, direction, bufferRemaining) + return nil, false, fmt.Errorf("failed to parse migration: state %d, direction: %v: unexpected unfinished SQL query: %q: missing semicolon?", stateMachine.state, direction, bufferRemaining) } return stmts, useTx, nil @@ -240,20 +276,3 @@ func endsWithSemicolon(line string) bool { return strings.HasSuffix(prev, ";") } - -var verbose bool - -func SetVersbose(b bool) { - verbose = b -} - -const ( - grayColor = "\033[90m" - resetColor = "\033[00m" -) - -func verboseInfo(s string, args ...interface{}) { - if verbose { - log.Printf(grayColor+s+resetColor, args...) - } -} diff --git a/internal/sqlparser/parser_test.go b/internal/sqlparser/parser_test.go index 2a4a78c4b..d95eecd05 100644 --- a/internal/sqlparser/parser_test.go +++ b/internal/sqlparser/parser_test.go @@ -11,6 +11,15 @@ import ( "github.com/pressly/goose/v3/internal/check" ) +var ( + debug = false +) + +func TestMain(m *testing.M) { + debug, _ = strconv.ParseBool(os.Getenv("DEBUG_TEST")) + os.Exit(m.Run()) +} + func TestSemicolons(t *testing.T) { t.Parallel() @@ -38,7 +47,6 @@ func TestSemicolons(t *testing.T) { func TestSplitStatements(t *testing.T) { t.Parallel() - // SetVerbose(true) type testData struct { sql string @@ -59,7 +67,7 @@ func TestSplitStatements(t *testing.T) { for i, test := range tt { // up - stmts, _, err := ParseSQLMigration(strings.NewReader(test.sql), true) + stmts, _, err := ParseSQLMigration(strings.NewReader(test.sql), DirectionUp, debug) if err != nil { t.Error(fmt.Errorf("tt[%v] unexpected error: %w", i, err)) } @@ -68,7 +76,7 @@ func TestSplitStatements(t *testing.T) { } // down - stmts, _, err = ParseSQLMigration(strings.NewReader(test.sql), false) + stmts, _, err = ParseSQLMigration(strings.NewReader(test.sql), DirectionDown, debug) if err != nil { t.Error(fmt.Errorf("tt[%v] unexpected error: %w", i, err)) } @@ -97,7 +105,7 @@ func TestUseTransactions(t *testing.T) { if err != nil { t.Error(err) } - _, useTx, err := ParseSQLMigration(f, true) + _, useTx, err := ParseSQLMigration(f, DirectionUp, debug) if err != nil { t.Error(err) } @@ -117,7 +125,7 @@ func TestParsingErrors(t *testing.T) { downFirst, } for i, sql := range tt { - _, _, err := ParseSQLMigration(strings.NewReader(sql), true) + _, _, err := ParseSQLMigration(strings.NewReader(sql), DirectionUp, debug) if err == nil { t.Errorf("expected error on tt[%v] %q", i, sql) } @@ -386,7 +394,7 @@ func testValidUp(t *testing.T, dir string, count int) { f, err := os.Open(filepath.Join(dir, "input.sql")) check.NoError(t, err) t.Cleanup(func() { f.Close() }) - statements, _, err := ParseSQLMigration(f, true) + statements, _, err := ParseSQLMigration(f, DirectionUp, debug) check.NoError(t, err) check.Number(t, len(statements), count) compareStatements(t, dir, statements) diff --git a/migration.go b/migration.go index 495eb757a..0762d6496 100644 --- a/migration.go +++ b/migration.go @@ -61,8 +61,7 @@ func (m *Migration) run(db *sql.DB, direction bool) error { } defer f.Close() - sqlparser.SetVersbose(verbose) - statements, useTx, err := sqlparser.ParseSQLMigration(f, direction) + statements, useTx, err := sqlparser.ParseSQLMigration(f, sqlparser.FromBool(direction), verbose) if err != nil { return fmt.Errorf("ERROR %v: failed to parse SQL migration file: %w", filepath.Base(m.Source), err) } diff --git a/tests/e2e/allow_missing_test.go b/tests/e2e/allow_missing_test.go index 33cae57c5..cc18f29c4 100644 --- a/tests/e2e/allow_missing_test.go +++ b/tests/e2e/allow_missing_test.go @@ -321,8 +321,6 @@ func setupTestDB(t *testing.T, version int64) *sql.DB { db, err := newDockerDB(t) check.NoError(t, err) - check.NoError(t, goose.SetDialect(*dialect)) - // Create goose table. current, err := goose.EnsureDBVersion(db) check.NoError(t, err) diff --git a/tests/e2e/main_test.go b/tests/e2e/main_test.go index 628e366c3..4fb18957a 100644 --- a/tests/e2e/main_test.go +++ b/tests/e2e/main_test.go @@ -11,6 +11,7 @@ import ( "syscall" "testing" + "github.com/pressly/goose/v3" "github.com/pressly/goose/v3/internal/check" "github.com/pressly/goose/v3/internal/testdb" ) @@ -73,6 +74,11 @@ func TestMain(m *testing.M) { migrationsDir = filepath.Join("testdata", *dialect, "migrations") seedDir = filepath.Join("testdata", *dialect, "seed") + if err := goose.SetDialect(*dialect); err != nil { + log.Printf("failed to set dialect %q: %v\n", *dialect, err) + os.Exit(1) + } + exitCode := m.Run() // Useful for debugging test services. if *debug { diff --git a/tests/e2e/migrations_test.go b/tests/e2e/migrations_test.go index b0bd949ed..f721965ba 100644 --- a/tests/e2e/migrations_test.go +++ b/tests/e2e/migrations_test.go @@ -17,7 +17,6 @@ func TestMigrateUpWithReset(t *testing.T) { db, err := newDockerDB(t) check.NoError(t, err) - check.NoError(t, goose.SetDialect(*dialect)) migrations, err := goose.CollectMigrations(migrationsDir, 0, goose.MaxVersion) check.NoError(t, err) check.NumberNotZero(t, len(migrations)) @@ -46,7 +45,6 @@ func TestMigrateUpWithRedo(t *testing.T) { db, err := newDockerDB(t) check.NoError(t, err) - check.NoError(t, goose.SetDialect(*dialect)) migrations, err := goose.CollectMigrations(migrationsDir, 0, goose.MaxVersion) check.NoError(t, err) @@ -84,7 +82,6 @@ func TestMigrateUpTo(t *testing.T) { ) db, err := newDockerDB(t) check.NoError(t, err) - check.NoError(t, goose.SetDialect(*dialect)) migrations, err := goose.CollectMigrations(migrationsDir, 0, goose.MaxVersion) check.NoError(t, err) check.NumberNotZero(t, len(migrations)) @@ -106,7 +103,6 @@ func TestMigrateUpByOne(t *testing.T) { db, err := newDockerDB(t) check.NoError(t, err) - check.NoError(t, goose.SetDialect(*dialect)) migrations, err := goose.CollectMigrations(migrationsDir, 0, goose.MaxVersion) check.NoError(t, err) check.NumberNotZero(t, len(migrations)) @@ -137,7 +133,6 @@ func TestMigrateFull(t *testing.T) { db, err := newDockerDB(t) check.NoError(t, err) - check.NoError(t, goose.SetDialect(*dialect)) migrations, err := goose.CollectMigrations(migrationsDir, 0, goose.MaxVersion) check.NoError(t, err) check.NumberNotZero(t, len(migrations)) diff --git a/tests/e2e/no_versioning_test.go b/tests/e2e/no_versioning_test.go index 9945c29f9..dc0c5d8dd 100644 --- a/tests/e2e/no_versioning_test.go +++ b/tests/e2e/no_versioning_test.go @@ -20,7 +20,6 @@ func TestNoVersioning(t *testing.T) { ) db, err := newDockerDB(t) check.NoError(t, err) - check.NoError(t, goose.SetDialect(*dialect)) err = goose.Up(db, migrationsDir) check.NoError(t, err)