Repository: gotestyourself/gotestsum
Pull request #144 (2020-08-29 06:46:53)

rerun-fails: fix a bug that would cause an incorrect successful exit
When multiple tests in a package fail, but the last test run for the package passed.

Also includes a commit which prevents the re-run fails report from being printed if the re-run was aborted because of too many initial test failures.


Patch:
diff --git a/exitcode.go b/exitcode.go
deleted file mode 100644
index f5d2a2a..0000000
--- a/exitcode.go
+++ /dev/null
@@ -1,35 +0,0 @@
-package main
-
-import (
-	"os/exec"
-	"syscall"
-
-	"github.com/pkg/errors"
-)
-
-// Copied from gotestyourself/icmd
-
-// GetExitCode returns the ExitStatus of a process from the error returned by
-// exec.Run(). If the exit status is not available an error is returned.
-func GetExitCode(err error) (int, error) {
-	if exiterr, ok := err.(*exec.ExitError); ok {
-		if procExit, ok := exiterr.Sys().(syscall.WaitStatus); ok {
-			return procExit.ExitStatus(), nil
-		}
-	}
-	return 0, errors.Wrap(err, "failed to get exit code")
-}
-
-// ExitCodeWithDefault returns ExitStatus of a process from the error returned by
-// exec.Run(). If the exit status is not available return a default of 127.
-func ExitCodeWithDefault(err error) int {
-	if err == nil {
-		return 0
-	}
-	exitCode, exiterr := GetExitCode(err)
-	if exiterr != nil {
-		// we've failed to retrieve exit code, so we set it to 127
-		return 127
-	}
-	return exitCode
-}
diff --git a/main.go b/main.go
index c63c9bb..32119e6 100644
--- a/main.go
+++ b/main.go
@@ -27,7 +27,7 @@ func main() {
 	case *exec.ExitError:
 		// go test should already report the error to stderr, exit with
 		// the same status code
-		os.Exit(ExitCodeWithDefault(err))
+		os.Exit(exitCodeWithDefault(err))
 	default:
 		log.Error(err.Error())
 		os.Exit(3)
@@ -197,9 +197,9 @@ func run(opts *options) error {
 		return err
 	}
 
-	goTestProc, err := startGoTest(ctx, goTestCmdArgs(opts, rerunOpts{}))
+	goTestProc, err := startGoTestFn(ctx, goTestCmdArgs(opts, rerunOpts{}))
 	if err != nil {
-		return errors.Wrapf(err, "failed to run %s", strings.Join(goTestProc.cmd.Args, " "))
+		return err
 	}
 
 	handler, err := newEventHandler(opts)
@@ -216,27 +216,40 @@ func run(opts *options) error {
 	if err != nil {
 		return err
 	}
-	goTestExitErr := goTestProc.cmd.Wait()
+	exitErr := goTestProc.cmd.Wait()
+	if exitErr == nil || opts.rerunFailsMaxAttempts == 0 {
+		return finishRun(opts, exec, exitErr)
+	}
+	if err := hasErrors(exitErr, exec); err != nil {
+		return finishRun(opts, exec, err)
+	}
 
-	if goTestExitErr != nil && opts.rerunFailsMaxAttempts > 0 {
-		goTestExitErr = hasErrors(goTestExitErr, exec)
-		if goTestExitErr == nil {
-			cfg := testjson.ScanConfig{Execution: exec, Handler: handler}
-			goTestExitErr = rerunFailed(ctx, opts, cfg)
-		}
+	failed := len(rerunFailsFilter(opts)(exec.Failed()))
+	if failed > opts.rerunFailsMaxInitialFailures {
+		err := fmt.Errorf(
+			"number of test failures (%d) exceeds maximum (%d) set by --rerun-fails-max-failures",
+			failed, opts.rerunFailsMaxInitialFailures)
+		return finishRun(opts, exec, err)
 	}
 
-	testjson.PrintSummary(opts.stdout, exec, opts.noSummary.value)
-	if err := writeJUnitFile(opts, exec); err != nil {
+	cfg = testjson.ScanConfig{Execution: exec, Handler: handler}
+	exitErr = rerunFailed(ctx, opts, cfg)
+	if err := writeRerunFailsReport(opts, exec); err != nil {
 		return err
 	}
-	if err := writeRerunFailsReport(opts, exec); err != nil {
+	return finishRun(opts, exec, exitErr)
+}
+
+func finishRun(opts *options, exec *testjson.Execution, exitErr error) error {
+	testjson.PrintSummary(opts.stdout, exec, opts.noSummary.value)
+
+	if err := writeJUnitFile(opts, exec); err != nil {
 		return err
 	}
 	if err := postRunHook(opts, exec); err != nil {
 		return err
 	}
-	return goTestExitErr
+	return exitErr
 }
 
 func goTestCmdArgs(opts *options, rerunOpts rerunOpts) []string {
@@ -327,32 +340,55 @@ func findPkgArgPosition(args []string) int {
 }
 
 type proc struct {
-	cmd    *exec.Cmd
+	cmd    waiter
 	stdout io.Reader
 	stderr io.Reader
 }
 
+type waiter interface {
+	Wait() error
+}
+
 func startGoTest(ctx context.Context, args []string) (proc, error) {
 	if len(args) == 0 {
 		return proc{}, errors.New("missing command to run")
 	}
 
-	p := proc{
-		cmd: exec.CommandContext(ctx, args[0], args[1:]...),
-	}
-	log.Debugf("exec: %s", p.cmd.Args)
+	cmd := exec.CommandContext(ctx, args[0], args[1:]...)
+	p := proc{cmd: cmd}
+	log.Debugf("exec: %s", cmd.Args)
 	var err error
-	p.stdout, err = p.cmd.StdoutPipe()
+	p.stdout, err = cmd.StdoutPipe()
 	if err != nil {
 		return p, err
 	}
-	p.stderr, err = p.cmd.StderrPipe()
+	p.stderr, err = cmd.StderrPipe()
 	if err != nil {
 		return p, err
 	}
-	err = p.cmd.Start()
+	if err := cmd.Start(); err != nil {
+		return p, errors.Wrapf(err, "failed to run %s", strings.Join(cmd.Args, " "))
+	}
+	log.Debugf("go test pid: %d", cmd.Process.Pid)
+	return p, nil
+}
+
+// GetExitCode returns the ExitStatus of a process from the error returned by
+// exec.Run(). If the exit status is not available an error is returned.
+func exitCodeWithDefault(err error) int {
 	if err == nil {
-		log.Debugf("go test pid: %d", p.cmd.Process.Pid)
+		return 0
+	}
+	if exiterr, ok := err.(exitCoder); ok {
+		if code := exiterr.ExitCode(); code != -1 {
+			return code
+		}
 	}
-	return p, err
+	return 127
 }
+
+type exitCoder interface {
+	ExitCode() int
+}
+
+var _ exitCoder = &exec.ExitError{}
diff --git a/rerunfails.go b/rerunfails.go
index 0c06a0d..cf22239 100644
--- a/rerunfails.go
+++ b/rerunfails.go
@@ -5,9 +5,7 @@ import (
 	"fmt"
 	"os"
 	"sort"
-	"strings"
 
-	"github.com/pkg/errors"
 	"gotest.tools/gotestsum/testjson"
 )
 
@@ -47,24 +45,17 @@ func rerunFailsFilter(o *options) testCaseFilter {
 
 func rerunFailed(ctx context.Context, opts *options, scanConfig testjson.ScanConfig) error {
 	tcFilter := rerunFailsFilter(opts)
-	failed := len(tcFilter(scanConfig.Execution.Failed()))
-	if failed > opts.rerunFailsMaxInitialFailures {
-		return fmt.Errorf(
-			"number of test failures (%d) exceeds maximum (%d) set by --rerun-fails-max-failures",
-			failed, opts.rerunFailsMaxInitialFailures)
-	}
 
 	rec := newFailureRecorderFromExecution(scanConfig.Execution)
-	var lastErr error
 	for attempts := 0; rec.count() > 0 && attempts < opts.rerunFailsMaxAttempts; attempts++ {
 		testjson.PrintSummary(opts.stdout, scanConfig.Execution, testjson.SummarizeNone)
 		opts.stdout.Write([]byte("\n")) // nolint: errcheck
 
 		nextRec := newFailureRecorder(scanConfig.Handler)
 		for _, tc := range tcFilter(rec.failures) {
-			goTestProc, err := startGoTest(ctx, goTestCmdArgs(opts, newRerunOptsFromTestCase(tc)))
+			goTestProc, err := startGoTestFn(ctx, goTestCmdArgs(opts, newRerunOptsFromTestCase(tc)))
 			if err != nil {
-				return errors.Wrapf(err, "failed to run %s", strings.Join(goTestProc.cmd.Args, " "))
+				return err
 			}
 
 			cfg := testjson.ScanConfig{
@@ -77,22 +68,28 @@ func rerunFailed(ctx context.Context, opts *options, scanConfig testjson.ScanCon
 			if _, err := testjson.ScanTestOutput(cfg); err != nil {
 				return err
 			}
-			lastErr = goTestProc.cmd.Wait()
-			if err := hasErrors(lastErr, scanConfig.Execution); err != nil {
+			exitErr := goTestProc.cmd.Wait()
+			if exitErr != nil {
+				nextRec.lastErr = exitErr
+			}
+			if err := hasErrors(exitErr, scanConfig.Execution); err != nil {
 				return err
 			}
 		}
 		rec = nextRec
 	}
-	return lastErr
+	return rec.lastErr
 }
 
+// startGoTestFn is a shim for testing
+var startGoTestFn = startGoTest
+
 func hasErrors(err error, exec *testjson.Execution) error {
 	switch {
 	case len(exec.Errors()) > 0:
 		return fmt.Errorf("rerun aborted because previous run had errors")
 	// Exit code 0 and 1 are expected.
-	case ExitCodeWithDefault(err) > 1:
+	case exitCodeWithDefault(err) > 1:
 		return fmt.Errorf("unexpected go test exit code: %v", err)
 	default:
 		return nil
@@ -102,6 +99,7 @@ func hasErrors(err error, exec *testjson.Execution) error {
 type failureRecorder struct {
 	testjson.EventHandler
 	failures []testjson.TestCase
+	lastErr  error
 }
 
 func newFailureRecorder(handler testjson.EventHandler) *failureRecorder {


Test patch:
diff --git a/main_test.go b/main_test.go
index 9e1b5d2..dd558f9 100644
--- a/main_test.go
+++ b/main_test.go
@@ -2,6 +2,8 @@ package main
 
 import (
 	"bytes"
+	"os"
+	"strings"
 	"testing"
 
 	"gotest.tools/v3/assert"
@@ -286,3 +288,71 @@ func TestGoTestCmdArgs(t *testing.T) {
 		})
 	}
 }
+
+func TestRun_RerunFails_WithTooManyInitialFailures(t *testing.T) {
+	jsonFailed := `{"Package": "pkg", "Action": "run"}
+{"Package": "pkg", "Test": "TestOne", "Action": "run"}
+{"Package": "pkg", "Test": "TestOne", "Action": "fail"}
+{"Package": "pkg", "Test": "TestTwo", "Action": "run"}
+{"Package": "pkg", "Test": "TestTwo", "Action": "fail"}
+{"Package": "pkg", "Action": "fail"}
+`
+
+	fn := func(args []string) proc {
+		return proc{
+			cmd:    fakeWaiter{result: newExitCode("failed", 1)},
+			stdout: strings.NewReader(jsonFailed),
+			stderr: bytes.NewReader(nil),
+		}
+	}
+	reset := patchStartGoTestFn(fn)
+	defer reset()
+
+	out := new(bytes.Buffer)
+	opts := &options{
+		rawCommand:                   true,
+		args:                         []string{"./test.test"},
+		format:                       "testname",
+		rerunFailsMaxAttempts:        3,
+		rerunFailsMaxInitialFailures: 1,
+		stdout:                       out,
+		stderr:                       os.Stderr,
+		noSummary:                    newNoSummaryValue(),
+	}
+	err := run(opts)
+	assert.ErrorContains(t, err, "number of test failures (2) exceeds maximum (1)", out.String())
+}
+
+func TestRun_RerunFails_BuildErrorPreventsRerun(t *testing.T) {
+	jsonFailed := `{"Package": "pkg", "Action": "run"}
+{"Package": "pkg", "Test": "TestOne", "Action": "run"}
+{"Package": "pkg", "Test": "TestOne", "Action": "fail"}
+{"Package": "pkg", "Test": "TestTwo", "Action": "run"}
+{"Package": "pkg", "Test": "TestTwo", "Action": "fail"}
+{"Package": "pkg", "Action": "fail"}
+`
+
+	fn := func(args []string) proc {
+		return proc{
+			cmd:    fakeWaiter{result: newExitCode("failed", 1)},
+			stdout: strings.NewReader(jsonFailed),
+			stderr: strings.NewReader("anything here is an error\n"),
+		}
+	}
+	reset := patchStartGoTestFn(fn)
+	defer reset()
+
+	out := new(bytes.Buffer)
+	opts := &options{
+		rawCommand:                   true,
+		args:                         []string{"./test.test"},
+		format:                       "testname",
+		rerunFailsMaxAttempts:        3,
+		rerunFailsMaxInitialFailures: 1,
+		stdout:                       out,
+		stderr:                       os.Stderr,
+		noSummary:                    newNoSummaryValue(),
+	}
+	err := run(opts)
+	assert.ErrorContains(t, err, "rerun aborted because previous run had errors", out.String())
+}
diff --git a/rerunfails_test.go b/rerunfails_test.go
index 8fc6f55..225d431 100644
--- a/rerunfails_test.go
+++ b/rerunfails_test.go
@@ -2,7 +2,10 @@ package main
 
 import (
 	"bytes"
+	"context"
+	"fmt"
 	"io/ioutil"
+	"strings"
 	"testing"
 
 	"gotest.tools/gotestsum/testjson"
@@ -59,3 +62,112 @@ func TestGoTestRunFlagFromTestCases(t *testing.T) {
 		})
 	}
 }
+
+func TestRerunFailed_ReturnsAnErrorWhenTheLastTestIsSuccessful(t *testing.T) {
+	type result struct {
+		out string
+		err error
+	}
+	jsonFailed := `{"Package": "pkg", "Action": "run"}
+{"Package": "pkg", "Test": "TestOne", "Action": "run"}
+{"Package": "pkg", "Test": "TestOne", "Action": "fail"}
+{"Package": "pkg", "Action": "fail"}
+`
+	events := []result{
+		{out: jsonFailed, err: newExitCode("run-failed-1", 1)},
+		{out: jsonFailed, err: newExitCode("run-failed-2", 1)},
+		{out: jsonFailed, err: newExitCode("run-failed-3", 1)},
+		{
+			out: `{"Package": "pkg", "Action": "run"}
+{"Package": "pkg", "Test": "TestOne", "Action": "run"}
+{"Package": "pkg", "Test": "TestOne", "Action": "pass"}
+{"Package": "pkg", "Action": "pass"}
+`,
+		},
+	}
+
+	fn := func(args []string) proc {
+		next := events[0]
+		events = events[1:]
+		return proc{
+			cmd:    fakeWaiter{result: next.err},
+			stdout: strings.NewReader(next.out),
+			stderr: bytes.NewReader(nil),
+		}
+	}
+	reset := patchStartGoTestFn(fn)
+	defer reset()
+
+	stdout := new(bytes.Buffer)
+	ctx := context.Background()
+	opts := &options{
+		rerunFailsMaxInitialFailures: 10,
+		rerunFailsMaxAttempts:        2,
+		stdout:                       stdout,
+	}
+	cfg := testjson.ScanConfig{
+		Execution: newExecutionWithTwoFailures(t),
+		Handler:   noopHandler{},
+	}
+	err := rerunFailed(ctx, opts, cfg)
+	assert.Error(t, err, "run-failed-3")
+}
+
+func patchStartGoTestFn(f func(args []string) proc) func() {
+	orig := startGoTestFn
+	startGoTestFn = func(ctx context.Context, args []string) (proc, error) {
+		return f(args), nil
+	}
+	return func() {
+		startGoTestFn = orig
+	}
+}
+
+func newExecutionWithTwoFailures(t *testing.T) *testjson.Execution {
+	t.Helper()
+
+	out := `{"Package": "pkg", "Action": "run"}
+{"Package": "pkg", "Test": "TestOne", "Action": "run"}
+{"Package": "pkg", "Test": "TestOne", "Action": "fail"}
+{"Package": "pkg", "Test": "TestTwo", "Action": "run"}
+{"Package": "pkg", "Test": "TestTwo", "Action": "fail"}
+{"Package": "pkg", "Action": "fail"}
+`
+	exec, err := testjson.ScanTestOutput(testjson.ScanConfig{
+		Stdout: strings.NewReader(out),
+		Stderr: strings.NewReader(""),
+	})
+	assert.NilError(t, err)
+	return exec
+}
+
+type fakeWaiter struct {
+	result error
+}
+
+func (f fakeWaiter) Wait() error {
+	return f.result
+}
+
+type exitCodeError struct {
+	error
+	code int
+}
+
+func (e exitCodeError) ExitCode() int {
+	return e.code
+}
+
+func newExitCode(msg string, code int) error {
+	return exitCodeError{error: fmt.Errorf(msg), code: code}
+}
+
+type noopHandler struct{}
+
+func (s noopHandler) Event(testjson.TestEvent, *testjson.Execution) error {
+	return nil
+}
+
+func (s noopHandler) Err(string) error {
+	return nil
+}