From 01e1f5433a5170e45fb90906a83e54effc3694a8 Mon Sep 17 00:00:00 2001 From: Simon Pasquier Date: Fri, 13 Jan 2023 11:58:56 +0100 Subject: [PATCH] test: reduce flakiness of acceptance tests The CI environment isn't as performant as local machines: the time needed to fully initialize the test environment can be significant and skew the verification. Rather than setting the "virtual" clock used to measure alert timings at the beginning of the acceptance test, it is better to wait for the test bed to be ready. Signed-off-by: Simon Pasquier --- test/cli/acceptance.go | 8 ++-- test/with_api_v1/acceptance.go | 47 +++++++++++---------- test/with_api_v2/acceptance.go | 47 ++++++++++----------- test/with_api_v2/acceptance/cluster_test.go | 12 +++--- 4 files changed, 59 insertions(+), 55 deletions(-) diff --git a/test/cli/acceptance.go b/test/cli/acceptance.go index cff3e9ee..a0bc09ac 100644 --- 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 index 90ea5348..4f5ecd15 100644 --- 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) - } - cas = append(cas, al) - } - - alertAPI := NewAlertAPI(am.client) - 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) + } + + alertAPI := NewAlertAPI(am.client) + 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 index 37214008..f01225a1 100644 --- 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 - } - cas = append(cas, alert) - } - 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) + } + 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 index 18bc3516..0d61efae 100644 --- 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) }