Support random order of command execution in unit tests

Now that we run code concurrently in our loaders, we need to handle that in our tests.
We could enforce a deterministic ordering by mocking waitgroup or something like that,
but I think it's fine to let our tests handle some randomness given that prod itself
will have that randomness.

I've removed the patch test file because it was clunky, not providing much value, and
it would have been hard to refactor to the new pattern

pkg/commands/git_commands/commit_loader_test.go

@@ -1,12 +1,12 @@
 package git_commands
 
 import (
-	"errors"
 	"path/filepath"
 	"strings"
 	"testing"
 
 	"github.com/fsmiamoto/git-todo-parser/todo"
+	"github.com/go-errors/errors"
 	"github.com/jesseduffield/lazygit/pkg/commands/models"
 	"github.com/jesseduffield/lazygit/pkg/commands/oscommands"
 	"github.com/jesseduffield/lazygit/pkg/commands/types/enums"

pkg/commands/git_commands/deps_test.go

@@ -118,7 +118,7 @@ func buildWorkingTreeCommands(deps commonDeps) *WorkingTreeCommands {
 	return NewWorkingTreeCommands(gitCommon, submoduleCommands, fileLoader)
 }
 
-func buildPatchCommands(deps commonDeps) *PatchCommands {
+func buildPatchCommands(deps commonDeps) *PatchCommands { //nolint:golint,unused
 	gitCommon := buildGitCommon(deps)
 	rebaseCommands := buildRebaseCommands(deps)
 	commitCommands := buildCommitCommands(deps)
@@ -132,7 +132,7 @@ func buildPatchCommands(deps commonDeps) *PatchCommands {
 	return NewPatchCommands(gitCommon, rebaseCommands, commitCommands, statusCommands, stashCommands, patchBuilder)
 }
 
-func buildStatusCommands(deps commonDeps) *StatusCommands {
+func buildStatusCommands(deps commonDeps) *StatusCommands { //nolint:golint,unused
 	gitCommon := buildGitCommon(deps)
 
 	return NewStatusCommands(gitCommon)

pkg/commands/git_commands/patch_test.go

@@ -1,68 +0,0 @@
-package git_commands
-
-import (
-	"fmt"
-	"os"
-	"testing"
-
-	"github.com/go-errors/errors"
-	"github.com/jesseduffield/lazygit/pkg/commands/oscommands"
-	"github.com/stretchr/testify/assert"
-)
-
-func TestPatchApplyPatch(t *testing.T) {
-	type scenario struct {
-		testName string
-		opts     ApplyPatchOpts
-		runner   *oscommands.FakeCmdObjRunner
-		test     func(error)
-	}
-
-	// expectedArgs excludes the last argument which is an indeterminate filename
-	expectFn := func(expectedArgs []string, errToReturn error) func(cmdObj oscommands.ICmdObj) (string, error) {
-		return func(cmdObj oscommands.ICmdObj) (string, error) {
-			args := cmdObj.Args()
-
-			assert.Equal(t, len(args), len(expectedArgs)+1, fmt.Sprintf("unexpected command: %s", cmdObj.ToString()))
-
-			filename := args[len(args)-1]
-
-			content, err := os.ReadFile(filename)
-			assert.NoError(t, err)
-
-			assert.Equal(t, "test", string(content))
-
-			return "", errToReturn
-		}
-	}
-
-	scenarios := []scenario{
-		{
-			testName: "valid case",
-			opts:     ApplyPatchOpts{Cached: true},
-			runner: oscommands.NewFakeRunner(t).
-				ExpectFunc(expectFn([]string{"git", "apply", "--cached"}, nil)),
-			test: func(err error) {
-				assert.NoError(t, err)
-			},
-		},
-		{
-			testName: "command returns error",
-			opts:     ApplyPatchOpts{Cached: true},
-			runner: oscommands.NewFakeRunner(t).
-				ExpectFunc(expectFn([]string{"git", "apply", "--cached"}, errors.New("error"))),
-			test: func(err error) {
-				assert.Error(t, err)
-			},
-		},
-	}
-
-	for _, s := range scenarios {
-		s := s
-		t.Run(s.testName, func(t *testing.T) {
-			instance := buildPatchCommands(commonDeps{runner: s.runner})
-			s.test(instance.ApplyPatch("test", s.opts))
-			s.runner.CheckForMissingCalls()
-		})
-	}
-}

pkg/commands/git_commands/rebase_test.go

@@ -79,7 +79,7 @@ func TestRebaseRebaseBranch(t *testing.T) {
 // environment variables that suppress an interactive editor
 func TestRebaseSkipEditorCommand(t *testing.T) {
 	cmdArgs := []string{"git", "blah"}
-	runner := oscommands.NewFakeRunner(t).ExpectFunc(func(cmdObj oscommands.ICmdObj) (string, error) {
+	runner := oscommands.NewFakeRunner(t).ExpectFunc("matches editor env var", func(cmdObj oscommands.ICmdObj) bool {
 		assert.EqualValues(t, cmdArgs, cmdObj.Args())
 		envVars := cmdObj.GetEnvVars()
 		for _, regexStr := range []string{
@@ -94,11 +94,11 @@ func TestRebaseSkipEditorCommand(t *testing.T) {
 				return regexp.MustCompile(regexStr).MatchString(envVar)
 			})
 			if !foundMatch {
-				t.Errorf("expected environment variable %s to be set", regexStr)
+				return false
 			}
 		}
-		return "", nil
-	})
+		return true
+	}, "", nil)
 	instance := buildRebaseCommands(commonDeps{runner: runner})
 	err := instance.runSkipEditorCommand(instance.cmd.New(cmdArgs))
 	assert.NoError(t, err)

pkg/commands/oscommands/fake_cmd_obj_runner.go

@@ -6,18 +6,36 @@ import (
 	"regexp"
 	"runtime"
 	"strings"
+	"sync"
 	"testing"
 
 	"github.com/go-errors/errors"
-	"github.com/stretchr/testify/assert"
+	"github.com/samber/lo"
+	"golang.org/x/exp/slices"
 )
 
 // for use in testing
 
 type FakeCmdObjRunner struct {
-	t                *testing.T
-	expectedCmds     []func(ICmdObj) (string, error)
-	expectedCmdIndex int
+	t *testing.T
+	// commands can be run in any order; mimicking the concurrent behaviour of
+	// production code.
+	expectedCmds []CmdObjMatcher
+
+	invokedCmdIndexes []int
+
+	mutex sync.Mutex
+}
+
+type CmdObjMatcher struct {
+	description string
+	// returns true if the matcher matches the command object
+	test func(ICmdObj) bool
+
+	// output of the command
+	output string
+	// error of the command
+	err error
 }
 
 var _ ICmdObjRunner = &FakeCmdObjRunner{}
@@ -26,23 +44,40 @@ func NewFakeRunner(t *testing.T) *FakeCmdObjRunner { //nolint:thelper
 	return &FakeCmdObjRunner{t: t}
 }
 
+func (self *FakeCmdObjRunner) remainingExpectedCmds() []CmdObjMatcher {
+	return lo.Filter(self.expectedCmds, func(_ CmdObjMatcher, i int) bool {
+		return !lo.Contains(self.invokedCmdIndexes, i)
+	})
+}
+
 func (self *FakeCmdObjRunner) Run(cmdObj ICmdObj) error {
 	_, err := self.RunWithOutput(cmdObj)
 	return err
 }
 
 func (self *FakeCmdObjRunner) RunWithOutput(cmdObj ICmdObj) (string, error) {
-	if self.expectedCmdIndex > len(self.expectedCmds)-1 {
+	self.mutex.Lock()
+	defer self.mutex.Unlock()
+
+	if len(self.remainingExpectedCmds()) == 0 {
 		self.t.Errorf("ran too many commands. Unexpected command: `%s`", cmdObj.ToString())
 		return "", errors.New("ran too many commands")
 	}
 
-	expectedCmd := self.expectedCmds[self.expectedCmdIndex]
-	output, err := expectedCmd(cmdObj)
+	for i := range self.expectedCmds {
+		if lo.Contains(self.invokedCmdIndexes, i) {
+			continue
+		}
+		expectedCmd := self.expectedCmds[i]
+		matched := expectedCmd.test(cmdObj)
+		if matched {
+			self.invokedCmdIndexes = append(self.invokedCmdIndexes, i)
+			return expectedCmd.output, expectedCmd.err
+		}
+	}
 
-	self.expectedCmdIndex++
-
-	return output, err
+	self.t.Errorf("Unexpected command: `%s`", cmdObj.ToString())
+	return "", nil
 }
 
 func (self *FakeCmdObjRunner) RunWithOutputs(cmdObj ICmdObj) (string, string, error) {
@@ -72,63 +107,84 @@ func (self *FakeCmdObjRunner) RunAndProcessLines(cmdObj ICmdObj, onLine func(lin
 	return nil
 }
 
-func (self *FakeCmdObjRunner) ExpectFunc(fn func(cmdObj ICmdObj) (string, error)) *FakeCmdObjRunner {
-	self.expectedCmds = append(self.expectedCmds, fn)
+func (self *FakeCmdObjRunner) ExpectFunc(description string, fn func(cmdObj ICmdObj) bool, output string, err error) *FakeCmdObjRunner {
+	self.mutex.Lock()
+	defer self.mutex.Unlock()
 
-	return self
-}
-
-func (self *FakeCmdObjRunner) Expect(expectedCmdStr string, output string, err error) *FakeCmdObjRunner {
-	self.ExpectFunc(func(cmdObj ICmdObj) (string, error) {
-		cmdStr := cmdObj.ToString()
-		assert.Equal(self.t, expectedCmdStr, cmdStr, fmt.Sprintf("expected command %d to be %s, but was %s", self.expectedCmdIndex+1, expectedCmdStr, cmdStr))
-
-		return output, err
+	self.expectedCmds = append(self.expectedCmds, CmdObjMatcher{
+		test:        fn,
+		output:      output,
+		err:         err,
+		description: description,
 	})
 
 	return self
 }
 
 func (self *FakeCmdObjRunner) ExpectArgs(expectedArgs []string, output string, err error) *FakeCmdObjRunner {
-	self.ExpectFunc(func(cmdObj ICmdObj) (string, error) {
+	description := fmt.Sprintf("matches args %s", strings.Join(expectedArgs, " "))
+	self.ExpectFunc(description, func(cmdObj ICmdObj) bool {
 		args := cmdObj.GetCmd().Args
 
 		if runtime.GOOS == "windows" {
 			// thanks to the secureexec package, the first arg is something like
 			// '"C:\\Program Files\\Git\\mingw64\\bin\\<command>.exe"
 			// on windows so we'll just ensure it contains our program
-			assert.Contains(self.t, args[0], expectedArgs[0])
+			if !strings.Contains(args[0], expectedArgs[0]) {
+				return false
+			}
 		} else {
 			// first arg is the program name
-			assert.Equal(self.t, expectedArgs[0], args[0])
+			if expectedArgs[0] != args[0] {
+				return false
+			}
 		}
 
-		assert.EqualValues(self.t, expectedArgs[1:], args[1:], fmt.Sprintf("command %d did not match expectation", self.expectedCmdIndex+1))
+		if !slices.Equal(expectedArgs[1:], args[1:]) {
+			return false
+		}
 
-		return output, err
-	})
+		return true
+	}, output, err)
 
 	return self
 }
 
 func (self *FakeCmdObjRunner) ExpectGitArgs(expectedArgs []string, output string, err error) *FakeCmdObjRunner {
-	self.ExpectFunc(func(cmdObj ICmdObj) (string, error) {
+	description := fmt.Sprintf("matches git args %s", strings.Join(expectedArgs, " "))
+	self.ExpectFunc(description, func(cmdObj ICmdObj) bool {
 		// first arg is 'git' on unix and something like '"C:\\Program Files\\Git\\mingw64\\bin\\git.exe" on windows so we'll just ensure it ends in either 'git' or 'git.exe'
 		re := regexp.MustCompile(`git(\.exe)?$`)
 		args := cmdObj.GetCmd().Args
 		if !re.MatchString(args[0]) {
-			self.t.Errorf("expected first arg to end in .git or .git.exe but was %s", args[0])
+			return false
 		}
-		assert.EqualValues(self.t, expectedArgs, args[1:], fmt.Sprintf("command %d did not match expectation", self.expectedCmdIndex+1))
 
-		return output, err
-	})
+		if !slices.Equal(expectedArgs, args[1:]) {
+			return false
+		}
+
+		return true
+	}, output, err)
 
 	return self
 }
 
 func (self *FakeCmdObjRunner) CheckForMissingCalls() {
-	if self.expectedCmdIndex < len(self.expectedCmds) {
-		self.t.Errorf("expected command %d to be called, but was not", self.expectedCmdIndex+1)
+	self.mutex.Lock()
+	defer self.mutex.Unlock()
+
+	remaining := self.remainingExpectedCmds()
+	if len(remaining) > 0 {
+		self.t.Errorf(
+			"expected %d more command(s) to be run. Remaining commands:\n%s",
+			len(remaining),
+			strings.Join(
+				lo.Map(remaining, func(cmdObj CmdObjMatcher, _ int) string {
+					return cmdObj.description
+				}),
+				"\n",
+			),
+		)
 	}
 }