[TT-3972] Mark/skip problematic tests, resolve some root issues (#4035)

* Improve CI tests

- Run go vet over whole repo, once
- Avoid bash no-ops in favor of 'set -e'
- Remove no-op stdout redirects
- Fail fast with -failfast flag
- No caching with -count=1 flag

* Add testing utilities to mark Flaky and Racy tests with

* Allow configuring python version for tests

* Add debug output to print which python version we seek

* Remove dead code, verified with @sredny

* Mark TT-5220 flaky test

* Mark TT-5222 flaky test

* Mark TT-5223 flaky test

* Resolve TT-5000, add cancellation ctx to redis to fix some racy tests

* Mark TT-5224 as racy

* Mark TT-5225 as racy

* Add http client/transport utilities to avoid net.DefaultResolver

* Fix: TT-5112 only replace defaultResolver once

Since we test packages individually, we can avoid DNS Mock data races by not resetting the DefaultResolver to it's original value.
While the ultimate goal is removal, we need to add replacements, which mostly add up to a Dialer function.

* Use NewClientLocal where we know we have flaky tests caused by the DNS Mock

* Tentative improvement for TT-5226, marking flaky tests

* Fix TT-5227 by having a dirty dns mock shutdown

* Mark TT-5228 as flaky

* add test util function GetPythonVersion
get python version from env in TestValueExtractorHeaderSource
[changelog]
added: test util function GetPythonVersion.

* Mark  TT-3973 as flaky

* mark TT-4069 as flaky

* fix flaky TT-4788

* Mark TT-5233 as racy

* mark TestHMACAuthSessionSHA512Pass as flaky TT-3973

* Allow passing options to bin/ci-tests.sh for go test

* Print notice test can be skipped with CI env

* Remove unnecessary go background key deletion, race over b.store

* Upgrade redis/v8 to get patches for all the data races

* Mark TT-5236 as flaky

* Fix goroutine leak in rpcReloadLoop

* Fix: panic in tests if we can't connect to redis

* Testing: add a console monitor for runtime stats, goroutines

* Add flaky test to verify how many goroutines we leak in a test case

* Remove running go-vet, it's run twice as part of golangci-lint

* Mark TT-5249 as flaky

* Mark flaky tests TT-5250, TT-5251

* Remove unused *gateway.cancelFn

* Mark TT-5254 flaky test

* Mark TT-5257 as flaky, note frequency

* Mark TT-5258 as flaky

* Mark TT-5259 as flaky

* Mark TT-5260 flaky, improve tests fragility

* Fix: Improve flakyness in Gateway tests

* Mark TT-5112, 5261, 5261 as flaky

* Remove unnecessary time.Sleep values from tests, tests still pass

* Add enabling of pprof server with TEST_PPROF_ENABLE and TEST_PPROF_ADDR (tests only)

* Improve TestAPISpec_StripListenPath flakyness, Mark TT-5255 as flaky

* Mark TT-5263 as flaky

* Mark flaky tests, remove SkipEmptyRedis

* Mark TT-5264 as flaky

* Test utilities improvements (signficant):

- disable the DNS mock (unsafe, TT-5112)
- disable emptying redis from tests (race conditions as redis is a singleton with reuse)
- some cleanup for readibility, s.Gw to gw (scope)
- remove useless s.gwMu mock, cleaner test cancellation

* Fix leaking pubsub channels, decrease deprecated redis api surface to use channels

* Fix golangci-lint reported issues in modified code

* Fix resource leak with time.After in tests, analytics records flusher

* Removed TestAnalytics, fixing incorrect rebase

* Add comment to explain why func is empty, sonarcloud

Co-authored-by: Jeff <jeffy.mathew100@gmail.com>

apidef/validator_test.go

@@ -43,7 +43,8 @@ func TestValidationResult_ErrorStrings(t *testing.T) {
 func runValidationTest(apiDef *APIDefinition, ruleSet ValidationRuleSet, expectedValidationResult ValidationResult) func(t *testing.T) {
 	return func(t *testing.T) {
 		result := Validate(apiDef, ruleSet)
-		assert.EqualValues(t, expectedValidationResult, result)
+		assert.Equal(t, expectedValidationResult.IsValid, result.IsValid)
+		assert.ElementsMatch(t, expectedValidationResult.Errors, result.Errors)
 	}
 }
 

bin/ci-tests.sh

@@ -2,46 +2,35 @@
 
 TEST_TIMEOUT=15m
 
-# print a command and execute it
-show() {
-	echo "$@" >&2
-	eval "$@"
-}
-
-fatal() {
-	echo "$@" >&2
-	exit 1
-}
-
 PKGS="$(go list ./...)"
 
+# Support passing custom flags (-json, etc.)
+OPTS="$@"
+if [[ -z "$OPTS" ]]; then
+	OPTS="-race -count=1 -failfast -v"
+fi
+
 export PKG_PATH=${GOPATH}/src/github.com/TykTechnologies/tyk
 
+# exit on non-zero exit from go test/vet
+set -e
+
 # build Go-plugin used in tests
-go build -o ./test/goplugins/goplugins.so -buildmode=plugin ./test/goplugins || fatal "building Go-plugin failed"
+echo "Building go plugin"
+go build -o ./test/goplugins/goplugins.so -buildmode=plugin ./test/goplugins
 
 for pkg in ${PKGS}; do
     tags=""
-
-    # TODO: Remove skipRace variable after solving race conditions in tests.
-    skipRace=false
-    if [[ ${pkg} == *"grpc" ]]; then
-        skipRace=true
-    elif [[ ${pkg} == *"goplugin" ]]; then
-        skipRace=true
+    if [[ ${pkg} == *"goplugin" ]]; then
         tags="-tags 'goplugin'"
     fi
 
-    race="-race"
-
-    if [[ ${skipRace} = true ]]; then
-        race=""
-    fi
     coveragefile=`echo "$pkg" | awk -F/ '{print $NF}'`
-    show go test ${race} -timeout ${TEST_TIMEOUT} -v -coverprofile=${coveragefile}.cov ${pkg} ${tags} || fatal "Test Failed"
-    show go vet ${tags} ${pkg} || fatal "go vet errored"
+
+    echo go test ${OPTS} -timeout ${TEST_TIMEOUT} -coverprofile=${coveragefile}.cov ${pkg} ${tags}
+    go test ${OPTS} -timeout ${TEST_TIMEOUT} -coverprofile=${coveragefile}.cov ${pkg} ${tags}
 done
 
 # run rpc tests separately
 rpc_tests='SyncAPISpecsRPC|OrgSessionWithRPCDown'
-show go test -timeout ${TEST_TIMEOUT} -v -coverprofile=gateway-rpc.cov github.com/TykTechnologies/tyk/gateway -p 1 -run '"'${rpc_tests}'"' || fatal "Test Failed"
+go test -count=1 -timeout ${TEST_TIMEOUT} -v -coverprofile=gateway-rpc.cov github.com/TykTechnologies/tyk/gateway -p 1 -run '"'${rpc_tests}'"'

certs/manager_test.go

@@ -18,6 +18,8 @@ import (
 	"time"
 
 	"github.com/stretchr/testify/assert"
+
+	"github.com/TykTechnologies/tyk/test"
 )
 
 type dummyStorage struct {
@@ -246,6 +248,8 @@ func leafSubjectName(cert *tls.Certificate) string {
 }
 
 func TestAddCertificate(t *testing.T) {
+	test.Flaky(t) // TT-5249
+
 	m := newManager()
 
 	expiredCertPem, _ := genCertificateFromCommonName("expired", true)

coprocess/grpc/coprocess_grpc_test.go

@@ -300,7 +300,10 @@ func startTykWithGRPC() (*gateway.Test, *grpc.Server) {
 		GRPCRecvMaxSize:     grpcTestMaxSize,
 		GRPCSendMaxSize:     grpcTestMaxSize,
 	}
-	ts := gateway.StartTest(nil, gateway.TestConfig{CoprocessConfig: cfg})
+	ts := gateway.StartTest(nil, gateway.TestConfig{
+		CoprocessConfig:   cfg,
+		EnableTestDNSMock: false,
+	})
 
 	// Load test APIs:
 	loadTestGRPCAPIs(ts)
@@ -403,6 +406,8 @@ func TestGRPCDispatch(t *testing.T) {
 	})
 
 	t.Run("Post Hook with allowed message length", func(t *testing.T) {
+		test.Flaky(t)
+
 		s := randStringBytes(20000000)
 		ts.Run(t, test.TestCase{
 			Path:    "/grpc-test-api-3/",
@@ -414,6 +419,8 @@ func TestGRPCDispatch(t *testing.T) {
 	})
 
 	t.Run("Post Hook with with unallowed message length", func(t *testing.T) {
+		test.Flaky(t)
+
 		s := randStringBytes(150000000)
 		ts.Run(t, test.TestCase{
 			Path:    "/grpc-test-api-3/",
@@ -450,13 +457,7 @@ func BenchmarkGRPCDispatch(b *testing.B) {
 const letters = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ"
 
 func randStringBytes(n int) string {
-	b := make([]byte, n)
-
-	for i := range b {
-		b[i] = letters[rand.Intn(len(letters))]
-	}
-
-	return string(b)
+	return strings.Repeat(string(letters[rand.Intn(len(letters))]), n)
 }
 
 func TestGRPCIgnore(t *testing.T) {

coprocess/python/coprocess_id_extractor_python_test.go

@@ -146,11 +146,12 @@ def MyAuthHook(request, session, metadata, spec):
 // Our `pythonBundleWithAuthCheck` plugin restrict more then 1 call
 // With ID extractor, it should run multiple times (because cache)
 func TestValueExtractorHeaderSource(t *testing.T) {
+	pythonVersion := test.GetPythonVersion()
 	ts := gateway.StartTest(nil, gateway.TestConfig{
 		CoprocessConfig: config.CoProcessConfig{
 			EnableCoProcess:  true,
 			PythonPathPrefix: pkgPath,
-			PythonVersion:    "3.5",
+			PythonVersion:    pythonVersion,
 		},
 		Delay: 10 * time.Millisecond,
 	})

dlpython/main.go

@@ -43,6 +43,8 @@ var (
 
 // FindPythonConfig scans PATH for common python-config locations.
 func FindPythonConfig(customVersion string) (selectedVersion string, err error) {
+	logger.Debugf("Requested python version: %q", customVersion)
+
 	// Not sure if this can be replaced with os.LookPath:
 	if paths == "" {
 		return selectedVersion, errEmptyPath

gateway/analytics.go

@@ -142,6 +142,7 @@ func (r *RedisAnalyticsHandler) recordWorker() {
 		}
 		serliazerSuffix := r.analyticsSerializer.GetSuffix()
 		analyticKey += serliazerSuffix
+
 		readyToSend := false
 
 		flushTimer := time.NewTimer(recordsBufferFlushInterval)

gateway/api_definition_test.go

@@ -554,6 +554,8 @@ func TestIgnored(t *testing.T) {
 }
 
 func TestOldMockResponse(t *testing.T) {
+	test.Racy(t) // TODO: TT-5225
+
 	ts := StartTest(nil)
 	defer ts.Close()
 
@@ -971,7 +973,6 @@ func TestGetVersionFromRequest(t *testing.T) {
 	t.Run("Header location", func(t *testing.T) {
 		ts := StartTest(nil)
 		defer func() {
-			time.Sleep(1 * time.Second)
 			ts.Close()
 		}()
 
@@ -1295,6 +1296,8 @@ func TestAPISpec_SanitizeProxyPaths(t *testing.T) {
 }
 
 func TestEnforcedTimeout(t *testing.T) {
+	test.Flaky(t) // TODO TT-5222
+
 	ts := StartTest(nil)
 	defer ts.Close()
 

gateway/api_test.go

@@ -2,6 +2,7 @@ package gateway
 
 import (
 	"bytes"
+	"context"
 	"crypto/x509"
 	"encoding/json"
 	"io/ioutil"
@@ -15,6 +16,7 @@ import (
 	"strings"
 	"sync"
 	"testing"
+	"time"
 
 	"github.com/TykTechnologies/tyk/certs"
 
@@ -751,7 +753,7 @@ func TestKeyHandler_CheckKeysNotDuplicateOnUpdate(t *testing.T) {
 }
 
 func TestHashKeyHandler(t *testing.T) {
-
+	test.Racy(t) // TODO: TT-5233
 	conf := func(globalConf *config.Config) {
 		// make it to use hashes for Redis keys
 		globalConf.HashKeys = true
@@ -789,6 +791,7 @@ func TestHashKeyHandler(t *testing.T) {
 }
 
 func TestHashKeyHandlerLegacyWithHashFunc(t *testing.T) {
+	test.Racy(t) // TODO: TT-5233
 	ts := StartTest(nil)
 	defer ts.Close()
 
@@ -1112,6 +1115,8 @@ func TestHashKeyListingDisabled(t *testing.T) {
 }
 
 func TestKeyHandler_HashingDisabled(t *testing.T) {
+	test.Racy(t) // TODO: TT-5524
+
 	ts := StartTest(nil)
 	defer ts.Close()
 
@@ -1475,16 +1480,16 @@ func TestGroupResetHandler(t *testing.T) {
 	ts := StartTest(nil)
 	defer ts.Close()
 
-	didSubscribe := make(chan bool)
 	didReload := make(chan bool)
 	cacheStore := storage.RedisCluster{RedisController: ts.Gw.RedisController}
 	cacheStore.Connect()
 
+	ctx, cancel := context.WithTimeout(context.Background(), time.Second*5)
+	defer cancel()
+
 	go func() {
-		err := cacheStore.StartPubSubHandler(RedisPubSubChannel, func(v interface{}) {
+		err := cacheStore.StartPubSubHandler(ctx, RedisPubSubChannel, func(v interface{}) {
 			switch x := v.(type) {
-			case *redis.Subscription:
-				didSubscribe <- true
 			case *redis.Message:
 				notf := Notification{Gw: ts.Gw}
 				if err := json.Unmarshal([]byte(x.Payload), &notf); err != nil {
@@ -1498,8 +1503,8 @@ func TestGroupResetHandler(t *testing.T) {
 		if err != nil {
 			t.Log(err)
 			t.Fail()
-			close(didReload)
 		}
+		close(didReload)
 	}()
 
 	uri := "/tyk/reload/group"
@@ -1514,7 +1519,6 @@ func TestGroupResetHandler(t *testing.T) {
 
 	// If we don't wait for the subscription to be done, we might do
 	// the reload before pub/sub is in place to receive our message.
-	<-didSubscribe
 	req := ts.withAuth(TestReq(t, "GET", uri, nil))
 
 	ts.mainRouter().ServeHTTP(recorder, req)
@@ -1532,7 +1536,14 @@ func TestGroupResetHandler(t *testing.T) {
 	// We wait for the right notification (NoticeGroupReload), other
 	// type of notifications may be received during tests, as this
 	// is the cluster channel:
-	<-didReload
+	select {
+	case <-time.After(time.Second):
+		t.Fatal("Timeout waiting for reload signal")
+	case ok := <-didReload:
+		if !ok {
+			t.Fatal("Reload failed (closed pubsub)")
+		}
+	}
 }
 
 func TestHotReloadSingle(t *testing.T) {
@@ -1659,6 +1670,7 @@ func TestApiLoaderLongestPathFirst(t *testing.T) {
 
 	for hp := range inputs {
 		testCases = append(testCases, test.TestCase{
+			Client:    test.NewClientLocal(),
 			Path:      "/" + hp.path,
 			Domain:    hp.host,
 			Code:      200,

gateway/auth_manager.go

@@ -63,15 +63,15 @@ func (b *DefaultSessionManager) ResetQuota(keyName string, session *user.Session
 	}).Info("Reset quota for key.")
 
 	rateLimiterSentinelKey := RateLimitKeyPrefix + keyName + ".BLOCKED"
+
 	// Clear the rate limiter
-	go b.store.DeleteRawKey(rateLimiterSentinelKey)
+	b.store.DeleteRawKey(rateLimiterSentinelKey)
 	// Fix the raw key
-	go b.store.DeleteRawKey(rawKey)
-	//go b.store.SetKey(rawKey, "0", session.QuotaRenewalRate)
+	b.store.DeleteRawKey(rawKey)
 
 	for _, acl := range session.AccessRights {
 		rawKey = QuotaKeyPrefix + acl.AllowanceScope + "-" + keyName
-		go b.store.DeleteRawKey(rawKey)
+		b.store.DeleteRawKey(rawKey)
 	}
 }
 

gateway/batch_requests_test.go

@@ -12,7 +12,6 @@ import (
 	"strings"
 	"sync/atomic"
 	"testing"
-	"time"
 
 	"github.com/TykTechnologies/tyk/certs"
 
@@ -240,9 +239,6 @@ func TestBatchIgnoreCanonicalHeaderKey(t *testing.T) {
 		})
 	})
 
-	// Let the server start
-	time.Sleep(500 * time.Millisecond)
-
 	ts.Run(t, test.TestCase{Path: "/virt", Code: 202})
 	got := header.Load().(string)
 	if got != NonCanonicalHeaderKey {