feat(execute): test support duration for agg window #3177
Conversation
ec8fa10 to
662ca04
Compare
execute/window_test.go
Outdated
| name: "simple months", | ||
| w: MustWindow( | ||
| values.ConvertDurationMonths(5), | ||
| values.ConvertDurationMonths(5), | ||
| values.ConvertDurationMonths(0)), | ||
| t: values.ConvertTime(time.Date(1970, time.January, 1, 0, 0, 0, 0, time.UTC)), | ||
| want: execute.Bounds{ | ||
| Start: values.ConvertTime(time.Date(1970, time.January, 1, 0, 0, 0, 0, time.UTC)), | ||
| Stop: values.ConvertTime(time.Date(1970, time.June, 1, 0, 0, 0, 0, time.UTC)), | ||
| }, |
There was a problem hiding this comment.
This is a great test case. What bounds does it output for a time that is before Jan 1, 1970? Can you add that test case too?
There was a problem hiding this comment.
Yeah, I updated the offset test to test this.
execute/window_test.go
Outdated
| want: execute.Bounds{ | ||
| Start: values.ConvertTime(time.Date(1970, time.January, 2, 4, 30, 0, 0, time.UTC)), | ||
| Stop: values.ConvertTime(time.Date(1970, time.June, 2, 4, 30, 0, 0, time.UTC)), | ||
| }, |
There was a problem hiding this comment.
Can you explain why it outputs these bounds? Maybe I don't understand how monthly offsets work, but I would have expected a window with period equal to offset to be the same as a window with that same period and no offset.
There was a problem hiding this comment.
Whoa, sorry I thought I fixed that. Should be fixed now.
662ca04 to
4f26f22
Compare
| func MustWindow(every, period, offset execute.Duration, months bool) execute.Window { | ||
| w, err := execute.NewWindow(every, period, offset, months) |
There was a problem hiding this comment.
@jlapacik the fix for that bug you saw requires adding awareness of nanoseconds vs months when creating the window.
| func NewWindow(every, period, offset Duration, months bool) (Window, error) { | ||
| if !months { | ||
| // Normalize nanosecond offsets to a small positive duration | ||
| offset = offset.Normalize(every) | ||
| } |
There was a problem hiding this comment.
@jlapacik we need to know if we're working with nanoseconds or not so that we can know whether to scale durations here or not.
There was a problem hiding this comment.
I see so it wasn't a bug in GetEarliestBounds? Just in how we were creating the window in the first place? Is it possible to add that test case back in?
There was a problem hiding this comment.
Sure, I can add another case that's similar to the initial one.
4f26f22 to
afe54e5
Compare
Codecov Report
@@ Coverage Diff @@
## master #3177 +/- ##
==========================================
- Coverage 50.40% 50.38% -0.03%
==========================================
Files 337 337
Lines 41237 41246 +9
==========================================
- Hits 20786 20782 -4
- Misses 17989 18003 +14
+ Partials 2462 2461 -1
Continue to review full report at Codecov.
|
afe54e5 to
fcd36a2
Compare
| name: "months with equivalent offset", | ||
| w: MustWindow( | ||
| values.ConvertDurationMonths(5), | ||
| values.ConvertDurationMonths(5), | ||
| values.ConvertDurationMonths(5), true), | ||
| t: values.ConvertTime(time.Date(1970, time.January, 1, 0, 0, 0, 0, time.UTC)), | ||
| want: execute.Bounds{ | ||
| Start: values.ConvertTime(time.Date(1970, time.January, 1, 0, 0, 0, 0, time.UTC)), | ||
| Stop: values.ConvertTime(time.Date(1970, time.June, 1, 0, 0, 0, 0, time.UTC)), | ||
| }, |
There was a problem hiding this comment.
@jlapacik here's that original test case.
Closes #3025.
Adding testing to ensure that windows using monthly durations set bounds properly.
Done checklist