From 82bb5fabe04c7a32afed8c71f0dabdcfb0f3faaf Mon Sep 17 00:00:00 2001
From: Erik Seliger <erikseliger@me.com>
Date: Mon, 17 May 2021 20:29:59 +0200
Subject: [PATCH] Fix skip errors flag

We didn't return the specs in case of an error, so we would ignore the error, but still have no changeset specs uploaded, which resulted in the generated batch spec in the preview being empty.
---
 internal/batches/executor/executor.go      |  2 +-
 internal/batches/executor/executor_test.go | 38 ++++++++++++++++++++--
 2 files changed, 37 insertions(+), 3 deletions(-)

diff --git a/internal/batches/executor/executor.go b/internal/batches/executor/executor.go
index e8287a32b9..9e58008ef5 100644
--- a/internal/batches/executor/executor.go
+++ b/internal/batches/executor/executor.go
@@ -171,7 +171,7 @@ func (x *executor) Wait(ctx context.Context) ([]*batches.ChangesetSpec, error) {
 	case err := <-result:
 		close(result)
 		if err != nil {
-			return nil, err
+			return x.specs, err
 		}
 	}
 
diff --git a/internal/batches/executor/executor_test.go b/internal/batches/executor/executor_test.go
index 6547d88a59..0df1fca54c 100644
--- a/internal/batches/executor/executor_test.go
+++ b/internal/batches/executor/executor_test.go
@@ -82,6 +82,8 @@ func TestExecutor_Integration(t *testing.T) {
 		wantAuthorEmail   string
 
 		wantErrInclude string
+		// wantCacheHits overwrites the desired count of cache entries found after execution.
+		wantCacheHits int
 	}{
 		{
 			name: "success",
@@ -407,6 +409,36 @@ output4=integration-test-batch-change`,
 				},
 			},
 		},
+		{
+			name: "skips errors",
+			archives: []mock.RepoArchive{
+				{Repo: srcCLIRepo, Files: map[string]string{
+					"README.md": "# Welcome to the README\n",
+				}},
+				{Repo: sourcegraphRepo, Files: map[string]string{
+					"README.md": "# Sourcegraph README\n",
+				}},
+			},
+			steps: []batches.Step{
+				{Run: `echo -e "foobar\n" >> README.md`},
+				{
+					Run: `exit 1`,
+					If:  fmt.Sprintf(`${{ eq repository.name %q }}`, sourcegraphRepo.Name),
+				},
+			},
+			tasks: []*Task{
+				{Repository: srcCLIRepo},
+				{Repository: sourcegraphRepo},
+			},
+			wantFilesChanged: filesByRepository{
+				srcCLIRepo.ID: filesByBranch{
+					changesetTemplateBranch: []string{"README.md"},
+				},
+				sourcegraphRepo.ID: {},
+			},
+			wantErrInclude: "execution in github.com/sourcegraph/sourcegraph failed: run: exit 1",
+			wantCacheHits:  1,
+		},
 	}
 
 	for _, tc := range tests {
@@ -488,11 +520,10 @@ output4=integration-test-batch-change`,
 					if err == nil {
 						t.Fatalf("expected error to include %q, but got no error", tc.wantErrInclude)
 					} else {
-						if !strings.Contains(err.Error(), tc.wantErrInclude) {
+						if !strings.Contains(strings.ToLower(err.Error()), strings.ToLower(tc.wantErrInclude)) {
 							t.Errorf("wrong error. have=%q want included=%q", err, tc.wantErrInclude)
 						}
 					}
-					return
 				}
 
 				wantSpecs := 0
@@ -578,6 +609,9 @@ output4=integration-test-batch-change`,
 				if tc.wantErrInclude != "" {
 					want = 0
 				}
+				if tc.wantCacheHits != 0 {
+					want = tc.wantCacheHits
+				}
 
 				// Verify that there is a cache entry for each repo.
 				if have := cache.size(); have != want {