Call to retrieve on a separate function436-fix-client-cache-test-failures

2 files changed, 14 insertions, 17 deletions

diff --git a/internal/source/gitlab/cache/cache_test.go b/internal/source/gitlab/cache/cache_test.go
index 52a3a489..c6e7d978 100644
--- a/internal/source/gitlab/cache/cache_test.go
+++ b/internal/source/gitlab/cache/cache_test.go
@@ -60,12 +60,13 @@ func (c *client) GetLookup(ctx context.Context, _ string) api.Lookup {
 	defer c.stats.bumpLookups()
 
 	lookup := api.Lookup{}
-	if c.failure == nil {
-		lookup.Name = <-c.domain
-	} else {
+	if c.failure != nil {
 		lookup.Error = c.failure
+		return lookup
 	}
 
+	lookup.Name = <-c.domain
+
 	return lookup
 }
 
@@ -74,12 +75,9 @@ func (c *client) Status() error {
 }
 
 func withTestCache(config resolverConfig, cacheConfig *cacheConfig, block func(*Cache, *client)) {
-	var chanSize int
-
+	chanSize := 0
 	if config.buffered {
 		chanSize = 1
-	} else {
-		chanSize = 0
 	}
 
 	resolver := &client{
@@ -252,12 +250,11 @@ func TestResolve(t *testing.T) {
 		withTestCache(resolverConfig{}, nil, func(cache *Cache, resolver *client) {
 			cache.withTestEntry(entryConfig{expired: false, retrieved: false}, func(entry *Entry) {
 				ctx, cancel := context.WithCancel(context.Background())
+				cancel() // <- test purpose
 
 				response := make(chan *api.Lookup, 1)
 				go func() { response <- cache.Resolve(ctx, "my.gitlab.com") }()
 
-				cancel()
-
 				resolver.domain <- "my.gitlab.com"
 				lookup := <-response
 
diff --git a/internal/source/gitlab/cache/entry.go b/internal/source/gitlab/cache/entry.go
index 162d0d3b..ca8b9817 100644
--- a/internal/source/gitlab/cache/entry.go
+++ b/internal/source/gitlab/cache/entry.go
@@ -65,14 +65,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 func() {
-			// `e.setResponse` closes the `e.retrieved` channel when done.
-			// allocating a new anonymous function gives the runtime enough time to process the context.Cancel
-			// for TestResolve/when_retrieval_failed_because_of_resolution_context_being_canceled
-			e.setResponse(e.retriever.Retrieve(e.domain))
-		}()
-	})
+	e.retrieve.Do(func() { go e.retrieveAndSetResponse() })
 
 	select {
 	case <-ctx.Done():
@@ -84,6 +77,13 @@ func (e *Entry) Retrieve(ctx context.Context, client api.Client) (lookup *api.Lo
 	return lookup
 }
 
+// retrieveAndSetResponse done on a separate function to avoid some intermittent failures
+// with TestResolve/when_retrieval_failed_because_of_resolution_context_being_canceled
+// see https://gitlab.com/gitlab-org/gitlab-pages/-/merge_requests/323#note_394716942
+func (e *Entry) retrieveAndSetResponse() {
+	e.setResponse(e.retriever.Retrieve(e.domain))
+}
+
 // Refresh will update the entry in the store only when it gets resolved.
 func (e *Entry) Refresh(client api.Client, store Store) {
 	e.refresh.Do(func() {