Use go-cmp instead of reflect.DeepEqual

[ernesto-jimenez]

The package github.com/google/go-cmp is better suited for comparison in tests than reflect.DeepEqual.

We should replace our DeepEqual usage with go-cmp.

Replacement should be backwards-compatible.

[dnephin]

From what I've seen, having used go-cmp to create an assertion, there's no way to make it backwards compatible.

go-cmp requires that for each struct in a comparison that has any unexported fields you must explicitly use either AllowUnexporterted or IgnoreUnexported. This requirement seems like one of the advantages of using go-cmp instead of reflect.DeepEqual.

[mei-rune]

advantages of using go-cmp instead of reflect.DeepEqual.

result of go-cmp is OK, but result of reflect.DeepEqual is

                        Diff:
                        --- Expected
                        +++ Actual
                        @@ -12,3 +12,3 @@
                           CurrentValue: (string) (len=2) "ww",
                        -  TriggeredAt: (time.Time) 2018-02-07 08:54:01.123 +0000 UTC,
                        +  TriggeredAt: (time.Time) 2018-02-07 16:54:01.123 +0800 HKT,
                           ConfirmContent: (string) "",

[mattayes]

@dsnet might be a good person to loop into this conversation (if they're interested).

[posener]

Tried to implement this.
Used what @dsnet suggested with the deepAllowUnexported option.
The go-cmp got into an infinite recursion in the testify tests. I guess because of a cyclic list.
I opened an issue in go-cmp, but it seems like it is not going to be supported.
google/go-cmp#74

[mattayes]

@posener Do you have that branch around still?

[posener]

can push it, nothing to fancy
https://github.com/posener/testify/tree/cmp

[posener]

If this is merged, we can have go-cmp used here
google/go-cmp#78

[ernesto-jimenez]

@posener being able to use go-cmp would be fantastic. 🙌

[posener]

@ernesto-jimenez another issue here: I guess we are intending also to use the cmp.Diff function. However, it's output is different then the current one. I guess this is API breaking change. Any ideas?

[posener]

I've used my branch in go-cmp and my branch in testify.

• It now runs (no stack overflows...)
• Diff is different since go-cmp has different format.
• Types equality fails for some reason.

Here are the result of the tetify unit-tests: results.txt

[dnephin]

How are you planning on accepting https://godoc.org/github.com/google/go-cmp/cmp#Option to Equal ? Without Option, and always using deepAllowUnexported, the only advantage over reflect.DeepEqual is the change in output format.

[posener]

Another blocking issue for using go-cmp: google/go-cmp#80.

[posener]

@dnephin , I think the main advantage is that it gives more control over comparing: For example, if you want extra control on comparing time objects: ignore timezone or wall time, it could be done in the cmp.Equal but not in reflect.DeepEqual.

[dsnet]

To expand: reflect.DeepEqual does a decent job at comparisons and has serve the Go community well for many years. However, compatibility issues due to its pervasive usage has increased as the scale of Go code also increases. For example, when monotonic timestamps were added, it could not compare time.Time well as it would implicitly compare upon the unexported monotonic timestamps fields. It had no understanding of the time.Time.Equal method that should probably be used instead.

cmp.Equal tries to do a better job at this by:

1. Allowing the user to customize any portion of the comparison via Options.
2. Allowing the author of a type to specify the correct definition of comparison by implementing an Equal method.
3. Falling back on reflect.DeepEqual-like behavior.

The concept of Options itself is a powerful way to tailor the comparison to your tests exact needs. The most common cmpopts helpers are EquateApprox and EquateEmpty, which demonstrate at least two areas where people often want to customize the behavior of DeepEqual.

I should note that cmp does not claim to be the perfect comparison 100% of the time either, but the ability to specify Options allows an easy fix in the rare event of a comparison failure. For example, a type that your package depends on may add a new field that causes your comparison to fail. In some cases you would have wanted to ignore any newly added fields. In other cases, you do want to compare across newly added fields as well. Whatever decision the comparison library chooses to do will never be correct all the time, and Options help alleviate these rare (but inevitable) cases.

[dnephin]

@posener Yes I understand the value of go-cmp. I have been using it instead of reflect.DeepEqual.

My question was:

How are you planning on accepting https://godoc.org/github.com/google/go-cmp/cmp#Option to assert.Equal ?

testify/assert.Equal already has a var args, so you can't use a var args like cmp.Equal (and gotestyourself/assert).

If you only apply a static set of Options that mimic the behaviour of reflect.DeepEqual you are not actually getting any of the value of having Options.

So either there needs to be a different function added, or you break compatiblity and add an Options arg. Or is there some other option I'm missing?

[posener]

I agree - and we are also breaking API by changing the diff format.

[posener]

Rethinking:

I see two more options:

1. We can just use cmp.Equal without any user-define problems, and define "general" options that can solve issues in testify, such as time comparing.

2. We can have an ugly workaround for user define options:

• We can pass compare options to assert.New
• Then pass an object that wraps testing.T to the Equal function here
• Then have type assertion in the Equal function, and extract the options there...

[icholy]

Another option is to allow overriding default behaviour.

t.Eq = func(a, b interface{}) bool {
  return reflect.DeepEqual(a, b)
}

Do the same thing for the Diff function.

[ansel1]

Yet another option: leave the current function as is, to maintain compatibility, and add a new function which uses go-cmp (and takes options, etc). EqualEx or Equivalent or Comparable or something.

[shaunc]

This would also allow fixing issues such as:
#624

I have NaNs in structures, and want a comparison that I can use to assert equality, which I can do with go-cmp.

[e11ni]

I have similar problem with floats, in my structures there are floats that cannot be compared just for ==. Go-cmp provide approximate equality for floats.

Yet another option: leave the current function as is, to maintain compatibility, and add a new function which uses go-cmp (and takes options, etc). EqualEx or Equivalent or Comparable or something.

What about this option? I can contribute to make it happen.

[mbana]

Any news on this?

[lcd1232]

Any updates?

[allenhaozi]

The package github.com/google/go-cmp is better suited for comparison in tests than reflect.DeepEqual.

We should replace our DeepEqual usage with go-cmp.

Replacement should be backwards-compatible.

found a bad case
`

type Item struct {
Media string
Size int64
}

func main() {

standard := []Item{
	Item{
		Media: "SSD",
		Size:  100,
	},

	Item{
		Media: "HDD",
		Size:  200,
	},
}

test1 := []Item{
	Item{
		Media: "HDD",
		Size:  200,
	},
	Item{
		Media: "SSD",
		Size:  100,
	},
}

fmt.Println(cmp.Equal(standard, test1))

}`

want : true , got: fase

[posener]

I guess that is ok.
We can pass default options that will imitate the behavior of assert.Equal.

See https://github.com/stretchr/testify/pull/579/files

[cbandy]

@allenhaozi I think you are trying or want to assert with ElementsMatch.

[boyan-soubachov]

Of all the options, I think it would make sense to gauge community interest in making a testify/v2 that's based on go-cmp. That way we can see follow usage patterns. Unfortunately that also means two versions of the repo to maintain but we can use the data provided to us and start deprecating the lesser-used version where necessary over time.

I don't think we can (feasibly) get around the diff output changes without breaking the API and the point about go-cmp's power being in its options (thanks @posener) make too much sense to not expose them to our end users.

@glesica , thoughts?

[posener]

The new go-cmp Reporter option can be used to create custom diff.

[rustyx]

There is already an assertion framework based on go-cmp, gotest.tools/assert

So a project like testify/v2 might be too late, unfortunately.

[yangxikun]

When I need advance comparison, I use a.Empty(cmp.Diff(x, y, opts...)) to compose go-cmp with testify.