stretchr / testify

A toolkit with common assertions and mocks that plays nicely with the standard library
MIT License
22.51k stars 1.56k forks source link

Revert "assert: ObjectsAreEqual: use time.Equal for time.Time type" #1537

Closed brackendawson closed 4 months ago

brackendawson commented 4 months ago

Summary

This reverts commit 34763e0df3d560ca247996a0e9291fa919a3abc6.

time.Time.Equal only tests that the two instances refer to the same instant, but time.Time also carries zone information, so this caused two non-equal instances to be considered equal.

Changes

Revert the change which cases assert.Equal to consider time.Time objects with different zones equal.

Motivation

Related issues

Fixes #1536

Antonboom commented 4 months ago

offtop: please introduce more obvious API or improve documentation – when people should use assert.Equal, when assert.True(t, t1.Equal(t2), when assert.EqualUTCTime(...) (or similar)

brackendawson commented 4 months ago

While you are at it, you might as well fix this:

https://github.com/stretchr/testify/blob/7b3de0842561fd516ab69ed866bc50c95a48e664/assert/assertions_test.go#L123

I think this was meant to be

  {time.Now(), time.Now(), false},

we should support testing for state first

Can you explain this please?

Coincidence, I think. That's saying that functions are not comparable, it just happens that the author chose time.Now. If we did change it to your suggestion then the test would flake if the two calls were made in the same nanosecond which seems likely.

brackendawson commented 4 months ago

offtop: please introduce more obvious API or improve documentation – when people should use assert.Equal, when assert.True(t, t1.Equal(t2), when assert.EqualUTCTime(...) (or similar)

Thanks, I think #1078 might be the best approach here? Make the failure easier to understand, rather than clutter the Equal doc with information about something you may not be trying to do.

Also for nested time.Time objects something along the lines of #843 might be a good approach?