[Enhancement] detect incorrect global assertions in Eventually

[aecay]

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

There are two ways of passing functions to gomega's Eventually. One is to pass a function that returns values, and assert on the return values. The other is to pass a function that does assertions itself (link). In the latter case, the function should take g Gomega as an argument and call g.Expect in order to assert.

Failing to do this will assert against the "global" gomega object -- which will only pass (or fail) the first time and will not continue to poll the assertions. This is a bug, and it can be quite timing dependent at runtime (e.g. if your local dev machine is fast enough that the async assertion passes the first time, but the machine where CI runs is slower and so the first call to the assertion fails).

Describe the solution you'd like

We can catch this in the linter, by looking for code of the form:

Eventually(func () {
    // ...
    Expect(...) // (or Ω) <-- this is what the linter should alert on
}).Should(...)

This should even be auto-fixable (if the func is 0 arity at least) by adding the g Gomega argument and converting all the calls to Expect to g.Expect.

I don't know if the linter can see the source of a function that is passed by name to Eventually (like Eventually(someFuncThatMightCallExpect)) -- if it can, we can find erroneous global calls to Expect in that function as well. But even catching the error when the function is a literal, as in the earlier example, would be helpful.

Describe alternatives you've considered

I don't know of any other potential way to prevent or detect this error before runtime.

Additional context

n/a

[nunnatsa]

Thanks you so much for your suggestion, @aecay!

This idea was discussed before (#85).

Using the general Expect is a valid usage for real use-cases - for cases we don't wan't to retry, but to exit with error immediately, e.g. in case of a non-recoverable error. The ginkgolinter has no way to distinguish between these cases.

I tend to close the issue. WDYT?

[aecay]

Apologies, I missed the prior discussion.

I have never needed to bail out early from an Eventually in the way that you describe. But I can see how the code you describe would have that effect.

But gomega provides StopTrying(...).Now() to accomplish the same thing: link. I would tend to say that if you need to bail out early, you should use StopTrying since it is more explicit, and can't be mistaken for a bug (calling Expect when you should call g.Expect).

I guess this is getting into stylistic territory (rather than strictly finding bugs), since global Expect is equally valid as a way of signaling an early exit. But IMO it's worthwhile to provide a way for the linter to catch global Expect bugs, even if it requires stylistic concessions elsewhere in the code. Potentially the check could not be enabled by default though, to avoid annoyingly triggering lint errors in codebases that use another style. WDYT?