From 9fc02d941e5ba30df7a9d4106f801d6d57ea9770 Mon Sep 17 00:00:00 2001 From: Tom Wilkie Date: Wed, 8 Jul 2015 12:25:32 +0000 Subject: [PATCH] Fix flaky tests - Make poll take interfaces, do diff on error - Use poll in TestRegistryEvents - Improve the locking to prevent deadlocks and data races in registry_test.go --- probe/docker/container_test.go | 7 ++--- probe/docker/registry_test.go | 49 ++++++++++++++++----------------- test/poll.go | 11 ++++++-- xfer/collector_internal_test.go | 6 ++-- 4 files changed, 38 insertions(+), 35 deletions(-) diff --git a/probe/docker/container_test.go b/probe/docker/container_test.go index f940f0c44..21f8d080b 100644 --- a/probe/docker/container_test.go +++ b/probe/docker/container_test.go @@ -61,8 +61,7 @@ func TestContainer(t *testing.T) { } // Now see if we go them - test.Poll(t, 10*time.Millisecond, func() bool { - nmd := c.GetNodeMetadata() - return nmd[docker.MemoryUsage] == "12345" - }, "Failed to get stats") + test.Poll(t, 10*time.Millisecond, "12345", func() interface{} { + return c.GetNodeMetadata()[docker.MemoryUsage] + }) } diff --git a/probe/docker/registry_test.go b/probe/docker/registry_test.go index ea8bc8350..010be8725 100644 --- a/probe/docker/registry_test.go +++ b/probe/docker/registry_test.go @@ -46,7 +46,7 @@ func (c *mockContainer) GetNodeMetadata() report.NodeMetadata { } type mockDockerClient struct { - sync.Mutex + sync.RWMutex apiContainers []client.APIContainers containers map[string]*client.Container apiImages []client.APIImages @@ -54,16 +54,20 @@ type mockDockerClient struct { } func (m *mockDockerClient) ListContainers(client.ListContainersOptions) ([]client.APIContainers, error) { + m.RLock() + defer m.RUnlock() return m.apiContainers, nil } func (m *mockDockerClient) InspectContainer(id string) (*client.Container, error) { + m.RLock() + defer m.RUnlock() return m.containers[id], nil } func (m *mockDockerClient) ListImages(client.ListImagesOptions) ([]client.APIImages, error) { - m.Lock() - defer m.Unlock() + m.RLock() + defer m.RUnlock() return m.apiImages, nil } @@ -86,8 +90,8 @@ func (m *mockDockerClient) RemoveEventListener(events chan *client.APIEvents) er } func (m *mockDockerClient) send(event *client.APIEvents) { - m.Lock() - defer m.Unlock() + m.RLock() + defer m.RUnlock() for _, c := range m.events { c <- event } @@ -160,11 +164,12 @@ func TestRegistry(t *testing.T) { defer registry.Stop() runtime.Gosched() - test.Poll(t, 10*time.Millisecond, func() bool { - have := allContainers(registry) + { want := []docker.Container{&mockContainer{container1}} - return reflect.DeepEqual(want, have) - }, "Didn't get containers!") + test.Poll(t, 10*time.Millisecond, want, func() interface{} { + return allContainers(registry) + }) + } { have := allImages(registry) @@ -183,6 +188,12 @@ func TestRegistryEvents(t *testing.T) { defer registry.Stop() runtime.Gosched() + check := func(want []docker.Container) { + test.Poll(t, 10*time.Millisecond, want, func() interface{} { + return allContainers(registry) + }) + } + { mdc.Lock() mdc.containers["wiff"] = container2 @@ -190,11 +201,8 @@ func TestRegistryEvents(t *testing.T) { mdc.send(&client.APIEvents{Status: docker.StartEvent, ID: "wiff"}) runtime.Gosched() - have := allContainers(registry) want := []docker.Container{&mockContainer{container1}, &mockContainer{container2}} - if !reflect.DeepEqual(want, have) { - t.Errorf("%s", test.Diff(want, have)) - } + check(want) } { @@ -204,11 +212,8 @@ func TestRegistryEvents(t *testing.T) { mdc.send(&client.APIEvents{Status: docker.DieEvent, ID: "wiff"}) runtime.Gosched() - have := allContainers(registry) want := []docker.Container{&mockContainer{container1}} - if !reflect.DeepEqual(want, have) { - t.Errorf("%s", test.Diff(want, have)) - } + check(want) } { @@ -218,22 +223,16 @@ func TestRegistryEvents(t *testing.T) { mdc.send(&client.APIEvents{Status: docker.DieEvent, ID: "ping"}) runtime.Gosched() - have := allContainers(registry) want := []docker.Container{} - if !reflect.DeepEqual(want, have) { - t.Errorf("%s", test.Diff(want, have)) - } + check(want) } { mdc.send(&client.APIEvents{Status: docker.DieEvent, ID: "doesntexist"}) runtime.Gosched() - have := allContainers(registry) want := []docker.Container{} - if !reflect.DeepEqual(want, have) { - t.Errorf("%s", test.Diff(want, have)) - } + check(want) } }) } diff --git a/test/poll.go b/test/poll.go index 0b0133c54..593d6cfff 100644 --- a/test/poll.go +++ b/test/poll.go @@ -1,20 +1,25 @@ package test import ( + "reflect" "testing" "time" ) // Poll repeatedly evaluates condition until we either timeout, or it suceeds. -func Poll(t *testing.T, d time.Duration, condition func() bool, msg string) { +func Poll(t *testing.T, d time.Duration, want interface{}, have func() interface{}) { deadline := time.Now().Add(d) for { if time.Now().After(deadline) { - t.Fatal(msg) + break } - if condition() { + if reflect.DeepEqual(want, have()) { return } time.Sleep(d / 10) } + h := have() + if reflect.DeepEqual(want, h) { + t.Fatal(Diff(want, h)) + } } diff --git a/xfer/collector_internal_test.go b/xfer/collector_internal_test.go index 9e406a191..9e0c29c75 100644 --- a/xfer/collector_internal_test.go +++ b/xfer/collector_internal_test.go @@ -53,9 +53,9 @@ func TestCollector(t *testing.T) { } reports <- r - test.Poll(t, 100*time.Millisecond, func() bool { - return len(concreteCollector.peek().Address.NodeMetadatas) == 1 - }, "missed the report") + test.Poll(t, 100*time.Millisecond, 1, func() interface{} { + return len(concreteCollector.peek().Address.NodeMetadatas) + }) go func() { publish <- time.Now() }() collected := <-collector.Reports()