[Enhancement] Lint Eventually timeout and polling durations

[thediveo]

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

Being terribly stupid and writing an asynchronous expectation, where the polling/probing interval is larger than the overall timeout. For instance, from a real-world gotcha:

Eventually(buff.String).Within(2 * time.Second).ProbeEvery(100 * time.Second).Should(...)

Describe the solution you'd like

Linting the overall timeout set by Within/WithTimeout and the polling interval using ProbeEvery/WithPolling to be consistent in the sense that the polling interval is at most as large as the timeout.

Describe alternatives you've considered

Stopping doing stupid coding mistakes in unit tests.

[nunnatsa]

Thank @thediveo - this is nice enhancement. I labeled it with good first issue.

I think the tricky part here is to understand where are the timing parameters (the timeout and the polling time). There are few options - as parameters in the Eventually function, or as parameters in Within, WithTimeout, ProbeEvery, or WithPolling. The challenge is the first option, because the parameters can be in different locations. e.g. if there is a context parameter, which is an optional first parameter, then the location of the timing parameters may changed.

[thediveo]

I have to admit that I have no clue about linters (ASTs are for birds, aren't they?)...

[nunnatsa]

Requirements:

The linter should handle all the valid formats:

1. with or without context:

   Eventually(func() bool { return true }, 1, 2).Should(gomega.BeTrue())
   Eventually(context.ToDo(), func() bool { return true }, 1, 2).Should(gomega.BeTrue())

2. With timeout and polling as parameters

   Eventually(func() bool { return true }, 1, 2).Should(gomega.BeTrue())

3. with timeout as parameter and polling in WithPolling or ProbeEvery

   Eventually(func() bool { return true }, 1).WithPolling(2).Should(gomega.BeTrue())

4. timeout in Within or WithTimeout, polling in WithPolling or ProbeEvery

   Eventually(func() bool { return true }).WithTimeout(1).WithPolling(2).Should(gomega.BeTrue())

5. when using the Eventually parameters, the parameters may be:
   1. any kind of numeric (u/int/8/16/32/64, float32/64) - the duration will be the number * seconds
   2. a time.Duration.
   3. a valid duration string, like "10s" - the linter will trigger an error if the value is not a valid duration string.
   4. take into account that any of the above option can be a variable or a constant that defined elsewhere; e.g.

      const myTestTimeout = 10 * time.Second

The linter will:

1. ignore if there only timeout or only polling interval
2. compute the timeout and the polling as duration, and compare them. The linter will trigger an error if the computed timeout duration is shorter than the polling interval one.
3. the linter will trigger an error if there are multiple timeouts, or multiple polling. e.g. both Eventually parameter and a With method, for the same interval type, or Within and WithTimeout or ProbeEvery and `WithPolling , multiple calls to the same type of With method.
4. enforce both interval to be > 0

[nunnatsa]

@onsi - WDYT?

[onsi]

This makes sense to me - there is one final gotcha which is that the user can override the global timeouts with: https://github.com/onsi/gomega/blob/master/gomega_dsl.go#L474-L492

[nunnatsa]

I think that for now I will ignore global/default settings.

Currently I want to only check explicit intervals.

[onsi]

SGTM

[nunnatsa]

@thediveo , @onsi - after looking into it, this is not so simple suggestion (but a cool one (: ).

The main challenge is the timing arguments of the Eventually function. There are many cases where we won't be able to compute the duration value on build time - if part of the expression is a variable, e.g.

Eventually(..., time.Second * someNumericVeriable).

The With... function only receive time.Duration so there a fewer options.

What I'm going to is:

If the argument is not a duration, the linter will trigger an error. Numeric and string values is supported, but we can say we prefer time.Duration. This can be configurable.

For duration values, (both in Eventually itself and the With... methods), if the argument value is a variable, or an expression that contains variable, the linter will ignore it.