Skip to content

Commit

Permalink
New rule: validate async intervals
Browse files Browse the repository at this point in the history
The linter now supports new optional rul (turned off by default). This
rule validates the async functions' timeout and polling arguments.

The linter checks for three things:
1. best effort: if the time interval is known (when using integr
   literals or `time.Duration` values), the linter validates that
   the timeout is not shorter than the polling interval.
2. Forces the timeout and the polling arguments of the `Eventually` and
   `Consistently` functions, to be of type `time.Duration`.

   Best effort auto-fix: in cases that the linter can do that, e.g. for
   an integer value, it suggests replacing this value with a multiply
   with time.Second.
3. Forces up to one timeout and up to one polling arguments.
  • Loading branch information
nunnatsa committed Mar 17, 2024
1 parent 4ade2b4 commit 2973d69
Show file tree
Hide file tree
Showing 15 changed files with 650 additions and 33 deletions.
69 changes: 69 additions & 0 deletions README.md
Expand Up @@ -227,6 +227,28 @@ ginkgolinter checks the following:
* If the first parameter is a function with the format of `func(error)bool`, ginkgolinter makes sure that the second
parameter exists and its type is string.

### Async timing interval: timeout is shorter than polling interval [Bug]
***Note***: Only applied when the `suppress-async-assertion` flag is **not set** *and* the `validate-async-intervals`
flag **is** set.

***Note***: This rule work with best-effort approach. It can't find many cases, like const defined not in the same
package, or when using variables.

The timeout and polling intervals may be passed as optional arguments to the `Eventually` or `Constanly` functions, or
using the `WithTimeout` or , `Within` methods (timeout), and `WithPolling` or `ProbeEvery` methods (polling).

This rule checks if the async (`Eventually` or `Consistently`) timeout duration, is not shorter than the polling interval.

For example:
```go
Eventually(aFunc).WithTimeout(500 * time.Millisecond).WithPolling(10 * time.Second).Should(Succeed())
```

This will probably happen when using the old format:
```go
Eventually(aFunc, 500 * time.Millisecond /*timeout*/, 10 * time.Second /*polling*/).Should(Succeed())
```

### Wrong Length Assertion [STYLE]
The linter finds assertion of the golang built-in `len` function, with all kind of matchers, while there are already
gomega matchers for these usecases; We want to assert the item, rather than its length.
Expand Down Expand Up @@ -381,6 +403,53 @@ This rule support auto fixing.

***This rule is disabled by default***. Use the `--force-expect-to=true` command line flag to enable it.

### Async timing interval: multiple timeout or polling intervals [Style]
***Note***: Only applied when the `suppress-async-assertion` flag is **not set** *and* the `validate-async-intervals`
flag **is** set.

The timeout and polling intervals may be passed as optional arguments to the `Eventually` or `Constanly` functions, or
using the `WithTimeout` or , `Within` methods (timeout), and `WithPolling` or `ProbeEvery` methods (polling).

The linter checks that there is up to one polling argument and up to one timeout argument.

For example:

```go
// both WithTimeout() and Within()
Eventually(aFunc).WithTimeout(time.Second * 10).Within(time.Second * 10).WithPolling(time.Millisecond * 500).Should(BeTrue())
// both polling argument, and WithPolling() method
Eventually(aFunc, time.Second*10, time.Millisecond * 500).WithPolling(time.Millisecond * 500).Should(BeTrue())
```

### Async timing interval: non-time.Duration intervals [Bug]
***Note***: Only applied when the `suppress-async-assertion` flag is **not set** *and* the `validate-async-intervals`
flag **is** set.

gomega supports a few formats for timeout and polling intervals, when using the old format (the last two parameters of Eventually and Constantly):
* a `time.Duration` value
* any kind of numeric value (int(8/16/32/64), uint(8/16/32/64) or float(32/64), as the number of seconds.
* duration string like `"12s"`

The linter triggers a warning for any duration value that is not of the `time.Duration` type, assuming that this is
the desired type, given the type of the argument of the newer "WithTimeout", "WithPolling", "Within" and "ProbeEvery"
methods.

For example:
```go
Eventually(func() bool { return true }, "1s").Should(BeTrue())
Eventually(context.Background(), func() bool { return true }, time.Second*60, float64(2)).Should(BeTrue())
```

This rule offers a limited auto fix: for integer values, or integer consts, the linter will suggest multiply the
value with `time.Second`; e.g.
```go
const polling = 1
Eventually(aFunc, 5, polling)
```
will be changed to:
```go
Eventually(aFunc, time.Second*5, time.Second*polling)
```
## Suppress the linter
### Suppress warning from command line
* Use the `--suppress-len-assertion=true` flag to suppress the wrong length and cap assertions warning
Expand Down
1 change: 1 addition & 0 deletions analyzer.go
Expand Up @@ -37,6 +37,7 @@ func NewAnalyzer() *analysis.Analyzer {
a.Flags.Var(&config.SuppressErr, "suppress-err-assertion", "Suppress warning for wrong error assertions")
a.Flags.Var(&config.SuppressCompare, "suppress-compare-assertion", "Suppress warning for wrong comparison assertions")
a.Flags.Var(&config.SuppressAsync, "suppress-async-assertion", "Suppress warning for function call in async assertion, like Eventually")
a.Flags.Var(&config.ValidateAsyncIntervals, "validate-async-intervals", "best effort validation of async intervals (timeout and polling); ignored the suppress-async-assertion flag is true")
a.Flags.Var(&config.SuppressTypeCompare, "suppress-type-compare-assertion", "Suppress warning for comparing values from different types, like int32 and uint32")
a.Flags.Var(&config.AllowHaveLen0, "allow-havelen-0", "Do not warn for HaveLen(0); default = false")
a.Flags.Var(&config.ForceExpectTo, "force-expect-to", "force using `Expect` with `To`, `ToNot` or `NotTo`. reject using `Expect` with `Should` or `ShouldNot`; default = false (not forced)")
Expand Down
5 changes: 5 additions & 0 deletions analyzer_test.go
Expand Up @@ -151,6 +151,11 @@ func TestFlags(t *testing.T) {
testData: []string{"a/forceExpectTo"},
flags: map[string]string{"force-expect-to": "true"},
},
{
testName: "check async timing intervals",
testData: []string{"a/timing"},
flags: map[string]string{"validate-async-intervals": "true"},
},
} {
t.Run(tc.testName, func(tt *testing.T) {
analyzer := ginkgolinter.NewAnalyzer()
Expand Down
29 changes: 27 additions & 2 deletions doc.go
Expand Up @@ -8,7 +8,7 @@ or
version: %s
currently, the linter searches for following:
* trigger a warning when using Eventually or Constantly with a function call. This is in order to prevent the case when
* trigger a warning when using Eventually or Consistently with a function call. This is in order to prevent the case when
using a function call instead of a function. Function call returns a value only once, and so the original value
is tested again and again and is never changed. [Bug]
Expand All @@ -24,6 +24,12 @@ currently, the linter searches for following:
* trigger a warning when using the Equal or the BeIdentical matcher with two different types, as these matchers will
fail in runtime.
* async timing interval: timeout is shorter than polling interval [Bug]
For example:
Eventually(aFunc).WithTimeout(500 * time.Millisecond).WithPolling(10 * time.Second).Should(Succeed())
This will probably happen when using the old format:
Eventually(aFunc, 500 * time.Millisecond, 10 * time.Second).Should(Succeed())
* wrong length assertions. We want to assert the item rather than its length. [Style]
For example:
Expect(len(x)).Should(Equal(1))
Expand Down Expand Up @@ -56,5 +62,24 @@ This should be replaced with:
* replaces HaveLen(0) with BeEmpty() [Style]
* replaces Expect(...).Should(...) with Expect(...).To() [stype]
* replaces Expect(...).Should(...) with Expect(...).To() [Style]
* async timing interval: multiple timeout or polling interval [Style]
For example:
Eventually(context.Background(), func() bool { return true }, time.Second*10).WithTimeout(time.Second * 10).WithPolling(time.Millisecond * 500).Should(BeTrue())
Eventually(context.Background(), func() bool { return true }, time.Second*10).Within(time.Second * 10).WithPolling(time.Millisecond * 500).Should(BeTrue())
Eventually(func() bool { return true }, time.Second*10, 500*time.Millisecond).WithPolling(time.Millisecond * 500).Should(BeTrue())
Eventually(func() bool { return true }, time.Second*10, 500*time.Millisecond).ProbeEvery(time.Millisecond * 500).Should(BeTrue())
* async timing interval: non-time.Duration intervals [Style]
gomega supports a few formats for timeout and polling intervals, when using the old format (the last two parameters of Eventually and Constantly):
* time.Duration
* any kind of numeric value, as number of seconds
* duration string like "12s"
The linter triggers a warning for any duration value that is not of the time.Duration type, assuming that this is
the desired type, given the type of the argument of the newer "WithTimeout", "WithPolling", "Within" and "ProbeEvery"
methods.
For example:
Eventually(context.Background(), func() bool { return true }, "1s").Should(BeTrue())
Eventually(context.Background(), func() bool { return true }, time.Second*60, 15).Should(BeTrue())
`

0 comments on commit 2973d69

Please sign in to comment.