Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
checkers: use cmp.Diff instead of DeepEqual #129
Conversation
mhilton
approved these changes
Jul 18, 2017
In general LGTM, but I think it would be good to get buy in from some heavy users before just changing the semantics.
| - &gc.CheckerInfo{Name: "DeepEquals", Params: []string{"obtained", "expected"}}, | ||
| -} | ||
| +// and a nil map equal to an empty map and that | ||
| +// it will panic if called upon to compare unexported fields. |
mhilton
Jul 18, 2017
Member
Changing the behavior of unexported fields like this seems a little too drastic a change to me. perhaps leave DeepEquals alone and generally use CmpEquals for new tests.
| + | ||
| +// CmpEquals uses cmp.Diff (see http://godoc.org/github.com/google/go-cmp/cmp#Diff) | ||
| +// to compare two values, passing opts to the comparer to enable custom | ||
| +// comparison. Passing DeepEqualsOption to CmpEquals makes it |
| + check func(params []interface{}, names []string) (result bool, error string) | ||
| +} | ||
| + | ||
| +func (c *cmpEqualsChecker) Check(params []interface{}, names []string) (result bool, error string) { |
jameinel
requested changes
Jul 18, 2017
I think we should continue to support comparing unexported fields. If you are doing blackbox tests and want to check internal state you should be able to do so. (I don't know tests doing it, but it doesn't feel like it should be forbidden)
Though I personally would probably not be trying to compare all internal state in one go, vs some subset that was relevant.
anyway, having cmp.Diff sounds great. Is there something we get from changing DeepEquals?
It's not forbidden - you just have to enable it explicitly. There are many tests currently that are doing dubious things like comparing the internal fields of a sync.Mutex. I don't believe this is intentional, and I think that our code would be better off not doing so by default. The cmp package provides great flexibility in deciding what things to check, and I believe their policy decision to disallow checking of unexported fields by default is the correct one.
I'm not sure I understand the question. The new DeepEquals implementation does use cmp.Diff. |
rogpeppe commentedJul 18, 2017
This enables the use of custom equality checkers, and
will panic by default when used on unexported fields which is
something that we should not be doing without explicit
allowance anyway.
Also add CmpEquals to allow the addition of custom equality
checker functionality.