From fbe1b0a03628a35f892a22ad3be946d22095b4e2 Mon Sep 17 00:00:00 2001 From: Gusted Date: Tue, 2 Aug 2022 04:14:21 +0200 Subject: [PATCH 1/5] Fix SecToTime edge-cases - Currently there are edge-cases where the code fails to produce the correct results, this is mainly due to the misusage of the modulo operator. Well this could be fixed, it's better to use a more performant way and simply distract the previous number's cost to calculate the variables. - Resolves #20589 --- modules/util/sec_to_time.go | 20 ++++++++++++++++---- modules/util/sec_to_time_test.go | 17 +++++++++++------ 2 files changed, 27 insertions(+), 10 deletions(-) diff --git a/modules/util/sec_to_time.go b/modules/util/sec_to_time.go index 9ce6fe1a13eb5..3904ca162fddb 100644 --- a/modules/util/sec_to_time.go +++ b/modules/util/sec_to_time.go @@ -16,12 +16,24 @@ import ( // 1563418 -> 2 weeks 4 days // 3937125s -> 1 month 2 weeks // 45677465s -> 1 year 6 months +// +// Magic numbers: +// 3600 = 60 * 60 (amount of seconds in a hour) +// 86400 = 60 * 60 * 24 (amount of seconds in a day) func SecToTime(duration int64) string { formattedTime := "" - years := duration / (3600 * 24 * 7 * 4 * 12) - months := (duration / (3600 * 24 * 30)) % 12 - weeks := (duration / (3600 * 24 * 7)) % 4 - days := (duration / (3600 * 24)) % 7 + years := (duration / 86400) / 365 + + // The following three variables are calculated with taking + // into account the previous calculated variables, this avoids + // pitfalls when using remainders. As that could lead to incorrect + // results when the calculated number equals the quotient number. + months := (duration/86400 - years*365) / 30 + weeks := (duration/86400 - years*365 - months*30) / 7 + days := duration/86400 - years*365 - months*30 - weeks*7 + + // The following three variables are calculated without depending + // on the previous calculated variables. hours := (duration / 3600) % 24 minutes := (duration / 60) % 60 seconds := duration % 60 diff --git a/modules/util/sec_to_time_test.go b/modules/util/sec_to_time_test.go index 854190462bc37..c67d7ea9debb2 100644 --- a/modules/util/sec_to_time_test.go +++ b/modules/util/sec_to_time_test.go @@ -11,10 +11,15 @@ import ( ) func TestSecToTime(t *testing.T) { - assert.Equal(t, SecToTime(66), "1 minute 6 seconds") - assert.Equal(t, SecToTime(52410), "14 hours 33 minutes") - assert.Equal(t, SecToTime(563418), "6 days 12 hours") - assert.Equal(t, SecToTime(1563418), "2 weeks 4 days") - assert.Equal(t, SecToTime(3937125), "1 month 2 weeks") - assert.Equal(t, SecToTime(45677465), "1 year 5 months") + assert.Equal(t, "1 minute 6 seconds", SecToTime(66)) + assert.Equal(t, "1 hour", SecToTime(3600)) + assert.Equal(t, "1 hour", SecToTime(3601)) + assert.Equal(t, "14 hours 33 minutes", SecToTime(52410)) + assert.Equal(t, "6 days 12 hours", SecToTime(563418)) + assert.Equal(t, "2 weeks 4 days", SecToTime(1563418)) + assert.Equal(t, "4 weeks", SecToTime(2419200)) + assert.Equal(t, "4 weeks 1 day", SecToTime(2505600)) + assert.Equal(t, "1 month 2 weeks", SecToTime(3937125)) + assert.Equal(t, "11 months 1 week", SecToTime(29376000)) + assert.Equal(t, "1 year 5 months", SecToTime(45677465)) } From db4b4aa9bb39310afa543dbec3513b9b31c35d18 Mon Sep 17 00:00:00 2001 From: Gusted Date: Tue, 2 Aug 2022 11:29:15 +0000 Subject: [PATCH 2/5] Apply suggestions from code review Co-authored-by: delvh --- modules/util/sec_to_time.go | 20 ++++++++++---------- modules/util/sec_to_time_test.go | 28 +++++++++++++++++----------- 2 files changed, 27 insertions(+), 21 deletions(-) diff --git a/modules/util/sec_to_time.go b/modules/util/sec_to_time.go index 3904ca162fddb..faa651233d5ec 100644 --- a/modules/util/sec_to_time.go +++ b/modules/util/sec_to_time.go @@ -16,21 +16,21 @@ import ( // 1563418 -> 2 weeks 4 days // 3937125s -> 1 month 2 weeks // 45677465s -> 1 year 6 months -// -// Magic numbers: -// 3600 = 60 * 60 (amount of seconds in a hour) -// 86400 = 60 * 60 * 24 (amount of seconds in a day) func SecToTime(duration int64) string { formattedTime := "" - years := (duration / 86400) / 365 - // The following three variables are calculated with taking - // into account the previous calculated variables, this avoids + // The following four variables are calculated by taking + // into account the previously calculated variables, this avoids // pitfalls when using remainders. As that could lead to incorrect // results when the calculated number equals the quotient number. - months := (duration/86400 - years*365) / 30 - weeks := (duration/86400 - years*365 - months*30) / 7 - days := duration/86400 - years*365 - months*30 - weeks*7 + remainingDays := duration / (60 * 60 * 24) + years := remainingDays / 365 + remainingDays -= years * 365 + months := remainingDays / 30 + remainingDays -= months * 30 + weeks := remainingDays / 7 + remainingDays -= weeks * 7 + days := remainingDays // The following three variables are calculated without depending // on the previous calculated variables. diff --git a/modules/util/sec_to_time_test.go b/modules/util/sec_to_time_test.go index c67d7ea9debb2..0fc0bf959543f 100644 --- a/modules/util/sec_to_time_test.go +++ b/modules/util/sec_to_time_test.go @@ -11,15 +11,21 @@ import ( ) func TestSecToTime(t *testing.T) { - assert.Equal(t, "1 minute 6 seconds", SecToTime(66)) - assert.Equal(t, "1 hour", SecToTime(3600)) - assert.Equal(t, "1 hour", SecToTime(3601)) - assert.Equal(t, "14 hours 33 minutes", SecToTime(52410)) - assert.Equal(t, "6 days 12 hours", SecToTime(563418)) - assert.Equal(t, "2 weeks 4 days", SecToTime(1563418)) - assert.Equal(t, "4 weeks", SecToTime(2419200)) - assert.Equal(t, "4 weeks 1 day", SecToTime(2505600)) - assert.Equal(t, "1 month 2 weeks", SecToTime(3937125)) - assert.Equal(t, "11 months 1 week", SecToTime(29376000)) - assert.Equal(t, "1 year 5 months", SecToTime(45677465)) + second := 1 + minute := 60 * second + hour := 60 * minute + day := 24 * hour + year := 365 * day + + assert.Equal(t, "1 minute 6 seconds", SecToTime(minute + 6 * second)) + assert.Equal(t, "1 hour", SecToTime(hour)) + assert.Equal(t, "1 hour", SecToTime(hour + second)) + assert.Equal(t, "14 hours 33 minutes", SecToTime(14 * hour + 33 * minute + 30 * second)) + assert.Equal(t, "6 days 12 hours", SecToTime(6 * day + 12 * hour + 30 * minute + 18 * second)) + assert.Equal(t, "2 weeks 4 days", SecToTime((2 * 7 + 4) * day + 2 * hour + 16 * minute + 58 * second)) + assert.Equal(t, "4 weeks", SecToTime(4 * 7 * day)) + assert.Equal(t, "4 weeks 1 day", SecToTime((4 * 7 + 1) * day)) + assert.Equal(t, "1 month 2 weeks", SecToTime((6 * 7 + 3) * day + 13 * hour + 38 * minute + 45 * second)) + assert.Equal(t, "11 months 1 week", SecToTime(year - 25 * day)) + assert.Equal(t, "1 year 5 months", SecToTime(year + 163 * day + 10 * hour + 11 * minute + 5 * second)) } From d6468e15669ea0fbd9958e0caefa1bd06e8aa307 Mon Sep 17 00:00:00 2001 From: Gusted Date: Wed, 3 Aug 2022 10:56:54 +0000 Subject: [PATCH 3/5] Update modules/util/sec_to_time.go Co-authored-by: Edgar Bonet --- modules/util/sec_to_time.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/modules/util/sec_to_time.go b/modules/util/sec_to_time.go index faa651233d5ec..13461915e6f35 100644 --- a/modules/util/sec_to_time.go +++ b/modules/util/sec_to_time.go @@ -26,8 +26,8 @@ func SecToTime(duration int64) string { remainingDays := duration / (60 * 60 * 24) years := remainingDays / 365 remainingDays -= years * 365 - months := remainingDays / 30 - remainingDays -= months * 30 + months := remainingDays * 12 / 365 + remainingDays -= months * 365 / 12 weeks := remainingDays / 7 remainingDays -= weeks * 7 days := remainingDays From 750b7978092d6d12ab9160821354a9adc677d387 Mon Sep 17 00:00:00 2001 From: Gusted Date: Sun, 7 Aug 2022 14:24:13 +0200 Subject: [PATCH 4/5] Fix tests --- modules/util/sec_to_time_test.go | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/modules/util/sec_to_time_test.go b/modules/util/sec_to_time_test.go index 0fc0bf959543f..c9ad8e9209a28 100644 --- a/modules/util/sec_to_time_test.go +++ b/modules/util/sec_to_time_test.go @@ -11,21 +11,21 @@ import ( ) func TestSecToTime(t *testing.T) { - second := 1 + second := int64(1) minute := 60 * second hour := 60 * minute day := 24 * hour year := 365 * day - - assert.Equal(t, "1 minute 6 seconds", SecToTime(minute + 6 * second)) + + assert.Equal(t, "1 minute 6 seconds", SecToTime(minute+6*second)) assert.Equal(t, "1 hour", SecToTime(hour)) - assert.Equal(t, "1 hour", SecToTime(hour + second)) - assert.Equal(t, "14 hours 33 minutes", SecToTime(14 * hour + 33 * minute + 30 * second)) - assert.Equal(t, "6 days 12 hours", SecToTime(6 * day + 12 * hour + 30 * minute + 18 * second)) - assert.Equal(t, "2 weeks 4 days", SecToTime((2 * 7 + 4) * day + 2 * hour + 16 * minute + 58 * second)) - assert.Equal(t, "4 weeks", SecToTime(4 * 7 * day)) - assert.Equal(t, "4 weeks 1 day", SecToTime((4 * 7 + 1) * day)) - assert.Equal(t, "1 month 2 weeks", SecToTime((6 * 7 + 3) * day + 13 * hour + 38 * minute + 45 * second)) - assert.Equal(t, "11 months 1 week", SecToTime(year - 25 * day)) - assert.Equal(t, "1 year 5 months", SecToTime(year + 163 * day + 10 * hour + 11 * minute + 5 * second)) + assert.Equal(t, "1 hour", SecToTime(hour+second)) + assert.Equal(t, "14 hours 33 minutes", SecToTime(14*hour+33*minute+30*second)) + assert.Equal(t, "6 days 12 hours", SecToTime(6*day+12*hour+30*minute+18*second)) + assert.Equal(t, "2 weeks 4 days", SecToTime((2*7+4)*day+2*hour+16*minute+58*second)) + assert.Equal(t, "4 weeks", SecToTime(4*7*day)) + assert.Equal(t, "4 weeks 1 day", SecToTime((4*7+1)*day)) + assert.Equal(t, "1 month 2 weeks", SecToTime((6*7+3)*day+13*hour+38*minute+45*second)) + assert.Equal(t, "11 months 1 week", SecToTime(year-25*day)) + assert.Equal(t, "1 year 5 months", SecToTime(year+163*day+10*hour+11*minute+5*second)) } From 6847342f3b9a5e013ee7893bcb2927446ea41cdd Mon Sep 17 00:00:00 2001 From: Gusted Date: Sun, 7 Aug 2022 23:05:35 +0200 Subject: [PATCH 5/5] Fix tests --- modules/util/sec_to_time_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/util/sec_to_time_test.go b/modules/util/sec_to_time_test.go index c9ad8e9209a28..1e256aa8650cf 100644 --- a/modules/util/sec_to_time_test.go +++ b/modules/util/sec_to_time_test.go @@ -26,6 +26,6 @@ func TestSecToTime(t *testing.T) { assert.Equal(t, "4 weeks", SecToTime(4*7*day)) assert.Equal(t, "4 weeks 1 day", SecToTime((4*7+1)*day)) assert.Equal(t, "1 month 2 weeks", SecToTime((6*7+3)*day+13*hour+38*minute+45*second)) - assert.Equal(t, "11 months 1 week", SecToTime(year-25*day)) + assert.Equal(t, "11 months", SecToTime(year-25*day)) assert.Equal(t, "1 year 5 months", SecToTime(year+163*day+10*hour+11*minute+5*second)) }