Tests for DateUtils and FileExtensions #1111

sgallese · 2021-09-20T04:56:49Z

Test coverage for FileExtensions and DateUtils
Prepare classes for migration in #1075

sgallese · 2021-09-22T04:45:15Z

@iSoron These tests pass, but I am a bit confused by the DateUtil implementations for:
getStartOfTodayCalendarWithOffset() (test1, test 2)
getStartOfTodayWithOffset() (test1, test2)

For the above methods, these implementations are returning a non-offset time/calendar.
For example, if the offset is 3 hours, they still return a Midnight time/calendar.

This differs from the behavior of getStartOfTomorrowWithOffset() (test 1, test 2)
For example, if the offset is 3 hours, they return a 3AM time/calendar.

If this is not expected, let me know the correct expectation and I can create an issue and resolve.

iSoron

Thank you for the tests, @sgallese. I think they are good. I left just some minor comments below.

uhabits-core/src/jvmMain/java/org/isoron/uhabits/core/utils/DateUtils.kt

iSoron · 2021-10-01T01:56:51Z

uhabits-core/src/jvmTest/java/org/isoron/uhabits/core/utils/DateUtilsTest.kt

+    @Test
+    fun testGetStartOfTomorrowWithOffset_priorToOffset() {
+        val hourOffset = 3
+        setStartDayOffset(hourOffset, 0)
+        setFixedTimeZone(TimeZone.getTimeZone("GMT"))
+        val startOfTomorrowWithOffset = unixTime(2017, Calendar.JANUARY, 1, hourOffset, 0)
+        val priorToOffset = unixTime(2017, Calendar.JANUARY, 1, hourOffset - 1, 0)
+        setFixedLocalTime(priorToOffset)
+        val startOfTomorrow = DateUtils.getStartOfTomorrowWithOffset()
+        assertThat(startOfTomorrowWithOffset, equalTo(startOfTomorrow))
+    }
+
+    @Test
+    fun testGetStartOfTomorrowWithOffset_afterOffset() {
+        val hourOffset = 3
+        setStartDayOffset(hourOffset, 0)
+        setFixedTimeZone(TimeZone.getTimeZone("GMT"))
+        val startOfTomorrowWithOffset = unixTime(2017, Calendar.JANUARY, 2, hourOffset, 0)
+        val afterOffset = unixTime(2017, Calendar.JANUARY, 1, hourOffset + 1, 0)
+        setFixedLocalTime(afterOffset)
+        val startOfTomorrow = DateUtils.getStartOfTomorrowWithOffset()
+        assertThat(startOfTomorrowWithOffset, equalTo(startOfTomorrow))
+    }


These two tests share too much code; could you please merge them into one? It's fine to to more than one assertion per test if it removes a lot of duplication.

I was able to move the first three lines to a shared helper function, but the rest of the lines are different. If I'm missing something else obvious for DRYer test code, let me know! 707b2b4

Actually, ended up writing much DRYer offset tests 4fbf8a8
See if these are better.
Once merged, I'll want to update the new non-JVM tests to reflect this logic in #1120

uhabits-core/src/jvmTest/java/org/isoron/uhabits/core/utils/DateUtilsTest.kt

iSoron · 2021-10-01T02:02:04Z

uhabits-core/src/jvmTest/java/org/isoron/uhabits/core/utils/DateUtilsTest.kt

+    @Test
+    fun testGetStartOfTodayCalendarWithOffset_priorToOffset() {
+        val hourOffset = 3
+        setStartDayOffset(hourOffset, 0)
+        val priorToOffset = unixTime(2017, Calendar.JANUARY, 2, hourOffset - 1, 0)
+        setFixedLocalTime(priorToOffset)
+        setFixedLocale(Locale.GERMANY)
+        val startOfYesterday = unixTime(2017, Calendar.JANUARY, 2, 0, 0)
+        val expectedCalendar = GregorianCalendar(TimeZone.getTimeZone("GMT"), Locale.GERMANY)
+        expectedCalendar.timeInMillis = startOfYesterday
+        val startOfTodayCalendar = DateUtils.getStartOfTodayCalendar()
+        assertThat(expectedCalendar, equalTo(startOfTodayCalendar))
+    }
+
+    @Test
+    fun testGetStartOfTodayCalendarWithOffset_afterOffset() {
+        val hourOffset = 3
+        setStartDayOffset(hourOffset, 0)
+        val afterOffset = unixTime(2017, Calendar.JANUARY, 1, hourOffset + 1, 0)
+        setFixedLocalTime(afterOffset)
+        setFixedLocale(Locale.GERMANY)
+        val startOfToday = unixTime(2017, Calendar.JANUARY, 1, 0, 0)
+        val expectedCalendar = GregorianCalendar(TimeZone.getTimeZone("GMT"), Locale.GERMANY)
+        expectedCalendar.timeInMillis = startOfToday
+        val startOfTodayCalendar = DateUtils.getStartOfTodayCalendar()
+        assertThat(expectedCalendar, equalTo(startOfTodayCalendar))
+    }


Same comment for these two tests.

iSoron · 2021-10-01T02:22:43Z

For the above methods, these implementations are returning a non-offset time/calendar.
For example, if the offset is 3 hours, they still return a Midnight time/calendar.

It's also unclear to me. I suggest that we write tests for the current behavior. Changing it might introduce subtle bugs.

The underlying issue here is that, historically, we have stored all dates as milliseconds since Jan 1, 1970, but, since we don't actually care about hours/minutes/seconds in most situations, we have also implicitly assumed that most millis are a multiple of DAY_LENGTH. I suspect that the reason these functions are returning midnight is to keep this assumption valid. We should eventually move everything that doesn't care about hours/minutes/seconds to LocalDate, which solves this ambiguity by storing time at a lower granularity.

sgallese · 2021-10-31T05:38:05Z

@iSoron OK to merge this PR?

iSoron · 2021-11-04T01:01:45Z

@sgallese Sorry, I missed the 'review requested' notification. It looks good now, thanks!

sgallese force-pushed the feature/file-extensions-test branch 3 times, most recently from 95e20be to eb61444 Compare September 20, 2021 15:04

sgallese force-pushed the feature/file-extensions-test branch from c3a90f5 to 4f87dc3 Compare September 25, 2021 05:47

sgallese mentioned this pull request Sep 25, 2021

Remove JVM from DateUtils #1120

Closed

3 tasks

iSoron requested changes Oct 1, 2021

View reviewed changes

sgallese added 7 commits October 3, 2021 21:09

Tests for DateUtils and FileExtensions

7bddfbe

More DateUtils tests

66c61e2

DateUtils today offset tests

9e24128

Calendar tests for DateUtils

b15c02a

Complete tests for DateUtils, move internal functions to private

c12a6c6

Rename variable

aae85c1

Add test helper for hour offset

707b2b4

sgallese force-pushed the feature/file-extensions-test branch from 4f87dc3 to 707b2b4 Compare October 4, 2021 04:19

DRYer offset tests

4fbf8a8

sgallese requested a review from iSoron October 4, 2021 05:45

Merge branch 'iSoron:dev' into feature/file-extensions-test

2615795

sgallese force-pushed the feature/file-extensions-test branch from 5c774cb to 2615795 Compare October 31, 2021 19:05

iSoron merged commit eb041bf into iSoron:dev Nov 4, 2021

sgallese deleted the feature/file-extensions-test branch November 4, 2021 19:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tests for DateUtils and FileExtensions #1111

Tests for DateUtils and FileExtensions #1111

sgallese commented Sep 20, 2021 •

edited

Loading

sgallese commented Sep 22, 2021

iSoron left a comment

iSoron Oct 1, 2021

sgallese Oct 4, 2021

sgallese Oct 4, 2021

iSoron Oct 1, 2021

iSoron commented Oct 1, 2021

sgallese commented Oct 31, 2021

iSoron commented Nov 4, 2021

Tests for DateUtils and FileExtensions #1111

Tests for DateUtils and FileExtensions #1111

Conversation

sgallese commented Sep 20, 2021 • edited Loading

sgallese commented Sep 22, 2021

iSoron left a comment

Choose a reason for hiding this comment

iSoron Oct 1, 2021

Choose a reason for hiding this comment

sgallese Oct 4, 2021

Choose a reason for hiding this comment

sgallese Oct 4, 2021

Choose a reason for hiding this comment

iSoron Oct 1, 2021

Choose a reason for hiding this comment

iSoron commented Oct 1, 2021

sgallese commented Oct 31, 2021

iSoron commented Nov 4, 2021

sgallese commented Sep 20, 2021 •

edited

Loading