ci: Enabled SA2002 staticcheck check

And handle errors in the main test goroutine

.golangci.yml

@@ -21,10 +21,6 @@ issues:
     - linters: [staticcheck]
       text: "SA4006:"
 
-    # Temp ignore SA2002: the goroutine calls T.Fatalf, which must be called in the same goroutine as the test
-    - linters: [staticcheck]
-      text: "SA2002:"
-
 linters-settings:
   gofmt:
     simplify: false

agent/cache-types/connect_ca_leaf_test.go

@@ -695,7 +695,6 @@ func TestConnectCALeaf_CSRRateLimiting(t *testing.T) {
 func TestConnectCALeaf_watchRootsDedupingMultipleCallers(t *testing.T) {
 	t.Parallel()
 
-	require := require.New(t)
 	rpc := TestRPC(t)
 	defer rpc.AssertExpectations(t)
 
@@ -737,8 +736,8 @@ func TestConnectCALeaf_watchRootsDedupingMultipleCallers(t *testing.T) {
 	// initial cert delivered and is blocking before the root changes. It's not a
 	// wait group since we want to be able to timeout the main test goroutine if
 	// one of the clients gets stuck. Instead it's a buffered chan.
-	setupDoneCh := make(chan struct{}, n)
+	setupDoneCh := make(chan error, n)
-	testDoneCh := make(chan struct{}, n)
+	testDoneCh := make(chan error, n)
 	// rootsUpdate is used to coordinate clients so they know when they should
 	// expect to see leaf renewed after root change.
 	rootsUpdatedCh := make(chan struct{})
@@ -755,7 +754,8 @@ func TestConnectCALeaf_watchRootsDedupingMultipleCallers(t *testing.T) {
 		fetchCh := TestFetchCh(t, typ, opts, req)
 		select {
 		case <-time.After(100 * time.Millisecond):
-			t.Fatal("shouldn't block waiting for fetch")
+			setupDoneCh <- fmt.Errorf("shouldn't block waiting for fetch")
+			return
 		case result := <-fetchCh:
 			v := mustFetchResult(t, result)
 			opts.LastResult = &v
@@ -766,33 +766,38 @@ func TestConnectCALeaf_watchRootsDedupingMultipleCallers(t *testing.T) {
 		fetchCh = TestFetchCh(t, typ, opts, req)
 		select {
 		case result := <-fetchCh:
-			t.Fatalf("should not return: %#v", result)
+			setupDoneCh <- fmt.Errorf("should not return: %#v", result)
+			return
 		case <-time.After(100 * time.Millisecond):
 		}
 
 		// We're done with setup and the blocking call is still blocking in
 		// background.
-		setupDoneCh <- struct{}{}
+		setupDoneCh <- nil
 
 		// Wait until all others are also done and roots change incase there are
 		// stragglers delaying the root update.
 		select {
 		case <-rootsUpdatedCh:
 		case <-time.After(200 * time.Millisecond):
-			t.Fatalf("waited too long for root update")
+			testDoneCh <- fmt.Errorf("waited too long for root update")
+			return
 		}
 
 		// Now we should see root update within a short period
 		select {
 		case <-time.After(100 * time.Millisecond):
-			t.Fatal("shouldn't block waiting for fetch")
+			testDoneCh <- fmt.Errorf("shouldn't block waiting for fetch")
+			return
 		case result := <-fetchCh:
 			v := mustFetchResult(t, result)
-			// Index must be different
+			if opts.MinIndex == v.Value.(*structs.IssuedCert).CreateIndex {
-			require.NotEqual(opts.MinIndex, v.Value.(*structs.IssuedCert).CreateIndex)
+				testDoneCh <- fmt.Errorf("index must be different")
+				return
+			}
 		}
 
-		testDoneCh <- struct{}{}
+		testDoneCh <- nil
 	}
 
 	// Sanity check the roots watcher is not running yet
@@ -808,7 +813,10 @@ func TestConnectCALeaf_watchRootsDedupingMultipleCallers(t *testing.T) {
 		select {
 		case <-timeoutCh:
 			t.Fatal("timed out waiting for clients")
-		case <-setupDoneCh:
+		case err := <-setupDoneCh:
+			if err != nil {
+				t.Fatalf(err.Error())
+			}
 		}
 	}
 
@@ -837,7 +845,10 @@ func TestConnectCALeaf_watchRootsDedupingMultipleCallers(t *testing.T) {
 		select {
 		case <-timeoutCh:
 			t.Fatalf("timed out waiting for %d of %d clients to renew after root change", n-i, n)
-		case <-testDoneCh:
+		case err := <-testDoneCh:
+			if err != nil {
+				t.Fatalf(err.Error())
+			}
 		}
 	}
 

agent/catalog_endpoint_test.go

@@ -315,7 +315,7 @@ func TestCatalogNodes_Blocking(t *testing.T) {
 		}
 		var out struct{}
 		if err := a.RPC("Catalog.Register", args, &out); err != nil {
-			t.Fatalf("err: %v", err)
+			t.Errorf("err: %v", err)
 		}
 	}()
 

agent/consul/catalog_endpoint_test.go

@@ -1418,10 +1418,11 @@ func TestCatalog_ListServices_Blocking(t *testing.T) {
 	go func() {
 		time.Sleep(100 * time.Millisecond)
 		if err := s1.fsm.State().EnsureNode(idx+1, &structs.Node{Node: "foo", Address: "127.0.0.1"}); err != nil {
-			t.Fatalf("err: %v", err)
+			t.Errorf("err: %v", err)
+			return
 		}
 		if err := s1.fsm.State().EnsureService(idx+2, "foo", &structs.NodeService{ID: "db", Service: "db", Tags: []string{"primary"}, Address: "127.0.0.1", Port: 5000}); err != nil {
-			t.Fatalf("err: %v", err)
+			t.Errorf("err: %v", err)
 		}
 	}()
 

agent/consul/kvs_endpoint_test.go

@@ -354,7 +354,7 @@ func TestKVSEndpoint_List_Blocking(t *testing.T) {
 		}
 		var out bool
 		if err := msgpackrpc.CallWithCodec(codec, "KVS.Apply", &arg, &out); err != nil {
-			t.Fatalf("err: %v", err)
+			t.Errorf("RPC call failed: %v", err)
 		}
 	}()
 
@@ -891,7 +891,8 @@ func TestKVS_Issue_1626(t *testing.T) {
 		}
 		var dirent structs.IndexedDirEntries
 		if err := msgpackrpc.CallWithCodec(codec, "KVS.Get", &getR, &dirent); err != nil {
-			t.Fatalf("err: %v", err)
+			t.Errorf("RPC call failed: %v", err)
+			return
 		}
 		doneCh <- &dirent
 	}()

agent/event_endpoint_test.go

@@ -269,7 +269,7 @@ func TestEventList_Blocking(t *testing.T) {
 		time.Sleep(50 * time.Millisecond)
 		p := &UserEvent{Name: "second"}
 		if err := a.UserEvent("dc1", "root", p); err != nil {
-			t.Fatalf("err: %v", err)
+			t.Errorf("err: %v", err)
 		}
 	}()
 

api/kv_test.go

@@ -6,6 +6,8 @@ import (
 	"strings"
 	"testing"
 	"time"
+
+	"github.com/stretchr/testify/require"
 )
 
 func TestAPI_ClientPutGetDelete(t *testing.T) {
@@ -246,16 +248,14 @@ func TestAPI_ClientWatchGet(t *testing.T) {
 
 	// Put the key
 	value := []byte("test")
-	doneCh := make(chan struct{})
+	doneCh := make(chan error)
 	go func() {
 		kv := c.KV()
 
 		time.Sleep(100 * time.Millisecond)
 		p := &KVPair{Key: key, Flags: 42, Value: value}
-		if _, err := kv.Put(p, nil); err != nil {
+		_, err := kv.Put(p, nil)
-			t.Fatalf("err: %v", err)
+		doneCh <- err
-		}
-		doneCh <- struct{}{}
 	}()
 
 	// Get should work
@@ -278,7 +278,8 @@ func TestAPI_ClientWatchGet(t *testing.T) {
 	}
 
 	// Block until put finishes to avoid a race between it and deferred s.Stop()
-	<-doneCh
+	err = <-doneCh
+	require.NoError(t, err)
 }
 
 func TestAPI_ClientWatchList(t *testing.T) {
@@ -304,16 +305,14 @@ func TestAPI_ClientWatchList(t *testing.T) {
 
 	// Put the key
 	value := []byte("test")
-	doneCh := make(chan struct{})
+	doneCh := make(chan error)
 	go func() {
 		kv := c.KV()
 
 		time.Sleep(100 * time.Millisecond)
 		p := &KVPair{Key: key, Flags: 42, Value: value}
-		if _, err := kv.Put(p, nil); err != nil {
+		_, err := kv.Put(p, nil)
-			t.Fatalf("err: %v", err)
+		doneCh <- err
-		}
-		doneCh <- struct{}{}
 	}()
 
 	// Get should work
@@ -336,7 +335,8 @@ func TestAPI_ClientWatchList(t *testing.T) {
 	}
 
 	// Block until put finishes to avoid a race between it and deferred s.Stop()
-	<-doneCh
+	err = <-doneCh
+	require.NoError(t, err)
 }
 
 func TestAPI_ClientKeys_DeleteRecurse(t *testing.T) {

api/lock_test.go

@@ -189,10 +189,12 @@ func TestAPI_LockContend(t *testing.T) {
 			// Should work eventually, will contend
 			leaderCh, err := lock.Lock(nil)
 			if err != nil {
-				t.Fatalf("err: %v", err)
+				t.Errorf("err: %v", err)
+				return
 			}
 			if leaderCh == nil {
-				t.Fatalf("not leader")
+				t.Errorf("not leader")
+				return
 			}
 			defer lock.Unlock()
 			log.Printf("Contender %d acquired", idx)
@@ -358,7 +360,7 @@ func TestAPI_LockReclaimLock(t *testing.T) {
 	go func() {
 		l2Ch, err := l2.Lock(nil)
 		if err != nil {
-			t.Fatalf("not locked: %v", err)
+			t.Errorf("not locked: %v", err)
 		}
 		reclaimed <- l2Ch
 	}()

api/semaphore_test.go

@@ -184,10 +184,12 @@ func TestAPI_SemaphoreContend(t *testing.T) {
 			// Should work eventually, will contend
 			lockCh, err := sema.Acquire(nil)
 			if err != nil {
-				t.Fatalf("err: %v", err)
+				t.Errorf("err: %v", err)
+				return
 			}
 			if lockCh == nil {
-				t.Fatalf("not locked")
+				t.Errorf("not locked")
+				return
 			}
 			defer sema.Release()
 			log.Printf("Contender %d acquired", idx)

api/watch/funcs_test.go

@@ -127,7 +127,7 @@ func TestKeyWatch(t *testing.T) {
 	go func() {
 		defer wg.Done()
 		if err := plan.Run(s.HTTPAddr); err != nil {
-			t.Fatalf("err: %v", err)
+			t.Errorf("err: %v", err)
 		}
 	}()
 	defer plan.Stop()
@@ -194,7 +194,7 @@ func TestKeyWatch_With_PrefixDelete(t *testing.T) {
 	go func() {
 		defer wg.Done()
 		if err := plan.Run(s.HTTPAddr); err != nil {
-			t.Fatalf("err: %v", err)
+			t.Errorf("err: %v", err)
 		}
 	}()
 	defer plan.Stop()
@@ -259,7 +259,7 @@ func TestKeyPrefixWatch(t *testing.T) {
 	go func() {
 		defer wg.Done()
 		if err := plan.Run(s.HTTPAddr); err != nil {
-			t.Fatalf("err: %v", err)
+			t.Errorf("err: %v", err)
 		}
 	}()
 	defer plan.Stop()
@@ -325,7 +325,7 @@ func TestServicesWatch(t *testing.T) {
 	go func() {
 		defer wg.Done()
 		if err := plan.Run(s.HTTPAddr); err != nil {
-			t.Fatalf("err: %v", err)
+			t.Errorf("err: %v", err)
 		}
 	}()
 	defer plan.Stop()
@@ -399,7 +399,7 @@ func TestNodesWatch(t *testing.T) {
 	go func() {
 		defer wg.Done()
 		if err := plan.Run(s.HTTPAddr); err != nil {
-			t.Fatalf("err: %v", err)
+			t.Errorf("err: %v", err)
 		}
 	}()
 	defer plan.Stop()
@@ -475,7 +475,7 @@ func TestServiceWatch(t *testing.T) {
 	go func() {
 		defer wg.Done()
 		if err := plan.Run(s.HTTPAddr); err != nil {
-			t.Fatalf("err: %v", err)
+			t.Errorf("err: %v", err)
 		}
 	}()
 	defer plan.Stop()
@@ -545,7 +545,7 @@ func TestServiceMultipleTagsWatch(t *testing.T) {
 	go func() {
 		defer wg.Done()
 		if err := plan.Run(s.HTTPAddr); err != nil {
-			t.Fatalf("err: %v", err)
+			t.Errorf("err: %v", err)
 		}
 	}()
 	defer plan.Stop()
@@ -637,7 +637,7 @@ func TestChecksWatch_State(t *testing.T) {
 	go func() {
 		defer wg.Done()
 		if err := plan.Run(s.HTTPAddr); err != nil {
-			t.Fatalf("err: %v", err)
+			t.Errorf("err: %v", err)
 		}
 	}()
 	defer plan.Stop()
@@ -713,7 +713,7 @@ func TestChecksWatch_Service(t *testing.T) {
 	go func() {
 		defer wg.Done()
 		if err := plan.Run(s.HTTPAddr); err != nil {
-			t.Fatalf("err: %v", err)
+			t.Errorf("err: %v", err)
 		}
 	}()
 	defer plan.Stop()
@@ -791,7 +791,7 @@ func TestEventWatch(t *testing.T) {
 	go func() {
 		defer wg.Done()
 		if err := plan.Run(s.HTTPAddr); err != nil {
-			t.Fatalf("err: %v", err)
+			t.Errorf("err: %v", err)
 		}
 	}()
 	defer plan.Stop()
@@ -855,7 +855,7 @@ func TestConnectRootsWatch(t *testing.T) {
 	go func() {
 		defer wg.Done()
 		if err := plan.Run(s.HTTPAddr); err != nil {
-			t.Fatalf("err: %v", err)
+			t.Errorf("err: %v", err)
 		}
 	}()
 	defer plan.Stop()
@@ -931,7 +931,7 @@ func TestConnectLeafWatch(t *testing.T) {
 	go func() {
 		defer wg.Done()
 		if err := plan.Run(s.HTTPAddr); err != nil {
-			t.Fatalf("err: %v", err)
+			t.Errorf("err: %v", err)
 		}
 	}()
 	defer plan.Stop()
@@ -1013,7 +1013,7 @@ func TestAgentServiceWatch(t *testing.T) {
 	go func() {
 		defer wg.Done()
 		if err := plan.Run(s.HTTPAddr); err != nil {
-			t.Fatalf("err: %v", err)
+			t.Errorf("err: %v", err)
 		}
 	}()
 	defer plan.Stop()