Refactor conf write function into own function

This change (1) reduces the complexity of the Run function and
(2) makes more parts of the CLI testable. Now we are able to test
that the writing portion is actually correct and handles errors
appropriately.

pkg/tstune/backup.go

@@ -19,7 +19,7 @@ const (
 
 // allows us to substitute mock versions in tests
 var filepathGlobFn = filepath.Glob
-var osCreateFn = func(path string) (io.Writer, error) {
+var osCreateFn = func(path string) (io.WriteCloser, error) {
 	return os.Create(path)
 }
 

pkg/tstune/backup_test.go

@@ -12,6 +12,20 @@ import (
 	"time"
 )
 
+type testBufferCloser struct {
+	b         bytes.Buffer
+	shouldErr bool
+}
+
+func (b *testBufferCloser) Write(p []byte) (int, error) {
+	if b.shouldErr {
+		return 0, fmt.Errorf(errTestWriter)
+	}
+	return b.b.Write(p)
+}
+
+func (b *testBufferCloser) Close() error { return nil }
+
 func TestBackup(t *testing.T) {
 	oldOSCreateFn := osCreateFn
 	now := time.Now()
@@ -20,7 +34,7 @@ func TestBackup(t *testing.T) {
 	wantFileName := backupFilePrefix + now.Format(backupDateFmt)
 	wantPath := path.Join(os.TempDir(), wantFileName)
 
-	osCreateFn = func(_ string) (io.Writer, error) {
+	osCreateFn = func(_ string) (io.WriteCloser, error) {
 		return nil, fmt.Errorf("erroring")
 	}
 
@@ -36,8 +50,8 @@ func TestBackup(t *testing.T) {
 		t.Errorf("incorrect error: got\n%s\nwant\n%s", got, want)
 	}
 
-	var buf bytes.Buffer
-	osCreateFn = func(p string) (io.Writer, error) {
+	var buf testBufferCloser
+	osCreateFn = func(p string) (io.WriteCloser, error) {
 		if p != wantPath {
 			t.Errorf("incorrect backup path: got %s want %s", p, wantPath)
 		}
@@ -51,7 +65,7 @@ func TestBackup(t *testing.T) {
 		t.Errorf("unexpected error for backup: %v", err)
 	}
 
-	scanner := bufio.NewScanner(bytes.NewReader(buf.Bytes()))
+	scanner := bufio.NewScanner(bytes.NewReader(buf.b.Bytes()))
 	i := 0
 	for scanner.Scan() {
 		if scanner.Err() != nil {

pkg/tstune/io_handler_test.go

@@ -5,7 +5,7 @@ import (
 	"testing"
 )
 
-const errTestWriter = "erroring"
+const errTestWriter = "error on write"
 
 type testWriter struct {
 	shouldErr bool

pkg/tstune/tuner.go

@@ -59,6 +59,8 @@ const (
 
 	successQuiet = "all settings tuned, no changes needed"
 
+	errCouldNotWriteFmt = "could not open %s for writing: %v"
+
 	fmtTunableParam     = "%s = %s%s\n"
 	fmtLastTuned        = "timescaledb.last_tuned = '%s'"
 	fmtLastTunedVersion = "timescaledb.last_tuned_version = '%s'"
@@ -73,6 +75,7 @@ const (
 var (
 	// allows us to substitute mock versions in tests
 	getPGConfigVersionFn = getPGConfigVersion
+	filepathAbsFn        = filepath.Abs
 
 	pgVersionRegex = regexp.MustCompile("^PostgreSQL ([0-9]+?).([0-9]+?).*")
 	pgVersions     = []string{pgMajor11, pgMajor10, pgMajor96}
@@ -295,22 +298,7 @@ func (t *Tuner) Run(flags *TunerFlags, in io.Reader, out io.Writer, outErr io.Wr
 
 	// Wrap up: Either write it out, or show success in --dry-run
 	if !t.flags.DryRun {
-		outPath := t.flags.DestPath
-		if len(outPath) == 0 {
-			outPath, err = filepath.Abs(filePath)
-			if err != nil {
-				t.handler.exit(1, "could not open %s for writing: %v", filePath, err)
-			}
-		}
-
-		t.handler.p.Statement("Saving changes to: " + outPath)
-		f, err := os.Create(outPath)
-		if err != nil {
-			t.handler.exit(1, "could not open %s for writing: %v", outPath, err)
-		}
-		defer f.Close()
-
-		_, err = t.cfs.WriteTo(f)
+		err = t.writeConfFile(filePath)
 		ifErrHandle(err)
 	} else {
 		t.handler.p.Statement("Success, but not writing due to --dry-run flag")
@@ -628,3 +616,27 @@ func (t *Tuner) processQuiet(config *pgtune.SystemConfig) error {
 
 	return nil
 }
+
+func (t *Tuner) writeConfFile(confPath string) error {
+	var err error
+	outPath := t.flags.DestPath
+	if len(outPath) == 0 {
+		outPath, err = filepathAbsFn(confPath)
+		if err != nil {
+			return fmt.Errorf(errCouldNotWriteFmt, confPath, err)
+		}
+	}
+
+	t.handler.p.Statement("Saving changes to: " + outPath)
+	f, err := osCreateFn(outPath)
+	if err != nil {
+		return fmt.Errorf(errCouldNotWriteFmt, outPath, err)
+	}
+	defer f.Close()
+
+	_, err = t.cfs.WriteTo(f)
+	if err != nil {
+		return fmt.Errorf(errCouldNotWriteFmt, outPath, err)
+	}
+	return nil
+}

pkg/tstune/tuner_test.go

@@ -4,6 +4,7 @@ import (
 	"bufio"
 	"bytes"
 	"fmt"
+	"io"
 	"os"
 	"os/exec"
 	"path"
@@ -1552,3 +1553,112 @@ func TestTunerProcessQuiet(t *testing.T) {
 		}
 	}
 }
+
+func TestTunerWriteConfFile(t *testing.T) {
+	wantPath := "postgresql.conf"
+	errCreateFmt := "path does not exist: %s"
+	errAbsPath := "could not get absolute path"
+	confFileLines := []string{"shared_preload_libraries = 'timescaledb'", "foo"}
+	wantConfFile := strings.Join(confFileLines, "\n")
+
+	cases := []struct {
+		desc             string
+		destPath         string
+		confPath         string
+		statements       uint64
+		shouldErrOnWrite bool
+		errMsg           string
+	}{
+		{
+			desc:       "success",
+			destPath:   wantPath,
+			statements: 1,
+		},
+		{
+			desc:       "success with derived path",
+			confPath:   wantPath,
+			statements: 1,
+		},
+		{
+			desc:     "error on absolute path",
+			confPath: "foo.out",
+			errMsg:   fmt.Sprintf(errCouldNotWriteFmt, "foo.out", errAbsPath),
+		},
+		{
+			desc:     "error on create",
+			destPath: "foo.out",
+			errMsg:   fmt.Sprintf(errCouldNotWriteFmt, "foo.out", fmt.Sprintf(errCreateFmt, "foo.out")),
+		},
+		{
+			desc:             "error on writeTo",
+			destPath:         wantPath,
+			shouldErrOnWrite: true,
+			errMsg:           fmt.Sprintf(errCouldNotWriteFmt, wantPath, errTestWriter),
+		},
+	}
+
+	oldOSCreateFn := osCreateFn
+	oldFilepathAbsFn := filepathAbsFn
+	filepathAbsFn = func(p string) (string, error) {
+		if p == wantPath {
+			return p, nil
+		}
+		return "", fmt.Errorf(errAbsPath)
+	}
+
+	for _, c := range cases {
+		var buf testBufferCloser
+		buf.shouldErr = c.shouldErrOnWrite
+		osCreateFn = func(p string) (io.WriteCloser, error) {
+			if !fileExists(p) && p != wantPath {
+				return nil, fmt.Errorf(errCreateFmt, p)
+			}
+			return &buf, nil
+		}
+
+		out := &testWriter{}
+		handler := &ioHandler{
+			p:      &testPrinter{},
+			out:    out,
+			outErr: out,
+		}
+		confFile := bytes.NewBufferString(wantConfFile)
+		cfs, err := getConfigFileState(confFile)
+		if err != nil {
+			t.Fatalf("could not parse config lines")
+		}
+		tuner := newTunerWithDefaultFlags(handler, cfs)
+		tuner.flags.DestPath = c.destPath
+		err = tuner.writeConfFile(c.confPath)
+		if c.errMsg == "" && err != nil {
+			t.Errorf("%s: unexpected error: got %v", c.desc, err)
+		} else if c.errMsg != "" {
+			if err == nil {
+				t.Errorf("%s: unexpected lack of error", c.desc)
+			} else if got := err.Error(); got != c.errMsg {
+				t.Errorf("%s: incorrect error:\ngot\n%s\nwant\n%s", c.desc, got, c.errMsg)
+			}
+		} else {
+			tp := handler.p.(*testPrinter)
+			if got := tp.statementCalls; got != c.statements {
+				t.Errorf("%s: incorrect number of statements: got %d want %d", c.desc, got, c.statements)
+			}
+
+			scanner := bufio.NewScanner(bytes.NewReader(buf.b.Bytes()))
+			i := 0
+			for scanner.Scan() {
+				if scanner.Err() != nil {
+					t.Errorf("%s: unexpected error while scanning: %v", c.desc, scanner.Err())
+				}
+				got := scanner.Text()
+				if want := confFileLines[i]; got != want {
+					t.Errorf("%s: incorrect line at %d:\ngot\n%s\nwant\n%s", c.desc, i, got, want)
+				}
+				i++
+			}
+		}
+	}
+
+	filepathAbsFn = oldFilepathAbsFn
+	osCreateFn = oldOSCreateFn
+}