Add default cache configuration and pass down to memstore and Entry.

Wrap global variables into one default struct.
Update test steps so that tests run every time so we  don't get
the cached results.

8 files changed, 147 insertions, 97 deletions

diff --git a/internal/source/gitlab/cache/cache.go b/internal/source/gitlab/cache/cache.go
index 9a4ab753..bb3567b4 100644
--- a/internal/source/gitlab/cache/cache.go
+++ b/internal/source/gitlab/cache/cache.go
@@ -2,22 +2,44 @@ package cache
 
 import (
 	"context"
+	"time"
 
 	"gitlab.com/gitlab-org/gitlab-pages/internal/source/gitlab/api"
 	"gitlab.com/gitlab-org/gitlab-pages/metrics"
 )
 
+var defaultCacheConfig = cacheConfig{
+	cacheExpiry:          10 * time.Minute,
+	entryRefreshTimeout:  30 * time.Second,
+	retrievalTimeout:     5 * time.Second,
+	maxRetrievalInterval: time.Second,
+	maxRetrievalRetries:  3,
+}
+
 // Cache is a short and long caching mechanism for GitLab source
 type Cache struct {
-	client api.Client
-	store  Store
+	client      api.Client
+	store       Store
+	cacheConfig *cacheConfig
+}
+
+type cacheConfig struct {
+	cacheExpiry          time.Duration
+	entryRefreshTimeout  time.Duration
+	retrievalTimeout     time.Duration
+	maxRetrievalInterval time.Duration
+	maxRetrievalRetries  int
 }
 
 // NewCache creates a new instance of Cache.
-func NewCache(client api.Client) *Cache {
+func NewCache(client api.Client, cc *cacheConfig) *Cache {
+	if cc == nil {
+		cc = &defaultCacheConfig
+	}
+
 	return &Cache{
 		client: client,
-		store:  newMemStore(),
+		store:  newMemStore(client, cc),
 	}
 }
 
@@ -25,8 +47,8 @@ func NewCache(client api.Client) *Cache {
 // algorithm works as follows:
 // - We first check if the cache entry exists, and if it is up-to-date. If it
 //   is fresh we return the lookup entry from cache and it is a cache hit.
-// - If entry is not up-to-date, what means that it has been created in a cache
-//   more than `shortCacheExpiry` duration ago,  we schedule an asynchronous
+// - If entry is not up-to-date, that means it has been created in a cache
+//   more than `entryRefreshTimeout` duration ago,  we schedule an asynchronous
 //   retrieval of the latest configuration we are going to obtain through the
 //   API, and we immediately return an old value, to avoid blocking clients. In
 //   this case it is also a cache hit.
diff --git a/internal/source/gitlab/cache/cache_test.go b/internal/source/gitlab/cache/cache_test.go
index a0ee507d..c3e03dec 100644
--- a/internal/source/gitlab/cache/cache_test.go
+++ b/internal/source/gitlab/cache/cache_test.go
@@ -69,7 +69,7 @@ func (c *client) GetLookup(ctx context.Context, _ string) api.Lookup {
 	return lookup
 }
 
-func withTestCache(config resolverConfig, block func(*Cache, *client)) {
+func withTestCache(config resolverConfig, cacheConfig *cacheConfig, block func(*Cache, *client)) {
 	var chanSize int
 
 	if config.buffered {
@@ -84,7 +84,7 @@ func withTestCache(config resolverConfig, block func(*Cache, *client)) {
 		failure: config.failure,
 	}
 
-	cache := NewCache(resolver)
+	cache := NewCache(resolver, cacheConfig)
 
 	block(cache, resolver)
 }
@@ -122,7 +122,7 @@ type entryConfig struct {
 
 func TestResolve(t *testing.T) {
 	t.Run("when item is not cached", func(t *testing.T) {
-		withTestCache(resolverConfig{buffered: true}, func(cache *Cache, resolver *client) {
+		withTestCache(resolverConfig{buffered: true}, nil, func(cache *Cache, resolver *client) {
 			resolver.domain <- "my.gitlab.com"
 
 			lookup := cache.Resolve(context.Background(), "my.gitlab.com")
@@ -134,7 +134,7 @@ func TestResolve(t *testing.T) {
 	})
 
 	t.Run("when item is not cached and accessed multiple times", func(t *testing.T) {
-		withTestCache(resolverConfig{}, func(cache *Cache, resolver *client) {
+		withTestCache(resolverConfig{}, nil, func(cache *Cache, resolver *client) {
 			wg := &sync.WaitGroup{}
 			ctx := context.Background()
 
@@ -158,7 +158,7 @@ func TestResolve(t *testing.T) {
 	})
 
 	t.Run("when item is in short cache", func(t *testing.T) {
-		withTestCache(resolverConfig{}, func(cache *Cache, resolver *client) {
+		withTestCache(resolverConfig{}, nil, func(cache *Cache, resolver *client) {
 			cache.withTestEntry(entryConfig{expired: false, retrieved: true}, func(*Entry) {
 				lookup := cache.Resolve(context.Background(), "my.gitlab.com")
 
@@ -169,7 +169,7 @@ func TestResolve(t *testing.T) {
 	})
 
 	t.Run("when a non-retrieved new item is in short cache", func(t *testing.T) {
-		withTestCache(resolverConfig{}, func(cache *Cache, resolver *client) {
+		withTestCache(resolverConfig{}, nil, func(cache *Cache, resolver *client) {
 			cache.withTestEntry(entryConfig{expired: false, retrieved: false}, func(*Entry) {
 				lookup := make(chan *api.Lookup, 1)
 
@@ -192,7 +192,7 @@ func TestResolve(t *testing.T) {
 	})
 
 	t.Run("when item is in long cache only", func(t *testing.T) {
-		withTestCache(resolverConfig{}, func(cache *Cache, resolver *client) {
+		withTestCache(resolverConfig{}, nil, func(cache *Cache, resolver *client) {
 			cache.withTestEntry(entryConfig{expired: true, retrieved: true}, func(*Entry) {
 				lookup := cache.Resolve(context.Background(), "my.gitlab.com")
 
@@ -206,7 +206,7 @@ func TestResolve(t *testing.T) {
 	})
 
 	t.Run("when item in long cache is requested multiple times", func(t *testing.T) {
-		withTestCache(resolverConfig{}, func(cache *Cache, resolver *client) {
+		withTestCache(resolverConfig{}, nil, func(cache *Cache, resolver *client) {
 			cache.withTestEntry(entryConfig{expired: true, retrieved: true}, func(*Entry) {
 				cache.Resolve(context.Background(), "my.gitlab.com")
 				cache.Resolve(context.Background(), "my.gitlab.com")
@@ -221,9 +221,10 @@ func TestResolve(t *testing.T) {
 	})
 
 	t.Run("when retrieval failed with an error", func(t *testing.T) {
-		withTestCache(resolverConfig{failure: errors.New("500 err")}, func(cache *Cache, resolver *client) {
-			maxRetrievalInterval = 0
+		cc := defaultCacheConfig
+		cc.maxRetrievalInterval = 0
 
+		withTestCache(resolverConfig{failure: errors.New("500 err")}, &cc, func(cache *Cache, resolver *client) {
 			lookup := cache.Resolve(context.Background(), "my.gitlab.com")
 
 			require.Equal(t, uint64(3), resolver.stats.getLookups())
@@ -235,7 +236,7 @@ func TestResolve(t *testing.T) {
 		ctx, cancel := context.WithCancel(context.Background())
 		cancel()
 
-		withTestCache(resolverConfig{}, func(cache *Cache, resolver *client) {
+		withTestCache(resolverConfig{}, nil, func(cache *Cache, resolver *client) {
 			lookup := cache.Resolve(ctx, "my.gitlab.com")
 
 			require.Equal(t, uint64(0), resolver.stats.getLookups())
@@ -244,12 +245,10 @@ func TestResolve(t *testing.T) {
 	})
 
 	t.Run("when retrieval failed because of an internal retriever context timeout", func(t *testing.T) {
-		t.Skip("Data race")
+		cc := defaultCacheConfig
+		cc.retrievalTimeout = 0
 
-		retrievalTimeout = 0
-		defer func() { retrievalTimeout = 5 * time.Second }()
-
-		withTestCache(resolverConfig{}, func(cache *Cache, resolver *client) {
+		withTestCache(resolverConfig{}, &cc, func(cache *Cache, resolver *client) {
 			lookup := cache.Resolve(context.Background(), "my.gitlab.com")
 
 			require.Equal(t, uint64(0), resolver.stats.getLookups())
@@ -258,7 +257,7 @@ func TestResolve(t *testing.T) {
 	})
 
 	t.Run("when retrieval failed because of resolution context being canceled", func(t *testing.T) {
-		withTestCache(resolverConfig{}, func(cache *Cache, resolver *client) {
+		withTestCache(resolverConfig{}, nil, func(cache *Cache, resolver *client) {
 			cache.withTestEntry(entryConfig{expired: false, retrieved: false}, func(entry *Entry) {
 				ctx, cancel := context.WithCancel(context.Background())
 
diff --git a/internal/source/gitlab/cache/const.go b/internal/source/gitlab/cache/const.go
deleted file mode 100644
index 90c5bddc..00000000
--- a/internal/source/gitlab/cache/const.go
+++ /dev/null
@@ -1,11 +0,0 @@
-package cache
-
-import "time"
-
-var (
-	shortCacheExpiry     = 30 * time.Second
-	longCacheExpiry      = 10 * time.Minute
-	retrievalTimeout     = 5 * time.Second
-	maxRetrievalInterval = time.Second
-	maxRetrievalRetries  = 3
-)
diff --git a/internal/source/gitlab/cache/entry.go b/internal/source/gitlab/cache/entry.go
index d91bb331..191ef789 100644
--- a/internal/source/gitlab/cache/entry.go
+++ b/internal/source/gitlab/cache/entry.go
@@ -13,23 +13,28 @@ import (
 // holds a pointer to *api.Lookup when the domain lookup has been retrieved
 // successfully
 type Entry struct {
-	domain    string
-	created   time.Time
-	retrieve  *sync.Once
-	refresh   *sync.Once
-	mux       *sync.RWMutex
-	retrieved chan struct{}
-	response  *api.Lookup
+	domain         string
+	created        time.Time
+	retrieve       *sync.Once
+	refresh        *sync.Once
+	mux            *sync.RWMutex
+	retrieved      chan struct{}
+	response       *api.Lookup
+	refreshTimeout time.Duration
+	retriever      *Retriever
 }
 
-func newCacheEntry(domain string) *Entry {
+func newCacheEntry(domain string, refreshTimeout time.Duration, retriever *Retriever) *Entry {
+
 	return &Entry{
-		domain:    domain,
-		created:   time.Now(),
-		retrieve:  &sync.Once{},
-		refresh:   &sync.Once{},
-		mux:       &sync.RWMutex{},
-		retrieved: make(chan struct{}),
+		domain:         domain,
+		created:        time.Now(),
+		retrieve:       &sync.Once{},
+		refresh:        &sync.Once{},
+		mux:            &sync.RWMutex{},
+		retrieved:      make(chan struct{}),
+		refreshTimeout: refreshTimeout,
+		retriever:      retriever,
 	}
 }
 
@@ -61,7 +66,7 @@ func (e *Entry) Lookup() *api.Lookup {
 
 // Retrieve perform a blocking retrieval of the cache entry response.
 func (e *Entry) Retrieve(ctx context.Context, client api.Client) (lookup *api.Lookup) {
-	e.retrieve.Do(func() { go e.retrieveWithClient(client) })
+	e.retrieve.Do(func() { go e.setResponse(e.retriever.Retrieve(e.domain)) })
 
 	select {
 	case <-ctx.Done():
@@ -77,7 +82,7 @@ func (e *Entry) Retrieve(ctx context.Context, client api.Client) (lookup *api.Lo
 func (e *Entry) Refresh(client api.Client, store Store) {
 	e.refresh.Do(func() {
 		go func() {
-			entry := newCacheEntry(e.domain)
+			entry := newCacheEntry(e.domain, e.refreshTimeout, e.retriever)
 
 			entry.Retrieve(context.Background(), client)
 
@@ -86,12 +91,6 @@ func (e *Entry) Refresh(client api.Client, store Store) {
 	})
 }
 
-func (e *Entry) retrieveWithClient(client api.Client) {
-	retriever := Retriever{client: client}
-
-	e.setResponse(retriever.Retrieve(e.domain))
-}
-
 func (e *Entry) setResponse(lookup api.Lookup) {
 	e.mux.Lock()
 	defer e.mux.Unlock()
@@ -101,7 +100,7 @@ func (e *Entry) setResponse(lookup api.Lookup) {
 }
 
 func (e *Entry) isExpired() bool {
-	return time.Since(e.created) > shortCacheExpiry
+	return time.Since(e.created) > e.refreshTimeout
 }
 
 func (e *Entry) isResolved() bool {
diff --git a/internal/source/gitlab/cache/entry_test.go b/internal/source/gitlab/cache/entry_test.go
index 118981d0..e6a86557 100644
--- a/internal/source/gitlab/cache/entry_test.go
+++ b/internal/source/gitlab/cache/entry_test.go
@@ -10,35 +10,55 @@ import (
 )
 
 func TestIsUpToDateAndNeedsRefresh(t *testing.T) {
-	t.Run("when is resolved and not expired", func(t *testing.T) {
-		entry := newCacheEntry("my.gitlab.com")
-		entry.response = &api.Lookup{}
+	tests := []struct {
+		name                string
+		resolved            bool
+		expired             bool
+		expectedIsUpToDate  bool
+		expectedNeedRefresh bool
+	}{
+		{
+			name:                "resolved_and_not_expired",
+			resolved:            true,
+			expired:             false,
+			expectedIsUpToDate:  true,
+			expectedNeedRefresh: false,
+		},
+		{
+			name:                "resolved_and_expired",
+			resolved:            true,
+			expired:             true,
+			expectedIsUpToDate:  false,
+			expectedNeedRefresh: true,
+		},
+		{
+			name:                "not_resolved_and_not_expired",
+			resolved:            false,
+			expired:             false,
+			expectedIsUpToDate:  false,
+			expectedNeedRefresh: false,
+		},
+		{
+			name:                "not_resolved_and_expired",
+			resolved:            false,
+			expired:             true,
+			expectedIsUpToDate:  false,
+			expectedNeedRefresh: false,
+		},
+	}
 
-		require.True(t, entry.IsUpToDate())
-		require.False(t, entry.NeedsRefresh())
-	})
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			entry := newCacheEntry("my.gitlab.com", defaultCacheConfig.entryRefreshTimeout, nil)
+			if tt.resolved {
+				entry.response = &api.Lookup{}
+			}
+			if tt.expired {
+				entry.created = time.Now().Add(-time.Hour)
+			}
 
-	t.Run("when is resolved and is expired", func(t *testing.T) {
-		entry := newCacheEntry("my.gitlab.com")
-		entry.response = &api.Lookup{}
-		entry.created = time.Now().Add(-time.Hour)
-
-		require.False(t, entry.IsUpToDate())
-		require.True(t, entry.NeedsRefresh())
-	})
-
-	t.Run("when is not resolved and not expired", func(t *testing.T) {
-		entry := newCacheEntry("my.gitlab.com")
-
-		require.False(t, entry.IsUpToDate())
-		require.False(t, entry.NeedsRefresh())
-	})
-
-	t.Run("when is not resolved and is expired", func(t *testing.T) {
-		entry := newCacheEntry("my.gitlab.com")
-		entry.created = time.Now().Add(-time.Hour)
-
-		require.False(t, entry.IsUpToDate())
-		require.False(t, entry.NeedsRefresh())
-	})
+			require.Equal(t, tt.expectedIsUpToDate, entry.IsUpToDate())
+			require.Equal(t, tt.expectedNeedRefresh, entry.NeedsRefresh())
+		})
+	}
 }
diff --git a/internal/source/gitlab/cache/memstore.go b/internal/source/gitlab/cache/memstore.go
index 08eccd94..1d7c678d 100644
--- a/internal/source/gitlab/cache/memstore.go
+++ b/internal/source/gitlab/cache/memstore.go
@@ -5,17 +5,24 @@ import (
 	"time"
 
 	cache "github.com/patrickmn/go-cache"
+
+	"gitlab.com/gitlab-org/gitlab-pages/internal/source/gitlab/api"
 )
 
 type memstore struct {
-	store *cache.Cache
-	mux   *sync.RWMutex
+	store               *cache.Cache
+	mux                 *sync.RWMutex
+	retriever           *Retriever
+	entryRefreshTimeout time.Duration
 }
 
-func newMemStore() Store {
+func newMemStore(client api.Client, cc *cacheConfig) Store {
+	retriever := NewRetriever(client, cc.retrievalTimeout, cc.maxRetrievalInterval, cc.maxRetrievalRetries)
 	return &memstore{
-		store: cache.New(longCacheExpiry, time.Minute),
-		mux:   &sync.RWMutex{},
+		store:               cache.New(cc.cacheExpiry, time.Minute),
+		mux:                 &sync.RWMutex{},
+		retriever:           retriever,
+		entryRefreshTimeout: cc.entryRefreshTimeout,
 	}
 }
 
@@ -37,7 +44,7 @@ func (m *memstore) LoadOrCreate(domain string) *Entry {
 		return entry.(*Entry)
 	}
 
-	newEntry := newCacheEntry(domain)
+	newEntry := newCacheEntry(domain, m.entryRefreshTimeout, m.retriever)
 	m.store.SetDefault(domain, newEntry)
 
 	return newEntry
diff --git a/internal/source/gitlab/cache/retriever.go b/internal/source/gitlab/cache/retriever.go
index cf0541b1..de37c231 100644
--- a/internal/source/gitlab/cache/retriever.go
+++ b/internal/source/gitlab/cache/retriever.go
@@ -13,13 +13,26 @@ import (
 // Retriever is an utility type that performs an HTTP request with backoff in
 // case of errors
 type Retriever struct {
-	client api.Client
+	client               api.Client
+	retrievalTimeout     time.Duration
+	maxRetrievalInterval time.Duration
+	maxRetrievalRetries  int
+}
+
+// NewRetriever creates a Retriever with a client
+func NewRetriever(client api.Client, retrievalTimeout, maxRetrievalInterval time.Duration, maxRetrievalRetries int) *Retriever {
+	return &Retriever{
+		client:               client,
+		retrievalTimeout:     retrievalTimeout,
+		maxRetrievalInterval: maxRetrievalInterval,
+		maxRetrievalRetries:  maxRetrievalRetries,
+	}
 }
 
 // Retrieve retrieves a lookup response from external source with timeout and
 // backoff. It has its own context with timeout.
 func (r *Retriever) Retrieve(domain string) (lookup api.Lookup) {
-	ctx, cancel := context.WithTimeout(context.Background(), retrievalTimeout)
+	ctx, cancel := context.WithTimeout(context.Background(), r.retrievalTimeout)
 	defer cancel()
 
 	select {
@@ -39,11 +52,11 @@ func (r *Retriever) resolveWithBackoff(ctx context.Context, domain string) <-cha
 	go func() {
 		var lookup api.Lookup
 
-		for i := 1; i <= maxRetrievalRetries; i++ {
+		for i := 1; i <= r.maxRetrievalRetries; i++ {
 			lookup = r.client.GetLookup(ctx, domain)
 
 			if lookup.Error != nil {
-				time.Sleep(maxRetrievalInterval)
+				time.Sleep(r.maxRetrievalInterval)
 			} else {
 				break
 			}
diff --git a/internal/source/gitlab/gitlab.go b/internal/source/gitlab/gitlab.go
index 6260200a..12da9af1 100644
--- a/internal/source/gitlab/gitlab.go
+++ b/internal/source/gitlab/gitlab.go
@@ -28,7 +28,8 @@ func New(config client.Config) (*Gitlab, error) {
 		return nil, err
 	}
 
-	return &Gitlab{client: cache.NewCache(client)}, nil
+	// using nil for cache config will use the default values specified in internal/source/gitlab/cache/cache.go#12
+	return &Gitlab{client: cache.NewCache(client, nil)}, nil
 }
 
 // GetDomain return a representation of a domain that we have fetched from