Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

🏃 tests: move NewWithT(t) to inside the t.Run where applicable #2799

Merged
merged 1 commit into from
Mar 27, 2020
Merged

🏃 tests: move NewWithT(t) to inside the t.Run where applicable #2799

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 22 additions & 8 deletions api/v1alpha2/conversion_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,10 +42,10 @@ func TestFuzzyConversion(t *testing.T) {
}

func TestConvertCluster(t *testing.T) {
g := NewWithT(t)

t.Run("to hub", func(t *testing.T) {
t.Run("should convert the first value in Status.APIEndpoints to Spec.ControlPlaneEndpoint", func(t *testing.T) {
g := NewWithT(t)

src := &Cluster{
Status: ClusterStatus{
APIEndpoints: []APIEndpoint{
Expand All @@ -66,6 +66,8 @@ func TestConvertCluster(t *testing.T) {

t.Run("from hub", func(t *testing.T) {
t.Run("preserves fields from hub version", func(t *testing.T) {
g := NewWithT(t)

src := &v1alpha3.Cluster{
ObjectMeta: metav1.ObjectMeta{
Name: "hub",
Expand All @@ -92,6 +94,8 @@ func TestConvertCluster(t *testing.T) {
})

t.Run("should convert Spec.ControlPlaneEndpoint to Status.APIEndpoints[0]", func(t *testing.T) {
g := NewWithT(t)

src := &v1alpha3.Cluster{
Spec: v1alpha3.ClusterSpec{
ControlPlaneEndpoint: v1alpha3.APIEndpoint{
Expand All @@ -110,10 +114,10 @@ func TestConvertCluster(t *testing.T) {
}

func TestConvertMachine(t *testing.T) {
g := NewWithT(t)

t.Run("to hub", func(t *testing.T) {
t.Run("should convert the Spec.ClusterName from label", func(t *testing.T) {
g := NewWithT(t)

src := &Machine{
ObjectMeta: metav1.ObjectMeta{
Labels: map[string]string{
Expand All @@ -130,6 +134,8 @@ func TestConvertMachine(t *testing.T) {

t.Run("from hub", func(t *testing.T) {
t.Run("preserves fields from hub version", func(t *testing.T) {
g := NewWithT(t)

failureDomain := "my failure domain"
src := &v1alpha3.Machine{
ObjectMeta: metav1.ObjectMeta{
Expand Down Expand Up @@ -159,10 +165,10 @@ func TestConvertMachine(t *testing.T) {
}

func TestConvertMachineSet(t *testing.T) {
g := NewWithT(t)

t.Run("to hub", func(t *testing.T) {
t.Run("should convert the Spec.ClusterName from label", func(t *testing.T) {
g := NewWithT(t)

src := &MachineSet{
ObjectMeta: metav1.ObjectMeta{
Labels: map[string]string{
Expand All @@ -180,6 +186,8 @@ func TestConvertMachineSet(t *testing.T) {

t.Run("from hub", func(t *testing.T) {
t.Run("preserves field from hub version", func(t *testing.T) {
g := NewWithT(t)

src := &v1alpha3.MachineSet{
ObjectMeta: metav1.ObjectMeta{
Name: "hub",
Expand Down Expand Up @@ -208,10 +216,10 @@ func TestConvertMachineSet(t *testing.T) {
}

func TestConvertMachineDeployment(t *testing.T) {
g := NewWithT(t)

t.Run("to hub", func(t *testing.T) {
t.Run("should convert the Spec.ClusterName from label", func(t *testing.T) {
g := NewWithT(t)

src := &MachineDeployment{
ObjectMeta: metav1.ObjectMeta{
Labels: map[string]string{
Expand All @@ -227,6 +235,8 @@ func TestConvertMachineDeployment(t *testing.T) {
})

t.Run("should convert the annotations", func(t *testing.T) {
g := NewWithT(t)

src := &MachineDeployment{
ObjectMeta: metav1.ObjectMeta{
Annotations: map[string]string{
Expand All @@ -249,6 +259,8 @@ func TestConvertMachineDeployment(t *testing.T) {

t.Run("from hub", func(t *testing.T) {
t.Run("preserves fields from hub version", func(t *testing.T) {
g := NewWithT(t)

src := &v1alpha3.MachineDeployment{
ObjectMeta: metav1.ObjectMeta{
Name: "hub",
Expand Down Expand Up @@ -278,6 +290,8 @@ func TestConvertMachineDeployment(t *testing.T) {
})

t.Run("should convert the annotations", func(t *testing.T) {
g := NewWithT(t)

src := &v1alpha3.MachineDeployment{
ObjectMeta: metav1.ObjectMeta{
Annotations: map[string]string{
Expand Down
5 changes: 2 additions & 3 deletions bootstrap/kubeadm/api/v1alpha2/conversion_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,10 +27,10 @@ import (
)

func TestConvertKubeadmConfig(t *testing.T) {
g := NewWithT(t)

t.Run("from hub", func(t *testing.T) {
t.Run("preserves fields from hub version", func(t *testing.T) {
g := NewWithT(t)

src := &v1alpha3.KubeadmConfig{
ObjectMeta: metav1.ObjectMeta{
Name: "hub",
Expand All @@ -52,6 +52,5 @@ func TestConvertKubeadmConfig(t *testing.T) {
g.Expect(restored.Status.Ready).To(Equal(src.Status.Ready))
g.Expect(restored.Status.DataSecretName).To(Equal(src.Status.DataSecretName))
})

})
}
44 changes: 22 additions & 22 deletions bootstrap/kubeadm/controllers/kubeadmconfig_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -362,8 +362,6 @@ func TestKubeadmConfigReconciler_Reconcile_ReturnNilIfAssociatedClusterIsNotFoun

// If the control plane isn't initialized then there is no cluster for either a worker or control plane node to join.
func TestKubeadmConfigReconciler_Reconcile_RequeueJoiningNodesIfControlPlaneNotInitialized(t *testing.T) {
g := NewWithT(t)

cluster := newCluster("cluster")
cluster.Status.InfrastructureReady = true

Expand Down Expand Up @@ -409,6 +407,8 @@ func TestKubeadmConfigReconciler_Reconcile_RequeueJoiningNodesIfControlPlaneNotI
}
for _, tc := range testcases {
t.Run(tc.name, func(t *testing.T) {
g := NewWithT(t)

myclient := fake.NewFakeClientWithScheme(setupScheme(), tc.objects...)

k := &KubeadmConfigReconciler{
Expand Down Expand Up @@ -552,8 +552,6 @@ func TestKubeadmConfigReconciler_Reconcile_RequeueIfControlPlaneIsMissingAPIEndp
}

func TestReconcileIfJoinNodesAndControlPlaneIsReady(t *testing.T) {
g := NewWithT(t)

cluster := newCluster("cluster")
cluster.Status.InfrastructureReady = true
cluster.Status.ControlPlaneInitialized = true
Expand Down Expand Up @@ -596,6 +594,8 @@ func TestReconcileIfJoinNodesAndControlPlaneIsReady(t *testing.T) {
for _, rt := range useCases {
rt := rt // pin!
t.Run(rt.name, func(t *testing.T) {
g := NewWithT(t)

config := rt.configBuilder(rt.machine, rt.configName)

objects := []runtime.Object{
Expand Down Expand Up @@ -638,8 +638,6 @@ func TestReconcileIfJoinNodesAndControlPlaneIsReady(t *testing.T) {
}

func TestReconcileIfJoinNodePoolsAndControlPlaneIsReady(t *testing.T) {
g := NewWithT(t)

_ = feature.MutableGates.Set("MachinePool=true")

cluster := newCluster("cluster")
Expand Down Expand Up @@ -672,6 +670,8 @@ func TestReconcileIfJoinNodePoolsAndControlPlaneIsReady(t *testing.T) {
for _, rt := range useCases {
rt := rt // pin!
t.Run(rt.name, func(t *testing.T) {
g := NewWithT(t)

config := rt.configBuilder(rt.machinePool, rt.configName)

objects := []runtime.Object{
Expand Down Expand Up @@ -862,9 +862,7 @@ func TestBootstrapTokenTTLExtension(t *testing.T) {
}

// Ensure the discovery portion of the JoinConfiguration gets generated correctly.
func TestKubeadmConfigReconciler_Reconcile_DisocveryReconcileBehaviors(t *testing.T) {
g := NewWithT(t)

func TestKubeadmConfigReconciler_Reconcile_DiscoveryReconcileBehaviors(t *testing.T) {
k := &KubeadmConfigReconciler{
Log: log.Log,
Client: fake.NewFakeClientWithScheme(setupScheme()),
Expand All @@ -890,7 +888,7 @@ func TestKubeadmConfigReconciler_Reconcile_DisocveryReconcileBehaviors(t *testin
name string
cluster *clusterv1.Cluster
config *bootstrapv1.KubeadmConfig
validateDiscovery func(*bootstrapv1.KubeadmConfig) error
validateDiscovery func(*WithT, *bootstrapv1.KubeadmConfig) error
}{
{
name: "Automatically generate token if discovery not specified",
Expand All @@ -902,7 +900,7 @@ func TestKubeadmConfigReconciler_Reconcile_DisocveryReconcileBehaviors(t *testin
},
},
},
validateDiscovery: func(c *bootstrapv1.KubeadmConfig) error {
validateDiscovery: func(g *WithT, c *bootstrapv1.KubeadmConfig) error {
d := c.Spec.JoinConfiguration.Discovery
g.Expect(d.BootstrapToken).NotTo(BeNil())
g.Expect(d.BootstrapToken.Token).NotTo(Equal(""))
Expand All @@ -923,7 +921,7 @@ func TestKubeadmConfigReconciler_Reconcile_DisocveryReconcileBehaviors(t *testin
},
},
},
validateDiscovery: func(c *bootstrapv1.KubeadmConfig) error {
validateDiscovery: func(g *WithT, c *bootstrapv1.KubeadmConfig) error {
d := c.Spec.JoinConfiguration.Discovery
g.Expect(d.BootstrapToken).To(BeNil())
return nil
Expand All @@ -944,7 +942,7 @@ func TestKubeadmConfigReconciler_Reconcile_DisocveryReconcileBehaviors(t *testin
},
},
},
validateDiscovery: func(c *bootstrapv1.KubeadmConfig) error {
validateDiscovery: func(g *WithT, c *bootstrapv1.KubeadmConfig) error {
d := c.Spec.JoinConfiguration.Discovery
g.Expect(d.BootstrapToken.APIServerEndpoint).To(Equal("bar.com:6443"))
return nil
Expand All @@ -965,7 +963,7 @@ func TestKubeadmConfigReconciler_Reconcile_DisocveryReconcileBehaviors(t *testin
},
},
},
validateDiscovery: func(c *bootstrapv1.KubeadmConfig) error {
validateDiscovery: func(g *WithT, c *bootstrapv1.KubeadmConfig) error {
d := c.Spec.JoinConfiguration.Discovery
g.Expect(d.BootstrapToken.Token).To(Equal("abcdef.0123456789abcdef"))
return nil
Expand All @@ -985,7 +983,7 @@ func TestKubeadmConfigReconciler_Reconcile_DisocveryReconcileBehaviors(t *testin
},
},
},
validateDiscovery: func(c *bootstrapv1.KubeadmConfig) error {
validateDiscovery: func(g *WithT, c *bootstrapv1.KubeadmConfig) error {
d := c.Spec.JoinConfiguration.Discovery
g.Expect(reflect.DeepEqual(d.BootstrapToken.CACertHashes, dummyCAHash)).To(BeTrue())
return nil
Expand All @@ -995,19 +993,19 @@ func TestKubeadmConfigReconciler_Reconcile_DisocveryReconcileBehaviors(t *testin

for _, tc := range testcases {
t.Run(tc.name, func(t *testing.T) {
g := NewWithT(t)

err := k.reconcileDiscovery(context.Background(), tc.cluster, tc.config, secret.Certificates{})
g.Expect(err).NotTo(HaveOccurred())

err = tc.validateDiscovery(tc.config)
err = tc.validateDiscovery(g, tc.config)
g.Expect(err).NotTo(HaveOccurred())
})
}
}

// Test failure cases for the discovery reconcile function.
func TestKubeadmConfigReconciler_Reconcile_DisocveryReconcileFailureBehaviors(t *testing.T) {
g := NewWithT(t)

k := &KubeadmConfigReconciler{
Log: log.Log,
Client: nil,
Expand Down Expand Up @@ -1037,6 +1035,8 @@ func TestKubeadmConfigReconciler_Reconcile_DisocveryReconcileFailureBehaviors(t

for _, tc := range testcases {
t.Run(tc.name, func(t *testing.T) {
g := NewWithT(t)

err := k.reconcileDiscovery(context.Background(), tc.cluster, tc.config, secret.Certificates{})
g.Expect(err).To(HaveOccurred())
})
Expand All @@ -1045,8 +1045,6 @@ func TestKubeadmConfigReconciler_Reconcile_DisocveryReconcileFailureBehaviors(t

// Set cluster configuration defaults based on dynamic values from the cluster object.
func TestKubeadmConfigReconciler_Reconcile_DynamicDefaultsForClusterConfiguration(t *testing.T) {
g := NewWithT(t)

k := &KubeadmConfigReconciler{
Log: log.Log,
Client: nil,
Expand Down Expand Up @@ -1123,6 +1121,8 @@ func TestKubeadmConfigReconciler_Reconcile_DynamicDefaultsForClusterConfiguratio

for _, tc := range testcases {
t.Run(tc.name, func(t *testing.T) {
g := NewWithT(t)

k.reconcileTopLevelObjectSettings(tc.cluster, tc.machine, tc.config)

g.Expect(tc.config.Spec.ClusterConfiguration.ControlPlaneEndpoint).To(Equal("myControlPlaneEndpoint:6443"))
Expand All @@ -1137,8 +1137,6 @@ func TestKubeadmConfigReconciler_Reconcile_DynamicDefaultsForClusterConfiguratio

// Allow users to skip CA Verification if they *really* want to.
func TestKubeadmConfigReconciler_Reconcile_AlwaysCheckCAVerificationUnlessRequestedToSkip(t *testing.T) {
g := NewWithT(t)

// Setup work for an initialized cluster
clusterName := "my-cluster"
cluster := newCluster(clusterName)
Expand Down Expand Up @@ -1194,6 +1192,8 @@ func TestKubeadmConfigReconciler_Reconcile_AlwaysCheckCAVerificationUnlessReques
}
for _, tc := range testcases {
t.Run(tc.name, func(t *testing.T) {
g := NewWithT(t)

myclient := fake.NewFakeClientWithScheme(setupScheme(), objects...)
reconciler := KubeadmConfigReconciler{
Client: myclient,
Expand Down
4 changes: 2 additions & 2 deletions bootstrap/kubeadm/internal/cloudinit/controlplane_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,6 @@ import (
)

func TestTemplateYAMLIndent(t *testing.T) {
g := NewWithT(t)

testcases := []struct {
name string
input string
Expand All @@ -48,6 +46,8 @@ func TestTemplateYAMLIndent(t *testing.T) {
for _, tc := range testcases {
tc := tc
t.Run(tc.name, func(t *testing.T) {
g := NewWithT(t)

g.Expect(templateYAMLIndent(tc.indent, tc.input)).To(Equal(tc.expected))
})
}
Expand Down
8 changes: 6 additions & 2 deletions bootstrap/kubeadm/internal/locking/control_plane_init_mutex_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,8 @@ func TestControlPlaneInitMutex_Lock(t *testing.T) {
for _, tc := range tests {
tc := tc
t.Run(tc.name, func(t *testing.T) {
gs := NewWithT(t)
Copy link
Member

@neolit123 neolit123 Mar 27, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there a reason for using gs here instead of g?
is the original g near the function prolog needed?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was a suggestion of mine as there is a g in the outer scope that this was then shadowing, thought it might make it more obvious to use a different name for the variable

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#2799 (review)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we need the parent g?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the cases where gs is used, yes, the parent g is required

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, for the reason that Joel explained


l := &ControlPlaneInitMutex{
log: log.Log,
client: tc.client,
Expand All @@ -128,7 +130,7 @@ func TestControlPlaneInitMutex_Lock(t *testing.T) {
},
}

g.Expect(l.Lock(context.Background(), cluster, machine)).To(Equal(tc.shouldAcquire))
gs.Expect(l.Lock(context.Background(), cluster, machine)).To(Equal(tc.shouldAcquire))
})
}
}
Expand Down Expand Up @@ -192,6 +194,8 @@ func TestControlPlaneInitMutex_UnLock(t *testing.T) {
for _, tc := range tests {
tc := tc
t.Run(tc.name, func(t *testing.T) {
gs := NewWithT(t)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto


l := &ControlPlaneInitMutex{
log: log.Log,
client: tc.client,
Expand All @@ -205,7 +209,7 @@ func TestControlPlaneInitMutex_UnLock(t *testing.T) {
},
}

g.Expect(l.Unlock(context.Background(), cluster)).To(Equal(tc.shouldRelease))
gs.Expect(l.Unlock(context.Background(), cluster)).To(Equal(tc.shouldRelease))
})
}
}
Expand Down
Loading