Equality matcher should not compare types about ginkgo HOT 8 CLOSED

Ginkgo (the matcher library Gomega, actually) uses Golang's reflect.DeepEqual under the hood to do the equality matching (http://golang.org/pkg/reflect/#DeepEqual). DeepEqual's docs claim to use "==" first under the hood but a look at the code says otherwise. Looks like they perform a type assertion before attempting to do any sort of equality.

I think Gomega'sEqual matcher could use == first and then fallback on DeepEqual when that fails. That would at least work for cases where one is type aliasing primitives that work well with ==. Type aliasing a more complex type (e.g. a slice or a map or a custom type) however, would still require a cast as DeepEqual is more suited to copmaring complex types than ==.

If all goes well with == there should be a fix on master by EOD.

from ginkgo.

Hmm... so I've found a fix for this, but I'm conflicted about it. I can modify the Equality matcher so that it converts the actual value (the left hand side) to have the same value as the expected value (the right hand side) if such a conversion is possible. The converted actual is then passed in to reflect.DeepEqual.

This works with your string alias example, and doesn't break any other tests that I've written....

But it has the undesirable property that it will convert numbers... This would mean that the float64 5.1 will actually "Equal" the int 5... boo... this is terrible.

I'm inclined to leave the matcher as is: anal about types, and put the onus of converting types on to the developer. This does mean you'd need:

Ω(info.User).Should(Equal(User("guest"))

Which, while wordy, is actually nice and explicit -- (BTW I think User("guest") on the RHS would be preferred to string(info.User) on the LHS)

That said, I'm open to discussion (and would love to have some discussion) about this. I could imagine a separate equality matcher that doesn't care about types.... but I'm not sure what I'd call it.

from ginkgo.

As for the new matcher name, Be and BeEqual come to mind. It may be worth renaming the existing matcher to DeepEqual, although I understand that it would break existing code. But it's now or never, you are only going to have more projects using Gomega in the future.

from ginkgo.

I think I'd prefer Equal to be strict. Since my main concern is that
converting numbers can lead to false positives, I'll look into making the
matcher strict about not performing numerical conversions... that's what
the BeNumerically matcher is for.

On Sat, Oct 26, 2013 at 9:19 PM, Michael Klishin
[email protected]:

As for the new matcher name, Be and BeEqual come to mind. It may be worth
renaming the existing matcher to DeepEqual, although I understand that it
would break existing code. But it's now or never, you are only going to
have more projects using Gomega in the future.

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/11#issuecomment-27162601
.

from ginkgo.

I agree. What do you think about ValueEqual or BeEquivalent (for the new matcher)?

from ginkgo.

I think BeEquivalentTo works and conveys the intent (these are equivalent, but not - stritctly speaking - equal.). Will add it...

from ginkgo.

Just added BeEquivalentTo matcher to gomega:

onsi/gomega@27c7621

from ginkgo.

Thank you!

from ginkgo.