nunnatsa / ginkgolinter

golang linter for ginkgo and gomega
MIT License
24 stars 6 forks source link

[Enhancement] check types for gomega.Equal #109

Closed pohly closed 9 months ago

pohly commented 9 months ago

Is your feature request related to a problem? Please describe.

gomega.Equal is strict about types. The following check will never pass:

package main

import (
    "testing"

    "github.com/onsi/gomega"
)

func TestEqual(t *testing.T) {
    g := gomega.NewWithT(t)
    var value int64
    g.Expect(value).To(gomega.Equal(0))
}
--- FAIL: TestEqual (0.00s)
    test_test.go:14: 
        Expected
            <int64>: 0
        to equal
            <int>: 0

Describe the solution you'd like

It would be great of the type of the value passed to Expect could be compared against the type of the value passed to Equal.

Additional context

This isn't a problem for tests that get executed during a pre-merge test because they'll fail. But in Kubernetes we often have to modify E2E tests which only get executed later during some periodic job. A linter check would help with such changes.

pohly commented 9 months ago

We had one such "bad" PR recently, I just don't have the link at hand. It's not a very important problem.

nunnatsa commented 9 months ago

Thanks for the offer @pohly!

Sound interesting. And I also work on several projects that use ginkgo+gomega for e2e, so I know what you mean.

I want to add that this also happen when aliasing types. e.g. from K8s' core v1:

type PersistentVolumeAccessMode string

I can imagine a test that will check something like:

Expect(someStringVar).Should(Equal(corev1.ReadWriteOnce))

So I have two questions:

  1. What do you want to offer the user when you find such code? I though about replacing Equal with BeEquivalentTo, but I'm not sure this is the best option. If we're comparing with a value, and not with a variable, maybe it's better to use casting instead?
  2. Do you want to implement this?
pohly commented 9 months ago

I'm also undecided about to recommend. I just reviewed a PR which used lots of Equal(int64(<some constant>)) and wasn't sure whether BeEquivalentTo(<some constant>) would have been better. In the end I didn't bring it up.

I won't have time to implement this check anytime soon.

nunnatsa commented 9 months ago

Another thing we should maybe add to the description, is the negative test. This is more problematic because it will always be true, and the test will not fail. e.g.

Expect(int64(1)).ShouldNot(Equal(uint32(1)))

will give false positive.

Another potential trap for the implementation is pointers and interfaces. We'll need to check what is gomega behavior in these cases.

nunnatsa commented 9 months ago

@pohly - #110 applays this feature request. Do you want to take a look? you can look at the changes in testdata to check if this is what you want.