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

cpanato · 2020-03-27T11:05:00Z

What this PR does / why we need it:
move NewWithT(t) to inside the t.Run where applicable

/kind cleanup

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):

Fixes Move g:=NewWithT(t) to be inside the t.Run #2798

cc @wfernandes @fabriziopandini

JoelSpeed

Looks good to me for the most part. I've added a couple of suggestions. One being to not have multiple gs within a subtest and the other about not shadowing. There's a few places where we declare g := NewWith(t) within the subtest, when there's already a g within the outer scope, I wonder if it's worth calling the subtest ones gs instead so it's obvious this gomega is failing the subtest? WDYT? I've tried to highlight all the places I noticed this.

bootstrap/kubeadm/controllers/kubeadmconfig_controller_test.go

bootstrap/kubeadm/internal/locking/control_plane_init_mutex_test.go

cmd/clusterctl/client/config/reader_viper_test.go

cmd/clusterctl/client/config_test.go

cmd/clusterctl/client/repository/components_client_test.go

cmd/clusterctl/client/repository/client_test.go

controllers/machineset_controller_test.go

controllers/remote/cluster_test.go

controlplane/kubeadm/internal/workload_cluster_test.go

neolit123 · 2020-03-27T14:25:08Z

bootstrap/kubeadm/controllers/kubeadmconfig_controller_test.go

@@ -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(g *WithT, kubeAdm *bootstrapv1.KubeadmConfig) error


we can omit the argument names from the declaration (as it was before) func(*WithT, *bootstrapv1.KubeadmConfig) error these can be on the side of the definition.

neolit123 · 2020-03-27T14:25:55Z

bootstrap/kubeadm/internal/locking/control_plane_init_mutex_test.go

@@ -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)


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

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

#2799 (review)

do we need the parent g?

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

yes, for the reason that Joel explained

neolit123 · 2020-03-27T14:26:11Z

bootstrap/kubeadm/internal/locking/control_plane_init_mutex_test.go

@@ -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)


neolit123 · 2020-03-27T14:28:05Z

cmd/clusterctl/client/config/reader_viper_test.go

@@ -73,18 +73,20 @@ func Test_viperReader_Get(t *testing.T) {
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
+			gs := NewWithT(t)


neolit123 · 2020-03-27T14:28:14Z

cmd/clusterctl/client/config/reader_viper_test.go

@@ -122,15 +124,17 @@ func Test_viperReader_Set(t *testing.T) {
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
+			gs := NewWithT(t)


neolit123 · 2020-03-27T14:28:27Z

cmd/clusterctl/client/config_test.go

@@ -462,19 +462,21 @@ func Test_clusterctlClient_GetClusterTemplate(t *testing.T) {
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
+			gs := NewWithT(t)


neolit123 · 2020-03-27T14:28:48Z

cmd/clusterctl/client/repository/client_test.go

@@ -61,11 +61,13 @@ func Test_newRepositoryClient_LocalFileSystemRepository(t *testing.T) {
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
+			gs := NewWithT(t)


neolit123 · 2020-03-27T14:29:22Z

controllers/remote/cluster_test.go

@@ -94,28 +94,33 @@ func TestNewClusterClient(t *testing.T) {
 	g.Expect(scheme.AddToScheme(testScheme)).To(Succeed())
 	ctx := context.Background()
 	t.Run("cluster with valid kubeconfig", func(t *testing.T) {
+		gs := NewWithT(t)


neolit123 · 2020-03-27T14:29:34Z

controlplane/kubeadm/internal/workload_cluster_test.go

@@ -236,6 +234,8 @@ func TestUpdateKubeProxyImageInfo(t *testing.T) {

 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
+			gs := NewWithT(t)


neolit123

+1 on this change as NewWithT is supposed to be scoped and cheap.

JoelSpeed

Just re-reviewed now you've fixed up the comments I made @cpanato, I'm happy with this once https://github.com/kubernetes-sigs/cluster-api/pull/2799/files#r399301885 is addressed 🙂

neolit123 · 2020-03-27T14:42:21Z

/approve

k8s-ci-robot · 2020-03-27T14:42:31Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cpanato, neolit123

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [neolit123]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

cpanato · 2020-03-27T14:52:29Z

@JoelSpeed done the change. PTAL :)

JoelSpeed

Great! LGTM, thanks @cpanato

/lgtm

vincepri · 2020-03-27T15:14:33Z

/milestone v0.3.3

k8s-ci-robot added kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Mar 27, 2020

k8s-ci-robot requested review from CecileRobertMichon and fabriziopandini March 27, 2020 11:05

k8s-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Mar 27, 2020

cpanato force-pushed the GH-2798 branch from 59036db to 6578349 Compare March 27, 2020 11:32

JoelSpeed reviewed Mar 27, 2020

View reviewed changes

cpanato force-pushed the GH-2798 branch from 6578349 to a7647a3 Compare March 27, 2020 12:22

neolit123 reviewed Mar 27, 2020

View reviewed changes

JoelSpeed reviewed Mar 27, 2020

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 27, 2020

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

6b830c4

cpanato force-pushed the GH-2798 branch from a7647a3 to 6b830c4 Compare March 27, 2020 14:52

JoelSpeed reviewed Mar 27, 2020

View reviewed changes

k8s-ci-robot assigned JoelSpeed Mar 27, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 27, 2020

k8s-ci-robot merged commit 1717c75 into kubernetes-sigs:master Mar 27, 2020

cpanato deleted the GH-2798 branch March 27, 2020 15:11

k8s-ci-robot added this to the v0.3.3 milestone Mar 27, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

cpanato commented Mar 27, 2020

JoelSpeed left a comment

neolit123 Mar 27, 2020

cpanato Mar 27, 2020

cpanato Mar 27, 2020

neolit123 Mar 27, 2020 •

edited

Loading

JoelSpeed Mar 27, 2020

cpanato Mar 27, 2020

neolit123 Mar 27, 2020

JoelSpeed Mar 27, 2020

cpanato Mar 27, 2020

neolit123 Mar 27, 2020

neolit123 Mar 27, 2020

neolit123 Mar 27, 2020

neolit123 Mar 27, 2020

neolit123 Mar 27, 2020

neolit123 Mar 27, 2020

neolit123 Mar 27, 2020

neolit123 left a comment

JoelSpeed left a comment

neolit123 commented Mar 27, 2020

k8s-ci-robot commented Mar 27, 2020

cpanato commented Mar 27, 2020

JoelSpeed left a comment

vincepri commented Mar 27, 2020

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

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

Conversation

cpanato commented Mar 27, 2020

JoelSpeed left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neolit123 Mar 27, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neolit123 left a comment

Choose a reason for hiding this comment

JoelSpeed left a comment

Choose a reason for hiding this comment

neolit123 commented Mar 27, 2020

k8s-ci-robot commented Mar 27, 2020

cpanato commented Mar 27, 2020

JoelSpeed left a comment

Choose a reason for hiding this comment

vincepri commented Mar 27, 2020

neolit123 Mar 27, 2020 •

edited

Loading