From 95435c7cdb8d6ccdaa14692785cd3dab8da54ba2 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Giedrius=20Statkevi=C4=8Dius?= <giedriuswork@gmail.com>
Date: Sun, 14 Feb 2021 23:30:54 +0200
Subject: [PATCH] reloader: try to fix test flakiness
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

This is an attempt of fixing the `TestReloader_DirectoriesApply` test
flakiness. I call this an attempt because I cannot reproduce it locally.
However, I have noticed that during runs where this fails the logs look
like this:

```
--- FAIL: TestReloader_DirectoriesApply (3.04s)
    reloader_test.go:256: Performing step number 0
    reloader_test.go:256: Performing step number 1
    reloader_test.go:256: Performing step number 2
    reloader_test.go:256: Performing step number 3
    reloader_test.go:256: Performing step number 4
    reloader_test.go:256: Performing step number 6
    reloader_test.go:343: reloader_test.go:343:

        	exp: 5

        	got: 6
```

It immediately jumps to another value. This gave me a hint and I think
that this is happening because potentially `i` can be written to/read
from by multiple goroutines. On very resource constrained systems like
the CircleCI runners, it could just happen that the `if` doesn't do what
it is supposed to.

Try to fix this problem by protecting the whole HTTP handler with a
mutex. Since this is only a test and not a performance critical path, I
think this is a reasonable change to do.

Add extra check for reload failures.

Signed-off-by: Giedrius Statkevičius <giedriuswork@gmail.com>
---
 pkg/reloader/reloader_test.go | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/pkg/reloader/reloader_test.go b/pkg/reloader/reloader_test.go
index e8cbdade64a..0c2567737eb 100644
--- a/pkg/reloader/reloader_test.go
+++ b/pkg/reloader/reloader_test.go
@@ -20,6 +20,8 @@ import (
 	"time"
 
 	"github.com/go-kit/kit/log"
+	"github.com/prometheus/client_golang/prometheus"
+	promtest "github.com/prometheus/client_golang/prometheus/testutil"
 	"go.uber.org/atomic"
 	"go.uber.org/goleak"
 
@@ -168,11 +170,15 @@ func TestReloader_DirectoriesApply(t *testing.T) {
 	l, err := net.Listen("tcp", "localhost:0")
 	testutil.Ok(t, err)
 
-	reloads := &atomic.Value{}
-	reloads.Store(0)
 	i := 0
+	reloads := 0
+	reloadsMtx := sync.Mutex{}
+
 	srv := &http.Server{}
 	srv.Handler = http.HandlerFunc(func(resp http.ResponseWriter, r *http.Request) {
+		reloadsMtx.Lock()
+		defer reloadsMtx.Unlock()
+
 		i++
 		if i%2 == 0 {
 			// Fail every second request to ensure that retry works.
@@ -180,7 +186,7 @@ func TestReloader_DirectoriesApply(t *testing.T) {
 			return
 		}
 
-		reloads.Store(reloads.Load().(int) + 1) // The only writer.
+		reloads++
 		resp.WriteHeader(http.StatusOK)
 	})
 	go func() {
@@ -202,9 +208,10 @@ func TestReloader_DirectoriesApply(t *testing.T) {
 	testutil.Ok(t, os.Symlink(path.Join(dir2, "rule-dir"), path.Join(dir, "rule-dir")))
 
 	logger := log.NewNopLogger()
+	r := prometheus.NewRegistry()
 	reloader := New(
 		logger,
-		nil,
+		r,
 		&Options{
 			ReloadURL:     reloadURL,
 			CfgFile:       "",
@@ -246,7 +253,7 @@ func TestReloader_DirectoriesApply(t *testing.T) {
 			case <-time.After(500 * time.Millisecond):
 			}
 
-			rel := reloads.Load().(int)
+			rel := reloads
 			if init && rel <= reloadsSeen {
 				continue
 			}
@@ -340,7 +347,8 @@ func TestReloader_DirectoriesApply(t *testing.T) {
 	g.Wait()
 
 	testutil.Ok(t, err)
-	testutil.Equals(t, 5, reloads.Load().(int))
+	testutil.Equals(t, 4.0, promtest.ToFloat64(reloader.reloadErrors))
+	testutil.Equals(t, 5, reloads)
 }
 
 func TestReloaderDirectoriesApplyBasedOnWatchInterval(t *testing.T) {