Merge pull request #144 from dnephin/fix-retry-fails-err

rerun-fails: fix a bug that would cause an incorrect successful exit

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{}

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())
+}

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 {