nunnatsa commented 1 year ago

Added New linter Rule: asserting a function call with async method

The linter now warns about using Eventually or Consistently with a function call. This is because when doing that, Eventually checks the same returned value again and again, instead of polling by calling the function.

For example:

func slowInt() int {
    time.Sleep(time.Second)
    return 42
}

...

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

In the above code, Eventually receives 42 as its argument. Then it polls this value to check if it equal 42. What we really wanted here is:

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

Now, Eventually calls the function until it returns the required value.

To suppress this warning entirely, add the --suppress-async-assertion=true command line flag, or the ginkgo-linter:ignore-async-assert-warning comment.

Additional Improvements

support WithOffset

add support in the WithOffset method. The linter previously missed the wrong pattern when WithOffset was used, e.g.

Expect(err != nil).WithOffset(1).To(BeTrue())

Now the linter checks these patterns as well.

len() comparison bug

Fix a bug, where comparing a len() result, triggers the wrong warning and replacement; e.g. Expect(len(x) == 5).Should(BeTrue()) was changed into Expect(len(x)).Should(Equal(5)). This pattern is also wrong (wrong length assertion).

The linter now suggests Expect(x).Should(HaveLen(5)) in this case.

avoiding double negative assertion

The linter now tries avoiding double negative assertion in some cases. For example, for this patter:

Expect(t).Should(Not(Equal(false)))

The linter used to change to

Expect(t).ShouldNot(BeFalse())

Now, the linter changes it to:

Expect(t).Should(BeTrue())

[x] Bug fix (non-breaking change which fixes an issue)
[x] New feature (non-breaking change which adds functionality)
[x] This change requires a documentation update

How Has This Been Tested?

[x] Added test case and related test data
[x] Update the functional test

Checklist:

[x] I have performed a self-review of my own code
[x] I have made corresponding changes to the documentation
[x] My changes generate no new warnings
[x] I have added tests that prove my fix is effective or that my feature works
[x] New and existing unit tests pass locally with my changes
[x] Any dependent changes have been merged and published in downstream modules
[x] I ran golangci-lint

coveralls commented 1 year ago

Pull Request Test Coverage Report for Build 4465513806

125 of 182 (68.68%) changed or added relevant lines in 4 files are covered.
7 unchanged lines in 2 files lost coverage.
Overall coverage decreased (-3.5%) to 83.24%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
types/config.go	2	3	66.67%
reverseassertion/reverse_assertion.go	0	5	0.0%
ginkgo_linter.go	121	137	88.32%
gomegahandler/handler.go	2	37	5.41%
<!--	Total:	125	182	68.68%	-->

Files with Coverage Reduction	New Missed Lines	%
types/config.go	1	33.33%
ginkgo_linter.go	6	88.55%
<!--	Total:	7	-->

Totals
Change from base Build 4401338548:	-3.5%
Covered Lines:	740
Relevant Lines:	889

💛 - Coveralls