Comparing structs should use reflect.DeepEqual

[slester]

This test fails with the current code, as it checks if two structs are equal by using the comparison operator == instead of using reflect's DeepEqual().

"DeepEqual tests for deep equality. It uses normal == equality where possible but will scan elements of arrays, slices, maps, and fields of structs. In maps, keys are compared with == but elements use deep equality. DeepEqual correctly handles recursive types. Functions are equal only if they are both nil. An empty slice is not equal to a nil slice."
http://golang.org/pkg/reflect/#DeepEqual

[kytrinyx]

That depends on the implementation. I think here that if the clock type is a fmt.Stringer then it will behave correctly using ==.

[dchapes]

I'm afraid you're both wrong.
Equality is well defined in Go and the test correctly is testing for the requirement that "[t]wo clocks that represent the same time should be equal to each other". That is, it's a stated requirement that for c1, c2, c3 := New(1,2), New(1,2), New(0,0).Add(62) that c1 == c2 && c1 == c3 just as a, b, c := 42, 42, 40+2 has a == b && a == c without DeepEqual. The requirement just means implementations cannot return a pointer from New.

fmt.Stringer doesn't enter into it. It's irrelevant that c1.String() == c2.String() just as it's irrelevant that strconv.Itoa(a) == strconv.Itoa(b).

[kytrinyx]

Ah, thanks for clarifying, @dchapes I completely misunderstood.

[soniakeys]

The readme does seem pretty clear about "equal." It seems fair enough that the test program interprets this as equal by Go comparable types. I agree it's fine to leave things as they are.

As an alternative, I suppose we could add a comment to the test file that Clock types must be comparable in the sense of the Go language specification. But we're not always that rigorous in specifying how the readme is interpreted by the test program.

It might be common though that people think first of using a struct for a defined type, and make the constructor return a pointer. Leaving == and != in the test program might teach or remind some people that they can define comparable types. This is a good lesson, but if it seems peripheral to the problem, @slester's PR moves this lesson to nitpicking. If we feel strongly about generalizing test programs to allow the widest range of solutions, we might still consider accepting this.

[dchapes]

@soniakeys, IMO the c1 == c2 requirement is a fine one to have (e.g. just as it's useful and expected to be able to use == with two time.Time or time.Duration variables) and it would be a mistake to remove the requirement (and definitely a mistake to remove the test without also removing the readme requirement).

I agree it might be useful to add some info to the test failure report to point people in the correct direction. Pointers and values can be confusing for people coming from certain other languages that don't have the concept of an efficient struct {h, m int} value. IMO, that's also another good reason to have this requirement, so that people might learn that.