Fixing flaky tests (grafana#3563)

* Fixing test race condition

* Fixing test race condition

* Fixing other test race condition

* Better fix for race condition grafana#1

* Attempt to reduce test flakyness

component/loki/source/file/file_test.go

@@ -2,8 +2,10 @@ package file
 
 import (
 	"context"
+	"errors"
 	"log"
 	"os"
+	"path/filepath"
 	"testing"
 	"time"
 
@@ -102,7 +104,6 @@ func TestTwoTargets(t *testing.T) {
 	require.NoError(t, err)
 
 	ctx, cancel := context.WithCancel(context.Background())
-	defer cancel()
 	go c.Run(ctx)
 	time.Sleep(100 * time.Millisecond)
 
@@ -129,4 +130,19 @@ func TestTwoTargets(t *testing.T) {
 	}
 	require.True(t, foundF1)
 	require.True(t, foundF2)
+	cancel()
+	// Verify that positions.yml is written. NOTE: if we didn't wait for it, there would be a race condition between
+	// temporary directory being cleaned up and this file being created.
+	require.Eventually(
+		t,
+		func() bool {
+			if _, err := os.Stat(filepath.Join(opts.DataPath, "positions.yml")); errors.Is(err, os.ErrNotExist) {
+				return false
+			}
+			return true
+		},
+		5*time.Second,
+		10*time.Millisecond,
+		"expected positions.yml file to be written eventually",
+	)
 }

component/module/file/file.go

@@ -154,10 +154,16 @@ func (c *Component) Handler() http.Handler {
 
 // CurrentHealth implements component.HealthComponent.
 func (c *Component) CurrentHealth() component.Health {
-	return component.LeastHealthy(
+	leastHealthy := component.LeastHealthy(
 		c.managedLocalFile.CurrentHealth(),
 		c.mod.CurrentHealth(),
 	)
+
+	// if both components are healthy - return c.mod's health, so we can have a stable Health.Message.
+	if leastHealthy.Health == component.HealthTypeHealthy {
+		return c.mod.CurrentHealth()
+	}
+	return leastHealthy
 }
 
 // getArgs is a goroutine safe way to get args

component/module/file/file_test.go

@@ -22,12 +22,12 @@ func TestModule(t *testing.T) {
 	os.WriteFile(debugLevelFilePath, []byte("info"), 0664)
 
 	tt := []struct {
-		name                              string
-		moduleContents                    string
-		expectedHealthType                component.HealthType
-		expectedHealthMessagePrefix       string
-		expectedModuleHealthType          component.HealthType
-		expectedModuleHealthMessagePrefix string
+		name                                   string
+		moduleContents                         string
+		expectedHealthType                     component.HealthType
+		expectedHealthMessagePrefix            string
+		expectedManagedFileHealthType          component.HealthType
+		expectedManagedFileHealthMessagePrefix string
 	}{
 		{
 			name: "Good Module",
@@ -38,8 +38,8 @@ func TestModule(t *testing.T) {
 			expectedHealthType:          component.HealthTypeHealthy,
 			expectedHealthMessagePrefix: "module content loaded",
 
-			expectedModuleHealthType:          component.HealthTypeHealthy,
-			expectedModuleHealthMessagePrefix: "read file",
+			expectedManagedFileHealthType:          component.HealthTypeHealthy,
+			expectedManagedFileHealthMessagePrefix: "read file",
 		},
 	}
 
@@ -65,13 +65,21 @@ func TestModule(t *testing.T) {
 			require.NoError(t, err)
 
 			go c.Run(context.Background())
-			time.Sleep(200 * time.Millisecond)
+			require.Eventually(
+				t,
+				func() bool { return tc.expectedHealthType == c.CurrentHealth().Health },
+				5*time.Second,
+				50*time.Millisecond,
+				"did not reach required health status before timeout: %v != %v",
+				tc.expectedHealthType,
+				c.CurrentHealth().Health,
+			)
 
 			require.Equal(t, tc.expectedHealthType, c.CurrentHealth().Health)
-			require.True(t, strings.HasPrefix(c.CurrentHealth().Message, tc.expectedHealthMessagePrefix))
+			requirePrefix(t, c.CurrentHealth().Message, tc.expectedHealthMessagePrefix)
 
-			require.Equal(t, tc.expectedModuleHealthType, c.managedLocalFile.CurrentHealth().Health)
-			require.True(t, strings.HasPrefix(c.managedLocalFile.CurrentHealth().Message, tc.expectedModuleHealthMessagePrefix))
+			require.Equal(t, tc.expectedManagedFileHealthType, c.managedLocalFile.CurrentHealth().Health)
+			requirePrefix(t, c.managedLocalFile.CurrentHealth().Message, tc.expectedManagedFileHealthMessagePrefix)
 		})
 	}
 }
@@ -132,3 +140,13 @@ func riverEscape(filePath string) string {
 
 	return filePath
 }
+
+func requirePrefix(t *testing.T, s string, prefix string) {
+	require.True(
+		t,
+		strings.HasPrefix(s, prefix),
+		"expected '%v' to have '%v' prefix",
+		s,
+		prefix,
+	)
+}

pkg/util/eventually.go

@@ -11,7 +11,7 @@ import (
 var backoffRetry = backoff.Config{
 	MinBackoff: 10 * time.Millisecond,
 	MaxBackoff: 1 * time.Second,
-	MaxRetries: 5,
+	MaxRetries: 10,
 }
 
 // Eventually calls the check function several times until it doesn't report an