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

Add WithinTimeRange method #1188

Merged
merged 6 commits into from Jun 28, 2022

Conversation

moolmanruan
Copy link
Contributor

@moolmanruan moolmanruan commented May 23, 2022

Summary

Add WithinTimeRange and WithinTimeRangef assertion methods.

Changes

Adds the WithinTimeRange assertion function that checks whether the expected time is on or after the start time and on or before the end time. If end is before start the function also fails.

Adds WithinTimeRange and WithTimeRangef methods on *Assertions that calls the WithinTimeRange function internally.

Motivation

There are cases where one would like to test that a time value is within a certain range, for example when the code being tested updates a timestamp:

before := time.Now()
actual := Foo()
after := time.Now()

assert.WithinTimeRange(t, actual.timeStamp, before, after)

Currently, the only time-related assertion method is WithinDuration, but in the above mentioned situation it leads to one of the following solutions:

  • Using two assert.True checks against the before and after times.
before := time.Now()
actual := Foo()
after := time.Now()

assert.True(t, actual.timeStamp.After(before))
assert.True(t, actual.timeStamp.Before(after))
  • Deriving a duration from the before and after times to use in the WithDuration assertion.
before := time.Now()
actual := Foo()
after := time.Now()
halfDur := after.Sub(before) / 2

assert.WithinDuration(t, actual.timeStamp, before.Add(halfDur), halfDur)
  • Using WithDuration to assert against time.Now() with an arbitrary duration, leading to flaky tests.
before := time.Now()
actual := Foo()
after := time.Now()

assert.WithinDuration(t, actual.timeStamp, time.Now(), time.Second)

Related issues

#689

@@ -1109,6 +1109,27 @@ func WithinDuration(t TestingT, expected, actual time.Time, delta time.Duration,
return true
}

// WithinTimeRange asserts that a time is within a time range (inclusive).
//
// assert.WithinTimeRange(t, time.Now(), time.Now(), time.Now())
Copy link
Collaborator

@boyan-soubachov boyan-soubachov Jun 27, 2022

Choose a reason for hiding this comment

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

I'm of the opinion that this example isn't the most intuitive. Would it make more sense if we changed it to something like:

assert.WithinTimeRange(t, time.Now(), time.Now().Sub(time.Second()), time.Now().Add(time.Second()))

If you can think of a simpler example that would explicitly demonstrate the differences between start and end, that would be awesome :)

Copy link
Contributor Author

@moolmanruan moolmanruan Jun 27, 2022

Choose a reason for hiding this comment

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

Yeah, I'll think about it a bit and have a look at other examples to see if there's a better way to present it.

Copy link
Contributor Author

@moolmanruan moolmanruan Jun 27, 2022

Choose a reason for hiding this comment

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

Changed to assert.WithinTimeRange(t, time.Now(), time.Now().Add(-time.Second), time.Now().Add(time.Second)).

// WithinTimeRange asserts that a time is within a time range (inclusive).
//
// assert.WithinTimeRange(t, time.Now(), time.Now(), time.Now())
func WithinTimeRange(t TestingT, expected, start, end time.Time, msgAndArgs ...interface{}) bool {
Copy link
Collaborator

@boyan-soubachov boyan-soubachov Jun 27, 2022

Choose a reason for hiding this comment

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

  1. For consistency with WithinDuration, should this function not be named WithinRange, the time.Time arguments explicitly state that the range being tested is a time range?
  2. Semantically, the 2nd argument should be called actual, not expected since it's the value we're testing

Copy link
Contributor Author

@moolmanruan moolmanruan Jun 27, 2022

Choose a reason for hiding this comment

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

  1. Sure. I added the Time to be more specific (since duration is a time related word, but range is not), but if it's not required I'm more than happy to change the naming to WithinRange.
  2. You are right, will make the change.

assert/assertions.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@boyan-soubachov boyan-soubachov left a comment

Looks great! Thank you for your contribution.

I hope you're enjoying your time at Luno, I had a blast there :)

@boyan-soubachov boyan-soubachov merged commit 2fab6df into stretchr:master Jun 28, 2022
3 checks passed
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