refactor: Change NewMigrationParams signature, fix tests

Simplifies the API and helps to fix a flaky test. It's a breaking change, but this hasn't reached v1 yet, so that's OK. Fix, cleanup tests.
rafaelespinoza · Apr 18, 2021 · 5bd0958 · 5bd0958
1 parent 6991485
commit 5bd0958
Show file tree

Hide file tree

Showing 5 changed files with 194 additions and 153 deletions.
diff --git a/godfish_test.go b/godfish_test.go
@@ -2,134 +2,28 @@ package godfish_test
 
 import (
 	"encoding/json"
-	"fmt"
 	"os"
-	"path/filepath"
-	"strconv"
 	"testing"
 
 	"github.com/rafaelespinoza/godfish"
 )
 
 const baseTestOutputDir = "/tmp/godfish_test"
 
-func TestMain(m *testing.M) {
-	os.MkdirAll(baseTestOutputDir, 0755)
-	m.Run()
-	os.RemoveAll(baseTestOutputDir)
-}
-
-func TestMigrationParams(t *testing.T) {
-	type testCase struct {
-		label      string
-		reversible bool
-	}
-
-	tests := []testCase{
-		{
-			label:      "foo",
-			reversible: true,
-		},
-		{
-			label:      "bar",
-			reversible: false,
-		},
-		{
-			label:      "foo-bar",
-			reversible: false,
-		},
+func TestInit(t *testing.T) {
+	var err error
+	testOutputDir := baseTestOutputDir + "/" + t.Name()
+	if err = os.MkdirAll(testOutputDir, 0755); err != nil {
+		t.Fatal(err)
 	}
 
-	for i, test := range tests {
-		var directory *os.File
-		var err error
-		pathToTestDir := baseTestOutputDir + "/" + t.Name() + "/" + strconv.Itoa(i)
-		if err = os.MkdirAll(pathToTestDir, 0755); err != nil {
-			t.Fatal(err)
-		}
-		if directory, err = os.Open(pathToTestDir); err != nil {
-			t.Fatal(err)
-		}
-		defer os.RemoveAll(directory.Name())
-
-		mig, err := godfish.NewMigrationParams(test.label, test.reversible, directory)
-		if err != nil {
-			t.Error(err)
-		}
-		if mig.Forward.Indirection().Value != godfish.DirForward {
-			t.Errorf(
-				"test %d; wrong Direction; expected %s, got %s",
-				i, godfish.DirForward, mig.Forward.Indirection().Value,
-			)
-		}
-		if test.reversible {
-			if mig.Reverse.Indirection().Value != godfish.DirReverse {
-				t.Errorf(
-					"test %d; wrong Direction; expected %s, got %s",
-					i, godfish.DirReverse, mig.Reverse.Indirection().Value,
-				)
-			}
-		}
-		migrations := []godfish.Migration{mig.Forward, mig.Reverse}
-		for j, mig := range migrations {
-			if j > 0 && !test.reversible {
-				continue
-			}
-			if mig.Label() != test.label {
-				t.Errorf("test [%d][%d]; Name should be unchanged", i, j)
-			}
-			if mig.Version().String() == "" { // TODO: think of better test
-				t.Errorf("test [%d][%d]; got empty Timestamp", i, j)
-			}
-		}
-
-		var filesBefore, filesAfter []string
-		if filesBefore, err = directory.Readdirnames(0); err != nil {
-			t.Fatalf("test %d; %v", err, i)
-		}
-		if err = mig.GenerateFiles(); err != nil {
-			t.Errorf("test %d; %v", i, err)
-		}
-		if filesAfter, err = directory.Readdirnames(0); err != nil {
-			t.Fatalf("test %d; %v", i, err)
-		}
-
-		actualNumFiles := len(filesAfter) - len(filesBefore)
-		numExpectedFiles := 2
-		expectedDirections := []string{"reverse", "forward"}
-		if !test.reversible {
-			numExpectedFiles--
-			// the list of filenames seem to be returned in ctime desc order...
-			expectedDirections = expectedDirections[1:]
-		}
-		if actualNumFiles != numExpectedFiles {
-			t.Errorf(
-				"test %d; expected to generate %d files, got %d",
-				i, numExpectedFiles,
-				actualNumFiles,
-			)
-			continue
-		}
-		for j, name := range filesAfter {
-			if j > 0 && !test.reversible {
-				continue
-			}
-			patt := fmt.Sprintf("%s-[0-9]*-%s.sql", expectedDirections[j], test.label)
-			if match, err := filepath.Match(patt, name); err != nil {
-				t.Fatalf("test [%d][%d]; %v", i, j, err)
-			} else if !match {
-				t.Errorf(
-					"test [%d][%d]; expected filename %q to match pattern %q",
-					i, j, name, patt,
-				)
-			}
-		}
+	var pathToFile string
+	if outdirPath, err := os.MkdirTemp(testOutputDir, ""); err != nil {
+		t.Fatal(err)
+	} else {
+		pathToFile = outdirPath + "/config.json"
 	}
-}
 
-func TestInit(t *testing.T) {
-	var err error
-	const pathToFile = baseTestOutputDir + "/config.json"
 	// setup: file should not exist at first
 	if _, err = os.Stat(pathToFile); !os.IsNotExist(err) {
 		t.Fatalf("setup error; file at %q should not exist", pathToFile)
@@ -145,7 +39,7 @@ func TestInit(t *testing.T) {
 	} else if err = json.Unmarshal(data, &conf); err != nil {
 		t.Fatal(err)
 	}
-	conf.PathToFiles = baseTestOutputDir + "/bar"
+	conf.PathToFiles = testOutputDir + "/bar"
 
 	// test2: write data and make sure it's not overwritten after calling Init
 	if data, err := json.MarshalIndent(conf, "", "\t"); err != nil {
@@ -166,7 +60,7 @@ func TestInit(t *testing.T) {
 	} else if err = json.Unmarshal(data, &conf2); err != nil {
 		t.Fatal(err)
 	}
-	if conf2.PathToFiles != baseTestOutputDir+"/bar" {
+	if conf2.PathToFiles != testOutputDir+"/bar" {
 		t.Errorf(
 			"expected conf.PathToFiles to be %q, got %q",
 			"foo", conf2.PathToFiles,

diff --git a/internal/commands/commands.go b/internal/commands/commands.go
@@ -197,11 +197,7 @@ var subcommands = map[string]*subcommand{
 			return flags
 		},
 		run: func(a arguments) error {
-			dir, err := os.Open(a.Files)
-			if err != nil {
-				return err
-			}
-			migration, err := godfish.NewMigrationParams(a.Name, a.Reversible, dir)
+			migration, err := godfish.NewMigrationParams(a.Name, a.Reversible, a.Files)
 			if err != nil {
 				return err
 			}

diff --git a/internal/test.go b/internal/test.go
@@ -767,10 +767,6 @@ func (m *migrationStub) Label() string                    { return m.label }
 func (m *migrationStub) Version() godfish.Version         { return m.version }
 
 func generateMigrationFiles(pathToTestDir string, stubs []testDriverStub) error {
-	testDir, err := os.Open(pathToTestDir)
-	if err != nil {
-		return err
-	}
 	for i, stub := range stubs {
 		var file *os.File
 		var err error
@@ -790,7 +786,7 @@ func generateMigrationFiles(pathToTestDir string, stubs []testDriverStub) error
 		if params, err = godfish.NewMigrationParams(
 			name,
 			reversible,
-			testDir,
+			pathToTestDir,
 		); err != nil {
 			return err
 		}

diff --git a/migration.go b/migration.go
@@ -70,38 +70,51 @@ type MigrationParams struct {
 	Forward    Migration
 	Reverse    Migration
 	Reversible bool
-	Directory  *os.File
+	Dirpath    string
+	directory  *os.File
 }
 
 // NewMigrationParams constructs a MigrationParams that's ready to use. Passing
 // in true for reversible means that a complementary SQL file will be made for
-// rolling back. The directory parameter specifies which directory to output the
-// files to.
-func NewMigrationParams(label string, reversible bool, directory *os.File) (*MigrationParams, error) {
-	var out MigrationParams
-	var err error
-	var info os.FileInfo
+// rolling back. The dirpath is the path to the directory for the files. An
+// error is returned if dirpath doesn't actually represent a directory.
+func NewMigrationParams(label string, reversible bool, dirpath string) (*MigrationParams, error) {
+	var (
+		err       error
+		info      os.FileInfo
+		directory *os.File
+		now       time.Time
+		version   timestamp
+	)
+
+	if directory, err = os.Open(dirpath); err != nil {
+		return nil, err
+	}
+	defer directory.Close()
 	if info, err = directory.Stat(); err != nil {
 		return nil, err
 	} else if !info.IsDir() {
-		return nil, fmt.Errorf("input dir %q should be a directory", info.Name())
-	}
-	out.Directory = directory
-
-	out.Reversible = reversible
-	now := time.Now().UTC()
-	version := &timestamp{value: now.Unix(), label: now.Format(TimeFormat)}
-	out.Forward = &mutation{
-		indirection: Indirection{Value: DirForward, Label: forwardDirections[0]},
-		label:       label,
-		version:     version,
+		return nil, fmt.Errorf("dirpath %q should be a path to a directory", dirpath)
 	}
-	out.Reverse = &mutation{
-		indirection: Indirection{Value: DirReverse, Label: reverseDirections[0]},
-		label:       label,
-		version:     version,
-	}
-	return &out, nil
+
+	now = time.Now().UTC()
+	version = timestamp{value: now.Unix(), label: now.Format(TimeFormat)}
+
+	return &MigrationParams{
+		Reversible: reversible,
+		Dirpath:    dirpath,
+		Forward: &mutation{
+			indirection: Indirection{Value: DirForward, Label: forwardDirections[0]},
+			label:       label,
+			version:     &version,
+		},
+		Reverse: &mutation{
+			indirection: Indirection{Value: DirReverse, Label: reverseDirections[0]},
+			label:       label,
+			version:     &version,
+		},
+		directory: directory,
+	}, nil
 }
 
 // GenerateFiles creates the migration files. If the migration is reversible it
@@ -114,7 +127,8 @@ func (m *MigrationParams) GenerateFiles() (err error) {
 		forwardFile.Close()
 		reverseFile.Close()
 	}()
-	baseDir := m.Directory.Name()
+	baseDir := m.directory.Name()
+
 	if forwardFile, err = newMigrationFile(m.Forward, baseDir); err != nil {
 		return
 	}