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 <[email protected]>
gopls-CI: kokoro <[email protected]>
Run-TryBot: Robert Findley <[email protected]>
TryBot-Result: Go Bot <[email protected]>
Reviewed-by: Rebecca Stambler <[email protected]>

Author: findleyr

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)
 			}
 		})