From 007ca4f69d7cddd06c120a7f8f3b50463e2f97c7 Mon Sep 17 00:00:00 2001 From: Dan Winship Date: Wed, 27 Apr 2022 10:05:09 -0400 Subject: [PATCH] Use t.Run() consistently in reconciler unit tests --- .../reconcilers/instancecount_test.go | 166 +++++++++--------- pkg/controlplane/reconcilers/lease_test.go | 74 ++++---- 2 files changed, 122 insertions(+), 118 deletions(-) diff --git a/pkg/controlplane/reconcilers/instancecount_test.go b/pkg/controlplane/reconcilers/instancecount_test.go index a17a61613ad..8970031afad 100644 --- a/pkg/controlplane/reconcilers/instancecount_test.go +++ b/pkg/controlplane/reconcilers/instancecount_test.go @@ -393,53 +393,54 @@ func TestMasterCountEndpointReconciler(t *testing.T) { }, } for _, test := range reconcileTests { - fakeClient := fake.NewSimpleClientset() - if test.endpoints != nil { - fakeClient = fake.NewSimpleClientset(test.endpoints) - } - epAdapter := NewEndpointsAdapter(fakeClient.CoreV1(), nil) - reconciler := NewMasterCountEndpointReconciler(test.additionalMasters+1, epAdapter) - err := reconciler.ReconcileEndpoints(test.serviceName, netutils.ParseIPSloppy(test.ip), test.endpointPorts, true) - if err != nil { - t.Errorf("case %q: unexpected error: %v", test.testName, err) - } + t.Run(test.testName, func(t *testing.T) { + fakeClient := fake.NewSimpleClientset() + if test.endpoints != nil { + fakeClient = fake.NewSimpleClientset(test.endpoints) + } + epAdapter := NewEndpointsAdapter(fakeClient.CoreV1(), nil) + reconciler := NewMasterCountEndpointReconciler(test.additionalMasters+1, epAdapter) + err := reconciler.ReconcileEndpoints(test.serviceName, netutils.ParseIPSloppy(test.ip), test.endpointPorts, true) + if err != nil { + t.Errorf("unexpected error: %v", err) + } - updates := []core.UpdateAction{} - for _, action := range fakeClient.Actions() { - if action.GetVerb() != "update" { - continue + updates := []core.UpdateAction{} + for _, action := range fakeClient.Actions() { + if action.GetVerb() != "update" { + continue + } + updates = append(updates, action.(core.UpdateAction)) } - updates = append(updates, action.(core.UpdateAction)) - } - if test.expectUpdate != nil { - if len(updates) != 1 { - t.Errorf("case %q: unexpected updates: %v", test.testName, updates) - } else if e, a := test.expectUpdate, updates[0].GetObject(); !reflect.DeepEqual(e, a) { - t.Errorf("case %q: expected update:\n%#v\ngot:\n%#v\n", test.testName, e, a) + if test.expectUpdate != nil { + if len(updates) != 1 { + t.Errorf("unexpected updates: %v", updates) + } else if e, a := test.expectUpdate, updates[0].GetObject(); !reflect.DeepEqual(e, a) { + t.Errorf("expected update:\n%#v\ngot:\n%#v\n", e, a) + } + } + if test.expectUpdate == nil && len(updates) > 0 { + t.Errorf("no update expected, yet saw: %v", updates) } - } - if test.expectUpdate == nil && len(updates) > 0 { - t.Errorf("case %q: no update expected, yet saw: %v", test.testName, updates) - } - creates := []core.CreateAction{} - for _, action := range fakeClient.Actions() { - if action.GetVerb() != "create" { - continue + creates := []core.CreateAction{} + for _, action := range fakeClient.Actions() { + if action.GetVerb() != "create" { + continue + } + creates = append(creates, action.(core.CreateAction)) } - creates = append(creates, action.(core.CreateAction)) - } - if test.expectCreate != nil { - if len(creates) != 1 { - t.Errorf("case %q: unexpected creates: %v", test.testName, creates) - } else if e, a := test.expectCreate, creates[0].GetObject(); !reflect.DeepEqual(e, a) { - t.Errorf("case %q: expected create:\n%#v\ngot:\n%#v\n", test.testName, e, a) + if test.expectCreate != nil { + if len(creates) != 1 { + t.Errorf("unexpected creates: %v", creates) + } else if e, a := test.expectCreate, creates[0].GetObject(); !reflect.DeepEqual(e, a) { + t.Errorf("expected create:\n%#v\ngot:\n%#v\n", e, a) + } } - } - if test.expectCreate == nil && len(creates) > 0 { - t.Errorf("case %q: no create expected, yet saw: %v", test.testName, creates) - } - + if test.expectCreate == nil && len(creates) > 0 { + t.Errorf("no create expected, yet saw: %v", creates) + } + }) } nonReconcileTests := []struct { @@ -512,53 +513,54 @@ func TestMasterCountEndpointReconciler(t *testing.T) { }, } for _, test := range nonReconcileTests { - fakeClient := fake.NewSimpleClientset() - if test.endpoints != nil { - fakeClient = fake.NewSimpleClientset(test.endpoints) - } - epAdapter := NewEndpointsAdapter(fakeClient.CoreV1(), nil) - reconciler := NewMasterCountEndpointReconciler(test.additionalMasters+1, epAdapter) - err := reconciler.ReconcileEndpoints(test.serviceName, netutils.ParseIPSloppy(test.ip), test.endpointPorts, false) - if err != nil { - t.Errorf("case %q: unexpected error: %v", test.testName, err) - } + t.Run(test.testName, func(t *testing.T) { + fakeClient := fake.NewSimpleClientset() + if test.endpoints != nil { + fakeClient = fake.NewSimpleClientset(test.endpoints) + } + epAdapter := NewEndpointsAdapter(fakeClient.CoreV1(), nil) + reconciler := NewMasterCountEndpointReconciler(test.additionalMasters+1, epAdapter) + err := reconciler.ReconcileEndpoints(test.serviceName, netutils.ParseIPSloppy(test.ip), test.endpointPorts, false) + if err != nil { + t.Errorf("unexpected error: %v", err) + } - updates := []core.UpdateAction{} - for _, action := range fakeClient.Actions() { - if action.GetVerb() != "update" { - continue + updates := []core.UpdateAction{} + for _, action := range fakeClient.Actions() { + if action.GetVerb() != "update" { + continue + } + updates = append(updates, action.(core.UpdateAction)) } - updates = append(updates, action.(core.UpdateAction)) - } - if test.expectUpdate != nil { - if len(updates) != 1 { - t.Errorf("case %q: unexpected updates: %v", test.testName, updates) - } else if e, a := test.expectUpdate, updates[0].GetObject(); !reflect.DeepEqual(e, a) { - t.Errorf("case %q: expected update:\n%#v\ngot:\n%#v\n", test.testName, e, a) + if test.expectUpdate != nil { + if len(updates) != 1 { + t.Errorf("unexpected updates: %v", updates) + } else if e, a := test.expectUpdate, updates[0].GetObject(); !reflect.DeepEqual(e, a) { + t.Errorf("expected update:\n%#v\ngot:\n%#v\n", e, a) + } + } + if test.expectUpdate == nil && len(updates) > 0 { + t.Errorf("no update expected, yet saw: %v", updates) } - } - if test.expectUpdate == nil && len(updates) > 0 { - t.Errorf("case %q: no update expected, yet saw: %v", test.testName, updates) - } - creates := []core.CreateAction{} - for _, action := range fakeClient.Actions() { - if action.GetVerb() != "create" { - continue + creates := []core.CreateAction{} + for _, action := range fakeClient.Actions() { + if action.GetVerb() != "create" { + continue + } + creates = append(creates, action.(core.CreateAction)) } - creates = append(creates, action.(core.CreateAction)) - } - if test.expectCreate != nil { - if len(creates) != 1 { - t.Errorf("case %q: unexpected creates: %v", test.testName, creates) - } else if e, a := test.expectCreate, creates[0].GetObject(); !reflect.DeepEqual(e, a) { - t.Errorf("case %q: expected create:\n%#v\ngot:\n%#v\n", test.testName, e, a) + if test.expectCreate != nil { + if len(creates) != 1 { + t.Errorf("unexpected creates: %v", creates) + } else if e, a := test.expectCreate, creates[0].GetObject(); !reflect.DeepEqual(e, a) { + t.Errorf("expected create:\n%#v\ngot:\n%#v\n", e, a) + } } - } - if test.expectCreate == nil && len(creates) > 0 { - t.Errorf("case %q: no create expected, yet saw: %v", test.testName, creates) - } - + if test.expectCreate == nil && len(creates) > 0 { + t.Errorf("no create expected, yet saw: %v", creates) + } + }) } } diff --git a/pkg/controlplane/reconcilers/lease_test.go b/pkg/controlplane/reconcilers/lease_test.go index 3f99897f222..fb58fa0faaa 100644 --- a/pkg/controlplane/reconcilers/lease_test.go +++ b/pkg/controlplane/reconcilers/lease_test.go @@ -448,36 +448,38 @@ func TestLeaseEndpointReconciler(t *testing.T) { }, } for _, test := range reconcileTests { - fakeLeases := newFakeLeases() - fakeLeases.SetKeys(test.endpointKeys) - clientset := fake.NewSimpleClientset() - if test.endpoints != nil { - for _, ep := range test.endpoints.Items { - if _, err := clientset.CoreV1().Endpoints(ep.Namespace).Create(context.TODO(), &ep, metav1.CreateOptions{}); err != nil { - t.Errorf("case %q: unexpected error: %v", test.testName, err) - continue + t.Run(test.testName, func(t *testing.T) { + fakeLeases := newFakeLeases() + fakeLeases.SetKeys(test.endpointKeys) + clientset := fake.NewSimpleClientset() + if test.endpoints != nil { + for _, ep := range test.endpoints.Items { + if _, err := clientset.CoreV1().Endpoints(ep.Namespace).Create(context.TODO(), &ep, metav1.CreateOptions{}); err != nil { + t.Errorf("unexpected error: %v", err) + continue + } } } - } - epAdapter := EndpointsAdapter{endpointClient: clientset.CoreV1()} - r := NewLeaseEndpointReconciler(epAdapter, fakeLeases) - err := r.ReconcileEndpoints(test.serviceName, netutils.ParseIPSloppy(test.ip), test.endpointPorts, true) - if err != nil { - t.Errorf("case %q: unexpected error: %v", test.testName, err) - } - actualEndpoints, err := clientset.CoreV1().Endpoints(corev1.NamespaceDefault).Get(context.TODO(), test.serviceName, metav1.GetOptions{}) - if err != nil { - t.Errorf("case %q: unexpected error: %v", test.testName, err) - } - if test.expectUpdate != nil { - if e, a := test.expectUpdate, actualEndpoints; !reflect.DeepEqual(e, a) { - t.Errorf("case %q: expected update:\n%#v\ngot:\n%#v\n", test.testName, e, a) + epAdapter := EndpointsAdapter{endpointClient: clientset.CoreV1()} + r := NewLeaseEndpointReconciler(epAdapter, fakeLeases) + err := r.ReconcileEndpoints(test.serviceName, netutils.ParseIPSloppy(test.ip), test.endpointPorts, true) + if err != nil { + t.Errorf("unexpected error: %v", err) } - } - if updatedKeys := fakeLeases.GetUpdatedKeys(); len(updatedKeys) != 1 || updatedKeys[0] != test.ip { - t.Errorf("case %q: expected the master's IP to be refreshed, but the following IPs were refreshed instead: %v", test.testName, updatedKeys) - } + actualEndpoints, err := clientset.CoreV1().Endpoints(corev1.NamespaceDefault).Get(context.TODO(), test.serviceName, metav1.GetOptions{}) + if err != nil { + t.Errorf("unexpected error: %v", err) + } + if test.expectUpdate != nil { + if e, a := test.expectUpdate, actualEndpoints; !reflect.DeepEqual(e, a) { + t.Errorf("expected update:\n%#v\ngot:\n%#v\n", e, a) + } + } + if updatedKeys := fakeLeases.GetUpdatedKeys(); len(updatedKeys) != 1 || updatedKeys[0] != test.ip { + t.Errorf("expected the master's IP to be refreshed, but the following IPs were refreshed instead: %v", updatedKeys) + } + }) } nonReconcileTests := []struct { @@ -556,7 +558,7 @@ func TestLeaseEndpointReconciler(t *testing.T) { if test.endpoints != nil { for _, ep := range test.endpoints.Items { if _, err := clientset.CoreV1().Endpoints(ep.Namespace).Create(context.TODO(), &ep, metav1.CreateOptions{}); err != nil { - t.Errorf("case %q: unexpected error: %v", test.testName, err) + t.Errorf("unexpected error: %v", err) continue } } @@ -565,19 +567,19 @@ func TestLeaseEndpointReconciler(t *testing.T) { r := NewLeaseEndpointReconciler(epAdapter, fakeLeases) err := r.ReconcileEndpoints(test.serviceName, netutils.ParseIPSloppy(test.ip), test.endpointPorts, false) if err != nil { - t.Errorf("case %q: unexpected error: %v", test.testName, err) + t.Errorf("unexpected error: %v", err) } actualEndpoints, err := clientset.CoreV1().Endpoints(corev1.NamespaceDefault).Get(context.TODO(), test.serviceName, metav1.GetOptions{}) if err != nil { - t.Errorf("case %q: unexpected error: %v", test.testName, err) + t.Errorf("unexpected error: %v", err) } if test.expectUpdate != nil { if e, a := test.expectUpdate, actualEndpoints; !reflect.DeepEqual(e, a) { - t.Errorf("case %q: expected update:\n%#v\ngot:\n%#v\n", test.testName, e, a) + t.Errorf("expected update:\n%#v\ngot:\n%#v\n", e, a) } } if updatedKeys := fakeLeases.GetUpdatedKeys(); len(updatedKeys) != 1 || updatedKeys[0] != test.ip { - t.Errorf("case %q: expected the master's IP to be refreshed, but the following IPs were refreshed instead: %v", test.testName, updatedKeys) + t.Errorf("expected the master's IP to be refreshed, but the following IPs were refreshed instead: %v", updatedKeys) } }) } @@ -677,7 +679,7 @@ func TestLeaseRemoveEndpoints(t *testing.T) { clientset := fake.NewSimpleClientset() for _, ep := range test.endpoints.Items { if _, err := clientset.CoreV1().Endpoints(ep.Namespace).Create(context.TODO(), &ep, metav1.CreateOptions{}); err != nil { - t.Errorf("case %q: unexpected error: %v", test.testName, err) + t.Errorf("unexpected error: %v", err) continue } } @@ -685,20 +687,20 @@ func TestLeaseRemoveEndpoints(t *testing.T) { r := NewLeaseEndpointReconciler(epAdapter, fakeLeases) err := r.RemoveEndpoints(test.serviceName, netutils.ParseIPSloppy(test.ip), test.endpointPorts) if err != nil { - t.Errorf("case %q: unexpected error: %v", test.testName, err) + t.Errorf("unexpected error: %v", err) } actualEndpoints, err := clientset.CoreV1().Endpoints(corev1.NamespaceDefault).Get(context.TODO(), test.serviceName, metav1.GetOptions{}) if err != nil { - t.Errorf("case %q: unexpected error: %v", test.testName, err) + t.Errorf("unexpected error: %v", err) } if test.expectUpdate != nil { if e, a := test.expectUpdate, actualEndpoints; !reflect.DeepEqual(e, a) { - t.Errorf("case %q: expected update:\n%#v\ngot:\n%#v\n", test.testName, e, a) + t.Errorf("expected update:\n%#v\ngot:\n%#v\n", e, a) } } for _, key := range fakeLeases.GetUpdatedKeys() { if key == test.ip { - t.Errorf("case %q: Found ip %s in leases but shouldn't be there", test.testName, key) + t.Errorf("Found ip %s in leases but shouldn't be there", key) } } })