Fix DateFormatter TimeZone Setter #1704

Kaiede · 2018-09-24T06:26:39Z

Because of the use of 'timeZone' instead of 'newValue' in the setter, setting the timeZone of a DateFormatter doesn't actually work. Instead, it winds up re-setting the old value, which happens to be whatever was originally set, usually what we get from copying the default down in CoreFoundation.

A good fix here is to avoid any ambiguity with the property name in the setter and getter.

Also adds a unit test to help prevent breaks like this again.

Because of the use of 'timeZone' instead of 'newValue' in the setter, setting the timeZone of a DateFormatter doesn't actually work. Instead, it winds up re-setting the old value, which happens to be whatever was originally set, usually what we get from copying the default down in CoreFoundation. Also adds a unit test to help prevent breaks like this again.

Kaiede · 2018-09-24T06:28:14Z

I found this by accident trying to debug/find a similar break in 4.1 (which is where we are stuck on arm32 at the moment due to 32-bit alignment problems in the compiler with 4.2). Still trying to find that one, sadly.

spevans · 2018-09-24T08:56:38Z

@swift-ci test

Kaiede · 2018-09-24T16:18:40Z

What's the general rules for bundling fixes? I found the bug in 4.1/4.2 I was looking for. It's related enough I could just add the commit to this PR, but it's also somewhat unrelated (TimeZone.current has effectively been "GMT" for at least a year, no matter what it actually reports).

This one is nasty, since it creates problems for DateFormatter, while appearing to behave correctly. (Mostly) By copying 1 too many bytes, we wind up with the null terminator in the CFString. This breaks DateFormatter in an annoying way that the system TimeZone is always considered to be GMT by the formatter. You can ask for the zone’s abbreviation, and it will be correct. The name will appear to be correct, except the length is 1 longer than it should be. All sorts of fun. But the moment you ask DateFormatter to give you a string (or parse one), it assumes the TimeZone is GMT and ruins your day. Included is a test case which can detect this. The downside is that it doesn’t detect the problem if GMT is already the current TimeZone. It is possible a second, more targeted test should also be added here? This also breaks getting the description on the NSTimeZone/CFTimeZone, but not the TimeZone because of the wrapping at play, because of the terminator that gets inserted into the string description.

Kaiede · 2018-09-24T16:34:58Z

I've added caa8d0a to the PR, since that's also required to unblock my scenario of using the system timezone to parse/format date strings (think logging, and interpreting time-of-day schedules without requiring a TZ in the string).

Kaiede · 2018-09-25T06:42:05Z

TestFoundation/TestDateFormatter.swift

+        f.timeZone = losAngeles
+        XCTAssertEqual(f.string(from: now), f.timeZone.abbreviation())
+
+        guard gmt != TimeZone.current else {


I'm not 100% happy with the way this test works, which pretty much is trying to make sure that we can use "TimeZone.current" for zones other than GMT and get what we expect when we then run it through DateFormatter.

If this is too much, I can try to write a more targeted test, although I'm not entirely sure that would be any less fragile.

TestFoundation/TestDateFormatter.swift

maksimorlovich · 2018-09-25T14:58:39Z

LGTM! Thanks @Kaiede!

parkera

LGTM. Also cc @itaiferber

parkera · 2018-09-26T00:14:29Z

Once this is in master, a PR with a cherry-pick to 4.2 will get it into the next version of that. Generally I approve those if they are manageable bug fixes that unblock clients or address serious issues. Other stuff can target the next major release (5.0).

TestFoundation/TestDateFormatter.swift

Cleans up the DateFormatter test, improves the information about how it doesn’t catch issues when the system time zone is already GMT, and adds a more focused test for the SystemTimeZone name issue that can catch it when GMT is the system time zone.

itaiferber · 2018-10-02T17:05:34Z

@swift-ci Please test

millenomi · 2018-10-10T13:22:55Z

@swift-ci please test and merge

millenomi · 2018-10-10T15:13:34Z

@swift-ci please test and merge

millenomi · 2018-10-10T17:24:37Z

11:06:16 Test Case 'TestNSDateComponents.test_dateDifferenceComponents' started at 2018-10-10 11:06:06.360
11:06:16 TestFoundation/TestCalendar.swift:338: error: TestNSDateComponents.test_dateDifferenceComponents : XCTAssertEqual failed: ("Optional(21)") is not equal to ("Optional(20)") - 
11:06:16 TestFoundation/TestCalendar.swift:373: error: TestNSDateComponents.test_dateDifferenceComponents : XCTAssertEqual failed: ("Optional(3)") is not equal to ("Optional(2)") - 
11:06:16 TestFoundation/TestCalendar.swift:387: error: TestNSDateComponents.test_dateDifferenceComponents : XCTAssertEqual failed: ("Optional(16)") is not equal to ("Optional(17)") - 
11:06:16 Test Case 'TestNSDateComponents.test_dateDifferenceComponents' failed (0.003 seconds)

@Kaiede Looks like something regressed, possibly?

millenomi · 2018-10-10T17:25:03Z

Or is newly uncovered.

Kaiede · 2018-10-10T17:50:18Z

@millenomi My bet is on newly uncovered. This was not something I was seeing fail with my changes locally, but, my sync point is now 16 days old, and the last PR push was 8 days old. A lot of stuff recently got merged here, and it's possible that for some reason I just wasn't paying close enough attention.

In general, the calendar test failing is interesting because it is relying on the current time zone to function properly, in addition to the current Calendar. The bug that I'm fixing is that TimeZone.current is always acting like GMT when using ICU/DateFormatter. So it kinda makes sense that my fix could make this test appear to fail, depending on the time zone the machine running the tests happens to be using.

Instead, the failing test should be using Calendar current with a fixed UTC timezone to avoid this sort of problem. It's testing too many variables at once in practice.

EDIT: I'll take a look at this tonight, and see if I can't add a fix for the test to this PR. It should get fixed with my PR since my fix revealed the problem.

millenomi · 2018-10-10T23:22:19Z

Thanks!

Kaiede · 2018-10-11T01:02:29Z

Yeah, my suspicions were right. If the test runs in GMT, it passes, if it runs in PDT, it fails with the exact failures the CI also hit.

I also realize I didn't see it before because there's another bug when running these tests on macOS using the xcworkspace where CoreFoundation can't find the current time zone and also defaults to GMT. I had hacked around it to run my own tests, but I didn't run the whole suite that way when I discovered that problem.

Is the xcworkspace not really used anymore? It seems to be getting progressively broken. I think I ran into a few new breaks attempting to pull the latest set of changes from master, and couldn't even get SwiftFoundation to link on Mojave anymore.

The test assumes that it is running as GMT/UTC. Because of the TimeZone.current fixes, the test is no longer guaranteed to be using GMT/UTC, and will use the system TimeZone and breaks when in PDT for example. By making the UTC assumption explicit, we remove one more variable on this test.

millenomi · 2018-10-11T01:11:39Z

I ran tests with the Mojave merge on Mac, so I'm surprised. @shahmishal was looking into possibly adding CI to run those tests.

Kaiede · 2018-10-11T01:14:48Z

@millenomi I just open the xcworkspace from the Terminal and attempt to run/build from there. So I'm wondering what I'm doing differently. Either way, the fix for the test has been added to the PR.

millenomi · 2018-10-11T01:39:06Z

@Kaiede Swift Foundation is revlocked to the Swift toolchain it ships with — so you must either build your own via $SOURCES/swift/utils/build-toolchain, or, much better, download the latest one from https://swift.org/download/ for your branch (in this case, the one under 'Trunk Development (master)').

I have just built and ran tests for #1600 successfully, though I am seeing the test failures you describe with the off-by-oneness. But I can build and run correctly.

millenomi · 2018-10-11T01:39:39Z

(Switch toolchains from Xcode > Preferences > Components > Toolchains.)

millenomi · 2018-10-11T02:36:48Z

@swift-ci please test

millenomi · 2018-10-11T02:43:14Z

@swift-ci please test

millenomi · 2018-10-11T02:43:28Z

@swift-ci please test

Kaiede · 2018-10-11T02:56:43Z

@Kaiede Swift Foundation is revlocked to the Swift toolchain it ships with — so you must either build your own via $SOURCES/swift/utils/build-toolchain, or, much better, download the latest one from https://swift.org/download/ for your branch (in this case, the one under 'Trunk Development (master)').

Okay, that's what I'm doing wrong. Thanks.

millenomi · 2018-10-11T03:02:39Z

We really ought to have that written down somewhere easy to find.

gparker42 · 2018-10-12T00:04:12Z

This broke CI. TestDateFormatter.swift's test_expectedTimeZone is failing. Reverting in #1721.

TestFoundation/TestDateFormatter.swift:401: error: TestDateFormatter.test_expectedTimeZone : XCTAssertEqual failed: ("Optional("GMT")") is not equal to ("Optional("UTC")")

There is a suspicious build warning in that test:

TestFoundation/TestDateFormatter.swift:381:13: warning: initialization of immutable value 'gmt' was never used; consider replacing with assignment to '_' or removing it
        let gmt = TimeZone(abbreviation: DEFAULT_TIMEZONE)

gparker42 · 2018-10-12T00:06:09Z

Looks like the build czar's force-merge privileges don't apply to swift-corelibs-foundation, so you have a bit of time to fix it before then.

millenomi · 2018-10-12T00:18:56Z

Per side discussion: let's revert, then we will put the PR back up again.

Kaiede · 2018-10-12T00:30:36Z

This broke CI. TestDateFormatter.swift's test_expectedTimeZone is failing. Reverting in #1721.

Sorry about that, looks like the pre-checkin CI just happened to be on a PT time zone, which doesn't expose this issue with the test. Same with my local test environment.

There is a suspicious build warning in that test:

TestFoundation/TestDateFormatter.swift:381:13: warning: initialization of immutable value 'gmt' was never used; consider replacing with assignment to '_' or removing it
        let gmt = TimeZone(abbreviation: DEFAULT_TIMEZONE)

Ironically, it looks suspicious, but is entirely unrelated code that was part of an "inconclusive test" message that was removed based on feedback, but not the let statement that helped feed the guard.

The issue with the break is that if TimeZone.current.abbreviation gives you "GMT", but then ask a DateFormatter using that TimeZone what the abbreviated zone is, it gives you the string "UTC". Neat. :(

Kaiede commented Sep 25, 2018

View reviewed changes

maksimorlovich reviewed Sep 25, 2018

View reviewed changes

TestFoundation/TestDateFormatter.swift Outdated Show resolved Hide resolved

parkera approved these changes Sep 26, 2018

View reviewed changes

itaiferber reviewed Sep 26, 2018

View reviewed changes

TestFoundation/TestDateFormatter.swift Outdated Show resolved Hide resolved

TestFoundation/TestDateFormatter.swift Show resolved Hide resolved

TestFoundation/TestDateFormatter.swift Outdated Show resolved Hide resolved

Cleanup SystemTimeZone tests

e6de371

Cleans up the DateFormatter test, improves the information about how it doesn’t catch issues when the system time zone is already GMT, and adds a more focused test for the SystemTimeZone name issue that can catch it when GMT is the system time zone.

itaiferber approved these changes Oct 2, 2018

View reviewed changes

millenomi merged commit 7418081 into swiftlang:master Oct 11, 2018

gparker42 mentioned this pull request Oct 12, 2018

Revert "Fix DateFormatter TimeZone Setter" #1721

Merged

Kaiede mentioned this pull request Oct 14, 2018

Reapply "Fix DateFormatter TimeZone Setter" #1725

Merged

Fix DateFormatter TimeZone Setter #1704

Fix DateFormatter TimeZone Setter #1704

Uh oh!

Conversation

Kaiede commented Sep 24, 2018

Uh oh!

Kaiede commented Sep 24, 2018

Uh oh!

spevans commented Sep 24, 2018

Uh oh!

Kaiede commented Sep 24, 2018

Uh oh!

Kaiede commented Sep 24, 2018

Uh oh!

Kaiede Sep 25, 2018

Choose a reason for hiding this comment

Uh oh!

Uh oh!

maksimorlovich commented Sep 25, 2018

Uh oh!

parkera left a comment

Choose a reason for hiding this comment

Uh oh!

parkera commented Sep 26, 2018

Uh oh!

Uh oh!

Uh oh!

Uh oh!

itaiferber commented Oct 2, 2018

Uh oh!

millenomi commented Oct 10, 2018

Uh oh!

millenomi commented Oct 10, 2018

Uh oh!

millenomi commented Oct 10, 2018

Uh oh!

millenomi commented Oct 10, 2018

Uh oh!

Kaiede commented Oct 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

millenomi commented Oct 10, 2018

Uh oh!

Kaiede commented Oct 11, 2018

Uh oh!

millenomi commented Oct 11, 2018

Uh oh!

Kaiede commented Oct 11, 2018

Uh oh!

millenomi commented Oct 11, 2018

Uh oh!

millenomi commented Oct 11, 2018

Uh oh!

millenomi commented Oct 11, 2018

Uh oh!

millenomi commented Oct 11, 2018

Uh oh!

millenomi commented Oct 11, 2018

Uh oh!

Kaiede commented Oct 11, 2018

Uh oh!

millenomi commented Oct 11, 2018

Uh oh!

gparker42 commented Oct 12, 2018

Uh oh!

gparker42 commented Oct 12, 2018

Uh oh!

millenomi commented Oct 12, 2018

Uh oh!

Kaiede commented Oct 12, 2018

Uh oh!

Uh oh!

Kaiede commented Oct 10, 2018 •

edited

Loading