Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add Compare using go-cmp #546

Merged
merged 3 commits into from
Apr 27, 2022

Conversation

xiantank
Copy link
Contributor

Add new matcher Compare, which use go-cmp to compare and we can pass cmp.Option as optional arguments.
here is example

Expect([]int{1, 2}).Should(Compare([]int{1, 2}))
Expect(myCustomType{s: "foo", n: 3, f: 2.0, arr: []string{"a", "b"}}).Should(Compare(myCustomType{s: "foo", n: 3, f: 2.0, arr: []string{"a", "b"}}, cmp.AllowUnexported(myCustomType{})))
Expect(myCustomType{s: "abc", n: 3, f: 2.0, arr: []string{"a", "b"}}).Should(Compare(myCustomType{s: "foo", n: 3, f: 2.0, arr: []string{"a", "b"}}, cmpopts.IgnoreUnexported(myCustomType{})))

related: #216, #264

@xiantank xiantank mentioned this pull request Apr 23, 2022
@thediveo
Copy link
Collaborator

Just my tiny observation as I happened to look at go-cmp just these days (while working on checking file descriptor objects for equality). I remember that go-cmp thankfully documents its purposedly designed behavior of panicking, because it is intended to work without any matcher framework. It cannot rely on the usual error handling pattern that the Gomega matchers uses: matchers do not only return their "verdict" but also an error return value. I would, erm, expect that without handling panics the Compare matcher cannot be used in inverse expectations such as Expect(foo).NotTo(Compare()). If I didn't overlook something in your PR code (and please forgive me if I do and correct me please) then there's a inverse check using Equal but not one using Compare?

@xiantank
Copy link
Contributor Author

Hi, I add some panic handling to make Compare matcher as usual error handling pattern, and remove a weird inverse check Equal (which I tried to show difference between Equal and Compare deal with different time location, but it's really weird to do so, thanks for pointing it out).
// please let me know if I misunderstood anything

@onsi
Copy link
Owner

onsi commented Apr 26, 2022

hey @xiantank - can we rename it to BeComparableTo? That would flow better for the DSL Expect(foo).To(BeComparableTo(bar))

I think I'd, eventually, like to make it possible to configure Gomega so that Equal can be backed by go-cmp instead of reflect.DeepEqual. This is a great step in that direction, thanks!

@xiantank xiantank force-pushed the feat/compare-gocmp-implement branch from 8824ca4 to 9906c15 Compare April 27, 2022 01:03
@onsi onsi merged commit e77ea75 into onsi:master Apr 27, 2022
@onsi
Copy link
Owner

onsi commented Apr 27, 2022

thanks! looks good to me!

@onsi
Copy link
Owner

onsi commented Apr 27, 2022

if you want to update the doc PR I can pull that in too

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants