From 78bf5ba562e464b5fea53ad875c838a4b5ce5c4d Mon Sep 17 00:00:00 2001 From: Garrett Delfosse Date: Mon, 4 Dec 2023 15:28:22 +0000 Subject: [PATCH 1/6] fix: add additional logs to flaky time tests --- coderd/insights.go | 4 ++-- coderd/insights_internal_test.go | 6 ++++++ 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/coderd/insights.go b/coderd/insights.go index 7b0d98a66a073..b02dc9444aaa4 100644 --- a/coderd/insights.go +++ b/coderd/insights.go @@ -604,7 +604,7 @@ func parseInsightsStartAndEndTime(ctx context.Context, rw http.ResponseWriter, s Validations: []codersdk.ValidationError{ { Field: qp.name, - Detail: fmt.Sprintf("Query param %q must have the clock set to 00:00:00", qp.name), + Detail: fmt.Sprintf("Query param %q must have the clock set to 00:00:00, got %s", qp.name, qp.value), }, }, }) @@ -615,7 +615,7 @@ func parseInsightsStartAndEndTime(ctx context.Context, rw http.ResponseWriter, s Validations: []codersdk.ValidationError{ { Field: qp.name, - Detail: fmt.Sprintf("Query param %q must have the clock set to %02d:00:00", qp.name, h), + Detail: fmt.Sprintf("Query param %q must have the clock set to %02d:00:00, got %s", qp.name, h, qp.value), }, }, }) diff --git a/coderd/insights_internal_test.go b/coderd/insights_internal_test.go index c5fcbfbe88916..a26207246bc96 100644 --- a/coderd/insights_internal_test.go +++ b/coderd/insights_internal_test.go @@ -15,12 +15,18 @@ import ( func Test_parseInsightsStartAndEndTime(t *testing.T) { t.Parallel() + t.Logf("local: %s", time.Local) layout := insightsTimeLayout now := time.Now().UTC() + t.Logf("now: %s", now) + t.Logf("location: %s", now.Location()) y, m, d := now.Date() today := time.Date(y, m, d, 0, 0, 0, 0, time.UTC) + t.Logf("today: %s", now) thisHour := time.Date(y, m, d, now.Hour(), 0, 0, 0, time.UTC) + t.Logf("thisHour: %s", now) thisHourRoundUp := thisHour.Add(time.Hour) + t.Logf("thisHourRoundUp: %s", now) helsinki, err := time.LoadLocation("Europe/Helsinki") require.NoError(t, err) From 0d8c561d76d5cafe3152925e67e4e0f852d688d7 Mon Sep 17 00:00:00 2001 From: Garrett Delfosse Date: Mon, 4 Dec 2023 15:38:52 +0000 Subject: [PATCH 2/6] strip monotonic clock from parsing --- coderd/insights.go | 3 +++ coderd/insights_internal_test.go | 10 +++++----- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/coderd/insights.go b/coderd/insights.go index b02dc9444aaa4..6e0dd18965bc5 100644 --- a/coderd/insights.go +++ b/coderd/insights.go @@ -550,6 +550,9 @@ func parseInsightsStartAndEndTime(ctx context.Context, rw http.ResponseWriter, s {"end_time", endTimeString, &endTime}, } { t, err := time.Parse(insightsTimeLayout, qp.value) + // strip monotonic clock reading + // so presentation time and arithmetic operations are consistent + t = t.Round(0) if err != nil { httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ Message: "Query parameter has invalid value.", diff --git a/coderd/insights_internal_test.go b/coderd/insights_internal_test.go index a26207246bc96..4a7a0e6b1fb9d 100644 --- a/coderd/insights_internal_test.go +++ b/coderd/insights_internal_test.go @@ -15,18 +15,18 @@ import ( func Test_parseInsightsStartAndEndTime(t *testing.T) { t.Parallel() - t.Logf("local: %s", time.Local) + t.Logf("machine location: %s", time.Now().Location()) layout := insightsTimeLayout now := time.Now().UTC() t.Logf("now: %s", now) - t.Logf("location: %s", now.Location()) + t.Logf("now location: %s", now.Location()) y, m, d := now.Date() today := time.Date(y, m, d, 0, 0, 0, 0, time.UTC) - t.Logf("today: %s", now) + t.Logf("today: %s", today) thisHour := time.Date(y, m, d, now.Hour(), 0, 0, 0, time.UTC) - t.Logf("thisHour: %s", now) + t.Logf("thisHour: %s", thisHour) thisHourRoundUp := thisHour.Add(time.Hour) - t.Logf("thisHourRoundUp: %s", now) + t.Logf("thisHourRoundUp: %s", thisHourRoundUp) helsinki, err := time.LoadLocation("Europe/Helsinki") require.NoError(t, err) From fd4656c4af34b8e97a85ff277fff4c423987efc0 Mon Sep 17 00:00:00 2001 From: Garrett Delfosse Date: Mon, 4 Dec 2023 16:06:26 +0000 Subject: [PATCH 3/6] move stripping --- coderd/insights.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/coderd/insights.go b/coderd/insights.go index 6e0dd18965bc5..6d9b3df222277 100644 --- a/coderd/insights.go +++ b/coderd/insights.go @@ -550,9 +550,6 @@ func parseInsightsStartAndEndTime(ctx context.Context, rw http.ResponseWriter, s {"end_time", endTimeString, &endTime}, } { t, err := time.Parse(insightsTimeLayout, qp.value) - // strip monotonic clock reading - // so presentation time and arithmetic operations are consistent - t = t.Round(0) if err != nil { httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ Message: "Query parameter has invalid value.", @@ -565,6 +562,9 @@ func parseInsightsStartAndEndTime(ctx context.Context, rw http.ResponseWriter, s }) return time.Time{}, time.Time{}, false } + // strip monotonic clock reading + // so presentation time and arithmetic operations are consistent + t = t.Round(0) if t.IsZero() { httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ From f4604ce69cd1c2cde1754f54ea9457d567b57553 Mon Sep 17 00:00:00 2001 From: Garrett Delfosse Date: Mon, 4 Dec 2023 16:13:23 +0000 Subject: [PATCH 4/6] remove stripping --- coderd/insights.go | 3 --- 1 file changed, 3 deletions(-) diff --git a/coderd/insights.go b/coderd/insights.go index 6d9b3df222277..b02dc9444aaa4 100644 --- a/coderd/insights.go +++ b/coderd/insights.go @@ -562,9 +562,6 @@ func parseInsightsStartAndEndTime(ctx context.Context, rw http.ResponseWriter, s }) return time.Time{}, time.Time{}, false } - // strip monotonic clock reading - // so presentation time and arithmetic operations are consistent - t = t.Round(0) if t.IsZero() { httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ From d714695990cea658301c2281a3b298989c6e3dac Mon Sep 17 00:00:00 2001 From: Dean Sheather Date: Mon, 4 Dec 2023 16:52:34 +0000 Subject: [PATCH 5/6] Use the correct timezone --- coderd/insights.go | 3 +++ coderd/insights_internal_test.go | 39 +++++++++++++++++++++++++++++--- 2 files changed, 39 insertions(+), 3 deletions(-) diff --git a/coderd/insights.go b/coderd/insights.go index b02dc9444aaa4..c93b15e86f974 100644 --- a/coderd/insights.go +++ b/coderd/insights.go @@ -563,6 +563,9 @@ func parseInsightsStartAndEndTime(ctx context.Context, rw http.ResponseWriter, s return time.Time{}, time.Time{}, false } + // Change now to the same timezone as the parsed time. + now := now.In(t.Location()) + if t.IsZero() { httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ Message: "Query parameter has invalid value.", diff --git a/coderd/insights_internal_test.go b/coderd/insights_internal_test.go index 4a7a0e6b1fb9d..cbfea6ec41123 100644 --- a/coderd/insights_internal_test.go +++ b/coderd/insights_internal_test.go @@ -42,6 +42,16 @@ func Test_parseInsightsStartAndEndTime(t *testing.T) { wantEndTime time.Time wantOk bool }{ + { + name: "Same", + args: args{ + startTime: "2023-07-10T00:00:00Z", + endTime: "2023-07-10T00:00:00Z", + }, + wantStartTime: time.Date(2023, 7, 10, 0, 0, 0, 0, time.UTC), + wantEndTime: time.Date(2023, 7, 10, 0, 0, 0, 0, time.UTC), + wantOk: true, + }, { name: "Week", args: args{ @@ -138,6 +148,13 @@ func Test_parseInsightsStartAndEndTime(t *testing.T) { t.Run(tt.name, func(t *testing.T) { t.Parallel() + t.Log("startTime: ", tt.args.startTime) + t.Log("endTime: ", tt.args.endTime) + if tt.wantOk { + t.Log("wantStartTime: ", tt.wantStartTime) + t.Log("wantEndTime: ", tt.wantEndTime) + } + rw := httptest.NewRecorder() gotStartTime, gotEndTime, gotOk := parseInsightsStartAndEndTime(context.Background(), rw, tt.args.startTime, tt.args.endTime) @@ -145,6 +162,7 @@ func Test_parseInsightsStartAndEndTime(t *testing.T) { //nolint:bodyclose t.Log("Status: ", rw.Result().StatusCode) t.Log("Body: ", rw.Body.String()) + return } // assert.Equal is unable to test time equality with different // (but same) locations because the *time.Location names differ @@ -164,7 +182,7 @@ func Test_parseInsightsInterval_week(t *testing.T) { sydneyLoc, err := time.LoadLocation("Australia/Sydney") // Random location require.NoError(t, err) - now := time.Now() + now := time.Now().In(sydneyLoc) t.Logf("now: %s", now) y, m, d := now.Date() @@ -212,7 +230,7 @@ func Test_parseInsightsInterval_week(t *testing.T) { name: "6 days are acceptable", args: args{ startTime: sixDaysAgo.Format(layout), - endTime: thisHour.Format(layout), + endTime: stripTime(thisHour).Format(layout), }, wantOk: true, }, @@ -228,16 +246,20 @@ func Test_parseInsightsInterval_week(t *testing.T) { name: "9 days (7 + 2) are not acceptable", args: args{ startTime: nineDaysAgo.Format(layout), - endTime: thisHour.Format(layout), + endTime: stripTime(thisHour).Format(layout), }, wantOk: false, }, } for _, tt := range tests { tt := tt + t.Run(tt.name, func(t *testing.T) { t.Parallel() + t.Log("startTime: ", tt.args.startTime) + t.Log("endTime: ", tt.args.endTime) + rw := httptest.NewRecorder() startTime, endTime, ok := parseInsightsStartAndEndTime(context.Background(), rw, tt.args.startTime, tt.args.endTime) if !ok { @@ -252,6 +274,7 @@ func Test_parseInsightsInterval_week(t *testing.T) { //nolint:bodyclose t.Log("Status: ", rw.Result().StatusCode) t.Log("Body: ", rw.Body.String()) + return } if tt.wantOk { assert.Equal(t, codersdk.InsightsReportIntervalWeek, parsedInterval) @@ -302,6 +325,10 @@ func TestLastReportIntervalHasAtLeastSixDays(t *testing.T) { tc := tc t.Run(tc.name, func(t *testing.T) { t.Parallel() + + t.Log("startTime: ", tc.startTime) + t.Log("endTime: ", tc.endTime) + result := lastReportIntervalHasAtLeastSixDays(tc.startTime, tc.endTime) if result != tc.expected { t.Errorf("Expected %v, but got %v for start time %v and end time %v", tc.expected, result, tc.startTime, tc.endTime) @@ -309,3 +336,9 @@ func TestLastReportIntervalHasAtLeastSixDays(t *testing.T) { }) } } + +// stripTime strips the time from a time.Time value, but keeps the date and TZ. +func stripTime(t time.Time) time.Time { + y, m, d := t.Date() + return time.Date(y, m, d, 0, 0, 0, 0, t.Location()) +} From e30f166b6f66d96fbee95faca6913cf57340f4a2 Mon Sep 17 00:00:00 2001 From: Garrett Delfosse Date: Mon, 4 Dec 2023 16:56:59 +0000 Subject: [PATCH 6/6] pass now into function --- coderd/insights.go | 10 ++++------ coderd/insights_internal_test.go | 4 ++-- 2 files changed, 6 insertions(+), 8 deletions(-) diff --git a/coderd/insights.go b/coderd/insights.go index c93b15e86f974..dfb7fddd39731 100644 --- a/coderd/insights.go +++ b/coderd/insights.go @@ -89,7 +89,7 @@ func (api *API) insightsUserActivity(rw http.ResponseWriter, r *http.Request) { return } - startTime, endTime, ok := parseInsightsStartAndEndTime(ctx, rw, startTimeString, endTimeString) + startTime, endTime, ok := parseInsightsStartAndEndTime(ctx, rw, time.Now(), startTimeString, endTimeString) if !ok { return } @@ -176,7 +176,7 @@ func (api *API) insightsUserLatency(rw http.ResponseWriter, r *http.Request) { return } - startTime, endTime, ok := parseInsightsStartAndEndTime(ctx, rw, startTimeString, endTimeString) + startTime, endTime, ok := parseInsightsStartAndEndTime(ctx, rw, time.Now(), startTimeString, endTimeString) if !ok { return } @@ -268,7 +268,7 @@ func (api *API) insightsTemplates(rw http.ResponseWriter, r *http.Request) { return } - startTime, endTime, ok := parseInsightsStartAndEndTime(ctx, rw, startTimeString, endTimeString) + startTime, endTime, ok := parseInsightsStartAndEndTime(ctx, rw, time.Now(), startTimeString, endTimeString) if !ok { return } @@ -539,9 +539,7 @@ func convertTemplateInsightsApps(usage database.GetTemplateInsightsRow, appUsage // time are not zero and that the end time is not before the start time. The // clock must be set to 00:00:00, except for "today", where end time is allowed // to provide the hour of the day (e.g. 14:00:00). -func parseInsightsStartAndEndTime(ctx context.Context, rw http.ResponseWriter, startTimeString, endTimeString string) (startTime, endTime time.Time, ok bool) { - now := time.Now() - +func parseInsightsStartAndEndTime(ctx context.Context, rw http.ResponseWriter, now time.Time, startTimeString, endTimeString string) (startTime, endTime time.Time, ok bool) { for _, qp := range []struct { name, value string dest *time.Time diff --git a/coderd/insights_internal_test.go b/coderd/insights_internal_test.go index cbfea6ec41123..40d12a854f8ed 100644 --- a/coderd/insights_internal_test.go +++ b/coderd/insights_internal_test.go @@ -156,7 +156,7 @@ func Test_parseInsightsStartAndEndTime(t *testing.T) { } rw := httptest.NewRecorder() - gotStartTime, gotEndTime, gotOk := parseInsightsStartAndEndTime(context.Background(), rw, tt.args.startTime, tt.args.endTime) + gotStartTime, gotEndTime, gotOk := parseInsightsStartAndEndTime(context.Background(), rw, now, tt.args.startTime, tt.args.endTime) if !assert.Equal(t, tt.wantOk, gotOk) { //nolint:bodyclose @@ -261,7 +261,7 @@ func Test_parseInsightsInterval_week(t *testing.T) { t.Log("endTime: ", tt.args.endTime) rw := httptest.NewRecorder() - startTime, endTime, ok := parseInsightsStartAndEndTime(context.Background(), rw, tt.args.startTime, tt.args.endTime) + startTime, endTime, ok := parseInsightsStartAndEndTime(context.Background(), rw, now, tt.args.startTime, tt.args.endTime) if !ok { //nolint:bodyclose t.Log("Status: ", rw.Result().StatusCode)