Skip to content

Commit

Permalink
Add new rule: disallow focus containers
Browse files Browse the repository at this point in the history 
Focus ginkgo containers (FDescribe, FContext, FWhen and FIt) should only be use locally, on debug.

This PR new rule prevents leaking of this kind of debug code into the source code in the repo.

New suppress comment to suppress this new rule: `// ginkgo-linter:ignore-focus-container-warning`

New commandline flag to suppress this new rule: `--suppress-focus-container`
  • Loading branch information
@nunnatsa
nunnatsa committed Jul 17, 2023
1 parent da4a8a1 commit ff98919
Show file tree
Hide file tree
Showing 16 changed files with 789 additions and 104 deletions.
2 changes: 1 addition & 1 deletion .github/workflows/sanity.yml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ jobs:
- uses: actions/checkout@v3

- name: Set up Go
uses: actions/setup-go@v3
uses: actions/setup-go@v4
with:
go-version: 1.19

Expand Down
204 changes: 118 additions & 86 deletions README.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,8 @@ It is not enabled by default, though. There are two ways to run ginkgolinter wit
- ginkgolinter
```
## Linter Checks
The linter checks the gomega assertions in golang test code. Gomega may be used together with ginkgo tests, For example:
The linter checks the ginkgo and gomega assertions in golang test code. Gomega may be used together with ginkgo tests,
For example:
```go
It("should test something", func() { // It is ginkgo test case function
Expect("abcd").To(HaveLen(4), "the string should have a length of 4") // Expect is the gomega assertion
Expand All @@ -71,7 +72,105 @@ The linter checks the `Expect`, `ExpectWithOffset` and the `Ω` "actual" functio

It also supports the embedded `Not()` matcher

### Wrong Length Assertion
Some checks find actual bugs, and some are more for style.

### Using a function call in async assertion [BUG]
This rule finds an actual bug in tests, where asserting a function call in an async function; e.g. `Eventually`. For
example:
```go
func slowInt(int val) int {
time.Sleep(time.Second)
return val
}

...

It("should test that slowInt returns 42, eventually", func() {
Eventually(slowInt(42)).WithPolling(time.Millisecond * 100).WithTimeout(time.Second * 2).Equal(42)
})
```
The problem with the above code is that it **should** poll - call the function - until it returns 42, but what actually
happens is that first the function is called, and we pass `42` to `Eventually` - not the function. This is not what we
tried to do here.

The linter will suggest replacing this code by:
```go
It("should test that slowInt returns 42, eventually", func() {
Eventually(slowInt).WithArguments(42).WithPolling(time.Millisecond * 100).WithTimeout(time.Second * 2).Equal(42)
})
```

The linter suggested replacing the function call by the function name.

If function arguments are used, the linter will add the `WithArguments()` method to pass them.

Please notice that `WithArguments()` is only supported from gomenga v1.22.0.

When using an older version of gomega, change the code manually. For example:

```go
It("should test that slowInt returns 42, eventually", func() {
Eventually(func() int {
slowint(42)
}).WithPolling(time.Millisecond * 100).WithTimeout(time.Second * 2).Equal(42)
})
```

### Comparing a pointer with a value [BUG]
The linter warns when comparing a pointer with a value.
These comparisons are always wrong and will always fail.

In case of a positive assertion (`To()` or `Should()`), the test will just fail.

But the main concern is for false positive tests, when using a negative assertion (`NotTo()`, `ToNot()`, `ShouldNot()`,
`Should(Not())` etc.); e.g.
```go
num := 5
...
pNum := &num
...
Expect(pNum).ShouldNot(Equal(6))
```
This assertion will pass, but for the wrong reasons: pNum is not equal 6, not because num == 5, but because pNum is
a pointer, while `6` is an `int`.

In the case above, the linter will suggest `Expect(pNum).ShouldNot(HaveValue(Equal(6)))`

This is also right for additional matchers: `BeTrue()` and `BeFalse()`, `BeIdenticalTo()`, `BeEquivalentTo()`
and `BeNumerically`.

### Missing Assertion Method [BUG]
The linter warns when calling an "actual" method (e.g. `Expect()`, `Eventually()` etc.), without an assertion method (e.g
`Should()`, `NotTo()` etc.)

For example:
```go
// no assertion for the result
Eventually(doSomething).WithTimeout(time.Seconds * 5).WithPolling(time.Milliseconds * 100)
```

The linter will not suggest a fix for this warning.

This rule cannot be suppressed.

### Focus Container Found [BUG]
This rule finds ginkgo focus containers in the code.

ginkgo supports the `FDescribe`, `FContext`, `FWhen` and `FIt` containers to allow the developer to focus
on a specific test or set of tests during test development or debug.

For example:
```go
var _ = Describe("checking something", func() {
FIt("this test is the only one that will run", func(){
...
})
})
```

These container must not be part of the final source code, and should only be used locally by the developer.

### 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.

There are several wrong patterns:
Expand Down Expand Up @@ -102,10 +201,10 @@ The output of the linter,when finding issues, looks like this:
./testdata/src/a/a.go:18:5: ginkgo-linter: wrong length assertion; consider using `Expect("").Should(BeEmpty())` instead
./testdata/src/a/a.go:22:5: ginkgo-linter: wrong length assertion; consider using `Expect("").Should(BeEmpty())` instead
```
#### use the `HaveLen(0)` matcher.
#### use the `HaveLen(0)` matcher. [STYLE]
The linter will also warn about the `HaveLen(0)` matcher, and will suggest to replace it with `BeEmpty()`

### Wrong `nil` Assertion
### Wrong `nil` Assertion [STYLE]
The linter finds assertion of the comparison to nil, with all kind of matchers, instead of using the existing `BeNil()` matcher; We want to assert the item, rather than a comparison result.

There are several wrong patterns:
Expand All @@ -127,7 +226,7 @@ Or even (double negative):

`Ω(x != nil).Should(Not(BeTrue()))` => `Ω(x).Should(BeNil())`

### Wrong boolean Assertion
### Wrong boolean Assertion [STYLE]
The linter finds assertion using the `Equal` method, with the values of to `true` or `false`, instead
of using the existing `BeTrue()` or `BeFalse()` matcher.

Expand All @@ -141,7 +240,7 @@ It also supports the embedded `Not()` matcher; e.g.

`Ω(x).Should(Not(Equal(True)))` => `Ω(x).ShouldNot(BeTrue())`

### Wrong Error Assertion
### Wrong Error Assertion [STYLE]
The linter finds assertion of errors compared with nil, or to be equal nil, or to be nil. The linter suggests to use `Succeed` for functions or `HaveOccurred` for error values..

There are several wrong patterns:
Expand All @@ -159,7 +258,7 @@ It also supports the embedded `Not()` matcher; e.g.

`Ω(err == nil).Should(Not(BeTrue()))` => `Ω(x).Should(HaveOccurred())`

### Wrong Comparison Assertion
### Wrong Comparison Assertion [STYLE]
The linter finds assertion of boolean comparisons, which are already supported by existing gomega matchers.

The linter assumes that when compared something to literals or constants, these values should be used for the assertion,
Expand Down Expand Up @@ -198,92 +297,14 @@ Expect(x1 == c1).Should(BeTrue()) // ==> Expect(x1).Should(Equal(c1))
Expect(c1 == x1).Should(BeTrue()) // ==> Expect(x1).Should(Equal(c1))
```

### Using a function call in async assertion
This rule finds an actual bug in tests, where asserting a function call in an async function; e.g. `Eventually`. For
example:
```go
func slowInt(int val) int {
time.Sleep(time.Second)
return val
}

...

It("should test that slowInt returns 42, eventually", func() {
Eventually(slowInt(42)).WithPolling(time.Millisecond * 100).WithTimeout(time.Second * 2).Equal(42)
})
```
The problem with the above code is that it **should** poll - call the function - until it returns 42, but what actually
happens is that first the function is called, and we pass `42` to `Eventually` - not the function. This is not what we
tried to do here.

The linter will suggest replacing this code by:
```go
It("should test that slowInt returns 42, eventually", func() {
Eventually(slowInt).WithArguments(42).WithPolling(time.Millisecond * 100).WithTimeout(time.Second * 2).Equal(42)
})
```

The linter suggested replacing the function call by the function name.

If function arguments are used, the linter will add the `WithArguments()` method to pass them.

Please notice that `WithArguments()` is only supported from gomenga v1.22.0.

When using an older version of gomega, change the code manually. For example:

```go
It("should test that slowInt returns 42, eventually", func() {
Eventually(func() int {
slowint(42)
}).WithPolling(time.Millisecond * 100).WithTimeout(time.Second * 2).Equal(42)
})
```

### Comparing a pointer with a value
The linter warns when comparing a pointer with a value.
These comparisons are always wrong and will always fail.

In case of a positive assertion (`To()` or `Should()`), the test will just fail.

But the main concern is for false positive tests, when using a negative assertion (`NotTo()`, `ToNot()`, `ShouldNot()`,
`Should(Not())` etc.); e.g.
```go
num := 5
...
pNum := &num
...
Expect(pNum).ShouldNot(Equal(6))
```
This assertion will pass, but for the wrong reasons: pNum is not equal 6, not because num == 5, but because pNum is
a pointer, while `6` is an `int`.

In the case above, the linter will suggest `Expect(pNum).ShouldNot(HaveValue(Equal(6)))`

This is also right for additional matchers: `BeTrue()` and `BeFalse()`, `BeIdenticalTo()`, `BeEquivalentTo()`
and `BeNumerically`.

### Missing Assertion Method
The linter warns when calling an "actual" method (e.g. `Expect()`, `Eventually()` etc.), without an assertion method (e.g
`Should()`, `NotTo()` etc.)

For example:
```go
// no assertion for the result
Eventually(doSomething).WithTimeout(time.Seconds * 5).WithPolling(time.Milliseconds * 100)
```

The linter will not suggest a fix for this warning.

This rule cannot be suppressed.

## Suppress the linter
### Suppress warning from command line
* Use the `--suppress-len-assertion=true` flag to suppress the wrong length assertion warning
* Use the `--suppress-nil-assertion=true` flag to suppress the wrong nil assertion warning
* Use the `--suppress-err-assertion=true` flag to suppress the wrong error assertion warning
* Use the `--suppress-compare-assertion=true` flag to suppress the wrong comparison assertion warning
* Use the `--suppress-async-assertion=true` flag to suppress the function call in async assertion warning
* Use the `--suppress-focus-container=true` flag to suppress the focus container warning
* Use the `--allow-havelen-0=true` flag to avoid warnings about `HaveLen(0)`; Note: this parameter is only supported from
command line, and not from a comment.

Expand All @@ -308,6 +329,17 @@ To suppress the wrong async assertion warning, add a comment with (only)

`ginkgo-linter:ignore-async-assert-warning`.

To supress the focus container warning, add a comment with (only)

`ginkgo-linter:ignore-focus-container-warning`

Notice that this comment will not work for an anonymous variable container like
```go
// ginkgo-linter:ignore-focus-container-warning (not working!!)
var _ = FDescribe(...)
```
In this case, use the file comment (see bellow).

There are two options to use these comments:
1. If the comment is at the top of the file, supress the warning for the whole file; e.g.:
```go
Expand Down
Loading

0 comments on commit ff98919

Please sign in to comment.