From 2e3c3c74faf1bd52e4289cce10570e8a07fc7d03 Mon Sep 17 00:00:00 2001 From: Jan-Erik Rediger Date: Tue, 8 Oct 2019 15:54:36 +0200 Subject: [PATCH] Take DST into account when converting a calendar into its items ZONE_OFFSET only provides the default offset from UTC without DST considerations. DST_OFFSET needs to be added to get the full offset for the calendar instance. There's a _very small_ hint in the deprecated `Date` class that one needs to do this[1]. Neither the docs for ZONE_OFFSET[2] nor the ones for DST_OFFSET[3] mention this explicitly. I guess the existence of both fields is the only indicator there. [1]: https://docs.oracle.com/javase/7/docs/api/java/util/Date.html#getTimezoneOffset() [2]: https://docs.oracle.com/javase/7/docs/api/java/util/Calendar.html#ZONE_OFFSET [3]: https://docs.oracle.com/javase/7/docs/api/java/util/Calendar.html#DST_OFFSET --- .../glean/private/DatetimeMetricType.kt | 4 +-- .../glean/private/DatetimeMetricTypeTest.kt | 27 +++++++++++++++++++ 2 files changed, 29 insertions(+), 2 deletions(-) diff --git a/glean-core/android/src/main/java/mozilla/telemetry/glean/private/DatetimeMetricType.kt b/glean-core/android/src/main/java/mozilla/telemetry/glean/private/DatetimeMetricType.kt index 90570e87b2..dcdf018fb7 100644 --- a/glean-core/android/src/main/java/mozilla/telemetry/glean/private/DatetimeMetricType.kt +++ b/glean-core/android/src/main/java/mozilla/telemetry/glean/private/DatetimeMetricType.kt @@ -95,7 +95,7 @@ class DatetimeMetricType internal constructor( second = cal.get(Calendar.SECOND), nano = AndroidTimeUnit.MILLISECONDS.toNanos(cal.get(Calendar.MILLISECOND).toLong()), offset_seconds = AndroidTimeUnit.MILLISECONDS.toSeconds( - cal.get(Calendar.ZONE_OFFSET).toLong() + cal.get(Calendar.ZONE_OFFSET).toLong() + cal.get(Calendar.DST_OFFSET) ).toInt() ) } @@ -127,7 +127,7 @@ class DatetimeMetricType internal constructor( second = value.get(Calendar.SECOND), nano = AndroidTimeUnit.MILLISECONDS.toNanos(value.get(Calendar.MILLISECOND).toLong()), offset_seconds = AndroidTimeUnit.MILLISECONDS.toSeconds( - value.get(Calendar.ZONE_OFFSET).toLong() + value.get(Calendar.ZONE_OFFSET).toLong() + value.get(Calendar.DST_OFFSET) ).toInt() ) } diff --git a/glean-core/android/src/test/java/mozilla/telemetry/glean/private/DatetimeMetricTypeTest.kt b/glean-core/android/src/test/java/mozilla/telemetry/glean/private/DatetimeMetricTypeTest.kt index b265d38cec..8f371e0589 100644 --- a/glean-core/android/src/test/java/mozilla/telemetry/glean/private/DatetimeMetricTypeTest.kt +++ b/glean-core/android/src/test/java/mozilla/telemetry/glean/private/DatetimeMetricTypeTest.kt @@ -20,8 +20,12 @@ import org.junit.Rule import org.junit.Test import org.junit.runner.RunWith import java.util.Calendar +import java.util.Date import java.util.TimeZone +const val MILLIS_PER_SEC = 1000L +private fun Date.asSeconds() = time / MILLIS_PER_SEC + @RunWith(AndroidJUnit4::class) class DatetimeMetricTypeTest { @@ -90,4 +94,27 @@ class DatetimeMetricTypeTest { datetimeMetric.set() assertFalse(datetimeMetric.testHasValue()) } + + @Test + fun `Regression test - setting date and reading results in the same`() { + // This test is adopted from `SyncTelemetryTest.kt` in android-components. + // Previously we failed to properly deal with DST when converting from `Calendar` into its pieces. + + val datetimeMetric = DatetimeMetricType( + disabled = false, + category = "telemetry", + lifetime = Lifetime.Ping, + name = "datetimeMetric", + sendInPings = listOf("store1"), + timeUnit = TimeUnit.Millisecond + ) + + val nowDate = Date() + val now = nowDate.asSeconds() + val timestamp = Date(now * MILLIS_PER_SEC) + + datetimeMetric.set(timestamp) + + assertEquals(now, datetimeMetric.testGetValue().asSeconds()) + } }