nunnatsa / ginkgolinter

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

[BUG] false positive for Eventually/Consistently #91

Closed pohly closed 1 year ago

pohly commented 1 year ago

Describe the bug

The following finding is a false positive:

test/e2e_node/dra_test.go:129:4: ginkgo-linter: use a function call in Consistently. This actually checks nothing, because Consistently receives the function returned value, instead of function itself, and this value is never changed; consider using `gomega.Consistently(ctx, e2epod.Get).WithArguments(f.ClientSet, pod).WithTimeout(podInPendingStateTimeout).Should(e2epod.BeInPhase(v1.PodPending),
    "Pod should be in Pending state as resource preparation time outed")` instead (ginkgolinter)
            gomega.Consistently(ctx, e2epod.Get(f.ClientSet, pod)).WithTimeout(podInPendingStateTimeout).Should(e2epod.BeInPhase(v1.PodPending),
            ^

It's a false positive because e2epod.Get is a function call which returns a function. That function gets called by Consistently repeatedly.

Expected behavior

I suspect this occurs because ginkgolinter checks whether the parameter is a function call expression. What it should do instead is check what the type of the expression is. Functions and channels are okay, everything else isn't.

Environment:

Additional context

This can be worked around:

diff --git a/test/e2e_node/dra_test.go b/test/e2e_node/dra_test.go
index fb3da210bf2..6ea97e91a1d 100644
--- a/test/e2e_node/dra_test.go
+++ b/test/e2e_node/dra_test.go
@@ -126,7 +126,8 @@ var _ = ginkgo.Describe("[sig-node] DRA [Feature:DynamicResourceAllocation][Node
            // TODO: Check condition or event when implemented
            // see https://github.com/kubernetes/kubernetes/issues/118468 for details
            ginkgo.By("check that pod is consistently in Pending state")
-           gomega.Consistently(ctx, e2epod.Get(f.ClientSet, pod)).WithTimeout(podInPendingStateTimeout).Should(e2epod.BeInPhase(v1.PodPending),
+           get := e2epod.Get(f.ClientSet, pod)
+           gomega.Consistently(ctx, get).WithTimeout(podInPendingStateTimeout).Should(e2epod.BeInPhase(v1.PodPending),
                "Pod should be in Pending state as resource preparation time outed")
        })
    })
nunnatsa commented 1 year ago

Thanks for reporting that @pohly.

Could you please tell what is the signature of e2epod.Get?

Asking because ginkgolinter does check for return values of function; i.e.

    case *gotypes.Signature, *gotypes.Chan, *gotypes.Pointer: // do nothing

The gotypes.Signature should cover it, in theory. So I want to try reproduce the issue.

m1kola commented 1 year ago

@nunnatsa I believe you can find the function here:

https://github.com/kubernetes/kubernetes/blob/ecd727e4c75c96fc61027fab3d356bb43bb2ce2b/test/e2e/framework/pod/get.go#L26-L31