Skip to content

Commit

Permalink
Track busy/idle state for integration tests (#2765)
Browse files Browse the repository at this point in the history
  • Loading branch information
jesseduffield committed Jul 10, 2023
2 parents 585ea36 + 16ed3c2 commit 2dddd90
Show file tree
Hide file tree
Showing 91 changed files with 1,081 additions and 402 deletions.
3 changes: 1 addition & 2 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -68,9 +68,8 @@ jobs:
restore-keys: |
${{runner.os}}-go-
- name: Test code
# LONG_WAIT_BEFORE_FAIL means that for a given test assertion, we'll wait longer before failing
run: |
LONG_WAIT_BEFORE_FAIL=true go test pkg/integration/clients/*.go
go test pkg/integration/clients/*.go
build:
runs-on: ubuntu-latest
env:
Expand Down
1 change: 0 additions & 1 deletion docs/Integration_Tests.md

This file was deleted.

1 change: 1 addition & 0 deletions docs/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,3 +6,4 @@
* [Keybindings](./keybindings)
* [Undo/Redo](./Undoing.md)
* [Searching/Filtering](./Searching.md)
* [Dev docs](./dev)
78 changes: 78 additions & 0 deletions docs/dev/Busy.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,78 @@
# Knowing when Lazygit is busy/idle

## The use-case

This topic deserves its own doc because there there are a few touch points for it. We have a use-case for knowing when Lazygit is idle or busy because integration tests follow the following process:
1) press a key
2) wait until Lazygit is idle
3) run assertion / press another key
4) repeat

In the past the process was:
1) press a key
2) run assertion
3) if assertion fails, wait a bit and retry
4) repeat

The old process was problematic because an assertion may give a false positive due to the contents of some view not yet having changed since the last key was pressed.

## The solution

First, it's important to distinguish three different types of goroutines:
* The UI goroutine, of which there is only one, which infinitely processes a queue of events
* Worker goroutines, which do some work and then typically enqueue an event in the UI goroutine to display the results
* Background goroutines, which periodically spawn worker goroutines (e.g. doing a git fetch every minute)

The point of distinguishing worker goroutines from background goroutines is that when any worker goroutine is running, we consider Lazygit to be 'busy', whereas this is not the case with background goroutines. It would be pointless to have background goroutines be considered 'busy' because then Lazygit would be considered busy for the entire duration of the program!

In gocui, the underlying package we use for managing the UI and events, we keep track of how many busy goroutines there are using the `Task` type. A task represents some work being done by lazygit. The gocui Gui struct holds a map of tasks and allows creating a new task (which adds it to the map), pausing/continuing a task, and marking a task as done (which removes it from the map). Lazygit is considered to be busy so long as there is at least one busy task in the map; otherwise it's considered idle. When Lazygit goes from busy to idle, it notifies the integration test.

It's important that we play by the rules below to ensure that after the user does anything, all the processing that follows happens in a contiguous block of busy-ness with no gaps.

### Spawning a worker goroutine

Here's the basic implementation of `OnWorker` (using the same flow as `WaitGroup`s):

```go
func (g *Gui) OnWorker(f func(*Task)) {
task := g.NewTask()
go func() {
f(task)
task.Done()
}()
}
```

The crucial thing here is that we create the task _before_ spawning the goroutine, because it means that we'll have at least one busy task in the map until the completion of the goroutine. If we created the task within the goroutine, the current function could exit and Lazygit would be considered idle before the goroutine starts, leading to our integration test prematurely progressing.

You typically invoke this with `self.c.OnWorker(f)`. Note that the callback function receives the task. This allows the callback to pause/continue the task (see below).

### Spawning a background goroutine

Spawning a background goroutine is as simple as:

```go
go utils.Safe(f)
```

Where `utils.Safe` is a helper function that ensures we clean up the gui if the goroutine panics.

### Programmatically enqueing a UI event

This is invoked with `self.c.OnUIThread(f)`. Internally, it creates a task before enqueuing the function as an event (including the task in the event struct) and once that event is processed by the event queue (and any other pending events are processed) the task is removed from the map by calling `task.Done()`.

### Pressing a key

If the user presses a key, an event will be enqueued automatically and a task will be created before (and `Done`'d after) the event is processed.

## Special cases

There are a couple of special cases where we manually pause/continue the task directly in the client code. These are subject to change but for the sake of completeness:

### Writing to the main view(s)

If the user focuses a file in the files panel, we run a `git diff` command for that file and write the output to the main view. But we only read enough of the command's output to fill the view's viewport: further loading only happens if the user scrolls. Given that we have a background goroutine for running the command and writing more output upon scrolling, we create our own task and call `Done` on it as soon as the viewport is filled.

### Requesting credentials from a git command

Some git commands (e.g. git push) may request credentials. This is the same deal as above; we use a worker goroutine and manually pause continue its task as we go from waiting on the git command to waiting on user input. This requires passing the task through to the `Push` method so that it can be paused/continued.
1 change: 1 addition & 0 deletions docs/dev/Integration_Tests.md
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
see new docs [here](../../pkg/integration/README.md)
4 changes: 4 additions & 0 deletions docs/dev/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
# Dev Documentation Overview

* [Busy/Idle tracking](./Busy.md).
* [Integration Tests](../../pkg/integration/README.md)
8 changes: 4 additions & 4 deletions go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ require (
github.com/integrii/flaggy v1.4.0
github.com/jesseduffield/generics v0.0.0-20220320043834-727e535cbe68
github.com/jesseduffield/go-git/v5 v5.1.2-0.20221018185014-fdd53fef665d
github.com/jesseduffield/gocui v0.3.1-0.20230702054502-d6c452fc12ce
github.com/jesseduffield/gocui v0.3.1-0.20230710004407-9bbfd873713b
github.com/jesseduffield/kill v0.0.0-20220618033138-bfbe04675d10
github.com/jesseduffield/lazycore v0.0.0-20221012050358-03d2e40243c5
github.com/jesseduffield/minimal/gitignore v0.3.3-0.20211018110810-9cde264e6b1e
Expand Down Expand Up @@ -67,8 +67,8 @@ require (
golang.org/x/crypto v0.0.0-20210921155107-089bfa567519 // indirect
golang.org/x/exp v0.0.0-20220318154914-8dddf5d87bd8 // indirect
golang.org/x/net v0.7.0 // indirect
golang.org/x/sys v0.9.0 // indirect
golang.org/x/term v0.9.0 // indirect
golang.org/x/text v0.10.0 // indirect
golang.org/x/sys v0.10.0 // indirect
golang.org/x/term v0.10.0 // indirect
golang.org/x/text v0.11.0 // indirect
gopkg.in/warnings.v0 v0.1.2 // indirect
)
16 changes: 8 additions & 8 deletions go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -72,8 +72,8 @@ github.com/jesseduffield/generics v0.0.0-20220320043834-727e535cbe68 h1:EQP2Tv8T
github.com/jesseduffield/generics v0.0.0-20220320043834-727e535cbe68/go.mod h1:+LLj9/WUPAP8LqCchs7P+7X0R98HiFujVFANdNaxhGk=
github.com/jesseduffield/go-git/v5 v5.1.2-0.20221018185014-fdd53fef665d h1:bO+OmbreIv91rCe8NmscRwhFSqkDJtzWCPV4Y+SQuXE=
github.com/jesseduffield/go-git/v5 v5.1.2-0.20221018185014-fdd53fef665d/go.mod h1:nGNEErzf+NRznT+N2SWqmHnDnF9aLgANB1CUNEan09o=
github.com/jesseduffield/gocui v0.3.1-0.20230702054502-d6c452fc12ce h1:Xgm21B1an/outcRxnkDfMT6wKb6SKBR05jXOyfPA8WQ=
github.com/jesseduffield/gocui v0.3.1-0.20230702054502-d6c452fc12ce/go.mod h1:dJ/BEUt3OWtaRg/PmuJWendRqREhre9JQ1SLvqrVJ8s=
github.com/jesseduffield/gocui v0.3.1-0.20230710004407-9bbfd873713b h1:8FmmdaYHes1m3oNyNdS+VIgkgkFpNZAWuwTnvp0tG14=
github.com/jesseduffield/gocui v0.3.1-0.20230710004407-9bbfd873713b/go.mod h1:dJ/BEUt3OWtaRg/PmuJWendRqREhre9JQ1SLvqrVJ8s=
github.com/jesseduffield/kill v0.0.0-20220618033138-bfbe04675d10 h1:jmpr7KpX2+2GRiE91zTgfq49QvgiqB0nbmlwZ8UnOx0=
github.com/jesseduffield/kill v0.0.0-20220618033138-bfbe04675d10/go.mod h1:aA97kHeNA+sj2Hbki0pvLslmE4CbDyhBeSSTUUnOuVo=
github.com/jesseduffield/lazycore v0.0.0-20221012050358-03d2e40243c5 h1:CDuQmfOjAtb1Gms6a1p5L2P8RhbLUq5t8aL7PiQd2uY=
Expand Down Expand Up @@ -207,21 +207,21 @@ golang.org/x/sys v0.0.0-20220520151302-bc2c85ada10a/go.mod h1:oPkhp1MJrh7nUepCBc
golang.org/x/sys v0.0.0-20220722155257-8c9f86f7a55f/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
golang.org/x/sys v0.5.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
golang.org/x/sys v0.8.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
golang.org/x/sys v0.9.0 h1:KS/R3tvhPqvJvwcKfnBHJwwthS11LRhmM5D59eEXa0s=
golang.org/x/sys v0.9.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
golang.org/x/sys v0.10.0 h1:SqMFp9UcQJZa+pmYuAKjd9xq1f0j5rLcDIk0mj4qAsA=
golang.org/x/sys v0.10.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
golang.org/x/term v0.0.0-20201126162022-7de9c90e9dd1/go.mod h1:bj7SfCRtBDWHUb9snDiAeCFNEtKQo2Wmx5Cou7ajbmo=
golang.org/x/term v0.0.0-20201210144234-2321bbc49cbf/go.mod h1:bj7SfCRtBDWHUb9snDiAeCFNEtKQo2Wmx5Cou7ajbmo=
golang.org/x/term v0.0.0-20210927222741-03fcf44c2211/go.mod h1:jbD1KX2456YbFQfuXm/mYQcufACuNUgVhRMnK/tPxf8=
golang.org/x/term v0.5.0/go.mod h1:jMB1sMXY+tzblOD4FWmEbocvup2/aLOaQEp7JmGp78k=
golang.org/x/term v0.9.0 h1:GRRCnKYhdQrD8kfRAdQ6Zcw1P0OcELxGLKJvtjVMZ28=
golang.org/x/term v0.9.0/go.mod h1:M6DEAAIenWoTxdKrOltXcmDY3rSplQUkrvaDU5FcQyo=
golang.org/x/term v0.10.0 h1:3R7pNqamzBraeqj/Tj8qt1aQ2HpmlC+Cx/qL/7hn4/c=
golang.org/x/term v0.10.0/go.mod h1:lpqdcUyK/oCiQxvxVrppt5ggO2KCZ5QblwqPnfZ6d5o=
golang.org/x/text v0.3.0/go.mod h1:NqM8EUOU14njkJ3fqMW+pc6Ldnwhi/IjpwHt7yyuwOQ=
golang.org/x/text v0.3.2/go.mod h1:bEr9sfX3Q8Zfm5fL9x+3itogRgK3+ptLWKqgva+5dAk=
golang.org/x/text v0.3.3/go.mod h1:5Zoc/QRtKVWzQhOtBMvqHzDpF6irO9z98xDceosuGiQ=
golang.org/x/text v0.3.7/go.mod h1:u+2+/6zg+i71rQMx5EYifcz6MCKuco9NR6JIITiCfzQ=
golang.org/x/text v0.7.0/go.mod h1:mrYo+phRRbMaCq/xk9113O4dZlRixOauAjOtrjsXDZ8=
golang.org/x/text v0.10.0 h1:UpjohKhiEgNc0CSauXmwYftY1+LlaC75SJwh0SgCX58=
golang.org/x/text v0.10.0/go.mod h1:TvPlkZtksWOMsz7fbANvkp4WM8x/WCo/om8BMLbz+aE=
golang.org/x/text v0.11.0 h1:LAntKIrcmeSKERyiOh0XMV39LXS8IE9UL2yP7+f5ij4=
golang.org/x/text v0.11.0/go.mod h1:TvPlkZtksWOMsz7fbANvkp4WM8x/WCo/om8BMLbz+aE=
golang.org/x/tools v0.0.0-20180917221912-90fa682c2a6e/go.mod h1:n7NCudcB/nEzxVGmLbDWY5pfWTLqBcC2KZ6jyYvM4mQ=
golang.org/x/tools v0.0.0-20191119224855-298f0cb1881e/go.mod h1:b+2E5dAYhXwXZwtnZ6UAqBI28+e2cm9otk0dWdXHAEo=
golang.org/x/tools v0.1.12/go.mod h1:hNGJHUnrk76NpqgfD5Aqm5Crs+Hm0VOH/i9J2+nxYbc=
Expand Down
43 changes: 41 additions & 2 deletions pkg/commands/git_cmd_obj_runner.go
Original file line number Diff line number Diff line change
@@ -1,12 +1,20 @@
package commands

import (
"strings"
"time"

"github.com/jesseduffield/lazygit/pkg/commands/oscommands"
"github.com/sirupsen/logrus"
)

// here we're wrapping the default command runner in some git-specific stuff e.g. retry logic if we get an error due to the presence of .git/index.lock

const (
WaitTime = 50 * time.Millisecond
RetryCount = 5
)

type gitCmdObjRunner struct {
log *logrus.Entry
innerRunner oscommands.ICmdObjRunner
Expand All @@ -18,13 +26,44 @@ func (self *gitCmdObjRunner) Run(cmdObj oscommands.ICmdObj) error {
}

func (self *gitCmdObjRunner) RunWithOutput(cmdObj oscommands.ICmdObj) (string, error) {
return self.innerRunner.RunWithOutput(cmdObj)
var output string
var err error
for i := 0; i < RetryCount; i++ {
newCmdObj := cmdObj.Clone()
output, err = self.innerRunner.RunWithOutput(newCmdObj)

if err == nil || !strings.Contains(output, ".git/index.lock") {
return output, err
}

// if we have an error based on the index lock, we should wait a bit and then retry
self.log.Warn("index.lock prevented command from running. Retrying command after a small wait")
time.Sleep(WaitTime)
}

return output, err
}

func (self *gitCmdObjRunner) RunWithOutputs(cmdObj oscommands.ICmdObj) (string, string, error) {
return self.innerRunner.RunWithOutputs(cmdObj)
var stdout, stderr string
var err error
for i := 0; i < RetryCount; i++ {
newCmdObj := cmdObj.Clone()
stdout, stderr, err = self.innerRunner.RunWithOutputs(newCmdObj)

if err == nil || !strings.Contains(stdout+stderr, ".git/index.lock") {
return stdout, stderr, err
}

// if we have an error based on the index lock, we should wait a bit and then retry
self.log.Warn("index.lock prevented command from running. Retrying command after a small wait")
time.Sleep(WaitTime)
}

return stdout, stderr, err
}

// Retry logic not implemented here, but these commands typically don't need to obtain a lock.
func (self *gitCmdObjRunner) RunAndProcessLines(cmdObj oscommands.ICmdObj, onLine func(line string) (bool, error)) error {
return self.innerRunner.RunAndProcessLines(cmdObj, onLine)
}
6 changes: 4 additions & 2 deletions pkg/commands/git_commands/remote.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@ package git_commands

import (
"fmt"

"github.com/jesseduffield/gocui"
)

type RemoteCommands struct {
Expand Down Expand Up @@ -46,12 +48,12 @@ func (self *RemoteCommands) UpdateRemoteUrl(remoteName string, updatedUrl string
return self.cmd.New(cmdArgs).Run()
}

func (self *RemoteCommands) DeleteRemoteBranch(remoteName string, branchName string) error {
func (self *RemoteCommands) DeleteRemoteBranch(task gocui.Task, remoteName string, branchName string) error {
cmdArgs := NewGitCmd("push").
Arg(remoteName, "--delete", branchName).
ToArgv()

return self.cmd.New(cmdArgs).PromptOnCredentialRequest().WithMutex(self.syncMutex).Run()
return self.cmd.New(cmdArgs).PromptOnCredentialRequest(task).WithMutex(self.syncMutex).Run()
}

// CheckRemoteBranchExists Returns remote branch
Expand Down
52 changes: 29 additions & 23 deletions pkg/commands/git_commands/sync.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package git_commands

import (
"github.com/go-errors/errors"
"github.com/jesseduffield/gocui"
"github.com/jesseduffield/lazygit/pkg/commands/oscommands"
)

Expand All @@ -23,7 +24,7 @@ type PushOpts struct {
SetUpstream bool
}

func (self *SyncCommands) PushCmdObj(opts PushOpts) (oscommands.ICmdObj, error) {
func (self *SyncCommands) PushCmdObj(task gocui.Task, opts PushOpts) (oscommands.ICmdObj, error) {
if opts.UpstreamBranch != "" && opts.UpstreamRemote == "" {
return nil, errors.New(self.Tr.MustSpecifyOriginError)
}
Expand All @@ -35,41 +36,46 @@ func (self *SyncCommands) PushCmdObj(opts PushOpts) (oscommands.ICmdObj, error)
ArgIf(opts.UpstreamBranch != "", opts.UpstreamBranch).
ToArgv()

cmdObj := self.cmd.New(cmdArgs).PromptOnCredentialRequest().WithMutex(self.syncMutex)
cmdObj := self.cmd.New(cmdArgs).PromptOnCredentialRequest(task).WithMutex(self.syncMutex)
return cmdObj, nil
}

func (self *SyncCommands) Push(opts PushOpts) error {
cmdObj, err := self.PushCmdObj(opts)
func (self *SyncCommands) Push(task gocui.Task, opts PushOpts) error {
cmdObj, err := self.PushCmdObj(task, opts)
if err != nil {
return err
}

return cmdObj.Run()
}

type FetchOptions struct {
Background bool
func (self *SyncCommands) FetchCmdObj(task gocui.Task) oscommands.ICmdObj {
cmdArgs := NewGitCmd("fetch").
ArgIf(self.UserConfig.Git.FetchAll, "--all").
ToArgv()

cmdObj := self.cmd.New(cmdArgs)
cmdObj.PromptOnCredentialRequest(task)
return cmdObj
}

// Fetch fetch git repo
func (self *SyncCommands) FetchCmdObj(opts FetchOptions) oscommands.ICmdObj {
func (self *SyncCommands) Fetch(task gocui.Task) error {
return self.FetchCmdObj(task).Run()
}

func (self *SyncCommands) FetchBackgroundCmdObj() oscommands.ICmdObj {
cmdArgs := NewGitCmd("fetch").
ArgIf(self.UserConfig.Git.FetchAll, "--all").
ToArgv()

cmdObj := self.cmd.New(cmdArgs)
if opts.Background {
cmdObj.DontLog().FailOnCredentialRequest()
} else {
cmdObj.PromptOnCredentialRequest()
}
return cmdObj.WithMutex(self.syncMutex)
cmdObj.DontLog().FailOnCredentialRequest()
cmdObj.WithMutex(self.syncMutex)
return cmdObj
}

func (self *SyncCommands) Fetch(opts FetchOptions) error {
cmdObj := self.FetchCmdObj(opts)
return cmdObj.Run()
func (self *SyncCommands) FetchBackground() error {
return self.FetchBackgroundCmdObj().Run()
}

type PullOptions struct {
Expand All @@ -78,7 +84,7 @@ type PullOptions struct {
FastForwardOnly bool
}

func (self *SyncCommands) Pull(opts PullOptions) error {
func (self *SyncCommands) Pull(task gocui.Task, opts PullOptions) error {
cmdArgs := NewGitCmd("pull").
Arg("--no-edit").
ArgIf(opts.FastForwardOnly, "--ff-only").
Expand All @@ -88,22 +94,22 @@ func (self *SyncCommands) Pull(opts PullOptions) error {

// setting GIT_SEQUENCE_EDITOR to ':' as a way of skipping it, in case the user
// has 'pull.rebase = interactive' configured.
return self.cmd.New(cmdArgs).AddEnvVars("GIT_SEQUENCE_EDITOR=:").PromptOnCredentialRequest().WithMutex(self.syncMutex).Run()
return self.cmd.New(cmdArgs).AddEnvVars("GIT_SEQUENCE_EDITOR=:").PromptOnCredentialRequest(task).WithMutex(self.syncMutex).Run()
}

func (self *SyncCommands) FastForward(branchName string, remoteName string, remoteBranchName string) error {
func (self *SyncCommands) FastForward(task gocui.Task, branchName string, remoteName string, remoteBranchName string) error {
cmdArgs := NewGitCmd("fetch").
Arg(remoteName).
Arg(remoteBranchName + ":" + branchName).
ToArgv()

return self.cmd.New(cmdArgs).PromptOnCredentialRequest().WithMutex(self.syncMutex).Run()
return self.cmd.New(cmdArgs).PromptOnCredentialRequest(task).WithMutex(self.syncMutex).Run()
}

func (self *SyncCommands) FetchRemote(remoteName string) error {
func (self *SyncCommands) FetchRemote(task gocui.Task, remoteName string) error {
cmdArgs := NewGitCmd("fetch").
Arg(remoteName).
ToArgv()

return self.cmd.New(cmdArgs).PromptOnCredentialRequest().WithMutex(self.syncMutex).Run()
return self.cmd.New(cmdArgs).PromptOnCredentialRequest(task).WithMutex(self.syncMutex).Run()
}
Loading

0 comments on commit 2dddd90

Please sign in to comment.