Improve unit tests for events

pkg/actuators/machine/actuator.go

@@ -30,6 +30,7 @@ import (
 
 const (
 	scopeFailFmt      = "%s: failed to create scope for machine: %v"
+	reconcilerFailFmt = "%s: reconciler failed to %s machine: %v"
 	createEventAction = "Create"
 	updateEventAction = "Update"
 	deleteEventAction = "Delete"
@@ -79,13 +80,15 @@ func (a *Actuator) Create(ctx context.Context, machine *machinev1.Machine) error
 		awsClientBuilder: a.awsClientBuilder,
 	})
 	if err != nil {
-		return a.handleMachineError(machine, err, createEventAction)
+		fmtErr := fmt.Sprintf(scopeFailFmt, machine.GetName(), err)
+		return a.handleMachineError(machine, fmt.Errorf(fmtErr), createEventAction)
 	}
 	if err := newReconciler(scope).create(); err != nil {
 		if err := scope.patchMachine(); err != nil {
 			return err
 		}
-		return a.handleMachineError(machine, err, createEventAction)
+		fmtErr := fmt.Sprintf(reconcilerFailFmt, machine.GetName(), createEventAction, err)
+		return a.handleMachineError(machine, fmt.Errorf(fmtErr), createEventAction)
 	}
 	a.eventRecorder.Eventf(machine, corev1.EventTypeNormal, createEventAction, "Created Machine %v", machine.GetName())
 	return scope.patchMachine()
@@ -125,7 +128,8 @@ func (a *Actuator) Update(ctx context.Context, machine *machinev1.Machine) error
 		if err := scope.patchMachine(); err != nil {
 			return err
 		}
-		return a.handleMachineError(machine, err, createEventAction)
+		fmtErr := fmt.Sprintf(reconcilerFailFmt, machine.GetName(), updateEventAction, err)
+		return a.handleMachineError(machine, fmt.Errorf(fmtErr), updateEventAction)
 	}
 
 	previousResourceVersion := scope.machine.ResourceVersion
@@ -161,7 +165,8 @@ func (a *Actuator) Delete(ctx context.Context, machine *machinev1.Machine) error
 		if err := scope.patchMachine(); err != nil {
 			return err
 		}
-		return a.handleMachineError(machine, err, deleteEventAction)
+		fmtErr := fmt.Sprintf(reconcilerFailFmt, machine.GetName(), deleteEventAction, err)
+		return a.handleMachineError(machine, fmt.Errorf(fmtErr), deleteEventAction)
 	}
 	a.eventRecorder.Eventf(machine, corev1.EventTypeNormal, deleteEventAction, "Deleted machine %v", machine.GetName())
 	return scope.patchMachine()

pkg/actuators/machine/actuator_test.go

@@ -3,184 +3,205 @@ package machine
 import (
 	"context"
 	"errors"
+	"fmt"
 	"testing"
 
 	"github.com/aws/aws-sdk-go/service/ec2"
 	"github.com/golang/mock/gomock"
+	. "github.com/onsi/gomega"
 	machinev1 "github.com/openshift/machine-api-operator/pkg/apis/machine/v1beta1"
-	"k8s.io/apimachinery/pkg/runtime"
+	v1 "k8s.io/api/core/v1"
+	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/client-go/kubernetes/scheme"
 	"k8s.io/client-go/tools/record"
 	awsclient "sigs.k8s.io/cluster-api-provider-aws/pkg/client"
 	mockaws "sigs.k8s.io/cluster-api-provider-aws/pkg/client/mock"
+	"sigs.k8s.io/controller-runtime/pkg/client"
 	runtimeclient "sigs.k8s.io/controller-runtime/pkg/client"
-	"sigs.k8s.io/controller-runtime/pkg/client/fake"
 )
 
 func init() {
 	// Add types to scheme
 	machinev1.AddToScheme(scheme.Scheme)
 }
 
-const (
-	noError             = ""
-	awsServiceError     = "error creating aws service"
-	launchInstanceError = "error launching instance"
-)
-
 func TestMachineEvents(t *testing.T) {
-	machine, err := stubMachine()
-	if err != nil {
-		t.Fatal(err)
-	}
+	g := NewWithT(t)
 
 	awsCredentialsSecret := stubAwsCredentialsSecret()
-	userDataSecret := stubUserDataSecret()
-
-	machineInvalidProviderConfig := machine.DeepCopy()
-	machineInvalidProviderConfig.Spec.ProviderSpec = machinev1.ProviderSpec{
-		Value: &runtime.RawExtension{
-			Raw: []byte("test"),
-		},
-	}
+	g.Expect(k8sClient.Create(context.TODO(), awsCredentialsSecret)).To(Succeed())
+	defer func() {
+		g.Expect(k8sClient.Delete(context.TODO(), awsCredentialsSecret)).To(Succeed())
+	}()
 
-	workerMachine := machine.DeepCopy()
-	workerMachine.Spec.Labels["node-role.kubernetes.io/worker"] = ""
+	userDataSecret := stubUserDataSecret()
+	g.Expect(k8sClient.Create(context.TODO(), userDataSecret)).To(Succeed())
+	defer func() {
+		g.Expect(k8sClient.Delete(context.TODO(), userDataSecret)).To(Succeed())
+	}()
 
 	cases := []struct {
-		name                  string
-		machine               *machinev1.Machine
-		error                 string
-		operation             func(actuator *Actuator, machine *machinev1.Machine)
-		event                 string
-		awsError              bool
-		runInstancesErr       error
-		terminateInstancesErr error
-		lbErr                 error
-		regInstancesWithLbErr error
+		name                string
+		error               string
+		operation           func(actuator *Actuator, machine *machinev1.Machine)
+		event               string
+		awsError            bool
+		invalidMachineScope bool
 	}{
 		{
-			name:    "Create machine event failed on invalid machine scope",
-			machine: machineInvalidProviderConfig,
+			name: "Create machine event failed on invalid machine scope",
 			operation: func(actuator *Actuator, machine *machinev1.Machine) {
 				actuator.Create(context.TODO(), machine)
 			},
-			event: "Warning FailedCreate failed to get machine config: error unmarshalling providerSpec: error unmarshaling JSON: while decoding JSON: json: cannot unmarshal string into Go value of type v1beta1.AWSMachineProviderConfig",
+			event:               "aws-actuator-testing-machine: failed to create scope for machine: failed to create aws client: AWS client error",
+			invalidMachineScope: true,
+			awsError:            false,
 		},
 		{
-			name:    "Create machine event failed, reconciler's create failed",
-			machine: machine,
+			name: "Create machine event failed, reconciler's create failed",
 			operation: func(actuator *Actuator, machine *machinev1.Machine) {
 				actuator.Create(context.TODO(), machine)
 			},
-			event:    "Warning FailedCreate unable to remove stopped machines: error getting stopped instances: error",
-			awsError: true,
+			event:               "aws-actuator-testing-machine: reconciler failed to Create machine: unable to remove stopped machines: error getting stopped instances: AWS error",
+			invalidMachineScope: false,
+			awsError:            true,
 		},
 		{
-			name:    "Create machine event succeed",
-			machine: machine,
+			name: "Create machine event succeed",
 			operation: func(actuator *Actuator, machine *machinev1.Machine) {
 				actuator.Create(context.TODO(), machine)
 			},
-			event: "Normal Create Created Machine aws-actuator-testing-machine",
+			event:               "Created Machine aws-actuator-testing-machine",
+			invalidMachineScope: false,
+			awsError:            false,
 		},
 		{
-			name:    "Update machine event failed on invalid machine scope",
-			machine: machineInvalidProviderConfig,
+			name: "Update machine event failed on invalid machine scope",
 			operation: func(actuator *Actuator, machine *machinev1.Machine) {
 				actuator.Update(context.TODO(), machine)
 			},
-			event: "Warning FailedUpdate aws-actuator-testing-machine: failed to create scope for machine: failed to get machine config: error unmarshalling providerSpec: error unmarshaling JSON: while decoding JSON: json: cannot unmarshal string into Go value of type v1beta1.AWSMachineProviderConfig",
+			event:               "aws-actuator-testing-machine: failed to create scope for machine: failed to create aws client: AWS client error",
+			invalidMachineScope: true,
+			awsError:            false,
 		},
 		{
-			name:    "Update machine event failed, reconciler's update failed",
-			machine: machine,
+			name: "Update machine event failed, reconciler's update failed",
 			operation: func(actuator *Actuator, machine *machinev1.Machine) {
 				actuator.Update(context.TODO(), machine)
 			},
-			event:    "Warning FailedCreate error",
-			awsError: true,
+			event:               "aws-actuator-testing-machine: reconciler failed to Update machine: AWS error",
+			invalidMachineScope: false,
+			awsError:            true,
 		},
 		{
-			name:    "Update machine event succeed",
-			machine: machine,
+			name: "Update machine event succeed and only one event is created",
 			operation: func(actuator *Actuator, machine *machinev1.Machine) {
 				actuator.Update(context.TODO(), machine)
+				actuator.Update(context.TODO(), machine)
 			},
-			event: "Normal Update Updated Machine aws-actuator-testing-machine",
+			event: "Updated Machine aws-actuator-testing-machine",
 		},
 		{
-			name:    "Delete machine event failed on invalid machine scope",
-			machine: machineInvalidProviderConfig,
+			name: "Delete machine event failed on invalid machine scope",
 			operation: func(actuator *Actuator, machine *machinev1.Machine) {
 				actuator.Delete(context.TODO(), machine)
 			},
-			event: "Warning FailedDelete aws-actuator-testing-machine: failed to create scope for machine: failed to get machine config: error unmarshalling providerSpec: error unmarshaling JSON: while decoding JSON: json: cannot unmarshal string into Go value of type v1beta1.AWSMachineProviderConfig",
+			event:               "aws-actuator-testing-machine: failed to create scope for machine: failed to create aws client: AWS client error",
+			invalidMachineScope: true,
+			awsError:            false,
 		},
 		{
-			name:    "Delete machine event failed, reconciler's delete failed",
-			machine: machine,
+			name: "Delete machine event failed, reconciler's delete failed",
 			operation: func(actuator *Actuator, machine *machinev1.Machine) {
 				actuator.Delete(context.TODO(), machine)
 			},
-			event:    "Warning FailedDelete error",
-			awsError: true,
+			event:               "aws-actuator-testing-machine: reconciler failed to Delete machine: AWS error",
+			invalidMachineScope: false,
+			awsError:            true,
 		},
 		{
-			name:    "Delete machine event succeed",
-			machine: machine,
+			name: "Delete machine event succeed",
 			operation: func(actuator *Actuator, machine *machinev1.Machine) {
 				actuator.Delete(context.TODO(), machine)
 			},
-			event: "Normal Delete Deleted machine aws-actuator-testing-machine",
+			event:               "Deleted machine aws-actuator-testing-machine",
+			invalidMachineScope: false,
+			awsError:            false,
 		},
 	}
 
 	for _, tc := range cases {
 		t.Run(tc.name, func(t *testing.T) {
+			ctx := context.TODO()
+			gs := NewWithT(t)
 
-			mockCtrl := gomock.NewController(t)
-			mockAWSClient := mockaws.NewMockClient(mockCtrl)
+			machine, err := stubMachine()
+			gs.Expect(err).ToNot(HaveOccurred())
+			gs.Expect(stubMachine).ToNot(BeNil())
 
-			eventsChannel := make(chan string, 1)
+			// Create the machine
+			gs.Expect(k8sClient.Create(ctx, machine)).To(Succeed())
+			defer func() {
+				gs.Expect(k8sClient.Delete(ctx, machine)).To(Succeed())
+			}()
 
-			params := ActuatorParams{
-				Client: fake.NewFakeClient(tc.machine, awsCredentialsSecret, userDataSecret),
-				// use fake recorder and store an event into one item long buffer for subsequent check
-				EventRecorder: &record.FakeRecorder{
-					Events: eventsChannel,
-				},
-				AwsClientBuilder: func(client runtimeclient.Client, secretName, namespace, region string) (awsclient.Client, error) {
-					return mockAWSClient, nil
-				},
+			// Ensure the machine has synced to the cache
+			getMachine := func() error {
+				machineKey := types.NamespacedName{Namespace: machine.Namespace, Name: machine.Name}
+				return k8sClient.Get(ctx, machineKey, machine)
+			}
+			gs.Eventually(getMachine, timeout).Should(Succeed())
+
+			mockCtrl := gomock.NewController(t)
+			mockAWSClient := mockaws.NewMockClient(mockCtrl)
+			awsClientBuilder := func(client runtimeclient.Client, secretName, namespace, region string) (awsclient.Client, error) {
+				return mockAWSClient, nil
+			}
+			if tc.invalidMachineScope {
+				awsClientBuilder = func(client runtimeclient.Client, secretName, namespace, region string) (awsclient.Client, error) {
+					return nil, errors.New("AWS client error")
+				}
 			}
 
 			if tc.awsError {
-				mockAWSClient.EXPECT().DescribeInstances(gomock.Any()).Return(nil, errors.New("error")).AnyTimes()
+				mockAWSClient.EXPECT().DescribeInstances(gomock.Any()).Return(nil, errors.New("AWS error")).AnyTimes()
 			} else {
-
 				mockAWSClient.EXPECT().DescribeInstances(gomock.Any()).Return(stubDescribeInstancesOutput("ami-a9acbbd6", "i-02fcb933c5da7085c", ec2.InstanceStateNameRunning), nil).AnyTimes()
 			}
 
-			mockAWSClient.EXPECT().RunInstances(gomock.Any()).Return(stubReservation("ami-a9acbbd6", "i-02fcb933c5da7085c"), tc.runInstancesErr).AnyTimes()
+			mockAWSClient.EXPECT().RunInstances(gomock.Any()).Return(stubReservation("ami-a9acbbd6", "i-02fcb933c5da7085c"), nil).AnyTimes()
 			mockAWSClient.EXPECT().TerminateInstances(gomock.Any()).Return(&ec2.TerminateInstancesOutput{}, nil)
 			mockAWSClient.EXPECT().RegisterInstancesWithLoadBalancer(gomock.Any()).Return(nil, nil).AnyTimes()
-			mockAWSClient.EXPECT().TerminateInstances(gomock.Any()).Return(&ec2.TerminateInstancesOutput{}, tc.terminateInstancesErr).AnyTimes()
-			mockAWSClient.EXPECT().RegisterInstancesWithLoadBalancer(gomock.Any()).Return(nil, tc.lbErr).AnyTimes()
-			mockAWSClient.EXPECT().ELBv2DescribeLoadBalancers(gomock.Any()).Return(stubDescribeLoadBalancersOutput(), tc.lbErr)
+			mockAWSClient.EXPECT().TerminateInstances(gomock.Any()).Return(&ec2.TerminateInstancesOutput{}, nil).AnyTimes()
+			mockAWSClient.EXPECT().RegisterInstancesWithLoadBalancer(gomock.Any()).Return(nil, nil).AnyTimes()
+			mockAWSClient.EXPECT().ELBv2DescribeLoadBalancers(gomock.Any()).Return(stubDescribeLoadBalancersOutput(), nil).AnyTimes()
 			mockAWSClient.EXPECT().ELBv2DescribeTargetGroups(gomock.Any()).Return(stubDescribeTargetGroupsOutput(), nil).AnyTimes()
 			mockAWSClient.EXPECT().ELBv2RegisterTargets(gomock.Any()).Return(nil, nil).AnyTimes()
 
+			params := ActuatorParams{
+				Client:           k8sClient,
+				EventRecorder:    eventRecorder,
+				AwsClientBuilder: awsClientBuilder,
+			}
 			actuator := NewActuator(params)
+			tc.operation(actuator, machine)
 
-			tc.operation(actuator, tc.machine)
-			select {
-			case event := <-eventsChannel:
-				if event != tc.event {
-					t.Errorf("Expected %q event, got %q", tc.event, event)
+			eventList := &v1.EventList{}
+			waitForEvent := func() error {
+				gs.Expect(k8sClient.List(ctx, eventList, client.InNamespace(machine.Namespace))).To(Succeed())
+				if len(eventList.Items) != 1 {
+					errorMsg := fmt.Sprintf("Expected len 1, got %d", len(eventList.Items))
+					return errors.New(errorMsg)
 				}
-			default:
-				t.Errorf("Expected %q event, got none", tc.event)
+				return nil
+			}
+
+			gs.Eventually(waitForEvent, timeout).Should(Succeed())
+
+			gs.Expect(eventList.Items[0].Message).To(Equal(tc.event))
+
+			for i := range eventList.Items {
+				gs.Expect(k8sClient.Delete(ctx, &eventList.Items[i])).To(Succeed())
 			}
 		})
 	}

pkg/actuators/machine/machine_scope_test.go

@@ -4,9 +4,7 @@ import (
 	"bytes"
 	"context"
 	"fmt"
-	"path/filepath"
 	"testing"
-	"time"
 
 	. "github.com/onsi/gomega"
 	machinev1 "github.com/openshift/machine-api-operator/pkg/apis/machine/v1beta1"
@@ -19,7 +17,10 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	runtimeclient "sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/client/fake"
+<<<<<<< HEAD
 	"sigs.k8s.io/controller-runtime/pkg/envtest"
+=======
+>>>>>>> Improve unit tests for events
 )
 
 const testNamespace = "aws-test"
@@ -142,7 +143,6 @@ func TestGetUserData(t *testing.T) {
 }
 
 func TestPatchMachine(t *testing.T) {
-	// BEGIN: Set up test environment
 	g := NewWithT(t)
 
 	testEnv := &envtest.Environment{
@@ -163,9 +163,15 @@ func TestPatchMachine(t *testing.T) {
 
 	awsCredentialsSecret := stubAwsCredentialsSecret()
 	g.Expect(k8sClient.Create(context.TODO(), awsCredentialsSecret)).To(Succeed())
+	defer func() {
+		g.Expect(k8sClient.Delete(context.TODO(), awsCredentialsSecret)).To(Succeed())
+	}()
 
 	userDataSecret := stubUserDataSecret()
 	g.Expect(k8sClient.Create(context.TODO(), userDataSecret)).To(Succeed())
+	defer func() {
+		g.Expect(k8sClient.Delete(context.TODO(), userDataSecret)).To(Succeed())
+	}()
 
 	failedPhase := "Failed"
 
@@ -229,7 +235,6 @@ func TestPatchMachine(t *testing.T) {
 
 	for _, tc := range testCases {
 		t.Run(tc.name, func(t *testing.T) {
-			timeout := 10 * time.Second
 			gs := NewWithT(t)
 
 			machine, err := stubMachine()