New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Specify time zone in example code to fix test instability across locales #2664
Conversation
@@ -872,7 +873,9 @@ object DateAsLongSerializer : KSerializer<Date> { | |||
|
|||
object DateAsSimpleTextSerializer: KSerializer<Date> { | |||
override val descriptor: SerialDescriptor = PrimitiveSerialDescriptor("DateAsSimpleText", PrimitiveKind.LONG) | |||
private val format = SimpleDateFormat("yyyy-MM-dd") | |||
private val format = SimpleDateFormat("yyyy-MM-dd").apply { | |||
setTimeZone(TimeZone.getTimeZone("UTC")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is an example code, I think a lot of people would simply copy-paste it in their projects. Some of them will likely not read it and are not familiar with whole timezones and stuff, so setting a timezone for format
may have unexpected results for them.
Yet, I can't come up with a better idea to solve this problem since adding a simple timezone qualifier (X
or z
) won't do — the date will still be printed in a local timezone.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Honestly, if someone isn't familiar with timezones, wouldn't it be better if they used UTC rather than their computer's default locale? :-) But I think if they get results they don't expect, the explicit presence of TimeZone in the code will remind them that this is something they can change to fit their purpose.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would setting the timezone via a system property in the Gradle Test task that runs this code work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I run TZ=US/Eastern ./gradlew :guide:test
, I see the same test failure.
So I added this to guide/build.gradle.kts
:
tasks.test {
systemProperty("user.timezone", "UTC")
}
Now when I run TZ=US/Eastern ./gradlew :guide:test
it succeeds.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, that works too. But I would still advocate for setting a time zone in the example, for two reasons:
- It ensures the example's behavior remains self-contained and stable, even outside of the context of the gradle build tree.
- Date-time related code is notoriously difficult to get right, and one of the most frequent problems is that people forget to specify time zones in their code. I don't think it really ever makes sense to serialize or deserialize a date without a time zone either conveyed in the data or specified at the serializer boundary. By making it part of the example, you help ensure that users handle dates correctly.
I think that if there were a large number of examples involving datetimes it might make sense to instead specify at the top of the file "hey, all of these assume you're setting a time zone somewhere" and then put user.timezone=UTC
in the test runner, but if it's just one example then I feel like inlining it is fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with @timmc here: we should be as explicit as possible, preferably without any alteration to build scripts or environment. So I'd leave setTimeZone(TimeZone.getTimeZone("UTC"))
as it seems the only reasonable solution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that makes sense.
On looking into it more, I see that the sample uses java.io.Date
. I think it'd be a good idea to demonstrate good practice and update the same to use newer replacement, java.time.LocalDate
. This requires an explicit timezone to encode/decode, so it'd be more natural to see the explicit timezone.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This requires an explicit timezone to encode/decode, so it'd be more natural to see the explicit timezone.
Not exactly true. It does not accept explicit timezone on parse/toString
, as being Local
implies that it doesn't have one. The only things where you need a timezone is converting to epoch seconds:
object DateAsLongSerializer : KSerializer<LocalDate> {
override val descriptor: SerialDescriptor = PrimitiveSerialDescriptor("DateAsLong", PrimitiveKind.LONG)
override fun serialize(encoder: Encoder, value: LocalDate) = encoder.encodeLong(value.atStartOfDay().toEpochSecond(ZoneOffset.UTC))
override fun deserialize(decoder: Decoder): LocalDate = LocalDate.ofEpochDay(decoder.decodeLong())
}
object DateAsSimpleTextSerializer: KSerializer<LocalDate> {
override val descriptor: SerialDescriptor = PrimitiveSerialDescriptor("DateAsSimpleText", PrimitiveKind.LONG)
override fun serialize(encoder: Encoder, value: LocalDate) = encoder.encodeString(value.toString())
override fun deserialize(decoder: Decoder): LocalDate = LocalDate.parse(decoder.decodeString())
}
typealias DateAsLong = @Serializable(DateAsLongSerializer::class) LocalDate
typealias DateAsText = @Serializable(DateAsSimpleTextSerializer::class) LocalDate
@Serializable
class ProgrammingLanguage(val stableReleaseDate: DateAsText, val lastReleaseTimestamp: DateAsLong)
fun main() {
val format = SimpleDateFormat("yyyy-MM-ddX")
val data = ProgrammingLanguage(LocalDate.parse("2016-02-15"), LocalDate.parse("2022-07-07"))
println(Json.encodeToString(data))
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that other samples above also use Date
and are part of one story, I think we have to stick to Date
unless we want to rewrite all the samples to ZonedDateTime
. We'll need to specify UTC
one way or another — either in Date<>String
conversion or in LocalDate<>Long
conversion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh yeah, moving to java.time would be great... agreed that that's a bigger change.
The only thing I would change is to add the comment like
|
I've pushed up something very similar -- only difference is "adjust or remove". |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Many thanks!
Addresses #2663