From c39a913f8a9c9805bebf4a0554c35ec0ca63aaed Mon Sep 17 00:00:00 2001 From: pasquier-s Date: Tue, 27 Feb 2018 18:18:53 +0100 Subject: [PATCH] test: enable race detection (#1262) This change enables race detection when running the tests. It also fixes a couple of existing race conditions. --- Makefile | 2 +- dispatch/dispatch_test.go | 22 ++++++++++++---------- test/acceptance.go | 30 ++++++++++++++++++++++-------- test/collector.go | 11 +++++++++++ test/mock.go | 19 ++++++++++++++++++- 5 files changed, 64 insertions(+), 20 deletions(-) diff --git a/Makefile b/Makefile index 9e76608f26..6cc31cd047 100644 --- a/Makefile +++ b/Makefile @@ -30,7 +30,7 @@ all: format build test test: @echo ">> running tests" - @$(GO) test -short $(pkgs) + @$(GO) test -race -short $(pkgs) style: @echo ">> checking code style" diff --git a/dispatch/dispatch_test.go b/dispatch/dispatch_test.go index c0e532cad6..12c3680a1a 100644 --- a/dispatch/dispatch_test.go +++ b/dispatch/dispatch_test.go @@ -192,7 +192,7 @@ func TestAggrGroup(t *testing.T) { case batch := <-alertsCh: if s := time.Since(last); s < opts.GroupWait { - t.Fatalf("received batch to early after %v", s) + t.Fatalf("received batch too early after %v", s) } exp := types.AlertSlice{a1} sort.Sort(batch) @@ -212,7 +212,7 @@ func TestAggrGroup(t *testing.T) { case batch := <-alertsCh: if s := time.Since(last); s < opts.GroupInterval { - t.Fatalf("received batch to early after %v", s) + t.Fatalf("received batch too early after %v", s) } exp := types.AlertSlice{a1, a3} sort.Sort(batch) @@ -262,7 +262,7 @@ func TestAggrGroup(t *testing.T) { s := time.Since(last) lastCurMtx.Unlock() if s < opts.GroupInterval { - t.Fatalf("received batch to early after %v", s) + t.Fatalf("received batch too early after %v", s) } exp := types.AlertSlice{a1, a2, a3} sort.Sort(batch) @@ -274,9 +274,12 @@ func TestAggrGroup(t *testing.T) { } // Resolve all alerts, they should be removed after the next batch was sent. - a1.EndsAt = time.Now() - a2.EndsAt = time.Now() - a3.EndsAt = time.Now() + a1r, a2r, a3r := *a1, *a2, *a3 + resolved := types.AlertSlice{&a1r, &a2r, &a3r} + for _, a := range resolved { + a.EndsAt = time.Now() + ag.insert(a) + } select { case <-time.After(2 * opts.GroupInterval): @@ -284,13 +287,12 @@ func TestAggrGroup(t *testing.T) { case batch := <-alertsCh: if s := time.Since(last); s < opts.GroupInterval { - t.Fatalf("received batch to early after %v", s) + t.Fatalf("received batch too early after %v", s) } - exp := types.AlertSlice{a1, a2, a3} sort.Sort(batch) - if !reflect.DeepEqual(batch, exp) { - t.Fatalf("expected alerts %v but got %v", exp, batch) + if !reflect.DeepEqual(batch, resolved) { + t.Fatalf("expected alerts %v but got %v", resolved, batch) } if !ag.empty() { diff --git a/test/acceptance.go b/test/acceptance.go index 23b496fc15..5e009dcceb 100644 --- a/test/acceptance.go +++ b/test/acceptance.go @@ -167,6 +167,8 @@ func (t *AcceptanceTest) Run() { defer func(am *Alertmanager) { am.Terminate() am.cleanup() + t.Logf("stdout:\n%v", am.cmd.Stdout) + t.Logf("stderr:\n%v", am.cmd.Stderr) }(am) } @@ -192,11 +194,6 @@ func (t *AcceptanceTest) Run() { report := coll.check() t.Log(report) } - - for _, am := range t.ams { - t.Logf("stdout:\n%v", am.cmd.Stdout) - t.Logf("stderr:\n%v", am.cmd.Stderr) - } } // runActions performs the stored actions at the defined times. @@ -219,6 +216,23 @@ func (t *AcceptanceTest) runActions() { wg.Wait() } +type buffer struct { + b bytes.Buffer + mtx sync.Mutex +} + +func (b *buffer) Write(p []byte) (int, error) { + b.mtx.Lock() + defer b.mtx.Unlock() + return b.b.Write(p) +} + +func (b *buffer) String() string { + b.mtx.Lock() + defer b.mtx.Unlock() + return b.b.String() +} + // Alertmanager encapsulates an Alertmanager process and allows // declaring alerts being pushed to it at fixed points in time. type Alertmanager struct { @@ -246,7 +260,7 @@ func (am *Alertmanager) Start() { ) if am.cmd == nil { - var outb, errb bytes.Buffer + var outb, errb buffer cmd.Stdout = &outb cmd.Stderr = &errb } else { @@ -344,14 +358,14 @@ func (am *Alertmanager) SetSilence(at float64, sil *TestSilence) { am.t.Errorf("error setting silence %v: %s", sil, err) return } - sil.ID = v.Data.SilenceID + sil.SetID(v.Data.SilenceID) }) } // DelSilence deletes the silence with the sid at the given time. func (am *Alertmanager) DelSilence(at float64, sil *TestSilence) { am.t.Do(at, func() { - req, err := http.NewRequest("DELETE", fmt.Sprintf("http://%s/api/v1/silence/%s", am.apiAddr, sil.ID), nil) + req, err := http.NewRequest("DELETE", fmt.Sprintf("http://%s/api/v1/silence/%s", am.apiAddr, sil.ID()), nil) if err != nil { am.t.Errorf("Error deleting silence %v: %s", sil, err) return diff --git a/test/collector.go b/test/collector.go index 9a9e1c816b..2e5dc6b634 100644 --- a/test/collector.go +++ b/test/collector.go @@ -15,6 +15,7 @@ package test import ( "fmt" + "sync" "testing" "time" @@ -30,6 +31,8 @@ type Collector struct { collected map[float64][]model.Alerts expected map[Interval][]model.Alerts + + mtx sync.RWMutex } func (c *Collector) String() string { @@ -59,6 +62,8 @@ func batchesEqual(as, bs model.Alerts, opts *AcceptanceOpts) bool { // latest returns the latest relative point in time where a notification is // expected. func (c *Collector) latest() float64 { + c.mtx.RLock() + defer c.mtx.RUnlock() var latest float64 for iv := range c.expected { if iv.end > latest { @@ -71,6 +76,8 @@ func (c *Collector) latest() float64 { // Want declares that the Collector expects to receive the given alerts // within the given time boundaries. func (c *Collector) Want(iv Interval, alerts ...*TestAlert) { + c.mtx.Lock() + defer c.mtx.Unlock() var nas model.Alerts for _, a := range alerts { nas = append(nas, a.nativeAlert(c.opts)) @@ -81,6 +88,8 @@ func (c *Collector) Want(iv Interval, alerts ...*TestAlert) { // add the given alerts to the collected alerts. func (c *Collector) add(alerts ...*model.Alert) { + c.mtx.Lock() + defer c.mtx.Unlock() arrival := c.opts.relativeTime(time.Now()) c.collected[arrival] = append(c.collected[arrival], model.Alerts(alerts)) @@ -89,6 +98,8 @@ func (c *Collector) add(alerts ...*model.Alert) { func (c *Collector) check() string { report := fmt.Sprintf("\ncollector %q:\n\n", c) + c.mtx.RLock() + defer c.mtx.RUnlock() for iv, expected := range c.expected { report += fmt.Sprintf("interval %v\n", iv) diff --git a/test/mock.go b/test/mock.go index e06e48b382..bfe50b90f4 100644 --- a/test/mock.go +++ b/test/mock.go @@ -19,6 +19,7 @@ import ( "net" "net/http" "reflect" + "sync" "time" "github.com/prometheus/common/model" @@ -53,10 +54,12 @@ func Between(start, end float64) Interval { // TestSilence models a model.Silence with relative times. type TestSilence struct { - ID string + id string match []string matchRE []string startsAt, endsAt float64 + + mtx sync.RWMutex } // Silence creates a new TestSilence active for the relative interval given @@ -83,6 +86,20 @@ func (s *TestSilence) MatchRE(v ...string) *TestSilence { return s } +// SetID sets the silence ID. +func (s *TestSilence) SetID(ID string) { + s.mtx.Lock() + defer s.mtx.Unlock() + s.id = ID +} + +// ID gets the silence ID. +func (s *TestSilence) ID() string { + s.mtx.RLock() + defer s.mtx.RUnlock() + return s.id +} + // nativeSilence converts the declared test silence into a regular // silence with resolved times. func (s *TestSilence) nativeSilence(opts *AcceptanceOpts) *types.Silence {