From caa692deea1e7c65c4b1c9f03fd88649df10d605 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Fri, 5 Jun 2020 16:52:31 -0400 Subject: [PATCH] ci: Enabled SA2002 staticcheck check And handle errors in the main test goroutine --- .golangci.yml | 4 --- agent/cache-types/connect_ca_leaf_test.go | 37 +++++++++++++++-------- agent/catalog_endpoint_test.go | 2 +- agent/consul/catalog_endpoint_test.go | 5 +-- agent/consul/kvs_endpoint_test.go | 5 +-- agent/event_endpoint_test.go | 2 +- api/kv_test.go | 24 +++++++-------- api/lock_test.go | 8 +++-- api/semaphore_test.go | 6 ++-- api/watch/funcs_test.go | 26 ++++++++-------- 10 files changed, 66 insertions(+), 53 deletions(-) diff --git a/.golangci.yml b/.golangci.yml index 1c27e71eab..5573610570 100644 --- a/.golangci.yml +++ b/.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 diff --git a/agent/cache-types/connect_ca_leaf_test.go b/agent/cache-types/connect_ca_leaf_test.go index 6d7b37caa7..90bd12082b 100644 --- a/agent/cache-types/connect_ca_leaf_test.go +++ b/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) - testDoneCh := make(chan struct{}, n) + setupDoneCh := make(chan error, 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 - require.NotEqual(opts.MinIndex, v.Value.(*structs.IssuedCert).CreateIndex) + if 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()) + } } } diff --git a/agent/catalog_endpoint_test.go b/agent/catalog_endpoint_test.go index 541e52656a..f4bc487a0a 100644 --- a/agent/catalog_endpoint_test.go +++ b/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) } }() diff --git a/agent/consul/catalog_endpoint_test.go b/agent/consul/catalog_endpoint_test.go index 3663b45753..1e24478f8c 100644 --- a/agent/consul/catalog_endpoint_test.go +++ b/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) } }() diff --git a/agent/consul/kvs_endpoint_test.go b/agent/consul/kvs_endpoint_test.go index f33d2fce31..6343f6eccf 100644 --- a/agent/consul/kvs_endpoint_test.go +++ b/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 }() diff --git a/agent/event_endpoint_test.go b/agent/event_endpoint_test.go index ef4e070bcc..f15ec8de1e 100644 --- a/agent/event_endpoint_test.go +++ b/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) } }() diff --git a/api/kv_test.go b/api/kv_test.go index b97d2bc266..cc0a507ba0 100644 --- a/api/kv_test.go +++ b/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 { - t.Fatalf("err: %v", err) - } - doneCh <- struct{}{} + _, err := kv.Put(p, nil) + doneCh <- err }() // 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 { - t.Fatalf("err: %v", err) - } - doneCh <- struct{}{} + _, err := kv.Put(p, nil) + doneCh <- err }() // 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) { diff --git a/api/lock_test.go b/api/lock_test.go index 82c4221da8..761a97dc3d 100644 --- a/api/lock_test.go +++ b/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 }() diff --git a/api/semaphore_test.go b/api/semaphore_test.go index fcba900e86..dfa3333a49 100644 --- a/api/semaphore_test.go +++ b/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) diff --git a/api/watch/funcs_test.go b/api/watch/funcs_test.go index 62d33c3914..46b0be3b2a 100644 --- a/api/watch/funcs_test.go +++ b/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()