Merge pull request #3216 from simonpasquier/investigate-acceptance-te…

…st-failures test: reduce flakiness of acceptance tests
prometheus · Jan 18, 2023 · 0ba6441 · 0ba6441
2 parents ecb66f7 + 01e1f54
commit 0ba6441
Show file tree

Hide file tree

Showing 4 changed files with 56 additions and 52 deletions.
diff --git a/test/cli/acceptance.go b/test/cli/acceptance.go
@@ -92,10 +92,6 @@ func NewAcceptanceTest(t *testing.T, opts *AcceptanceOpts) *AcceptanceTest {
 		opts:    opts,
 		actions: map[float64][]func(){},
 	}
-	// TODO: Should this really be set during creation time? Why not do this
-	// during Run() time, maybe there is something else long happening between
-	// creation and running.
-	opts.baseTime = time.Now()
 
 	return test
 }
@@ -206,6 +202,10 @@ func (t *AcceptanceTest) Run() {
 		t.T.Fatal(err)
 	}
 
+	// Set the reference time right before running the test actions to avoid
+	// test failures due to slow setup of the test environment.
+	t.opts.baseTime = time.Now()
+
 	go t.runActions()
 
 	var latest float64

diff --git a/test/with_api_v1/acceptance.go b/test/with_api_v1/acceptance.go
@@ -80,7 +80,6 @@ func NewAcceptanceTest(t *testing.T, opts *AcceptanceOpts) *AcceptanceTest {
 		opts:    opts,
 		actions: map[float64][]func(){},
 	}
-	opts.baseTime = time.Now()
 
 	return test
 }
@@ -177,6 +176,10 @@ func (t *AcceptanceTest) Run() {
 		}(am)
 	}
 
+	// Set the reference time right before running the test actions to avoid
+	// test failures due to slow setup of the test environment.
+	t.opts.baseTime = time.Now()
+
 	go t.runActions()
 
 	var latest float64
@@ -333,28 +336,28 @@ func (am *Alertmanager) cleanup() {
 // Push declares alerts that are to be pushed to the Alertmanager
 // server at a relative point in time.
 func (am *Alertmanager) Push(at float64, alerts ...*TestAlert) {
-	var cas []APIV1Alert
-	for i := range alerts {
-		a := alerts[i].nativeAlert(am.opts)
-		al := APIV1Alert{
-			Labels:       LabelSet{},
-			Annotations:  LabelSet{},
-			StartsAt:     a.StartsAt,
-			EndsAt:       a.EndsAt,
-			GeneratorURL: a.GeneratorURL,
-		}
-		for n, v := range a.Labels {
-			al.Labels[LabelName(n)] = LabelValue(v)
-		}
-		for n, v := range a.Annotations {
-			al.Annotations[LabelName(n)] = LabelValue(v)
+	am.t.Do(at, func() {
+		var cas []APIV1Alert
+		for i := range alerts {
+			a := alerts[i].nativeAlert(am.opts)
+			al := APIV1Alert{
+				Labels:       LabelSet{},
+				Annotations:  LabelSet{},
+				StartsAt:     a.StartsAt,
+				EndsAt:       a.EndsAt,
+				GeneratorURL: a.GeneratorURL,
+			}
+			for n, v := range a.Labels {
+				al.Labels[LabelName(n)] = LabelValue(v)
+			}
+			for n, v := range a.Annotations {
+				al.Annotations[LabelName(n)] = LabelValue(v)
+			}
+			cas = append(cas, al)
 		}
-		cas = append(cas, al)
-	}
 
-	alertAPI := NewAlertAPI(am.client)
+		alertAPI := NewAlertAPI(am.client)
 
-	am.t.Do(at, func() {
 		if err := alertAPI.Push(context.Background(), cas...); err != nil {
 			am.t.Errorf("Error pushing %v: %s", cas, err)
 		}

diff --git a/test/with_api_v2/acceptance.go b/test/with_api_v2/acceptance.go
@@ -83,11 +83,6 @@ func NewAcceptanceTest(t *testing.T, opts *AcceptanceOpts) *AcceptanceTest {
 		opts:    opts,
 		actions: map[float64][]func(){},
 	}
-	// TODO: Should this really be set during creation time? Why not do this
-	// during Run() time, maybe there is something else long happening between
-	// creation and running.
-	opts.baseTime = time.Now()
-
 	return test
 }
 
@@ -185,6 +180,10 @@ func (t *AcceptanceTest) Run() {
 		t.T.Fatal(err)
 	}
 
+	// Set the reference time right before running the test actions to avoid
+	// test failures due to slow setup of the test environment.
+	t.opts.baseTime = time.Now()
+
 	go t.runActions()
 
 	var latest float64
@@ -420,26 +419,26 @@ func (amc *AlertmanagerCluster) Push(at float64, alerts ...*TestAlert) {
 // Push declares alerts that are to be pushed to the Alertmanager
 // server at a relative point in time.
 func (am *Alertmanager) Push(at float64, alerts ...*TestAlert) {
-	var cas models.PostableAlerts
-	for i := range alerts {
-		a := alerts[i].nativeAlert(am.opts)
-		alert := &models.PostableAlert{
-			Alert: models.Alert{
-				Labels:       a.Labels,
-				GeneratorURL: a.GeneratorURL,
-			},
-			Annotations: a.Annotations,
-		}
-		if a.StartsAt != nil {
-			alert.StartsAt = *a.StartsAt
-		}
-		if a.EndsAt != nil {
-			alert.EndsAt = *a.EndsAt
+	am.t.Do(at, func() {
+		var cas models.PostableAlerts
+		for i := range alerts {
+			a := alerts[i].nativeAlert(am.opts)
+			alert := &models.PostableAlert{
+				Alert: models.Alert{
+					Labels:       a.Labels,
+					GeneratorURL: a.GeneratorURL,
+				},
+				Annotations: a.Annotations,
+			}
+			if a.StartsAt != nil {
+				alert.StartsAt = *a.StartsAt
+			}
+			if a.EndsAt != nil {
+				alert.EndsAt = *a.EndsAt
+			}
+			cas = append(cas, alert)
 		}
-		cas = append(cas, alert)
-	}
 
-	am.t.Do(at, func() {
 		params := alert.PostAlertsParams{}
 		params.WithContext(context.Background()).WithAlerts(cas)
 

diff --git a/test/with_api_v2/acceptance/cluster_test.go b/test/with_api_v2/acceptance/cluster_test.go
@@ -77,15 +77,17 @@ receivers:
   - url: 'http://%s'
 `
 
-	acceptanceOpts := &a.AcceptanceOpts{
-		Tolerance: 2 * time.Second,
+	acceptanceOpts := func() *a.AcceptanceOpts {
+		return &a.AcceptanceOpts{
+			Tolerance: 2 * time.Second,
+		}
 	}
 
 	clusterSizes := []int{1, 3}
 
 	tests := []*a.AcceptanceTest{
-		a.NewAcceptanceTest(t, acceptanceOpts),
-		a.NewAcceptanceTest(t, acceptanceOpts),
+		a.NewAcceptanceTest(t, acceptanceOpts()),
+		a.NewAcceptanceTest(t, acceptanceOpts()),
 	}
 
 	collectors := []*a.Collector{}
@@ -118,7 +120,7 @@ receivers:
 
 	wg.Wait()
 
-	_, err := a.CompareCollectors(collectors[0], collectors[1], acceptanceOpts)
+	_, err := a.CompareCollectors(collectors[0], collectors[1], acceptanceOpts())
 	if err != nil {
 		t.Fatal(err)
 	}