From 0eab471e7c55ab596bae36b8cc8f14edfff07785 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Roland=20Hu=C3=9F?= <roland@ro14nd.de>
Date: Wed, 12 Feb 2020 20:15:43 +0100
Subject: [PATCH] fix(service wait): Wrong check for error window

The error window introduced with #644 had a wrong conditional. Fixed that and added a test which would have detected this.

Also, this should fix some issues which we tried to detect with #659.
---
 pkg/wait/wait_for_ready.go      |  8 ++++--
 pkg/wait/wait_for_ready_test.go | 50 ++++++++++++++++++++-------------
 2 files changed, 35 insertions(+), 23 deletions(-)

diff --git a/pkg/wait/wait_for_ready.go b/pkg/wait/wait_for_ready.go
index d5a5c56377..e4c51776ad 100644
--- a/pkg/wait/wait_for_ready.go
+++ b/pkg/wait/wait_for_ready.go
@@ -41,7 +41,8 @@ type waitForReadyConfig struct {
 type WaitForReady interface {
 
 	// Wait on resource the resource with this name until a given timeout
-	// and write event messages for unknown event to the status writer
+	// and write event messages for unknown event to the status writer.
+	// Returns an error (if any) and the overall time it took to wait
 	Wait(name string, timeout time.Duration, msgCallback MessageCallback) (error, time.Duration)
 }
 
@@ -161,9 +162,10 @@ func (w *waitForReadyConfig) waitForReadyCondition(start time.Time, name string,
 						// to a true condition even after the condition went to false. If this is not the case within
 						// this window, then an error is returned.
 						// If there is already a timer running, we just log.
-						if errorTimer != nil {
+						if errorTimer == nil {
+							err := fmt.Errorf("%s: %s", cond.Reason, cond.Message)
 							errorTimer = time.AfterFunc(errorWindow, func() {
-								errChan <- fmt.Errorf("%s: %s", cond.Reason, cond.Message)
+								errChan <- err
 							})
 						}
 					}
diff --git a/pkg/wait/wait_for_ready_test.go b/pkg/wait/wait_for_ready_test.go
index ad80ad19f4..6ff89d8cd5 100644
--- a/pkg/wait/wait_for_ready_test.go
+++ b/pkg/wait/wait_for_ready_test.go
@@ -30,7 +30,7 @@ import (
 type waitForReadyTestCase struct {
 	events           []watch.Event
 	timeout          time.Duration
-	errorExpected    bool
+	errorText        string
 	messagesExpected []string
 }
 
@@ -54,12 +54,16 @@ func TestAddWaitForReady(t *testing.T) {
 		})
 		close(fakeWatchApi.eventChan)
 
-		if !tc.errorExpected && err != nil {
+		if tc.errorText == "" && err != nil {
 			t.Errorf("%d: Error received %v", i, err)
 			continue
 		}
-		if tc.errorExpected && err == nil {
-			t.Errorf("%d: No error but expected one", i)
+		if tc.errorText != "" {
+			if err == nil {
+				t.Errorf("%d: No error but expected one", i)
+			} else {
+				assert.ErrorContains(t, err, tc.errorText)
+			}
 		}
 
 		// check messages
@@ -75,20 +79,34 @@ func TestAddWaitForReady(t *testing.T) {
 // Test cases which consists of a series of events to send and the expected behaviour.
 func prepareTestCases(name string) []waitForReadyTestCase {
 	return []waitForReadyTestCase{
-		tc(peNormal, name, false),
-		tc(peError, name, true),
-		tc(peWrongGeneration, name, true),
-		tc(peTimeout, name, true),
-		tc(peReadyFalseWithinErrorWindow, name, false),
+		errorTest(name),
+		tc(peNormal, name, time.Second, ""),
+		tc(peWrongGeneration, name, 1*time.Second, "timeout"),
+		tc(peTimeout, name, time.Second, "timeout"),
+		tc(peReadyFalseWithinErrorWindow, name, time.Second, ""),
 	}
 }
 
-func tc(f func(name string) (evts []watch.Event, nrMessages int), name string, isError bool) waitForReadyTestCase {
+func errorTest(name string) waitForReadyTestCase {
+	events := []watch.Event{
+		{watch.Added, CreateTestServiceWithConditions(name, corev1.ConditionUnknown, corev1.ConditionUnknown, "", "msg1")},
+		{watch.Modified, CreateTestServiceWithConditions(name, corev1.ConditionFalse, corev1.ConditionTrue, "FakeError", "Test Error")},
+	}
+
+	return waitForReadyTestCase{
+		events:           events,
+		timeout:          3 * time.Second,
+		errorText:        "FakeError",
+		messagesExpected: []string{"msg1", "Test Error"},
+	}
+}
+
+func tc(f func(name string) (evts []watch.Event, nrMessages int), name string, timeout time.Duration, errorTxt string) waitForReadyTestCase {
 	events, nrMsgs := f(name)
 	return waitForReadyTestCase{
 		events,
-		time.Second,
-		isError,
+		timeout,
+		errorTxt,
 		pMessages(nrMsgs),
 	}
 }
@@ -110,14 +128,6 @@ func peNormal(name string) ([]watch.Event, int) {
 	}, len(messages)
 }
 
-func peError(name string) ([]watch.Event, int) {
-	messages := pMessages(1)
-	return []watch.Event{
-		{watch.Added, CreateTestServiceWithConditions(name, corev1.ConditionUnknown, corev1.ConditionUnknown, "", messages[0])},
-		{watch.Modified, CreateTestServiceWithConditions(name, corev1.ConditionFalse, corev1.ConditionTrue, "FakeError", "")},
-	}, len(messages)
-}
-
 func peTimeout(name string) ([]watch.Event, int) {
 	messages := pMessages(1)
 	return []watch.Event{