Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

assert: allow comparing time.Time #1149

Merged
merged 4 commits into from
Feb 15, 2022

Conversation

torkelrogstad
Copy link
Contributor

Summary

Add support for comparing time>Time and custom types derived from it.

Changes

When comparing types, see if we're comparing time.Time, or a custom type based on time.Time. If that's the case, compare their Unix timestamps, with the smallest possible resolution.

Motivation

This now allows sticking a time.Time into assert.Greater and assert.Less, which is nice. Time stamps have a natural ordering, so I'd expect this to work.

@torkelrogstad
Copy link
Contributor Author

Had to fiddle a bit with build tags to get this working across versions. Nw tests pass locally for me with both a new (1.17.6) and old (1.14.15). Looks like CI is fine as well.

Comment on lines +316 to +317
timeObj1 = obj1Value.Convert(timeType).Interface().(time.Time)
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we converting to timeType (i.e. time), then interface, then back to time?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Convert() returns a reflect.Value, Interface() takes us out of the reflection wrappers, and then finally we do a type assertion. It looks a bit convoluted, but it's all necessary!

assert/assertion_compare_legacy.go Show resolved Hide resolved
@torkelrogstad
Copy link
Contributor Author

Rebased, and added doc comments. Assumed you meant 1.16, as that's what's holding up the build tag stuff.

@torkelrogstad
Copy link
Contributor Author

Okay, this is a bit awkward... go fmt wants me to use a syntax that's not supported by earlier versions of Go, so I don't quite know how to make all Go versions happy here

@torkelrogstad
Copy link
Contributor Author

Nvm, got it working. Let's go 🚀

Copy link
Collaborator

@boyan-soubachov boyan-soubachov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you :)

@boyan-soubachov boyan-soubachov merged commit 1e36bfe into stretchr:master Feb 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants