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

cmpopts: add EquateApproxTime #155

Closed
rogpeppe opened this issue Aug 20, 2019 · 5 comments · Fixed by #158
Closed

cmpopts: add EquateApproxTime #155

rogpeppe opened this issue Aug 20, 2019 · 5 comments · Fixed by #158

Comments

@rogpeppe
Copy link
Contributor

I've needed this enough times (and it's just non-trivial enough) that perhaps it might be useful enough to add to the cmpopts package:

// EquateApproxTime returns a Comparer option that
// determines two time.Time values to be equal if they
// are within the given time interval of one another.
//
// The zero time is treated specially: it is only considered
// equal to another zero time value.
func EquateApproxTime(margin time.Duration) cmp.Option {
	return cmp.Comparer(func(t0, t1 time.Time) bool {
		if t0.IsZero() && t1.IsZero() {
			return true
		}
		if t0.IsZero() || t1.IsZero() {
			return false
		}
		diff := t0.Sub(t1)
		if diff < 0 {
			diff = -diff
		}
		return diff <= margin
	})
}
@dnephin
Copy link

dnephin commented Aug 20, 2019

I've implemented a version of this as well, and have used it often: https://github.com/gotestyourself/gotest.tools/blob/master/assert/opt/opt.go#L34-L46

@dsnet
Copy link
Collaborator

dsnet commented Aug 20, 2019

\cc @neild

Adding this as a helper seems reasonable. For prior art, there exists a EquateApprox function which supports both specifying "approximateness" by fraction or by margin. Should a similar set of knobs exist for EqualApproxTime? Why or why not?

Personally, I think margin alone is fine. Time has a unit and a pre-determined scale. On the other hand, floats are fundamentally unit-less and has no pre-determined scale, where specifying the fraction seems more useful. Also, what does fraction even mean for time? It must assume some form of epoch, but people will always disagree what the right epoch is.

rogpeppe added a commit to rogpeppe-contrib/go-cmp that referenced this issue Aug 22, 2019
@rogpeppe
Copy link
Contributor Author

Personally, I think margin alone is fine. Time has a unit and a pre-determined scale. On the other hand, floats are fundamentally unit-less and has no pre-determined scale, where specifying the fraction seems more useful. Also, what does fraction even mean for time? It must assume some form of epoch, but people will always disagree what the right epoch is.

@dsnet I agree with this reasoning - I don't think fraction is useful here.
I proposed #158 to add this function.

@neild
Copy link
Collaborator

neild commented Aug 22, 2019

I agree that margin alone is fine; I don't think fraction is likely to be useful for times.

I wonder about the use case for approximate time comparisons. This function seems tempting to use in tests, but difficult to use in a non-racy fashion. I can easily imagine a test along the lines of "add an event to a log, check that the time of the event is approximately now", for example, which is going to be flaky when the test runs unexpectedly slowly.

@rogpeppe
Copy link
Contributor Author

Fundamentally, using this is racy, yes. Just as specifying any timeout in a test is racy. But timeouts are still useful, because it's rare to have arbitrary delays lasting for multiple seconds, and similarly I find it useful to be able to sanity check timestamp values within the same kind of order of duration as one might use for a timeout.

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 a pull request may close this issue.

4 participants