Add warning if `(Async)Assertion.{Should*,*To*}` is not called

[timebertt]

It's easy to forget to call Should on async assertions when performing expectations in an async func, e.g.

var _ = Describe("sample", func() {
    It("asynchronous", func() {
        Eventually(func(g Gomega) {
            err := fmt.Errorf("foo")
            g.Expect(err).NotTo(HaveOccurred()) // fails
        })
        // oops, forgot .Should(Succeed())
        fmt.Println("Success")
    })
})

With this, the assertion always silently succeeds. Hence, the test code is wrong.

To reduce the likelihood of such erroneous test code, gomega could print a warning or even fail the assertion if neither Should nor ShouldNot are called.

[thediveo]

I'm wondering for quite some time now if it is possible at all to detect that neither Should nor ShouldNot haven't been called...

One idea that comes to my mind might be registering any AsyncAssertion implementing instances and these then need to cross themselves off the list of "leaky" AsyncAssertions when Should/ShouldNot has been properly called. I'm unsure when to check for "leaked" AsyncAssertions; an early check would be when creating the next AsyncAssertion, but otherwise at the end of the test specification ... but that might be a tough problem as it would effectively create a hard dependency on Ginkgo. That doesn't look good, as I've already seen Go modules that use Gomega, but not Ginkgo, but instead something else for their specifications.

[thediveo]

That's probably more for a linter?

[timebertt]

I just realized this applies to Expect as well:

err := fmt.Errorf("foo")
Expect(err)
// oops, forgot .To(Succeed())

So this is rather a general thing and not specific to AsyncAssertion.

[timebertt]

I'm wondering for quite some time now if it is possible at all to detect that neither Should nor ShouldNot haven't been called...

Yeah, I was wondering about that as well. That's why I opened this issue. Maybe someone comes up with a clever idea :)

That's probably more for a linter?

True, a linter would also be my fallback option.

[onsi]

sorry for the delay y'all. I do think this is better suited for a linter. @thediveo has captured the options that come to mind for me as well and I agree about their limitations.

[timebertt]

Thanks for the feedback! I explored the linter idea in https://github.com/gardener/gardener/pull/6455. I think this is a nice approach. If others are interested in using/augmenting it, we could move this to some other place.

[ecordell]

@timebertt I found this thread when trying to see if anyone had written exactly the tool you wrote. I would be interested in having it more easily available - would you be open to contributing it to golangci-lint, or would you be okay with someone else adding it?

[timebertt]

@ecordell I would be happy to see this linter move to a community-owned place to allow easier use and independent enhancements. That being said, I don't have the capacity to facilitate the move myself. But I would be more than happy if someone takes it on from here.

[slatteryjim]

I had an idea for checking this automatically in plain Go tests, using t.Cleanup.

Any helper function can call t.Cleanup to schedule a function to be called at the end of the test (or subtest).

Calling Expect could prepare an AsyncAssertion with a wrapper around it that would set a flag if any of the methods were called (like To, Should, etc.). Expect would then also schedule a t.Cleanup function to check if the AsyncAssertion had any methods called on it by the end of the test. If not, it would fail the test with t.Error.

In that way, any naked Expect calls would cause a test failure.