Struct containing a time.Time can't be compared correctly.

[leoleoasd]

#979 fixed comparison of time.Time, but struct containing time.Time still can't be compared correctly because still, we will use reflect.DeepEqual.

I'm working on a PR to fix this.

[boyan-soubachov]

Could you please add a unit test to cover the missed case. If you have issues, please let me know so that I can revert the change

[brackendawson]

I'm not sure what the success criteria are for this issue, what does "correctly" mean for comparing time.Time.

v1 uses reflect.DeepEqual and to change this would be a breaking change to anyone relying on that behaviour.

[mitar]

v1 uses reflect.DeepEqual and to change this would be a breaking change to anyone relying on that behaviour

I do not think that anyone can be relying on that behavior. Because current behavior means that nested timestamps are randomly different.

This is why time.Time defines its own Equal function:

Note that the Go == operator compares not just the time instant but also the Location and the monotonic clock reading. Therefore, Time values should not be used as map or database keys without first guaranteeing that the identical Location has been set for all values, which can be achieved through use of the UTC or Local method, and that the monotonic clock reading has been stripped by setting t = t.Round(0). In general, prefer t.Equal(u) to t == u, since t.Equal uses the most accurate comparison available and correctly handles the case when only one of its arguments has a monotonic clock reading.

I think the same holds for "expected values" in tests. See also this comment.

[brackendawson]

v1 uses reflect.DeepEqual and to change this would be a breaking change to anyone relying on that behaviour

I do not think that anyone can be relying on that behavior. Because current behavior means that nested timestamps are randomly different.

This is why time.Time defines its own Equal function:

Note that the Go == operator compares not just the time instant but also the Location and the monotonic clock reading. Therefore, Time values should not be used as map or database keys without first guaranteeing that the identical Location has been set for all values, which can be achieved through use of the UTC or Local method, and that the monotonic clock reading has been stripped by setting t = t.Round(0). In general, prefer t.Equal(u) to t == u, since t.Equal uses the most accurate comparison available and correctly handles the case when only one of its arguments has a monotonic clock reading.

I think the same holds for "expected values" in tests. See also this comment.

The location encoded in the time.Time object, which is not considered by the Equal method, is exported (public) information. Anyone writing calendar/invite software or flight booking software or any other application where the returned zone is important will be relying on the current comparison.

[mitar]

Beyond location there is also monotonic clock reading possibly in the struct. That one will never be the same unless you literally made a copy of an existing struct.

And sure, we might not want to break existing function, but there is definitely a need for having a deep-compare assertion which works on time structs.

[brackendawson]

I agree with you on that, we already have WithinDuration(t, expexted, actual, 0) which has the same result as time.Time.Equal. Possibly this needs documenting better. It is useless for struct fields though, what are your thoughts on this?

[mitar]

Ooo, struct tags. I like it.

[fedemengo]

is there a practical solution for this?