From 72139e193f01c72d3f8a1bc54cda35ea4a127848 Mon Sep 17 00:00:00 2001
From: Cedric <cedric.cordenier@smartcontract.com>
Date: Wed, 13 Sep 2023 12:08:15 +0100
Subject: [PATCH] [fix] Flakey test runner fixes (#10614)

Changing the format of the test output broke the flakey test runner. In
a previous PR I changed the runner to parse JSON output.

This addresses some bugs that PR introduced, namely:
- stopping on the first test run failure, rather than continuing
- stripping out package (not test) failures
---
 tools/flakeytests/runner.go      | 14 ++++++-
 tools/flakeytests/runner_test.go | 68 ++++++++++++++++++++++++++++++++
 2 files changed, 80 insertions(+), 2 deletions(-)

diff --git a/tools/flakeytests/runner.go b/tools/flakeytests/runner.go
index 316a23dcffb..28522b2aa70 100644
--- a/tools/flakeytests/runner.go
+++ b/tools/flakeytests/runner.go
@@ -84,16 +84,23 @@ func parseOutput(readers ...io.Reader) (map[string]map[string]int, error) {
 				continue
 			}
 
+			// Skip the line if doesn't start with a "{" --
+			// this mean it isn't JSON output.
 			if !strings.HasPrefix(string(t), "{") {
 				continue
 			}
 
 			e, err := newEvent(t)
 			if err != nil {
-
 				return nil, err
 			}
 
+			// We're only interested in test failures, for which
+			// both Package and Test would be present.
+			if e.Package == "" || e.Test == "" {
+				continue
+			}
+
 			switch e.Action {
 			case "fail":
 				if tests[e.Package] == nil {
@@ -133,9 +140,12 @@ func (r *Runner) runTests(failedTests map[string]map[string]int) (io.Reader, err
 		err := r.runTestFn(pkg, ts, r.numReruns, &out)
 		if err != nil {
 			log.Printf("Test command errored: %s\n", err)
+			// There was an error because the command failed with a non-zero
+			// exit code. This could just mean that the test failed again, so let's
+			// keep going.
 			var exErr exitCoder
 			if errors.As(err, &exErr) && exErr.ExitCode() > 0 {
-				return &out, nil
+				continue
 			}
 			return &out, err
 		}
diff --git a/tools/flakeytests/runner_test.go b/tools/flakeytests/runner_test.go
index be53ec7e8ec..6c1d1068765 100644
--- a/tools/flakeytests/runner_test.go
+++ b/tools/flakeytests/runner_test.go
@@ -108,6 +108,31 @@ func TestRunner_WithFlake(t *testing.T) {
 	assert.Equal(t, m.entries["core/assets"], []string{"TestLink"})
 }
 
+func TestRunner_WithFailedPackage(t *testing.T) {
+	output := `
+{"Time":"2023-09-07T15:39:46.378315+01:00","Action":"fail","Package":"github.com/smartcontractkit/chainlink/v2/core/assets","Test":"TestLink","Elapsed":0}
+{"Time":"2023-09-07T15:39:46.378315+01:00","Action":"fail","Package":"github.com/smartcontractkit/chainlink/v2/core/assets","Elapsed":0}
+`
+	m := newMockReporter()
+	r := &Runner{
+		numReruns: 2,
+		readers:   []io.Reader{strings.NewReader(output)},
+		runTestFn: func(pkg string, testNames []string, numReruns int, w io.Writer) error {
+			_, err := w.Write([]byte(output))
+			return err
+		},
+		parse:    parseOutput,
+		reporter: m,
+	}
+
+	// This will report a flake since we've mocked the rerun
+	// to only report one failure (not two as expected).
+	err := r.Run()
+	require.NoError(t, err)
+	assert.Len(t, m.entries, 1)
+	assert.Equal(t, m.entries["core/assets"], []string{"TestLink"})
+}
+
 func TestRunner_AllFailures(t *testing.T) {
 	output := `{"Time":"2023-09-07T15:39:46.378315+01:00","Action":"fail","Package":"github.com/smartcontractkit/chainlink/v2/core/assets","Test":"TestLink","Elapsed":0}`
 
@@ -185,3 +210,46 @@ func TestRunner_RerunFailsWithNonzeroExitCode(t *testing.T) {
 	require.NoError(t, err)
 	assert.Equal(t, m.entries["core/assets"], []string{"TestLink"})
 }
+
+func TestRunner_RerunWithNonZeroExitCodeDoesntStopCommand(t *testing.T) {
+	outputs := []io.Reader{
+		strings.NewReader(`
+{"Time":"2023-09-07T15:39:46.378315+01:00","Action":"fail","Package":"github.com/smartcontractkit/chainlink/v2/core/assets","Test":"TestLink","Elapsed":0}
+`),
+		strings.NewReader(`
+{"Time":"2023-09-07T15:39:46.378315+01:00","Action":"fail","Package":"github.com/smartcontractkit/chainlink/v2/core/services/vrf/v2","Test":"TestMaybeReservedLinkV2","Elapsed":0}
+`),
+	}
+
+	rerunOutputs := []string{
+		`
+{"Time":"2023-09-07T15:39:46.378315+01:00","Action":"fail","Package":"github.com/smartcontractkit/chainlink/v2/core/assets","Test":"TestLink","Elapsed":0}
+{"Time":"2023-09-07T15:39:46.378315+01:00","Action":"pass","Package":"github.com/smartcontractkit/chainlink/v2/core/assets","Test":"TestLink","Elapsed":0}
+`,
+		`
+{"Time":"2023-09-07T15:39:46.378315+01:00","Action":"fail","Package":"github.com/smartcontractkit/chainlink/v2/core/services/vrf/v2","Test":"TestMaybeReservedLinkV2","Elapsed":0}
+{"Time":"2023-09-07T15:39:46.378315+01:00","Action":"fail","Package":"github.com/smartcontractkit/chainlink/v2/core/services/vrf/v2","Test":"TestMaybeReservedLinkV2","Elapsed":0}
+`,
+	}
+
+	index := 0
+	m := newMockReporter()
+	r := &Runner{
+		numReruns: 2,
+		readers:   outputs,
+		runTestFn: func(pkg string, testNames []string, numReruns int, w io.Writer) error {
+
+			_, _ = w.Write([]byte(rerunOutputs[index]))
+			index++
+			return &exitError{}
+		},
+		parse:    parseOutput,
+		reporter: m,
+	}
+
+	err := r.Run()
+	require.NoError(t, err)
+	calls := index
+	assert.Equal(t, 2, calls)
+	assert.Equal(t, m.entries["core/assets"], []string{"TestLink"})
+}