1589698: Fix roundtripping of timezone offsets in dates #392
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mdboom
force-pushed
the
datetime-round-trip
branch
from
e4bc903 to
e92f1e8
Compare
Codecov Report
@@ Coverage Diff @@
## master #392 +/- ##
============================================
+ Coverage 71.57% 75.75% +4.17%
Complexity 308 308
============================================
Files 72 95 +23
Lines 4078 5320 +1242
Branches 629 629
============================================
+ Hits 2919 4030 +1111
- Misses 696 827 +131
Partials 463 463
Continue to review full report at Codecov.
|
badboy
approved these changes
Oct 21, 2019
| // the timezone specified in the Calendar.getInstance constructor. Here, we parse | ||
| // the offset suffix of the string to get a TimeZone object and apply that to the | ||
| // Calendar instance being created. | ||
| val offset = "GMT" + correctedDate.substring(correctedDate.length - 5, correctedDate.length) |
There was a problem hiding this comment.
Are we sure that for datetimes in GMT the string ends in "+0000"? Should be easy to test.
There was a problem hiding this comment.
Ran this test:
@Test
fun `test that timestamps in GMT are read correctly`() {
TimeZone.setDefault(TimeZone.getTimeZone("GMT"))
val cal = Calendar.getInstance()
cal.set(Calendar.YEAR, 2019)
cal.set(Calendar.MONTH, 8) // September
cal.set(Calendar.DAY_OF_MONTH, 13)
assertEquals("2019-09-13+00:00", getISOTimeString(cal, GleanTimeUnit.Day))
}All green. Testing in Fenix next.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a possible fix to 1589698. I say "possible" because I didn't manage to make a Fenix release with this fix to confirm.
However, I do think there's a real bug here regardless...
The Calendar object returned from
parseISOTimeStringAsCalendarwas always returning the result in UTC, which is the right thing wrt not having DST affect things, but doesn't preserve the original timezone of the input. It seems that while SimpleDateFormat.parse does handle the offset correctly, it converts it to the desired timezone, rather than setting it as the timezone. Seems the workaround is to parse the end as a timezone first, use that as the desired timezone, and then parse everything into that.