testing/checkers: implement DeepEquals

testing/checkers: implement DeepEquals

After coming across the nil vs empty slice equality
problem for the nth time, I think it might be time
to bite the bullet and provide a version of DeepEquals
that treats nil slices as equal to empty slices.

This proposal copies code from the Go reflect
package with a couple of small adaptations to enable
that.

Apart from the two trivial if statements deleted to
implement the logic differences, there are also some
extra cases required because unlike the reflect
implementation, we have not got privileged access
to a value's content (we can't call Interface on values
derived from private struct members).

I'm not entirely sure if the name should remain identical
("jc.DeepEquals" vs "gc.DeepEquals" is not an obvious
distinction) but since the semantics are so similar,
perhaps that's ok.

https://code.launchpad.net/~rogpeppe/juju-core/467-checkers-deepequals/+merge/196247

(do not edit description out of merge proposal)

[rog, 2013-11-22 10:20:04]

Please take a look.

[dimitern, 2013-11-22 10:37:31]

Looks great, apart from a few minor suggestions.

https://codereview.appspot.com/30770043/diff/20001/testing/checkers/deepequal.go
File testing/checkers/deepequal.go (right):

https://codereview.appspot.com/30770043/diff/20001/testing/checkers/deepequal...
testing/checkers/deepequal.go:12: import "reflect"
import (
    "reflect"
)

https://codereview.appspot.com/30770043/diff/20001/testing/checkers/deepequal...
testing/checkers/deepequal.go:35: // if depth > 10 { panic("deepValueEqual")
}	// for debugging
Perhaps a better message is in order here, like "deepValueEqual exceeds depth
10" or something ?

https://codereview.appspot.com/30770043/diff/20001/testing/checkers/deepequal...
testing/checkers/deepequal.go:80: // No check for nil == nil here.
Why?

https://codereview.appspot.com/30770043/diff/20001/testing/checkers/deepequal...
testing/checkers/deepequal.go:100: return deepValueEqual(v1.Elem(), v2.Elem(),
visited, depth+1)
Shouldn't the pointer check and interface check behave the same? I mean if
v1.IsNil()... block above in addition to this line?

https://codereview.appspot.com/30770043/diff/20001/testing/checkers/deepequal...
testing/checkers/deepequal.go:109: // No check for nil == nil here.
Why?

https://codereview.appspot.com/30770043/diff/20001/testing/checkers/deepequal...
File testing/checkers/deepequal_test.go (right):

https://codereview.appspot.com/30770043/diff/20001/testing/checkers/deepequal...
testing/checkers/deepequal_test.go:12: .
"launchpad.net/juju-core/testing/checkers"
import (
    "testing"

    jc "launchpad.net/juju-core/testing/checkers"
)

https://codereview.appspot.com/30770043/diff/20001/testing/checkers/deepequal...
testing/checkers/deepequal_test.go:109: type _Complex struct {
_Complex is ugly, how about ComplexType?

https://codereview.appspot.com/30770043/diff/20001/testing/checkers/deepequal...
testing/checkers/deepequal_test.go:138: type UnexpT struct {
s/UnexpT/Unexported/ ?

[rog, 2013-11-22 11:49:14]

In general, when copy-pasting, I think
it's good to leave things unchanged when possible.

https://codereview.appspot.com/30770043/diff/20001/testing/checkers/deepequal.go
File testing/checkers/deepequal.go (right):

https://codereview.appspot.com/30770043/diff/20001/testing/checkers/deepequal...
testing/checkers/deepequal.go:12: import "reflect"
On 2013/11/22 10:37:32, dimitern wrote:
> import (
>     "reflect"
> )

i'll leave as much as possible identical to the original code
from the go reflect package if that's ok, as it makes it easier
to patch if that changes.

https://codereview.appspot.com/30770043/diff/20001/testing/checkers/deepequal...
testing/checkers/deepequal.go:35: // if depth > 10 { panic("deepValueEqual")
}	// for debugging
On 2013/11/22 10:37:32, dimitern wrote:
> Perhaps a better message is in order here, like "deepValueEqual exceeds depth
> 10" or something ?

it's commented out, and identical to the line in the original.

https://codereview.appspot.com/30770043/diff/20001/testing/checkers/deepequal...
testing/checkers/deepequal.go:80: // No check for nil == nil here.
On 2013/11/22 10:37:32, dimitern wrote:
> Why?

That's *the* change from the reflect package.
It's the reason we're doing this whole thing.
I thought it was worth adding a comment.

https://codereview.appspot.com/30770043/diff/20001/testing/checkers/deepequal...
testing/checkers/deepequal.go:100: return deepValueEqual(v1.Elem(), v2.Elem(),
visited, depth+1)
On 2013/11/22 10:37:32, dimitern wrote:
> Shouldn't the pointer check and interface check behave the same? I mean if
> v1.IsNil()... block above in addition to this line?

actually, i think the if v1.IsNil()... block is unnecessary,
(i might propose a change to that effect in the go stdlib)
but i'd prefer to leave this code as similar as possible to
the original reflect package implementation unless it needs
to be different.

https://codereview.appspot.com/30770043/diff/20001/testing/checkers/deepequal...
testing/checkers/deepequal.go:109: // No check for nil == nil here.
On 2013/11/22 10:37:32, dimitern wrote:
> Why?

as for the reflect.Slice implementation above.

https://codereview.appspot.com/30770043/diff/20001/testing/checkers/deepequal...
File testing/checkers/deepequal_test.go (right):

https://codereview.appspot.com/30770043/diff/20001/testing/checkers/deepequal...
testing/checkers/deepequal_test.go:12: .
"launchpad.net/juju-core/testing/checkers"
On 2013/11/22 10:37:32, dimitern wrote:
> import (
>     "testing"
> 
>     jc "launchpad.net/juju-core/testing/checkers"
> )

Again, all this code is almost byte-for-byte identical
with the code in reflect. I'd like to keep it that way.

https://codereview.appspot.com/30770043/diff/20001/testing/checkers/deepequal...
testing/checkers/deepequal_test.go:109: type _Complex struct {
On 2013/11/22 10:37:32, dimitern wrote:
> _Complex is ugly, how about ComplexType?

ditto.

https://codereview.appspot.com/30770043/diff/20001/testing/checkers/deepequal...
testing/checkers/deepequal_test.go:138: type UnexpT struct {
On 2013/11/22 10:37:32, dimitern wrote:
> s/UnexpT/Unexported/ ?

ditto.

[dimitern, 2013-11-22 12:22:16]

LGTM, for the sake of having this useful code landed. I still disagree that we
need to use either a consistent formatting across the juju-core codebase, or
extract such bits of reusable helpers as external projects (all of checkers
perhaps).