diff --git a/pkg/gocui/gui.go b/pkg/gocui/gui.go index ceb570c59..9da5225c0 100644 --- a/pkg/gocui/gui.go +++ b/pkg/gocui/gui.go @@ -125,8 +125,11 @@ type clickInfo struct { // and keybindings. type Gui struct { RecordingConfig - // ReplayedEvents is for passing pre-recorded input events, for the purposes of testing - ReplayedEvents replayedEvents + // replayedEvents is for passing simulated input events, for the purposes + // of testing. Events must be submitted through the Replay* methods, which + // attach a task to each event; pushing into the channels directly would + // bypass the busy-tracking that integration tests rely on. + replayedEvents replayedEvents playRecording bool tabClickBindings []*tabClickBinding @@ -255,7 +258,7 @@ func NewGui(opts NewGuiOpts) (*Gui, error) { g.taskManager = newTaskManager() if opts.PlayRecording { - g.ReplayedEvents = replayedEvents{ + g.replayedEvents = replayedEvents{ Keys: make(chan *TcellKeyEventWrapper), Resizes: make(chan *TcellResizeEventWrapper), MouseEvents: make(chan *TcellMouseEventWrapper), @@ -291,6 +294,30 @@ func (g *Gui) NewBackgroundTask() *TaskImpl { return g.taskManager.NewTask(true) } +// ReplayKeyEvent simulates a key press, as if the user had typed it. It's used +// by integration tests. The event carries a task, so that the program counts +// as busy from before the event is submitted until the main loop has fully +// processed it; the test driver relies on this when it waits for the program +// to go idle after submitting an event. (If the task were only created once +// the main loop picks the event up, there would be a window in which the event +// is still in flight but nothing counts as busy.) +func (g *Gui) ReplayKeyEvent(ev *TcellKeyEventWrapper) { + ev.task = g.NewTask() + g.replayedEvents.Keys <- ev +} + +// ReplayMouseEvent is like ReplayKeyEvent, but for mouse events. +func (g *Gui) ReplayMouseEvent(ev *TcellMouseEventWrapper) { + ev.task = g.NewTask() + g.replayedEvents.MouseEvents <- ev +} + +// ReplayFocusEvent is like ReplayKeyEvent, but for focus events. +func (g *Gui) ReplayFocusEvent(ev *TcellFocusEventWrapper) { + ev.task = g.NewTask() + g.replayedEvents.FocusEvents <- ev +} + // Busy reports whether any foreground work is in flight, ignoring the event // currently being processed on the main goroutine (see currentTask). Background // routines (auto-fetch etc.) don't count. It's used to decide whether it's safe @@ -299,11 +326,11 @@ func (g *Gui) Busy() bool { return g.taskManager.hasBusyForegroundTaskExcept(g.currentTask) } -// An idle listener listens for when the program is idle. This is useful for -// integration tests which can wait for the program to be idle before taking -// the next step in the test. -func (g *Gui) AddIdleListener(c chan struct{}) { - g.taskManager.addIdleListener(c) +// WaitUntilIdle blocks until the program is idle (no busy tasks). This is +// useful for integration tests which want to wait for the program to finish +// processing before taking the next step in the test. +func (g *Gui) WaitUntilIdle() { + g.taskManager.WaitUntilIdle() } // Close finalizes the library. It should be called after a successful @@ -948,7 +975,12 @@ func (g *Gui) processEvent() error { // are always the primary event here. select { case ev := <-g.gEvents: - task := g.NewTask() + // Replayed test events already carry their task (see ReplayKeyEvent); + // organic events get theirs here. + task := ev.task + if task == nil { + task = g.NewTask() + } g.currentTask = task defer func() { g.currentTask = nil; task.Done() }() @@ -992,7 +1024,11 @@ func (g *Gui) processRemainingEvents() (bool, error) { select { case ev := <-g.gEvents: contentOnly = false - if err := g.handleError(g.handleEvent(&ev)); err != nil { + err := g.handleError(g.handleEvent(&ev)) + if ev.task != nil { + ev.task.Done() + } + if err != nil { return false, err } default: diff --git a/pkg/gocui/task_manager.go b/pkg/gocui/task_manager.go index 23ef0f77e..8d6daaa20 100644 --- a/pkg/gocui/task_manager.go +++ b/pkg/gocui/task_manager.go @@ -6,20 +6,23 @@ import "sync" // the main goroutine or a worker goroutine). Used by integration tests // to wait until the program is idle before progressing. type TaskManager struct { - // each of these listeners will be notified when the program goes from busy to idle - idleListeners []chan struct{} - tasks map[int]Task + tasks map[int]Task // auto-incrementing id for new tasks nextId int mutex sync.Mutex + // signalled whenever the program transitions from busy to idle; used by + // WaitUntilIdle + idleCond *sync.Cond } func newTaskManager() *TaskManager { - return &TaskManager{ - tasks: make(map[int]Task), - idleListeners: []chan struct{}{}, + self := &TaskManager{ + tasks: make(map[int]Task), } + self.idleCond = sync.NewCond(&self.mutex) + + return self } func (self *TaskManager) NewTask(background bool) *TaskImpl { @@ -58,8 +61,26 @@ func (self *TaskManager) hasBusyForegroundTaskExcept(ignore Task) bool { return false } -func (self *TaskManager) addIdleListener(c chan struct{}) { - self.idleListeners = append(self.idleListeners, c) +// WaitUntilIdle blocks until no task is busy. Integration tests use it to wait +// for the program to finish processing before taking the next step. +func (self *TaskManager) WaitUntilIdle() { + self.mutex.Lock() + defer self.mutex.Unlock() + + for self.hasBusyTask() { + self.idleCond.Wait() + } +} + +// caller must hold self.mutex +func (self *TaskManager) hasBusyTask() bool { + for _, task := range self.tasks { + if task.isBusy() { + return true + } + } + + return false } func (self *TaskManager) withMutex(f func()) { @@ -68,17 +89,12 @@ func (self *TaskManager) withMutex(f func()) { f() - // Check if all tasks are done - for _, task := range self.tasks { - if task.isBusy() { - return - } - } - - // If we get here, all tasks are done, so - // notify listeners that the program is idle - for _, listener := range self.idleListeners { - listener <- struct{}{} + // Wake up any goroutine blocked in WaitUntilIdle. This must not block on + // the waiter (we hold the mutex, and the waiter may itself be trying to + // acquire it, e.g. by creating a task, before it next waits) — which is + // exactly what Broadcast guarantees. + if !self.hasBusyTask() { + self.idleCond.Broadcast() } } diff --git a/pkg/gocui/task_manager_test.go b/pkg/gocui/task_manager_test.go index 7fe706d7a..b83b678ea 100644 --- a/pkg/gocui/task_manager_test.go +++ b/pkg/gocui/task_manager_test.go @@ -2,6 +2,7 @@ package gocui import ( "testing" + "time" "github.com/stretchr/testify/assert" ) @@ -61,3 +62,68 @@ func TestTaskManagerHasBusyForegroundTaskExcept(t *testing.T) { assert.False(t, tm.hasBusyForegroundTaskExcept(current)) }) } + +func TestTaskManagerWaitUntilIdle(t *testing.T) { + // returnsWithin reports whether f returns within the given duration. + returnsWithin := func(d time.Duration, f func()) bool { + done := make(chan struct{}) + go func() { + f() + close(done) + }() + select { + case <-done: + return true + case <-time.After(d): + return false + } + } + + t.Run("returns immediately when no task was ever created", func(t *testing.T) { + tm := newTaskManager() + assert.True(t, returnsWithin(time.Second, tm.WaitUntilIdle)) + }) + + t.Run("blocks while a task is busy", func(t *testing.T) { + tm := newTaskManager() + tm.NewTask(false) + assert.False(t, returnsWithin(50*time.Millisecond, tm.WaitUntilIdle)) + }) + + t.Run("wakes up when the last busy task completes", func(t *testing.T) { + tm := newTaskManager() + task := tm.NewTask(false) + go func() { + time.Sleep(10 * time.Millisecond) + task.Done() + }() + assert.True(t, returnsWithin(time.Second, tm.WaitUntilIdle)) + }) + + t.Run("a paused task counts as idle", func(t *testing.T) { + tm := newTaskManager() + task := tm.NewTask(false) + task.Pause() + assert.True(t, returnsWithin(time.Second, tm.WaitUntilIdle)) + }) + + t.Run("a task completing while nobody waits must not block", func(t *testing.T) { + // This is the deadlock case: the waiter (the integration-test runner) + // is between waits, and itself needs the task manager's mutex (it + // creates a task whenever it enqueues work) before it waits again. The + // idle notification must neither block the completing task while it + // holds the mutex, nor get lost. + tm := newTaskManager() + assert.True(t, returnsWithin(time.Second, func() { + // the program goes idle with nobody waiting... + tm.NewTask(true).Done() + + // ...and creating and completing more tasks afterwards must still + // be possible + task := tm.NewTask(false) + tm.NewTask(false).Done() + task.Done() + })) + assert.True(t, returnsWithin(time.Second, tm.WaitUntilIdle)) + }) +} diff --git a/pkg/gocui/tcell_driver.go b/pkg/gocui/tcell_driver.go index 226ee0580..885bcbabb 100644 --- a/pkg/gocui/tcell_driver.go +++ b/pkg/gocui/tcell_driver.go @@ -172,6 +172,12 @@ type GocuiEvent struct { Focused bool Start bool N int + + // task tracks the processing of this event for idle detection. Events + // replayed by integration tests carry a task from the moment they are + // submitted (see Gui.ReplayKeyEvent); for organic events it is nil, and + // the main loop creates a task when it picks the event up. + task Task } // Event types. @@ -208,6 +214,8 @@ type TcellKeyEventWrapper struct { Mod tcell.ModMask Key tcell.Key Ch string + + task Task // see GocuiEvent.task } func NewTcellKeyEventWrapper(event *tcell.EventKey, timestamp int64) *TcellKeyEventWrapper { @@ -229,6 +237,8 @@ type TcellMouseEventWrapper struct { Y int ButtonMask tcell.ButtonMask ModMask tcell.ModMask + + task Task // see GocuiEvent.task } func NewTcellMouseEventWrapper(event *tcell.EventMouse, timestamp int64) *TcellMouseEventWrapper { @@ -269,6 +279,8 @@ func (wrapper TcellResizeEventWrapper) toTcellEvent() tcell.Event { type TcellFocusEventWrapper struct { Timestamp int64 Focused bool + + task Task // see GocuiEvent.task } func NewTcellFocusEventWrapper(event *tcell.EventFocus, timestamp int64) *TcellFocusEventWrapper { @@ -285,21 +297,31 @@ func (wrapper TcellFocusEventWrapper) toTcellEvent() tcell.Event { // pollEvent get tcell.Event and transform it into gocuiEvent func (g *Gui) pollEvent() GocuiEvent { var tev tcell.Event + var task Task if g.playRecording { select { - case ev := <-g.ReplayedEvents.Keys: + case ev := <-g.replayedEvents.Keys: tev = (ev).toTcellEvent() - case ev := <-g.ReplayedEvents.Resizes: + task = ev.task + case ev := <-g.replayedEvents.Resizes: tev = (ev).toTcellEvent() - case ev := <-g.ReplayedEvents.MouseEvents: + case ev := <-g.replayedEvents.MouseEvents: tev = (ev).toTcellEvent() - case ev := <-g.ReplayedEvents.FocusEvents: + task = ev.task + case ev := <-g.replayedEvents.FocusEvents: tev = (ev).toTcellEvent() + task = ev.task } } else { tev = <-Screen.EventQ() } + event := gocuiEventFromTcellEvent(tev) + event.task = task + return event +} + +func gocuiEventFromTcellEvent(tev tcell.Event) GocuiEvent { switch tev := tev.(type) { case *tcell.EventInterrupt: return GocuiEvent{Type: eventInterrupt} diff --git a/pkg/gui/gui_driver.go b/pkg/gui/gui_driver.go index fef33bf66..31094b253 100644 --- a/pkg/gui/gui_driver.go +++ b/pkg/gui/gui_driver.go @@ -17,10 +17,9 @@ import ( // this gives our integration test a way of interacting with the gui for sending keypresses // and reading state. type GuiDriver struct { - gui *Gui - isIdleChan chan struct{} - toastChan chan string - headless bool + gui *Gui + toastChan chan string + headless bool } var _ integrationTypes.GuiDriver = &GuiDriver{} @@ -33,10 +32,10 @@ func (self *GuiDriver) PressKey(keyStr string) { self.Fail("Unrecognized key: " + keyStr) } - self.gui.g.ReplayedEvents.Keys <- gocui.NewTcellKeyEventWrapper( + self.gui.g.ReplayKeyEvent(gocui.NewTcellKeyEventWrapper( tcell.NewEventKey(tcell.Key(key.KeyName()), key.Str(), tcell.ModMask(key.Mod())), 0, - ) + )) self.waitTillIdle() } @@ -44,15 +43,15 @@ func (self *GuiDriver) PressKey(keyStr string) { func (self *GuiDriver) Click(x, y int) { self.CheckAllToastsAcknowledged() - self.gui.g.ReplayedEvents.MouseEvents <- gocui.NewTcellMouseEventWrapper( + self.gui.g.ReplayMouseEvent(gocui.NewTcellMouseEventWrapper( tcell.NewEventMouse(x, y, tcell.ButtonPrimary, 0), 0, - ) + )) self.waitTillIdle() - self.gui.g.ReplayedEvents.MouseEvents <- gocui.NewTcellMouseEventWrapper( + self.gui.g.ReplayMouseEvent(gocui.NewTcellMouseEventWrapper( tcell.NewEventMouse(x, y, tcell.ButtonNone, 0), 0, - ) + )) self.waitTillIdle() } @@ -60,10 +59,10 @@ func (self *GuiDriver) Click(x, y int) { // learns to reload changed config files. Tests use it to exercise the live // config-reload path. func (self *GuiDriver) FocusIn() { - self.gui.g.ReplayedEvents.FocusEvents <- gocui.NewTcellFocusEventWrapper( + self.gui.g.ReplayFocusEvent(gocui.NewTcellFocusEventWrapper( tcell.NewEventFocus(true), 0, - ) + )) self.waitTillIdle() } @@ -79,7 +78,7 @@ func (self *GuiDriver) PretendMergeOrRebaseStartedInLazygit() { // wait until lazygit is idle (i.e. all processing is done) before continuing func (self *GuiDriver) waitTillIdle() { - <-self.isIdleChan + self.gui.g.WaitUntilIdle() } func (self *GuiDriver) CheckAllToastsAcknowledged() { diff --git a/pkg/gui/test_mode.go b/pkg/gui/test_mode.go index 2ba381078..2d5958fbb 100644 --- a/pkg/gui/test_mode.go +++ b/pkg/gui/test_mode.go @@ -23,12 +23,8 @@ func (gui *Gui) handleTestMode() { } if test != nil { - isIdleChan := make(chan struct{}) - - gui.c.GocuiGui().AddIdleListener(isIdleChan) - waitUntilIdle := func() { - <-isIdleChan + gui.c.GocuiGui().WaitUntilIdle() } go func() { @@ -38,7 +34,7 @@ func (gui *Gui) handleTestMode() { gui.PopupHandler.(*popup.PopupHandler).SetToastFunc( func(message string, kind types.ToastKind) { toastChan <- message }) - test.Run(&GuiDriver{gui: gui, isIdleChan: isIdleChan, toastChan: toastChan, headless: Headless()}) + test.Run(&GuiDriver{gui: gui, toastChan: toastChan, headless: Headless()}) gui.g.Update(func(*gocui.Gui) error { return gocui.ErrQuit