Fix empty diff being returned when single *cached* step had to be executed (#567)

This PR fixes a bug in the step-wise caching logic that @eseliger and I ran into last week.

### Problem

When no cached result for the *complete* task was found, but a cached result for a single step and that step was the only left to execute then an empty diff was returned.

### How to reproduce

1. Execute a batch spec with the following steps

    ```yaml
    steps:
      - run: echo "this is step 1" >> README.txt
        container: alpine:3
      - run: echo "this is step 2" >> README.md
        container: alpine:3
    ```
    the complete results and the results for each step are now cached.
2. Update the batch spec and re-execute:
    ```yaml
    steps:
      - run: echo "this is step 1" >> README.txt
        container: alpine:3
    ```

This will produce an empty diff because the `Coordinator` did not find a cached result for the complete task/batch spec, but for the first step.

`runSteps` didn't handle this case though, when only a single step had to be executed but that was also cached.

### The fix

Take a look at my comments in the diffs here in GitHub to see what the fix is. Spoiler: it's just a condition and an early exit to handle the case of "cached step result == the only step to execute".

In order to get to this fix though I wrote a lot of tests, found one other bug (`Files` was not cached), and cleaned up the 300 line long `runSteps` function.

`runSteps` is now much easier to understand and doesn't contain _all_ levels of abstraction. The outer `runSteps` concerns itself with which steps need to be executed and how to handle their results and `executeSingleStep` does the lower-level work of rendering templates, starting containers, setting up the logger, etc.

Author: mrnugget

CHANGELOG.md

@@ -17,6 +17,8 @@ All notable changes to `src-cli` are documented in this file.
 
 ### Fixed
 
+- The per-step caching of batch spec execution results was broken when re-execution could use the cached results of a step and that step was the only one left to execute. That resulted in empty diffs being uploaded. This is now fixed. [#567](https://github.com/sourcegraph/src-cli/pull/567)
+
 ### Removed
 
 ## 3.30.0

internal/batches/executor/executor_test.go

@@ -14,9 +14,11 @@ import (
 	"testing"
 	"time"
 
+	"github.com/google/go-cmp/cmp"
 	"github.com/sourcegraph/go-diff/diff"
 	"github.com/sourcegraph/src-cli/internal/api"
 	"github.com/sourcegraph/src-cli/internal/batches"
+	"github.com/sourcegraph/src-cli/internal/batches/git"
 	"github.com/sourcegraph/src-cli/internal/batches/mock"
 	"github.com/sourcegraph/src-cli/internal/batches/workspace"
 )
@@ -466,3 +468,239 @@ func featuresAllEnabled() batches.FeatureFlags {
 		AllowOptionalPublished:   true,
 	}
 }
+
+func TestExecutor_CachedStepResults(t *testing.T) {
+	t.Run("single step cached", func(t *testing.T) {
+		archive := mock.RepoArchive{
+			Repo: testRepo1, Files: map[string]string{
+				"README.md": "# Welcome to the README\n",
+			},
+		}
+
+		cachedDiff := []byte(`diff --git README.md README.md
+index 02a19af..c9644dd 100644
+--- README.md
++++ README.md
+@@ -1 +1,2 @@
+ # Welcome to the README
++foobar
+`)
+
+		task := &Task{
+			BatchChangeAttributes: &BatchChangeAttributes{},
+			Steps: []batches.Step{
+				{Run: `echo -e "foobar\n" >> README.md`},
+			},
+			CachedResultFound: true,
+			CachedResult: stepExecutionResult{
+				StepIndex:          0,
+				Diff:               cachedDiff,
+				Outputs:            map[string]interface{}{},
+				PreviousStepResult: StepResult{},
+			},
+			Repository: testRepo1,
+		}
+
+		results, err := testExecuteTasks(t, []*Task{task}, archive)
+		if err != nil {
+			t.Fatalf("execution failed: %s", err)
+		}
+
+		if have, want := len(results), 1; have != want {
+			t.Fatalf("wrong number of execution results. want=%d, have=%d", want, have)
+		}
+
+		// We want the diff to be the same as the cached one, since we only had to
+		// execute a single step
+		executionResult := results[0].result
+		if diff := cmp.Diff(executionResult.Diff, string(cachedDiff)); diff != "" {
+			t.Fatalf("wrong diff: %s", diff)
+		}
+
+		if have, want := len(results[0].stepResults), 1; have != want {
+			t.Fatalf("wrong length of step results. have=%d, want=%d", have, want)
+		}
+
+		stepResult := results[0].stepResults[0]
+		if diff := cmp.Diff(stepResult, task.CachedResult); diff != "" {
+			t.Fatalf("wrong stepResult: %s", diff)
+		}
+	})
+
+	t.Run("one of multiple steps cached", func(t *testing.T) {
+		archive := mock.RepoArchive{
+			Repo: testRepo1,
+			Files: map[string]string{
+				"README.md": `# automation-testing
+This repository is used to test opening and closing pull request with Automation
+
+(c) Copyright Sourcegraph 2013-2020.
+(c) Copyright Sourcegraph 2013-2020.
+(c) Copyright Sourcegraph 2013-2020.`,
+			},
+		}
+
+		cachedDiff := []byte(`diff --git README.md README.md
+index 1914491..cd2ccbf 100644
+--- README.md
++++ README.md
+@@ -3,4 +3,5 @@ This repository is used to test opening and closing pull request with Automation
+
+ (c) Copyright Sourcegraph 2013-2020.
+ (c) Copyright Sourcegraph 2013-2020.
+-(c) Copyright Sourcegraph 2013-2020.
+\ No newline at end of file
++(c) Copyright Sourcegraph 2013-2020.this is step 2
++this is step 3
+diff --git README.txt README.txt
+new file mode 100644
+index 0000000..888e1ec
+--- /dev/null
++++ README.txt
+@@ -0,0 +1 @@
++this is step 1
+`)
+
+		wantFinalDiff := `diff --git README.md README.md
+index 1914491..d6782d3 100644
+--- README.md
++++ README.md
+@@ -3,4 +3,7 @@ This repository is used to test opening and closing pull request with Automation
+ 
+ (c) Copyright Sourcegraph 2013-2020.
+ (c) Copyright Sourcegraph 2013-2020.
+-(c) Copyright Sourcegraph 2013-2020.
+\ No newline at end of file
++(c) Copyright Sourcegraph 2013-2020.this is step 2
++this is step 3
++this is step 4
++previous_step.modified_files=[README.md]
+diff --git README.txt README.txt
+new file mode 100644
+index 0000000..888e1ec
+--- /dev/null
++++ README.txt
+@@ -0,0 +1 @@
++this is step 1
+diff --git my-output.txt my-output.txt
+new file mode 100644
+index 0000000..257ae8e
+--- /dev/null
++++ my-output.txt
+@@ -0,0 +1 @@
++this is step 5
+`
+
+		task := &Task{
+			Repository:            testRepo1,
+			BatchChangeAttributes: &BatchChangeAttributes{},
+			Steps: []batches.Step{
+				{Run: `echo "this is step 1" >> README.txt`},
+				{Run: `echo "this is step 2" >> README.md`},
+				{Run: `echo "this is step 3" >> README.md`, Outputs: batches.Outputs{
+					"myOutput": batches.Output{
+						Value: "my-output.txt",
+					},
+				}},
+				{Run: `echo "this is step 4" >> README.md
+echo "previous_step.modified_files=${{ previous_step.modified_files }}" >> README.md
+`},
+				{Run: `echo "this is step 5" >> ${{ outputs.myOutput }}`},
+			},
+			CachedResultFound: true,
+			CachedResult: stepExecutionResult{
+				StepIndex: 2,
+				Diff:      cachedDiff,
+				Outputs: map[string]interface{}{
+					"myOutput": "my-output.txt",
+				},
+				PreviousStepResult: StepResult{
+					Files: &git.Changes{
+						Modified: []string{"README.md"},
+						Added:    []string{"README.txt"},
+					},
+					Stdout: nil,
+					Stderr: nil,
+				},
+			},
+		}
+
+		results, err := testExecuteTasks(t, []*Task{task}, archive)
+		if err != nil {
+			t.Fatalf("execution failed: %s", err)
+		}
+
+		if have, want := len(results), 1; have != want {
+			t.Fatalf("wrong number of execution results. want=%d, have=%d", want, have)
+		}
+
+		executionResult := results[0].result
+		if diff := cmp.Diff(executionResult.Diff, wantFinalDiff); diff != "" {
+			t.Fatalf("wrong diff: %s", diff)
+		}
+
+		if diff := cmp.Diff(executionResult.Outputs, task.CachedResult.Outputs); diff != "" {
+			t.Fatalf("wrong execution result outputs: %s", diff)
+		}
+
+		// Only two steps should've been executed
+		if have, want := len(results[0].stepResults), 2; have != want {
+			t.Fatalf("wrong length of step results. have=%d, want=%d", have, want)
+		}
+
+		lastStepResult := results[0].stepResults[1]
+		if have, want := lastStepResult.StepIndex, 4; have != want {
+			t.Fatalf("wrong stepIndex. have=%d, want=%d", have, want)
+		}
+
+		if diff := cmp.Diff(lastStepResult.Outputs, task.CachedResult.Outputs); diff != "" {
+			t.Fatalf("wrong step result outputs: %s", diff)
+		}
+	})
+}
+
+func testExecuteTasks(t *testing.T, tasks []*Task, archives ...mock.RepoArchive) ([]taskResult, error) {
+	if runtime.GOOS == "windows" {
+		t.Skip("Test doesn't work on Windows because dummydocker is written in bash")
+	}
+
+	testTempDir, err := ioutil.TempDir("", "executor-integration-test-*")
+	if err != nil {
+		t.Fatal(err)
+	}
+	t.Cleanup(func() { os.Remove(testTempDir) })
+
+	// Setup dummydocker
+	addToPath(t, "testdata/dummydocker")
+
+	// Setup mock test server & client
+	mux := mock.NewZipArchivesMux(t, nil, archives...)
+	ts := httptest.NewServer(mux)
+	t.Cleanup(ts.Close)
+
+	var clientBuffer bytes.Buffer
+	client := api.NewClient(api.ClientOpts{Endpoint: ts.URL, Out: &clientBuffer})
+
+	// Prepare tasks
+	for _, t := range tasks {
+		for i := range t.Steps {
+			t.Steps[i].SetImage(&mock.Image{
+				RawDigest: t.Steps[i].Container,
+			})
+		}
+	}
+
+	// Setup executor
+	executor := newExecutor(newExecutorOpts{
+		Creator: workspace.NewCreator(context.Background(), "bind", testTempDir, testTempDir, []batches.Step{}),
+		Fetcher: batches.NewRepoFetcher(client, testTempDir, false),
+		Logger:  mock.LogNoOpManager{},
+
+		TempDir:     testTempDir,
+		Parallelism: runtime.GOMAXPROCS(0),
+		Timeout:     30 * time.Second,
+	})
+
+	executor.Start(context.Background(), tasks, NewTaskStatusCollection(tasks))
+	return executor.Wait(context.Background())
+}