From a7dfe3d2b74d6a3099565ec345412cea40001546 Mon Sep 17 00:00:00 2001 From: Rob Findley Date: Thu, 8 Jul 2021 16:54:32 -0400 Subject: [PATCH] internal/lsp: attempt to make TestDebouncer more robust CL 309276 added logic to retry TestDebouncer if its execution was determined to be invalid. Unfortunately it also reduced the delay period, which increases the likelihood of a flake on any individual execution. This appears to have more than offset any robustness resulting from the retries. This CL does a few things to try to improve the test: - Remove t.Parallel: we want goroutines to be scheduled quickly. - Increase the debouncing delay. - Improve the logic for determining if a test was invalid. - Guard the valid variable with a mutex, since this was actually racy. For golang/go#45085 Change-Id: Ib96c9a215d58606d3341f90774706945fcf9b06c Reviewed-on: https://go-review.googlesource.com/c/tools/+/333349 Trust: Robert Findley gopls-CI: kokoro Run-TryBot: Robert Findley TryBot-Result: Go Bot Reviewed-by: Rebecca Stambler --- internal/lsp/debounce_test.go | 24 +++++++++++++----------- 1 file changed, 13 insertions(+), 11 deletions(-) diff --git a/internal/lsp/debounce_test.go b/internal/lsp/debounce_test.go index 0d0453bf24..0712d6f284 100644 --- a/internal/lsp/debounce_test.go +++ b/internal/lsp/debounce_test.go @@ -18,7 +18,7 @@ func TestDebouncer(t *testing.T) { t.Parallel() const evtWait = 30 * time.Millisecond - const initialDelay = 100 * time.Millisecond + const initialDelay = 400 * time.Millisecond type event struct { key string @@ -65,26 +65,28 @@ func TestDebouncer(t *testing.T) { for _, test := range tests { test := test t.Run(test.label, func(t *testing.T) { - t.Parallel() - try := func(delay time.Duration) (bool, error) { d := newDebouncer() var wg sync.WaitGroup + var validMu sync.Mutex valid := true + prev := -1 for i, e := range test.events { wg.Add(1) - start := time.Now() - go func(e *event) { - if time.Since(start) > evtWait { - // Due to slow scheduling this event is likely to have fired out - // of order, so mark this attempt as invalid. + go func(i int, e *event) { + // Check if goroutines were not scheduled in the intended order. + validMu.Lock() + if prev != i-1 { valid = false } + prev = i + validMu.Unlock() + d.debounce(e.key, e.order, delay, func() { e.fired = true }) wg.Done() - }(e) + }(i, e) // For a bit more fidelity, sleep to try to make things actually // execute in order. This shouldn't have to be perfect: as long as we // don't have extreme pauses the test should still pass. @@ -93,6 +95,7 @@ func TestDebouncer(t *testing.T) { } } wg.Wait() + var errs []string for _, event := range test.events { if event.fired != event.wantFired { @@ -105,11 +108,10 @@ func TestDebouncer(t *testing.T) { if len(errs) > 0 { err = errors.New(strings.Join(errs, "\n")) } - // If the test took less than maxwait, no event before the return valid, err } - if err := retryInvalid(100*time.Millisecond, try); err != nil { + if err := retryInvalid(initialDelay, try); err != nil { t.Error(err) } })