test: fix flaky tests

ory · Sep 20, 2022 · 099bcf0 · 099bcf0
1 parent c1ed811
commit 099bcf0
Show file tree

Hide file tree

Showing 14 changed files with 52 additions and 39 deletions.
diff --git a/pipeline/authn/authenticator_anonymous_test.go b/pipeline/authn/authenticator_anonymous_test.go
@@ -34,6 +34,7 @@ import (
 )
 
 func TestAuthenticatorAnonymous(t *testing.T) {
+	t.Parallel()
 	conf := internal.NewConfigurationWithDefaults()
 	reg := internal.NewRegistry(conf)
 

diff --git a/pipeline/authn/authenticator_bearer_token_test.go b/pipeline/authn/authenticator_bearer_token_test.go
@@ -25,6 +25,7 @@ import (
 )
 
 func TestAuthenticatorBearerToken(t *testing.T) {
+	t.Parallel()
 	conf := internal.NewConfigurationWithDefaults()
 	reg := internal.NewRegistry(conf)
 

diff --git a/pipeline/authn/authenticator_cookie_session_test.go b/pipeline/authn/authenticator_cookie_session_test.go
@@ -22,6 +22,7 @@ import (
 )
 
 func TestAuthenticatorCookieSession(t *testing.T) {
+	t.Parallel()
 	conf := internal.NewConfigurationWithDefaults()
 	reg := internal.NewRegistry(conf)
 	session := new(AuthenticationSession)

diff --git a/pipeline/authn/authenticator_jwt_test.go b/pipeline/authn/authenticator_jwt_test.go
@@ -42,6 +42,7 @@ import (
 )
 
 func TestAuthenticatorJWT(t *testing.T) {
+	t.Parallel()
 	keys := []string{
 		"file://../../test/stub/jwks-hs.json",
 		"file://../../test/stub/jwks-rsa-multiple.json",

diff --git a/pipeline/authn/authenticator_noop_test.go b/pipeline/authn/authenticator_noop_test.go
@@ -31,6 +31,7 @@ import (
 )
 
 func TestAuthenticatorNoop(t *testing.T) {
+	t.Parallel()
 	conf := internal.NewConfigurationWithDefaults()
 	reg := internal.NewRegistry(conf)
 

diff --git a/pipeline/authn/authenticator_oauth2_client_credentials_cache_test.go b/pipeline/authn/authenticator_oauth2_client_credentials_cache_test.go
@@ -18,6 +18,7 @@ import (
 )
 
 func TestClientCredentialsCache(t *testing.T) {
+	t.Parallel()
 	ts := httptest.NewServer(httprouter.New())
 	t.Cleanup(ts.Close)
 

diff --git a/pipeline/authn/authenticator_oauth2_client_credentials_test.go b/pipeline/authn/authenticator_oauth2_client_credentials_test.go
@@ -51,6 +51,7 @@ func authOkDynamic(u string) *http.Request {
 }
 
 func TestAuthenticatorOAuth2ClientCredentials(t *testing.T) {
+	t.Parallel()
 	conf := internal.NewConfigurationWithDefaults(configx.SkipValidation())
 	reg := internal.NewRegistry(conf)
 

diff --git a/pipeline/authn/authenticator_oauth2_introspection_cache_test.go b/pipeline/authn/authenticator_oauth2_introspection_cache_test.go
@@ -15,6 +15,7 @@ import (
 )
 
 func TestCache(t *testing.T) {
+	t.Parallel()
 	logger := logrusx.New("", "")
 	c, err := configuration.NewKoanfProvider(
 		context.Background(),

diff --git a/pipeline/authn/authenticator_oauth2_introspection_test.go b/pipeline/authn/authenticator_oauth2_introspection_test.go
@@ -25,7 +25,6 @@ import (
 	"fmt"
 	"net/http"
 	"net/http/httptest"
-	"sync"
 	"testing"
 	"time"
 
@@ -581,12 +580,20 @@ func TestAuthenticatorOAuth2Introspection(t *testing.T) {
 	t.Run("method=authenticate-with-cache", func(t *testing.T) {
 		conf.SetForTest(t, "authenticators.oauth2_introspection.config.cache.enabled", true)
 
-		var didNotUseCache sync.WaitGroup
+		var handlerWasCalled bool
+		assertHandlerWasCalled := func(t *testing.T) {
+			assert.True(t, handlerWasCalled, "expected the handler to have been called")
+			handlerWasCalled = false
+		}
+		assertCacheWasUsed := func(t *testing.T) {
+			assert.False(t, handlerWasCalled, "expected the cache to have been used")
+			handlerWasCalled = false
+		}
 
 		setup := func(t *testing.T, config string) []byte {
 			router := httprouter.New()
 			router.POST("/oauth2/introspect", func(w http.ResponseWriter, r *http.Request, _ httprouter.Params) {
-				defer didNotUseCache.Done()
+				handlerWasCalled = true
 				require.NoError(t, r.ParseForm())
 				switch r.Form.Get("token") {
 				case "inactive-scope-b":
@@ -609,7 +616,7 @@ func TestAuthenticatorOAuth2Introspection(t *testing.T) {
 						Audience: []string{"audience"},
 						Issuer:   "foo",
 						Username: "username",
-						Expires:  time.Now().Add(time.Second).Unix(),
+						Expires:  time.Now().Add(2 * time.Second).Unix(),
 						Extra:    map[string]interface{}{"extra": "foo"},
 					}))
 				case "refresh-token":
@@ -647,10 +654,9 @@ func TestAuthenticatorOAuth2Introspection(t *testing.T) {
 			t.Run("case=initial request succeeds and caches", func(t *testing.T) {
 				config := setup(t, `{ "required_scope": [], "trusted_issuers": ["foo", "bar"], "target_audience": ["audience"] }`)
 
-				didNotUseCache.Add(1)
 				err = a.Authenticate(r, expected, config, nil)
-				didNotUseCache.Wait()
 				require.NoError(t, err)
+				assertHandlerWasCalled(t)
 			})
 
 			// We expect to use the cache here because we are not interested to validate the scope. Usually we would
@@ -660,30 +666,28 @@ func TestAuthenticatorOAuth2Introspection(t *testing.T) {
 				sess := new(AuthenticationSession)
 
 				err = a.Authenticate(r, sess, config, nil)
-				didNotUseCache.Wait() // Would result in a panic if wg.done was called!
 				require.NoError(t, err)
+				assertCacheWasUsed(t)
 				assertx.EqualAsJSON(t, expected, sess)
 			})
 
 			t.Run("case=second request does not use cache because scope strategy is disabled and scope was requested request succeeds", func(t *testing.T) {
 				config := setup(t, `{ "required_scope": ["scope-a"], "trusted_issuers": ["foo", "bar"], "target_audience": ["audience"] }`)
 				sess := new(AuthenticationSession)
 
-				didNotUseCache.Add(1)
 				err = a.Authenticate(r, sess, config, nil)
-				didNotUseCache.Wait()
 				require.NoError(t, err)
+				assertHandlerWasCalled(t)
 				assertx.EqualAsJSON(t, expected, sess)
 			})
 
 			t.Run("case=request fails because we requested a scope which the upstream does not validate", func(t *testing.T) {
 				config := setup(t, `{ "required_scope": ["scope-b"], "trusted_issuers": ["foo", "bar"], "target_audience": ["audience"] }`)
 				sess := new(AuthenticationSession)
 
-				didNotUseCache.Add(1)
 				err = a.Authenticate(r, sess, config, nil)
-				didNotUseCache.Wait()
 				require.Error(t, err)
+				assertHandlerWasCalled(t)
 			})
 		})
 
@@ -698,10 +702,10 @@ func TestAuthenticatorOAuth2Introspection(t *testing.T) {
 					config := setup(t, `{ "required_scope": ["scope-a"], "trusted_issuers": ["foo", "bar"], "target_audience": ["audience"] }`)
 
 					// Also doesn't use the cache the second time
-					didNotUseCache.Add(2)
 					require.Error(t, a.Authenticate(r, expected, config, nil))
+					assertHandlerWasCalled(t)
 					require.Error(t, a.Authenticate(r, expected, config, nil))
-					didNotUseCache.Wait()
+					assertHandlerWasCalled(t)
 				})
 			}
 		})
@@ -714,80 +718,75 @@ func TestAuthenticatorOAuth2Introspection(t *testing.T) {
 			// The initial request
 			config := setup(t, `{ "required_scope": ["scope-a"], "trusted_issuers": ["foo", "bar"], "target_audience": ["audience"] }`)
 
-			didNotUseCache.Add(1)
 			require.NoError(t, a.Authenticate(r, expected, config, nil))
-			didNotUseCache.Wait()
+			assertHandlerWasCalled(t)
 
 			t.Run("case=request succeeds and uses the cache", func(t *testing.T) {
 				config := setup(t, `{ "trusted_issuers": ["foo", "bar"], "target_audience": ["audience"] }`)
 				sess := new(AuthenticationSession)
 
 				err = a.Authenticate(r, sess, config, nil)
-				didNotUseCache.Wait()
 				require.NoError(t, err)
+				assertCacheWasUsed(t)
 				assertx.EqualAsJSON(t, expected, sess)
 			})
 
-			t.Run("case=request the initial request which also passes", func(t *testing.T) {
+			t.Run("case=cache the initial request which also passes", func(t *testing.T) {
 				config := setup(t, `{ "required_scope": ["scope-a"], "trusted_issuers": ["foo", "bar"], "target_audience": ["audience"] }`)
 				sess := new(AuthenticationSession)
 
 				err = a.Authenticate(r, sess, config, nil)
-				didNotUseCache.Wait()
 				require.NoError(t, err)
+				assertCacheWasUsed(t)
 				assertx.EqualAsJSON(t, expected, sess)
 			})
 
 			t.Run("case=requests a scope the token does not have", func(t *testing.T) {
 				require.Error(t, a.Authenticate(r, new(AuthenticationSession),
 					setup(t, `{ "required_scope": ["scope-b"], "trusted_issuers": ["foo", "bar"], "target_audience": ["audience"] }`),
 					nil))
-				didNotUseCache.Wait()
 			})
 
 			t.Run("case=requests an audience which the token does not have", func(t *testing.T) {
 				require.Error(t, a.Authenticate(r, new(AuthenticationSession),
 					setup(t, `{ "required_scope": ["scope-a"], "trusted_issuers": ["foo", "bar"], "target_audience": ["not-audience"] }`),
 					nil))
-				didNotUseCache.Wait()
 			})
 
 			t.Run("case=does not trust the issuer", func(t *testing.T) {
 				require.Error(t, a.Authenticate(r, new(AuthenticationSession),
 					setup(t, `{ "required_scope": ["scope-a"], "trusted_issuers": ["not-foo", "bar"], "target_audience": ["audience"] }`),
 					nil))
-				didNotUseCache.Wait()
 			})
 
 			t.Run("case=respects the expiry time", func(t *testing.T) {
 				setup(t, `{ "required_scope": ["scope-a"], "trusted_issuers": ["foo", "bar"], "target_audience": ["audience"] }`)
 				require.NoError(t, a.Authenticate(r, new(AuthenticationSession), config, nil))
-				time.Sleep(time.Second)
+				time.Sleep(2 * time.Second)
 				require.Error(t, a.Authenticate(r, new(AuthenticationSession), config, nil))
-				didNotUseCache.Wait()
 			})
 
 			t.Run("case=cache cleared after ttl", func(t *testing.T) {
 				//time.Sleep(time.Second)
 				config := setup(t, `{ "required_scope": ["scope-a"], "trusted_issuers": ["foo", "bar"], "target_audience": ["audience"], "cache": { "ttl": "100ms" } }`)
 
-				didNotUseCache.Add(1)
 				require.NoError(t, a.Authenticate(r, expected, config, nil))
-				didNotUseCache.Wait()
+				assertHandlerWasCalled(t)
 
 				// wait cache to save value
 				time.Sleep(time.Millisecond * 10)
 
 				require.NoError(t, a.Authenticate(r, new(AuthenticationSession), config, nil))
+				assertCacheWasUsed(t)
 				time.Sleep(50 * time.Millisecond)
 
 				require.NoError(t, a.Authenticate(r, new(AuthenticationSession), config, nil))
+				assertCacheWasUsed(t)
 				time.Sleep(50 * time.Millisecond)
 
 				// cache should have been cleared
-				didNotUseCache.Add(1)
 				require.NoError(t, a.Authenticate(r, new(AuthenticationSession), config, nil))
-				didNotUseCache.Wait()
+				assertHandlerWasCalled(t)
 			})
 		})
 	})

diff --git a/pipeline/authn/authenticator_test.go b/pipeline/authn/authenticator_test.go
@@ -17,6 +17,7 @@ const (
 )
 
 func TestSetHeader(t *testing.T) {
+	t.Parallel()
 	assert := assert.New(t)
 	for k, tc := range []struct {
 		a    *authn.AuthenticationSession
@@ -42,6 +43,7 @@ func TestSetHeader(t *testing.T) {
 }
 
 func TestCopy(t *testing.T) {
+	t.Parallel()
 	assert := assert.New(t)
 	original := &authn.AuthenticationSession{
 		Subject: "ab",

diff --git a/pipeline/authn/authenticator_unauthorized_test.go b/pipeline/authn/authenticator_unauthorized_test.go
@@ -32,6 +32,7 @@ import (
 )
 
 func TestAuthenticatorBroken(t *testing.T) {
+	t.Parallel()
 	conf := internal.NewConfigurationWithDefaults()
 	reg := internal.NewRegistry(conf)
 

diff --git a/pipeline/authz/remote_json.go b/pipeline/authz/remote_json.go
@@ -87,7 +87,7 @@ func (a *AuthorizerRemoteJSON) Authorize(r *http.Request, session *authn.Authent
 		return errors.Wrap(err, "payload is not a JSON text")
 	}
 
-	req, err := http.NewRequest("POST", c.Remote, &body)
+	req, err := http.NewRequestWithContext(r.Context(), "POST", c.Remote, &body)
 	if err != nil {
 		return errors.WithStack(err)
 	}

diff --git a/pipeline/authz/remote_json_test.go b/pipeline/authz/remote_json_test.go
@@ -3,10 +3,11 @@ package authz_test
 import (
 	"context"
 	"encoding/json"
-	"io/ioutil"
+	"io"
 	"net/http"
 	"net/http/httptest"
 	"testing"
+	"time"
 
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
@@ -91,7 +92,7 @@ func TestAuthorizerRemoteJSONAuthorize(t *testing.T) {
 					assert.Contains(t, r.Header["Content-Type"], "application/json")
 					assert.Contains(t, r.Header, "Authorization")
 					assert.Contains(t, r.Header["Authorization"], "Bearer token")
-					body, err := ioutil.ReadAll(r.Body)
+					body, err := io.ReadAll(r.Body)
 					require.NoError(t, err)
 					assert.Equal(t, string(body), "{}")
 					w.WriteHeader(http.StatusOK)
@@ -128,7 +129,7 @@ func TestAuthorizerRemoteJSONAuthorize(t *testing.T) {
 			name: "authentication session",
 			setup: func(t *testing.T) *httptest.Server {
 				return httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-					body, err := ioutil.ReadAll(r.Body)
+					body, err := io.ReadAll(r.Body)
 					require.NoError(t, err)
 					assert.Equal(t, string(body), `{"subject":"alice","extra":"bar","match":"baz"}`)
 					w.WriteHeader(http.StatusOK)
@@ -147,7 +148,7 @@ func TestAuthorizerRemoteJSONAuthorize(t *testing.T) {
 			name: "json array",
 			setup: func(t *testing.T) *httptest.Server {
 				return httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-					body, err := ioutil.ReadAll(r.Body)
+					body, err := io.ReadAll(r.Body)
 					require.NoError(t, err)
 					assert.Equal(t, string(body), `["foo","bar"]`)
 					w.WriteHeader(http.StatusOK)
@@ -168,17 +169,16 @@ func TestAuthorizerRemoteJSONAuthorize(t *testing.T) {
 			}
 
 			l := logrusx.New("", "")
-			p, err := configuration.NewKoanfProvider(
-				context.Background(), nil, l)
+			p, err := configuration.NewKoanfProvider(context.Background(), nil, l)
 			if err != nil {
 				l.WithError(err).Fatal("Failed to initialize configuration")
 			}
 			a := NewAuthorizerRemoteJSON(p)
-			r := &http.Request{
-				Header: map[string][]string{
-					"Authorization": {"Bearer token"},
-				},
-			}
+			ctx, cancel := context.WithTimeout(context.Background(), 1*time.Second)
+			defer cancel()
+			r, err := http.NewRequestWithContext(ctx, "", "", nil)
+			require.NoError(t, err)
+			r.Header = map[string][]string{"Authorization": {"Bearer token"}}
 			if err := a.Authorize(r, tt.session, tt.config, &rule.Rule{}); (err != nil) != tt.wantErr {
 				t.Errorf("Authorize() error = %v, wantErr %v", err, tt.wantErr)
 			}
@@ -249,7 +249,9 @@ func TestAuthorizerRemoteJSONValidate(t *testing.T) {
 		},
 	}
 	for _, tt := range tests {
+		tt := tt
 		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
 			p, err := configuration.NewKoanfProvider(
 				context.Background(), nil, logrusx.New("", ""),
 				configx.SkipValidation(),

diff --git a/pipeline/authz/remote_test.go b/pipeline/authz/remote_test.go
@@ -23,6 +23,7 @@ import (
 )
 
 func TestAuthorizerRemoteAuthorize(t *testing.T) {
+	t.Parallel()
 	tests := []struct {
 		name               string
 		setup              func(t *testing.T) *httptest.Server