From 429225da80b90c53da65e4eb1b5723ae58e3ba4c Mon Sep 17 00:00:00 2001 From: Jesse Duffield Date: Sat, 29 Jul 2023 12:32:59 +1000 Subject: [PATCH] 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 --- .../git_commands/commit_loader_test.go | 2 +- pkg/commands/git_commands/deps_test.go | 4 +- pkg/commands/git_commands/patch_test.go | 68 ---------- pkg/commands/git_commands/rebase_test.go | 8 +- .../oscommands/fake_cmd_obj_runner.go | 124 +++++++++++++----- 5 files changed, 97 insertions(+), 109 deletions(-) delete mode 100644 pkg/commands/git_commands/patch_test.go diff --git a/pkg/commands/git_commands/commit_loader_test.go b/pkg/commands/git_commands/commit_loader_test.go index 8398613a8..bcbc33c02 100644 --- a/pkg/commands/git_commands/commit_loader_test.go +++ b/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" diff --git a/pkg/commands/git_commands/deps_test.go b/pkg/commands/git_commands/deps_test.go index 5926584b3..a960098df 100644 --- a/pkg/commands/git_commands/deps_test.go +++ b/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) diff --git a/pkg/commands/git_commands/patch_test.go b/pkg/commands/git_commands/patch_test.go deleted file mode 100644 index 8acf471d4..000000000 --- a/pkg/commands/git_commands/patch_test.go +++ /dev/null @@ -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() - }) - } -} diff --git a/pkg/commands/git_commands/rebase_test.go b/pkg/commands/git_commands/rebase_test.go index b616ec609..d10746220 100644 --- a/pkg/commands/git_commands/rebase_test.go +++ b/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) diff --git a/pkg/commands/oscommands/fake_cmd_obj_runner.go b/pkg/commands/oscommands/fake_cmd_obj_runner.go index fa34f01be..dc75228a0 100644 --- a/pkg/commands/oscommands/fake_cmd_obj_runner.go +++ b/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\\.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", + ), + ) } }