From 2dd42886824381476cc80f55771676053399cd7b Mon Sep 17 00:00:00 2001
From: Patrick Ohly <patrick.ohly@intel.com>
Date: Mon, 18 Jan 2021 11:53:27 +0100
Subject: [PATCH] test: fix operator event checking

The code had two problems.

It stored pointers to the objects passed in by the caller, which is
dangerous because those instances might be reused by the caller, in
which case the content changes. In practice this doesn't seem to
happen, but we can't know that.

It relied on receiving events in a certain order. The apiserver
and (to some extend) the fake client may combine events, which
apparently can also cause them to be delivered out-of-order. This has
happened in practice and broke the
test (https://github.com/intel/pmem-csi/issues/852).

The solution is to de-duplicate after sorting. Because the order
depends on what we compare against, it is done during the actual
comparison together with de-duplication.
---
 .../deployment/deployment_controller_test.go  | 29 ++++++++++---------
 1 file changed, 15 insertions(+), 14 deletions(-)

diff --git a/pkg/pmem-csi-operator/controller/deployment/deployment_controller_test.go b/pkg/pmem-csi-operator/controller/deployment/deployment_controller_test.go
index cec5dcff40..0cbeef5aa5 100644
--- a/pkg/pmem-csi-operator/controller/deployment/deployment_controller_test.go
+++ b/pkg/pmem-csi-operator/controller/deployment/deployment_controller_test.go
@@ -8,6 +8,7 @@ package deployment_test
 import (
 	"context"
 	"fmt"
+	"sort"
 	"strings"
 	"sync"
 	"testing"
@@ -226,7 +227,7 @@ type testContext struct {
 	k8sVersion       version.Version
 
 	eventsMutex sync.Mutex
-	events      []*corev1.Event
+	events      []corev1.Event
 }
 
 func newTestContext(t *testing.T, k8sVersion version.Version, initObjs ...runtime.Object) *testContext {
@@ -264,15 +265,7 @@ func (tc *testContext) ResetReconciler() {
 	tc.evWatcher = rc.(*deployment.ReconcileDeployment).EventBroadcaster().StartEventWatcher(func(ev *corev1.Event) {
 		tc.eventsMutex.Lock()
 		defer tc.eventsMutex.Unlock()
-
-		// Discard consecutive duplicate events, mimicking the EventAggregator behavior
-		if len(tc.events) != 0 {
-			lastEvent := tc.events[len(tc.events)-1]
-			if lastEvent.Reason == ev.Reason && lastEvent.InvolvedObject.UID == ev.InvolvedObject.UID {
-				return
-			}
-		}
-		tc.events = append(tc.events, ev)
+		tc.events = append(tc.events, *ev)
 	})
 }
 
@@ -314,12 +307,20 @@ func TestDeploymentController(t *testing.T) {
 				tc.eventsMutex.Lock()
 				defer tc.eventsMutex.Unlock()
 
-				if len(tc.events) < len(expectedEvents) {
-					return false
-				}
+				// Before comparing against expected
+				// events, we must sort by the "first
+				// seen", time stamp because events
+				// may get delivered out-of-order.
+				sort.Slice(tc.events, func(i, j int) bool {
+					return tc.events[i].FirstTimestamp.Before(&tc.events[j].FirstTimestamp)
+				})
+
+				// Then we need to filter out events for the
+				// right deployment and remove duplicates.
 				events := []string{}
 				for _, e := range tc.events {
-					if e.InvolvedObject.UID == dep.GetUID() {
+					if e.InvolvedObject.UID == dep.GetUID() &&
+						(len(events) == 0 || events[len(events)-1] != e.Reason) {
 						events = append(events, e.Reason)
 					}
 				}