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 <rfindley@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
diff --git a/internal/lsp/debounce_test.go b/internal/lsp/debounce_test.go
index 0d0453b..0712d6f 100644
--- a/internal/lsp/debounce_test.go
+++ b/internal/lsp/debounce_test.go
@@ -18,7 +18,7 @@
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 @@
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 @@
}
}
wg.Wait()
+
var errs []string
for _, event := range test.events {
if event.fired != event.wantFired {
@@ -105,11 +108,10 @@
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)
}
})
