Merge pull request kubevirt#8051 from nunnatsa/ginkgo-linter

Add ginkgo linter

BUILD.bazel

@@ -54,6 +54,7 @@ nogo(
     # that are always correct).
     # You can see the what `go vet` does by running `go doc cmd/vet`.
     deps = [
+        "//tools/analyzers/ginkgolinter:go_default_library",
         "//vendor/github.com/gordonklaus/ineffassign/pkg/ineffassign:go_default_library",
         "@org_golang_x_tools//go/analysis/passes/asmdecl:go_default_library",
         "@org_golang_x_tools//go/analysis/passes/assign:go_default_library",

go.mod

@@ -60,6 +60,7 @@ require (
 	golang.org/x/sys v0.0.0-20220209214540-3681064d5158
 	golang.org/x/term v0.0.0-20210927222741-03fcf44c2211
 	golang.org/x/time v0.0.0-20210723032227-1f47c861a9ac
+	golang.org/x/tools v0.1.9
 	google.golang.org/grpc v1.40.0
 	gopkg.in/cheggaaa/pb.v1 v1.0.28
 	gopkg.in/yaml.v2 v2.4.0
@@ -138,7 +139,6 @@ require (
 	go.mongodb.org/mongo-driver v1.8.4 // indirect
 	golang.org/x/oauth2 v0.0.0-20210819190943-2bc19b11175f // indirect
 	golang.org/x/text v0.3.7 // indirect
-	golang.org/x/tools v0.1.9 // indirect
 	google.golang.org/appengine v1.6.7 // indirect
 	google.golang.org/genproto v0.0.0-20210831024726-fe130286e0e2 // indirect
 	google.golang.org/protobuf v1.27.1 // indirect

nogo_config.json

@@ -125,5 +125,11 @@
       "vendor/": "vendor doesn't pass vet",
       "external/": "externaldoesn't pass vet"
     }
+  },
+  "ginkgolinter": {
+    "exclude_files": {
+      "vendor/": "vendor doesn't pass vet",
+      "external/": "externaldoesn't pass vet"
+    }
   }
 }

pkg/virt-api/webhooks/validating-webhook/admitters/vmi-create-admitter_test.go

@@ -3540,7 +3540,7 @@ var _ = Describe("Validating VMICreate Admitter", func() {
 			vmi.Spec.Domain.Memory = &v1.Memory{Hugepages: &v1.Hugepages{PageSize: "2Mi"}}
 			vmi.Spec.Domain.CPU.NUMA = &v1.NUMA{GuestMappingPassthrough: &v1.NUMAGuestMappingPassthrough{}}
 			causes := ValidateVirtualMachineInstanceSpec(k8sfield.NewPath("fake"), &vmi.Spec, config)
-			Expect(len(causes)).To(BeNumerically(">=", 1))
+			Expect(causes).ToNot(BeEmpty())
 			Expect(causes).To(ContainElement(metav1.StatusCause{Type: metav1.CauseTypeFieldValueRequired, Field: "fake.domain.cpu.dedicatedCpuPlacement", Message: "fake.domain.cpu.dedicatedCpuPlacement must be set to true when fake.domain.cpu.realtime is used"}))
 		})
 		It("should reject the realtime knob when NUMA Guest Mapping Passthrough is not defined", func() {
@@ -4014,7 +4014,7 @@ var _ = Describe("Function getNumberOfPodInterfaces()", func() {
 		}
 		path := k8sfield.NewPath("spec")
 		causes := webhooks.ValidateVirtualMachineInstanceHypervFeatureDependencies(path, &vmi.Spec)
-		Expect(len(causes)).To(BeNumerically(">=", 1))
+		Expect(causes).ToNot(BeEmpty())
 	})
 
 	It("Should validate VMIs with correct hyperv deps", func() {

pkg/virt-controller/watch/export/export_test.go

@@ -218,7 +218,7 @@ var _ = Describe("Export controlleer", func() {
 		Expect(err).ToNot(HaveOccurred())
 		Expect(pod).ToNot(BeNil())
 		Expect(pod.Name).To(Equal(fmt.Sprintf("%s-%s", exportPrefix, testVMExport.Name)))
-		Expect(len(pod.Spec.Volumes)).To(Equal(3), "There should be 3 volumes, one pvc, and two secrets (token and certs)")
+		Expect(pod.Spec.Volumes).To(HaveLen(3), "There should be 3 volumes, one pvc, and two secrets (token and certs)")
 		certSecretName := ""
 		for _, volume := range pod.Spec.Volumes {
 			if volume.Name == certificates {
@@ -249,8 +249,8 @@ var _ = Describe("Export controlleer", func() {
 				},
 			},
 		}))
-		Expect(len(pod.Spec.Containers)).To(Equal(1))
-		Expect(len(pod.Spec.Containers[0].VolumeMounts)).To(Equal(3))
+		Expect(pod.Spec.Containers).To(HaveLen(1))
+		Expect(pod.Spec.Containers[0].VolumeMounts).To(HaveLen(3))
 		Expect(pod.Spec.Containers[0].VolumeMounts).To(ContainElement(k8sv1.VolumeMount{
 			Name:      "test-pvc",
 			ReadOnly:  true,

pkg/virt-controller/watch/snapshot/restore_test.go

@@ -671,7 +671,7 @@ var _ = Describe("Restore controlleer", func() {
 
 				l, err := cdiClient.CdiV1beta1().DataVolumes("").List(context.Background(), metav1.ListOptions{})
 				Expect(err).ToNot(HaveOccurred())
-				Expect(len(l.Items)).To(BeZero())
+				Expect(l.Items).To(BeEmpty())
 				testutils.ExpectEvent(recorder, "VirtualMachineRestoreComplete")
 			})
 

pkg/virt-handler/device-manager/mediated_devices_types_test.go

@@ -316,14 +316,14 @@ var _ = Describe("Mediated Devices Types configuration", func() {
 						Expect(numberOfCreatedMDEVs).To(BeZero(), msg)
 					}
 				}
-				Expect(len(desiredDevicesToConfigure)).To(BeZero(), "add types should be created")
+				Expect(desiredDevicesToConfigure).To(BeEmpty(), "add types should be created")
 			}
 
 			By("removing all created mdevs")
 			mdevManager.updateMDEVTypesConfiguration([]string{})
 			files, err := ioutil.ReadDir(fakeMdevDevicesPath)
 			Expect(err).ToNot(HaveOccurred())
-			Expect(len(files)).To(BeZero())
+			Expect(files).To(BeEmpty())
 		},
 			Entry("spread types accoss identical cards", spreadTypesAccossIdenticalCard),
 			Entry("one yype many cards", oneTypeManyCards),
@@ -426,7 +426,7 @@ var _ = Describe("Mediated Devices Types configuration", func() {
 						Expect(numberOfCreatedMDEVs).To(BeZero(), msg)
 					}
 				}
-				Expect(len(desiredDevicesToConfigure)).To(BeZero(), "add types should be created")
+				Expect(desiredDevicesToConfigure).To(BeEmpty(), "add types should be created")
 			}
 
 			By("removing all created mdevs")
@@ -435,7 +435,7 @@ var _ = Describe("Mediated Devices Types configuration", func() {
 			deviceController.refreshMediatedDevicesTypes()
 			files, err := ioutil.ReadDir(fakeMdevDevicesPath)
 			Expect(err).ToNot(HaveOccurred())
-			Expect(len(files)).To(BeZero())
+			Expect(files).To(BeEmpty())
 		},
 			Entry("configure default mdev types", deafultTypesNotNodeSpecific),
 			Entry("configure mdev types that match all node selectors", matchAllNodeLabels),

pkg/virt-handler/isolation/isolation_test.go

@@ -23,8 +23,7 @@ var _ = Describe("IsolationResult", func() {
 		It("Should have mounts", func() {
 			mounts, err := isolationResult.Mounts(nil)
 			Expect(err).ToNot(HaveOccurred())
-			Expect(mounts).ToNot(BeNil())
-			Expect(len(mounts)).ToNot(BeZero())
+			Expect(mounts).ToNot(BeEmpty())
 		})
 
 		It("Should have root mounted", func() {
@@ -72,8 +71,7 @@ var _ = Describe("IsolationResult", func() {
 		It("Should have mounts", func() {
 			mounts, err := isolationResult.Mounts(nil)
 			Expect(err).ToNot(HaveOccurred())
-			Expect(mounts).ToNot(BeNil())
-			Expect(len(mounts)).ToNot(BeZero())
+			Expect(mounts).ToNot(BeEmpty())
 		})
 	})
 

staging/src/kubevirt.io/client-go/api/defaults_test.go

@@ -44,8 +44,8 @@ var _ = Describe("Defaults", func() {
 	It("should add interface and pod network by default", func() {
 		vmi := &v1.VirtualMachineInstance{}
 		v1.SetDefaults_NetworkInterface(vmi)
-		Expect(len(vmi.Spec.Domain.Devices.Interfaces)).NotTo(BeZero())
-		Expect(len(vmi.Spec.Networks)).NotTo(BeZero())
+		Expect(vmi.Spec.Domain.Devices.Interfaces).NotTo(BeEmpty())
+		Expect(vmi.Spec.Networks).NotTo(BeEmpty())
 	})
 
 	It("should default to true to all defined features", func() {

tests/infra_test.go

@@ -423,7 +423,7 @@ var _ = Describe("[Serial][sig-compute]Infrastructure", func() {
 				By("finding all kubevirt pods")
 				pods, err := virtClient.CoreV1().Pods(flags.KubeVirtInstallNamespace).List(context.Background(), metav1.ListOptions{})
 				Expect(err).ShouldNot(HaveOccurred(), "failed listing kubevirt pods")
-				Expect(len(pods.Items)).To(BeNumerically(">", 0), "no kubevirt pods found")
+				Expect(pods.Items).ToNot(BeEmpty(), "no kubevirt pods found")
 
 				By("finding all schedulable nodes")
 				schedulableNodesList := libnode.GetAllSchedulableNodes(virtClient)

tools/analyzers/ginkgolinter/BUILD.bazel

@@ -0,0 +1,9 @@
+load("@io_bazel_rules_go//go:def.bzl", "go_library")
+
+go_library(
+    name = "go_default_library",
+    srcs = ["ginkgo_linter.go"],
+    importpath = "kubevirt.io/kubevirt/tools/analyzers/ginkgolinter",
+    visibility = ["//visibility:public"],
+    deps = ["@org_golang_x_tools//go/analysis:go_default_library"],
+)

tools/analyzers/ginkgolinter/README.md

@@ -0,0 +1,39 @@
+# ginkgo-linter
+
+This is a golang linter to check usage of the ginkgo and gomega packages.
+
+ginkgo is a testing framework and gomega is its assertion package.
+
+## Linter Checks
+### Wrong Length checks
+The linter finds usage of the golang built-in `len` function, and then all kind of matchers, while there are already gomega matchers for these usecases.
+
+There are several wrong patterns:
+```go
+Expect(len(x)).To(Equal(0)) // should be Expect(x).To(BeEmpty())
+Expect(len(x)).To(BeZero()) // should be Expect(x).To(BeEmpty())
+Expect(len(x)).To(BeNumerically(">", 0)) // should be Expect(x).ToNot(BeEmpty())
+Expect(len(x)).To(BeNumerically(">=", 1)) // should be Expect(x).ToNot(BeEmpty())
+Expect(len(x)).To(BeNumerically("==", 0)) // should be Expect(x).To(BeEmpty())
+Expect(len(x)).To(BeNumerically("!=", 0)) // should be Expect(x).ToNot(BeEmpty())
+
+Expect(len(x)).To(Equal(1)) // should be Expect(x).To(HaveLen(1))
+Expect(len(x)).To(BeNumerically("==", 2)) // should be Expect(x).To(HaveLen(2))
+Expect(len(x)).To(BeNumerically("!=", 3)) // should be Expect(x).ToNot(HaveLen(3))
+```
+
+The linter supports the `Expect`, `ExpectWithOffset` and the `Ω` functions, with the `Should`, `ShouldNot`, `To`, `ToNot` and `NotTo` assertion functions.
+
+It also supports the embedded `Not()` function; e.g.
+
+`Ω(len(x)).Should(Not(Equal(4)))` => `Ω(x).ShouldNot(HaveLen(4))`
+
+Or even (double negative):
+
+`Ω(len(x)).To(Not(BeNumerically(">", 0)))` => `Ω(x).To(BeEmpty())`
+
+The linter output looks like this:
+```
+pkg/virt-handler/isolation/isolation_test.go:27:4: ginkgo-linter: wrong length check; consider using Expect(mounts).ToNot(BeEmpty()) instead (ginkgolinter)
+pkg/virt-handler/isolation/isolation_test.go:76:4: ginkgo-linter: wrong length check; consider using Expect(mounts).ToNot(BeEmpty()) instead (ginkgolinter)
+```