RFC on error assertions for functions with multi-return values and trailing error value

[thediveo]

This is a request for comment on extending Gomega to more smoothly handle (phrase) error assertions for functions which return multiple values, including a trailing error. While working on my Gomega 3rd party extension "errxpect" (https://github.com/TheDiveO/errxpect) I noticed some things and I would like to get feedback on ways to do it better or if gomega could be opened to 3rd party implementors of gomega.Assertion.

So, instead of:

    _, _, err := foo(42)
    Expect(err).To(MatchError("DOH!"))

simply spell it out as:

    Errxpect(foo(42)).To(MatchError("DOH!"))

Things I noticed and had to work around:

• Gomega's internal implementation of gomega.Assertion basically does two separate things of relevance to our discussion here:

  • it implements the important logic for calling a fail handler and generating useful stack context information -- which is something that any 3rd party implementation of the gomega.Assertion probably would like to tap into ("inherit").
  • specific multi-return value checking in form of a single actual parameter, and a extra... parameters. Please note that this check conflicts with my specific usecase of having a non-zero error, or a zero error with potentially non-zero other return values.

• Gomega's ExpectWithOffset pattern cannot be applied to multi-return value functions. This actually can be easily remedied with a generic WithOffset(o int) function added to the gomega.Assertion interface. In my errexpect module I return a more specially typed assertion object, which then offers WithOffset(...), so I don't need to touch Gomega's Assertion interface.

My questions for feedback:

• At the moment I have to get Gomega's assertion functionality for triggering fail handlers and producing stack context information by running a transient, separate assertion when checking the multiple return values for calling the user's matcher(s). Could you imagine opening up the functionality for triggering fail handlers and stack context traces to 3rd party implementers of Assertion?

• With me coming up with Errxpect I'm throwing stones in my glasshouse ... but since ExpectWithOffset(...) slightly stutters and with Option receivers being all the rage, could you imagine a generic Assertion.WithOffset(o), so Gomega's assertions/expectations can also be spelled out as Expect(...).WithOffset(1).To(...)?

[onsi]

Hey @thediveo sorry for the delay.

I like pulling out .WithOffset and, more broadly, appreciate the problem you're trying to solve around Gomega better handling multiple return values. I wonder if there's a clean backwards compatible (i.e. can we avoid a major version bump) way to extend Expect to handle multiple return values more explicitly - and basically pull in what you're accomplishing with Errxpect into the core.

Here's a first sketch of what this might look like. We keep the default behavior but extend Assertion and AsyncAssertion to enable users to explicitly call out individual return values and then apply assertions. So maybe something like:

Expect(foo(42)).WithOffset(2).FirstValue().To(Equal(41))
                           .NthValue(1).To(Equal("42"))
                           .LastValue().To(MatchError("DOH!"))

I think that could be pulled off in a largely backward compatible way. Thoughts?

[thediveo]

Looks good to me! Errxpect (sorry for the name) was kind of a proof of concept on my side trying to mangle things into Gomega without me having any clue, and before starting to ask questions here.

[onsi]

......any interest in pulling together a PR? 😉 😬

[thediveo]

hehe, I can try, but right now I'm lost as to how the FirstValue etc. would actually work?

[onsi]

I'm imagining it looks like overhauling the implementation over at https://github.com/onsi/gomega/blob/master/internal/assertion/assertion.go to enable the chaining I outlined above. I can get to this eventually, so no worries - though i'm trying to focus my free coding time (which there isn't too much of due to covid and in-person schooling being closed!) on Ginkgo 2.0 right now... but if you have time to give it a shot, I'd start there.

[thediveo]

Here's a first sketch of what this might look like. We keep the default behavior but extend Assertion and AsyncAssertion to enable users to explicitly call out individual return values and then apply assertions. So maybe something like:

Expect(foo(42)).WithOffset(2).FirstValue().To(Equal(41))
                           .NthValue(1).To(Equal("42"))
                           .LastValue().To(MatchError("DOH!"))

I think that could be pulled off in a largely backward compatible way. Thoughts?

After finally getting more into the details I recon that today To, NotTo & co all return a bool, so that breaks chaining. I don't know how much the return value is used as of today, but I'm hesitant to break this unless you want to do so for v2.

However, if we can live with "only" a single pick per Expectation: how do you envision to handle the vetting of "extra" arguments in light of picking?

Aside from these questions: for chaining, would it be okay to introduce to assertions an "original-derived" relationship? So that NthValue would return a new derived internal.Assertion that still remembers its original internal.Assertion. A further NthValue on the derived assertion then could return another new assertion, but based on the original assertion. What do you think?

[onsi]

hey @thediveo thanks for bumping this. I've learned a lot working on Ginkgo 2.0 since my original comment - a pattern/approach that I've found to be flexible and powerful is to use Typed Parameters to extend the DSL. It's how the new Ginkgo 2.0 decorators are implemented. I think it would apply nicely here.

The basic idea would be to extend To(), NotTo(), etc.. to take arbitrary inputs and then use the types of those inputs to decide how to handle them. This would allow us to do stuff like:

Expect(foo(42)).To(Equal(41), HaveNthValue(1, Equal("42")), HaveLastValue(MatchError("DOH!"))

Under the hood I imagine a new type:

type GomegaIndexer struct {
    IndexRange [2]int
    Matcher GomegaMatcher
}

And DSL functions like HaveFirstValue(), HaveSecondValue(), HaveNthValue(), HaveLastValue() that configure and return a GomegaIndexer. Gomega's .To() etc. family of functions would then have the signature .To(args ...interface{}) and walk through the args and type-checked them to generate a final set of GomegaIndexers, automatically wrapping any bare matchers with HaveFirstValue(). When the assertion actually runs, the indexers are used to pick the correct arguments and the matches are ANDed together.

This would have some additional benefits. You could finally do stuff like:

Expect(42).To(BeNumerically(">", 40), BeNumerically("<", 42))

without having to use the And matcher-combiner. To me this reads fairly intuitively.

All this allows us to maintain the current behavior, including the (somewhat gnarly) optionalDescription stuff that exists today. It would also allow for future extensions without boxing us in today.

On the Expect side of the equation, I think we need to add extensions like .WithTimeout(), .WithInterval(), and .WithOffset() in the way you have (thanks for the PR, btw, I'll take a look today). Since Expect takes functions that can potentially return multiple arguments the Go compiler does not currently converge combining splats like

Expect(multiReturnFunction(), WithOffset(2), WithTimeout(time.Minute)).To(...))

This will return a compilation error. So the approach taken so far to extend the return value of Eventually and Expect with additional configuration methods looks good to me.

Thoughts?

[thediveo]

I've read more into your use of decorators (functions) for Ginkgo in order to get a feeling how you are intending to apply them to Gomega's realm. While I definitely see the benefit for Ginkgo (while still prefering a quickly done "FIt" over "It(..., Focus)") I'm not yet sure that applying them to Expect()ations looks as straightforward as in case of Ginkgo. Maybe, because the already existing matcher infrastructure looks like decorators to some degree.

We basically have two separate usages of Expect() with respect of function-under-test return values, based on the well-established Go error reporting idiom:

1. successful return of a set of values v1..vn, with the final value ve being nil.

   • at the moment, Gomega cannot directly express expectations for such a set of values v1..vn, whereas in case of a single struct v1 its gstruct package allows testers to express structured values expectations. Structs have the benefit of having named fields, whereas a set of function return values v1..vn hasn't. NthValue() won't give this lost expressiveness and clearness back (unless we add another annotation parameter to it).
   • based on my own, albeit limited experience, I would in most cases consider the expressiveness and clearness necessary to be easily achieved by the current practise of a single multi-value assignment v1, v2, v3, err := foo() by a set of Expect()ations about v1, v2, v3, and err. I currently lack to see the benefit of using decorators, especially due to my aforementioned concern that we loose the clear return value semantics because there aren't any names anymore.

2. error return with a set of values v1..vn that must be zero values, and a final non-zero error value (the error being quite like that famous Monty Python quote "I'm not quite dead yet!").

   • here we simply need to last value and expect everything else to be zero. This differs from the vet done by To() et al. This situation could be easily covered by chaining, so that a tester might write:

     Expect(foo()).Error().To(HaveOccurred())

   where Error() takes the assertion and checks that argument and extras are all zero, with the exception of the last extra value and returning a new asserting that now consists of only the last error value from the extras as the new actual value without any extras. In this situation, I lack to see usecases for further decorator chaining and additionally Error().To(HaveOccured()) seems to me to be more fluent and natural than To(HaveLastValue(HaveOccured()). Of course, this is rooted deeply in Go's error reporting idiom.

In your GomegaIndexer what is the function of the two-element IndexRange array? What semantics does the first element have, what semantics the second element?

[thediveo]

Just the usual after-thought about multiple return values versus named structure fields. What about something slightly crazy, such as, now also auto-checking for the usual error idiom in the sense of all values can be non-zero, but not the final one:

Expect(foo()).As("bar", "baz", "err").To(HaveValue("bar", Equal(42)), HaveValue("baz", Equal(666)))

As(...) would imply success error semantics, while Error(...) would imply failure semantics. HaveValue could have (sic) an alias HaveResult, depending on personal preferences.

[onsi]

hey - I just went back and reread the original issue up top and am realizing that I've misunderstood what you were proposing all along! I must have been reading it quickly or jumped through a bunch of unspoken assumptions on my part, but I ended up thinking the use case is "making arbitrary assertions against multiple return values within a single Expect call".

But now I see that the use case is "there are times when I want to assert that an error has occurred (or matches a specific error) and that all the other return values are nil/zero-valued" - which.... makes more sense.

Sorry for the confusion! I generally agree that v1, v2, v3, err := foo() is just clearer in the multi-return value use case.

Now that we're on the same page (my bad!) I can see adding Expect().Error() as a simple mechanism to accomplish this and we wouldn't need any additional chaining. The user is simply choosing between the default behavior "Assert on v1, ensure all others are niladic" and error-matching behavior "Assert on the last return value, ensure all others are niladic". And we could just stop there and see what further patterns emerge.

Sorry again for misreading your original issue. It's been quite the journey to get here but I think we're on the same page now!

[thediveo]

No need for sorry, as you helped me with your reasoning from a different perspective to clarify my own thinking. I cannot tell how often as a system architect I need to kick myself about unspoken and seemingly obvious assumptions...

[onsi]

lol so true. thanks! if you're still up for working on a PR that would be great. I literally just wrote some code where I thought "hey that .Error() thing would be useful here"

[thediveo]

will gladly do, especially we now have a clear view on it. If only most parts of our projects could be like this... 😆

[thediveo]

@onsi would you mind me slightly refactoring internal.Association in order to have more symmetry for error assertions, that is:

type Assertion struct {
    actuals []interface{}
    actualIndex int
    offset int
    g *Gomega
}

and then adding a vet() receiver that runs the correct vetting, either vetExtras as before, or a new vetActuals that doesn't touch the last error value. The match() receiver will then decide based on errorAssertion which actual to pass to its matcher.

Would this be fine to you? Any suggestions?

[onsi]

yes this makes sense to me and is probably how I'd approach it. thanks for checking! Doing this for AsyncAssertion might get gnarly - let me know if you'd like to think through that together as well.

[thediveo]

I pondered about AsyncAssertion and my conclusion at this time is: Error probably isn't needed as you need to write a wrapper anyway.

[onsi]

I think I buy that. It's rare that I need to assert that "eventually an error starts occurring".

[thediveo]

We can provide ItAllEndsInTears() for that.

[onsi]

thanks for the PRs @thediveo - these are great additions to Gomega!

I'll cut a new minor release soon/next week. I'm working on a new matcher that i'd like to include in the next release alongside these changes.

[thediveo]

I've noticed that I forgot to make a PR for the WithTransform enhancements, so I will do this soon. Other than that I think that we can close this RFC/feature request as successfully done. It was my pleasure!

[thediveo]

Gomega actually was my life-saver when starting with Golang, after having had a pleasant experience with Lua busted and its DSL. As I was looking around how big Go projects do it, I was steered towards Gomega and felt right at home. So thank you very much for Gomega in the first place!

[onsi]

❤️ thanks for the kind shout out!

Once you get the WithTransform PR in I'll a cut a release.

Unrelated BTW I just pushed a new HaveField() matcher that I think will make it a bit easier to write custom matchers. It'll be in the next release too!

[thediveo]

I think I buy that. It's rare that I need to assert that "eventually an error starts occurring".

I could actually see use for the opposite: Eventually(...).Error().ShouldNot(HaveOccurred()), waiting until something has settled correctly. But as you usually need to wrap anyway, I don't see (yet) any demand.

What a good feeling to close an issue as resolved 😁 🎉 .