Merge pull request #5 from WillAbides/cleaner

better tests and cleanup

Author: WillAbides

cmd/benchdiff/internal/benchdiff.go

@@ -46,21 +46,6 @@ func fileExists(path string) bool {
 	return true
 }
 
-func (c *Benchdiff) gitRunner() *gitRunner {
-	return &gitRunner{
-		gitExecutable: c.GitCmd,
-		repoPath:      c.Path,
-	}
-}
-
-func (c *Benchdiff) baseRefRunner() *refRunner {
-	gr := c.gitRunner()
-	return &refRunner{
-		ref:       c.BaseRef,
-		gitRunner: *gr,
-	}
-}
-
 func (c *Benchdiff) cacheKey() string {
 	var b []byte
 	b = append(b, []byte(c.BenchCmd)...)
@@ -70,6 +55,11 @@ func (c *Benchdiff) cacheKey() string {
 }
 
 func (c *Benchdiff) runBenchmarks() (result *runBenchmarksResults, err error) {
+	gitCmd := c.GitCmd
+	if gitCmd == "" {
+		gitCmd = "git"
+	}
+
 	result = new(runBenchmarksResults)
 	worktreeFilename := filepath.Join(c.ResultsDir, "benchdiff-worktree.out")
 	worktreeFile, err := os.Create(worktreeFilename)
@@ -91,19 +81,21 @@ func (c *Benchdiff) runBenchmarks() (result *runBenchmarksResults, err error) {
 		return nil, err
 	}
 
-	headSHA, err := c.gitRunner().getRefSha("HEAD")
+	headSHA, err := runGitCmd(gitCmd, c.Path, "rev-parse", "HEAD")
 	if err != nil {
 		return nil, err
 	}
-	baseSHA, err := c.gitRunner().getRefSha(c.BaseRef)
+	result.headSHA = strings.TrimSpace(string(headSHA))
+
+	baseSHA, err := runGitCmd(gitCmd, c.Path, "rev-parse", c.BaseRef)
 	if err != nil {
 		return nil, err
 	}
+	result.baseSHA = strings.TrimSpace(string(baseSHA))
 
 	baseFilename := fmt.Sprintf("benchdiff-%s-%s.out", baseSHA, c.cacheKey())
 	baseFilename = filepath.Join(c.ResultsDir, baseFilename)
-	result.headSHA = headSHA
-	result.baseSHA = baseSHA
+
 	result.baseOutputFile = baseFilename
 	result.worktreeOutputFile = worktreeFilename
 
@@ -126,7 +118,7 @@ func (c *Benchdiff) runBenchmarks() (result *runBenchmarksResults, err error) {
 	baseCmd.Stdout = baseFile
 	var baseCmdErr error
 
-	err = c.baseRefRunner().run(func() {
+	err = runAtGitRef(gitCmd, c.Path, c.BaseRef, func() {
 		baseCmdErr = baseCmd.Run()
 	})
 	if err != nil {

cmd/benchdiff/internal/benchdiff_test.go

@@ -40,11 +40,12 @@ func testInDir(t *testing.T, dir string) {
 	})
 }
 
-func TestBenchstat_Run(t *testing.T) {
+func TestBenchdiff_Run(t *testing.T) {
 	dir := tmpDir(t)
 	setupTestRepo(t, dir)
 	testInDir(t, dir)
 	differ := Benchdiff{
+		GitCmd:     "git",
 		BenchCmd:   "go",
 		BenchArgs:  "test -bench . -benchmem -count 10 -benchtime 10x .",
 		ResultsDir: "./tmp",

cmd/benchdiff/internal/gitrunner.go

@@ -7,28 +7,10 @@ import (
 	"path/filepath"
 )
 
-type gitRunner struct {
-	repoPath      string
-	gitExecutable string
-}
-
-func (r *gitRunner) getRefSha(ref string) (string, error) {
-	b, err := r.run("rev-parse", ref)
-	if err != nil {
-		return "", err
-	}
-	b = bytes.TrimSpace(b)
-	return string(b), nil
-}
-
-func (r *gitRunner) run(args ...string) ([]byte, error) {
-	executable := "git"
-	if r.gitExecutable != "" {
-		executable = r.gitExecutable
-	}
-	cmd := exec.Command(executable, args...) //nolint:gosec // this is fine
+func runGitCmd(gitCmd, repoPath string, args ...string) ([]byte, error) {
+	cmd := exec.Command(gitCmd, args...) //nolint:gosec // this is fine
 	var err error
-	cmd.Dir, err = filepath.Abs(r.repoPath)
+	cmd.Dir, err = filepath.Abs(repoPath)
 	if err != nil {
 		return nil, err
 	}
@@ -37,43 +19,39 @@ func (r *gitRunner) run(args ...string) ([]byte, error) {
 	if exitErr, ok := err.(*exec.ExitError); ok {
 		err = fmt.Errorf("error running git command: %s", string(exitErr.Stderr))
 	}
+	b = bytes.TrimSpace(b)
 	return b, err
 }
 
-type refRunner struct {
-	gitRunner gitRunner
-	ref       string
-}
-
-func (r *refRunner) stashAndReset() (revert func() error, err error) {
+func stashAndReset(gitCmd, repoPath string) (revert func() error, err error) {
 	revert = func() error {
 		return nil
 	}
-	stash, err := r.gitRunner.run("stash", "create", "--quiet")
+	stash, err := runGitCmd(gitCmd, repoPath, "stash", "create", "--quiet")
 	if err != nil {
 		return nil, err
 	}
 	stash = bytes.TrimSpace(stash)
 	if len(stash) > 0 {
 		revert = func() error {
-			_, revertErr := r.gitRunner.run("stash", "apply", "--quiet", string(stash))
+			_, revertErr := runGitCmd(gitCmd, repoPath, "stash", "apply", "--quiet", string(stash))
 			return revertErr
 		}
 	}
-	_, err = r.gitRunner.run("reset", "--hard", "--quiet")
+	_, err = runGitCmd(gitCmd, repoPath, "reset", "--hard", "--quiet")
 	if err != nil {
 		return nil, err
 	}
 	return revert, nil
 }
 
-func (r *refRunner) run(fn func()) error {
-	origRef, err := r.gitRunner.run("rev-parse", "--abbrev-ref", "HEAD")
+func runAtGitRef(gitCmd, repoPath, ref string, fn func()) error {
+	origRef, err := runGitCmd(gitCmd, repoPath, "rev-parse", "--abbrev-ref", "HEAD")
 	if err != nil {
 		return err
 	}
 	origRef = bytes.TrimSpace(origRef)
-	unstash, err := r.stashAndReset()
+	unstash, err := stashAndReset(gitCmd, repoPath)
 	if err != nil {
 		return err
 	}
@@ -83,12 +61,12 @@ func (r *refRunner) run(fn func()) error {
 			panic(unstashErr)
 		}
 	}()
-	_, err = r.gitRunner.run("checkout", "--quiet", r.ref)
+	_, err = runGitCmd(gitCmd, repoPath, "checkout", "--quiet", ref)
 	if err != nil {
 		return err
 	}
 	defer func() {
-		_, cerr := r.gitRunner.run("checkout", "--quiet", string(origRef))
+		_, cerr := runGitCmd(gitCmd, repoPath, "checkout", "--quiet", string(origRef))
 		if cerr != nil {
 			if exitErr, ok := cerr.(*exec.ExitError); ok {
 				fmt.Println(string(exitErr.Stderr))

cmd/benchdiff/internal/gitrunner_test.go

@@ -8,7 +8,7 @@ import (
 	"github.com/stretchr/testify/require"
 )
 
-func Test_refRunner_run(t *testing.T) {
+func Test_runAtGitRef(t *testing.T) {
 	dir := tmpDir(t)
 	fooPath := filepath.Join(dir, "foo")
 	err := ioutil.WriteFile(fooPath, []byte("OG content"), 0o600)
@@ -30,13 +30,7 @@ func Test_refRunner_run(t *testing.T) {
 		require.NoError(t, err)
 		require.Equal(t, "OG content", string(got))
 	}
-	runner := &refRunner{
-		ref: "HEAD",
-		gitRunner: gitRunner{
-			repoPath: dir,
-		},
-	}
-	err = runner.run(fn)
+	err = runAtGitRef("git", dir, "HEAD", fn)
 	require.NoError(t, err)
 	got, err := ioutil.ReadFile(fooPath)
 	require.NoError(t, err)

cmd/benchdiff/internal/testutil_test.go

@@ -29,21 +29,22 @@ func tmpDir(t *testing.T) string {
 	return tmpdir
 }
 
-func mustSetEnv(t *testing.T, key, value string) {
+func mustSetEnv(t *testing.T, env map[string]string) {
 	t.Helper()
-	assert.NoError(t, os.Setenv(key, value))
+	for k, v := range env {
+		assert.NoError(t, os.Setenv(k, v))
+	}
 }
 
 func mustGit(t *testing.T, repoPath string, args ...string) []byte {
 	t.Helper()
-	mustSetEnv(t, "GIT_AUTHOR_NAME", "author")
-	mustSetEnv(t, "GIT_AUTHOR_EMAIL", "author@localhost")
-	mustSetEnv(t, "GIT_COMMITTER_NAME", "committer")
-	mustSetEnv(t, "GIT_COMMITTER_EMAIL", "committer@localhost")
-	runner := &gitRunner{
-		repoPath: repoPath,
-	}
-	got, err := runner.run(args...)
+	mustSetEnv(t, map[string]string{
+		"GIT_AUTHOR_NAME":     "author",
+		"GIT_AUTHOR_EMAIL":    "author@localhost",
+		"GIT_COMMITTER_NAME":  "committer",
+		"GIT_COMMITTER_EMAIL": "committer@localhost",
+	})
+	got, err := runGitCmd("git", repoPath, args...)
 	assert.NoErrorf(t, err, "error running git:\noutput: %v", string(got))
 	return got
 }

go.sum

@@ -33,6 +33,7 @@ golang.org/x/oauth2 v0.0.0-20170207211851-4464e7848382/go.mod h1:N/0e6XlmueqKjAG
 golang.org/x/perf v0.0.0-20201207232921-bdcc6220ee90 h1:P+M61+mQKVHzooHFlNUTNBfj+TqHiQOGgx2kKL7mHbA=
 golang.org/x/perf v0.0.0-20201207232921-bdcc6220ee90/go.mod h1:KRSrLY7jerMEa0Ih7gBheQ3FYDiSx6liMnniX1o3j2g=
 golang.org/x/sync v0.0.0-20190227155943-e225da77a7e6/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM=
+golang.org/x/sys v0.0.0-20190215142949-d0b11bdaac8a h1:1BGLXjeY4akVXGgbC9HugT3Jv3hCI0z56oJR5vAMgBU=
 golang.org/x/sys v0.0.0-20190215142949-d0b11bdaac8a/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY=
 golang.org/x/text v0.3.0/go.mod h1:NqM8EUOU14njkJ3fqMW+pc6Ldnwhi/IjpwHt7yyuwOQ=
 google.golang.org/api v0.0.0-20170206182103-3d017632ea10/go.mod h1:4mhQ8q/RsB7i+udVvVy5NUi08OU8ZlA0gRVgrF7VFY0=

pkg/benchstatter/benchstat.go

@@ -147,8 +147,12 @@ func HTMLFormatter(opts *HTMLFormatterOptions) OutputFormatter {
 		}
 		var buf bytes.Buffer
 		benchstat.FormatHTML(&buf, tables)
+		_, err := w.Write(buf.Bytes())
+		if err != nil {
+			return err
+		}
 		if footer != "" {
-			_, err := w.Write([]byte(footer))
+			_, err = w.Write([]byte(footer))
 			if err != nil {
 				return err
 			}