DeepEqual comparison should tell you what's not equal

[benmoss]

Right now when you use Expect(..).To(Equal(...)) and it uses go's DeepEqual function, it doesn't really help you find what is not equal. I just spent a bunch of time trying to figure out that an interface field was using a value type instead of a pointer.

Along the way I found https://godoc.org/github.com/juju/testing/checkers#DeepEqual, which is an alternate implementation of DeepEqual that actually will show you the differences. Would you consider a PR that changed gomega Equal to use this instead?

[robdimsdale]

@benmoss would that introduce a new, third-party dependency to Gomega? If so, I foresee this causing pain for consumers of this library.

Is it better to re-implement that logic inside gomega than pull in a dependency?

[onsi]

+1 to reimplementing. This is dangerous territory... asserting equality can be tricky business in Go and relying on DeepEqual is a safe way to do so. Maintaining control over the code would be key.

[onsi]

My response wasn't very clear. I would love to have improved diff identification in Gomega. I'd want the code that does equality to be:

• a part of Gomega so we can be more responsive to issues
• forked off of Golang's deep equal
• super well-tested. I'd want to at least use Go's own tests as a starting point
• in it's own package under Gomega so it can be used as a standalone thing

Does that make sense?

[benmoss]

Yup, I'm gonna try to work on this now!

[benmoss]

ok after looking at this for a lil bit it seems way too hard to reimplement, my reflect-fu is far too lacking

[benmoss]

I ended up making my own lib for this, in case anyone wants it: https://github.com/benmoss/matchers/

The burden of the criteria needed to be included in Gomega itself was just too high, so it's better to be a buyer-beware third party thing :).

[alculquicondor]

Did you ever consider to depend on https://github.com/google/go-cmp?

[onsi]

hey go-cmp looks pretty good. I can imagine a Compare or BeComparableTo matcher that can accept go-cmp options and simply runs go-cmp in the background. I don't have a lot of bandwidth to work on that right now but would be open to a PR.

I hesitate to change the Equal matcher, though. Not without bumping the major version of Gomega...

[alculquicondor]

A separate comparer sounds good. We have set it up like this for now:

https://github.com/kubernetes-sigs/kueue/blob/main/pkg/util/testing/equal_cmp.go

[alculquicondor]

/reopen
Edit: Oops, this doesn't work in this repo :)

[onsi]

yes, exactly like that. I could also imagine a configurable flag on Gomega that would cause Equal to behave like go-cmp and so folks could try switching over more incrementally.

[YuviGold]

Hey, the Gomega equality operation still fails on time comparison as @sedyh noted on #264 (comment).
@onsi Might be another incentive to use the Google comparison package to fix this bug.

[onsi]

I'm on board with moving in this direction - but I'm a bit underwater with other projects at the moment. If there's anyone with bandwidth and interest to work on a PR I'd be happy to talk through a design that would allow users to use go-cmp with a new matcher and optionally configure Gomega's Equal matcher (and anywhere that Gomega uses Equal under the hood for comparisons - e.g. ContainElement) to use go-cmp. I think that would give folks an onramp to switch to go-cmp.

[onsi]

@xiantank just added BeComparableTo - it'll be in the next release of Gomega.

[ilearnio]

@onsi I find the description of BeComparableTo to be quite confusing. Is it capable of deep comparison of two structs?

[onsi]

Hey @ilearnio - which description are you referring to? The godocs are simply trying to say "this matcher uses gocmp.Equal to make comparisons."

[ilearnio]

@onsi Yeah, I'm referring to that description. It took me a while to understand that gocmp is a separate go module (though it was something from gomega)

[onsi]

Thanks @ilearnio - I've updated the docs to try and make it a bit clearer and pushed out a change.

[ilearnio]

@onsi Awesome! Thanks a lot bro