From e86c0dc01993c4e4f39fde0e72bf781ffca30125 Mon Sep 17 00:00:00 2001 From: Adam Thayer Date: Sun, 23 Sep 2018 23:14:35 -0700 Subject: [PATCH 1/4] Fix DateFormatter TimeZone Setter 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. --- Foundation/DateFormatter.swift | 6 +++--- TestFoundation/TestDateFormatter.swift | 19 +++++++++++++++++++ 2 files changed, 22 insertions(+), 3 deletions(-) diff --git a/Foundation/DateFormatter.swift b/Foundation/DateFormatter.swift index 69644c0fb6..80ae4bada7 100644 --- a/Foundation/DateFormatter.swift +++ b/Foundation/DateFormatter.swift @@ -176,13 +176,13 @@ open class DateFormatter : Formatter { /*@NSCopying*/ internal var _timeZone: TimeZone? { willSet { _reset() } } open var timeZone: TimeZone! { get { - guard let timeZone = _timeZone else { + guard let tz = _timeZone else { return (CFDateFormatterCopyProperty(_cfObject, kCFDateFormatterTimeZone) as! NSTimeZone)._swiftObject } - return timeZone + return tz } set { - _timeZone = timeZone + _timeZone = newValue } } diff --git a/TestFoundation/TestDateFormatter.swift b/TestFoundation/TestDateFormatter.swift index 98c636f620..3050a39d29 100644 --- a/TestFoundation/TestDateFormatter.swift +++ b/TestFoundation/TestDateFormatter.swift @@ -24,6 +24,7 @@ class TestDateFormatter: XCTestCase { ("test_dateFormatString", test_dateFormatString), ("test_setLocaleToNil", test_setLocaleToNil), ("test_setTimeZoneToNil", test_setTimeZoneToNil), + ("test_setTimeZone", test_setTimeZone), ] } @@ -356,4 +357,22 @@ class TestDateFormatter: XCTestCase { // Time zone should go back to the system one. XCTAssertEqual(f.timeZone, NSTimeZone.system) } + + func test_setTimeZone() { + // Test two different time zones. Should ensure that if one + // happens to be TimeZone.current, we still get a valid test. + let newYork = TimeZone(identifier: "America/New_York")! + let losAngeles = TimeZone(identifier: "America/Los_Angeles")! + + XCTAssertNotEqual(newYork, losAngeles) + + // Case 1: New York + let f = DateFormatter() + f.timeZone = newYork + XCTAssertEqual(f.timeZone, newYork) + + // Case 2: Los Angeles + f.timeZone = losAngeles + XCTAssertEqual(f.timeZone, losAngeles) + } } From caa8d0ae1365b0d459fe1f43961702cd52c0aab7 Mon Sep 17 00:00:00 2001 From: Adam Thayer Date: Mon, 24 Sep 2018 09:33:37 -0700 Subject: [PATCH 2/4] Fix Off-By-One Error With System TimeZone MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../NumberDate.subproj/CFTimeZone.c | 2 +- TestFoundation/TestDateFormatter.swift | 33 +++++++++++++++++++ 2 files changed, 34 insertions(+), 1 deletion(-) diff --git a/CoreFoundation/NumberDate.subproj/CFTimeZone.c b/CoreFoundation/NumberDate.subproj/CFTimeZone.c index 57ebeb6274..2f7c839fc2 100644 --- a/CoreFoundation/NumberDate.subproj/CFTimeZone.c +++ b/CoreFoundation/NumberDate.subproj/CFTimeZone.c @@ -793,7 +793,7 @@ static CFTimeZoneRef __CFTimeZoneCreateSystem(void) { size_t zoneInfoDirLen = CFStringGetLength(__tzZoneInfo); if (strncmp(linkbuf, tzZoneInfo, zoneInfoDirLen) == 0) { name = CFStringCreateWithBytes(kCFAllocatorSystemDefault, (uint8_t *)linkbuf + zoneInfoDirLen, - strlen(linkbuf) - zoneInfoDirLen + 1, kCFStringEncodingUTF8, false); + strlen(linkbuf) - zoneInfoDirLen, kCFStringEncodingUTF8, false); } else { name = CFStringCreateWithBytes(kCFAllocatorSystemDefault, (uint8_t *)linkbuf, strlen(linkbuf), kCFStringEncodingUTF8, false); } diff --git a/TestFoundation/TestDateFormatter.swift b/TestFoundation/TestDateFormatter.swift index 3050a39d29..12c70e6ab6 100644 --- a/TestFoundation/TestDateFormatter.swift +++ b/TestFoundation/TestDateFormatter.swift @@ -25,6 +25,7 @@ class TestDateFormatter: XCTestCase { ("test_setLocaleToNil", test_setLocaleToNil), ("test_setTimeZoneToNil", test_setTimeZoneToNil), ("test_setTimeZone", test_setTimeZone), + ("test_ExpectedTimeZone", test_ExpectedTimeZone), ] } @@ -375,4 +376,36 @@ class TestDateFormatter: XCTestCase { f.timeZone = losAngeles XCTAssertEqual(f.timeZone, losAngeles) } + + func test_ExpectedTimeZone() { + let gmt = TimeZone(abbreviation: DEFAULT_TIMEZONE) + let newYork = TimeZone(identifier: "America/New_York")! + let losAngeles = TimeZone(identifier: "America/Los_Angeles")! + + XCTAssertNotEqual(newYork, losAngeles) + + let now = Date() + + let f = DateFormatter() + f.dateFormat = "z" + + // Case 1: TimeZone.current + f.timeZone = TimeZone.current + XCTAssertEqual(f.string(from: now), f.timeZone.abbreviation()) + + // Case 2: New York + f.timeZone = newYork + XCTAssertEqual(f.string(from: now), f.timeZone.abbreviation()) + + // Case 3: Los Angeles + f.timeZone = losAngeles + XCTAssertEqual(f.string(from: now), f.timeZone.abbreviation()) + + guard gmt != TimeZone.current else { + print("Inconclusive: This test checks to see if the formatter produces the same TZ as TimeZone.current") + print("When it fails, TimeZone.current formats as GMT instead of normal.") + print("Unfortunately, we can't use GMT as TimeZone.current for this test to be conclusive.") + return + } + } } From e6de371e097ea193caa2810b8ccf79dda502291a Mon Sep 17 00:00:00 2001 From: Adam Thayer Date: Tue, 25 Sep 2018 07:50:54 -0700 Subject: [PATCH 3/4] Cleanup SystemTimeZone tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- TestFoundation/TestDateFormatter.swift | 24 ++++++++++++------------ TestFoundation/TestTimeZone.swift | 16 ++++++++++++++++ 2 files changed, 28 insertions(+), 12 deletions(-) diff --git a/TestFoundation/TestDateFormatter.swift b/TestFoundation/TestDateFormatter.swift index 12c70e6ab6..db8998f531 100644 --- a/TestFoundation/TestDateFormatter.swift +++ b/TestFoundation/TestDateFormatter.swift @@ -25,7 +25,7 @@ class TestDateFormatter: XCTestCase { ("test_setLocaleToNil", test_setLocaleToNil), ("test_setTimeZoneToNil", test_setTimeZoneToNil), ("test_setTimeZone", test_setTimeZone), - ("test_ExpectedTimeZone", test_ExpectedTimeZone), + ("test_expectedTimeZone", test_expectedTimeZone), ] } @@ -377,7 +377,7 @@ class TestDateFormatter: XCTestCase { XCTAssertEqual(f.timeZone, losAngeles) } - func test_ExpectedTimeZone() { + func test_expectedTimeZone() { let gmt = TimeZone(abbreviation: DEFAULT_TIMEZONE) let newYork = TimeZone(identifier: "America/New_York")! let losAngeles = TimeZone(identifier: "America/Los_Angeles")! @@ -388,24 +388,24 @@ class TestDateFormatter: XCTestCase { let f = DateFormatter() f.dateFormat = "z" + f.locale = Locale(identifier: "en_US_POSIX") // Case 1: TimeZone.current + // This case can catch some issues that cause TimeZone.current to be + // treated like GMT, but it doesn't work if TimeZone.current is GMT. + // If you do find an issue like this caused by this first case, + // it would benefit from a more specific test that fails when + // TimeZone.current is GMT as well. + // (ex. TestTimeZone.test_systemTimeZoneName) f.timeZone = TimeZone.current - XCTAssertEqual(f.string(from: now), f.timeZone.abbreviation()) + XCTAssertEqual(f.string(from: now), TimeZone.current.abbreviation()) // Case 2: New York f.timeZone = newYork - XCTAssertEqual(f.string(from: now), f.timeZone.abbreviation()) + XCTAssertEqual(f.string(from: now), newYork.abbreviation()) // Case 3: Los Angeles f.timeZone = losAngeles - XCTAssertEqual(f.string(from: now), f.timeZone.abbreviation()) - - guard gmt != TimeZone.current else { - print("Inconclusive: This test checks to see if the formatter produces the same TZ as TimeZone.current") - print("When it fails, TimeZone.current formats as GMT instead of normal.") - print("Unfortunately, we can't use GMT as TimeZone.current for this test to be conclusive.") - return - } + XCTAssertEqual(f.string(from: now), losAngeles.abbreviation()) } } diff --git a/TestFoundation/TestTimeZone.swift b/TestFoundation/TestTimeZone.swift index 92a53aab63..68555610eb 100644 --- a/TestFoundation/TestTimeZone.swift +++ b/TestFoundation/TestTimeZone.swift @@ -7,6 +7,8 @@ // See http://swift.org/CONTRIBUTORS.txt for the list of Swift project authors // +import CoreFoundation + class TestTimeZone: XCTestCase { static var allTests: [(String, (TestTimeZone) -> () throws -> Void)] { @@ -29,6 +31,7 @@ class TestTimeZone: XCTestCase { ("test_customMirror", test_tz_customMirror), ("test_knownTimeZones", test_knownTimeZones), + ("test_systemTimeZoneName", test_systemTimeZoneName), ] } @@ -198,4 +201,17 @@ class TestTimeZone: XCTestCase { XCTAssertNotNil(TimeZone(identifier: tz), "Cant instantiate valid timeZone: \(tz)") } } + + func test_systemTimeZoneName() { + // Ensure that the system time zone creates names the same way as creating them with an identifier. + // If it isn't the same, bugs in DateFormat can result, but in this specific case, the bad length + // is only visible to CoreFoundation APIs, and the Swift versions hide it, making it hard to detect. + let timeZone = CFTimeZoneCopySystem() + let timeZoneName = CFTimeZoneGetName(timeZone) + + let createdTimeZone = TimeZone(identifier: TimeZone.current.identifier)! + + XCTAssertEqual(CFStringGetLength(timeZoneName), TimeZone.current.identifier.count) + XCTAssertEqual(CFStringGetLength(timeZoneName), createdTimeZone.identifier.count) + } } From 218e0e248ee371c361dfbb867c1946424c6b8f29 Mon Sep 17 00:00:00 2001 From: Adam Thayer Date: Wed, 10 Oct 2018 18:11:25 -0700 Subject: [PATCH 4/4] Make UTC Explicit in test_dateDifferenceComponents 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. --- TestFoundation/TestCalendar.swift | 27 ++++++++++++++++++--------- 1 file changed, 18 insertions(+), 9 deletions(-) diff --git a/TestFoundation/TestCalendar.swift b/TestFoundation/TestCalendar.swift index f4d45bf878..551869b20d 100644 --- a/TestFoundation/TestCalendar.swift +++ b/TestFoundation/TestCalendar.swift @@ -331,7 +331,16 @@ class TestNSDateComponents: XCTestCase { // 2286-11-20 17:46:41 let date4 = Date(timeIntervalSince1970: 10_000_000_001) - let diff1 = Calendar.current.dateComponents([.month, .year, .day], from: date1, to: date2) + // The date components below assume UTC/GMT time zone. + guard let timeZone = TimeZone(abbreviation: "UTC") else { + XCTFail("Unable to create UTC TimeZone for Test") + return + } + + var calendar = Calendar.current + calendar.timeZone = timeZone + + let diff1 = calendar.dateComponents([.month, .year, .day], from: date1, to: date2) XCTAssertEqual(diff1.year, 1) XCTAssertEqual(diff1.month, 5) XCTAssertEqual(diff1.isLeapMonth, false) @@ -350,35 +359,35 @@ class TestNSDateComponents: XCTestCase { XCTAssertNil(diff1.calendar) XCTAssertNil(diff1.timeZone) - let diff2 = Calendar.current.dateComponents([.weekOfMonth], from: date2, to: date1) + let diff2 = calendar.dateComponents([.weekOfMonth], from: date2, to: date1) XCTAssertEqual(diff2.weekOfMonth, -76) XCTAssertEqual(diff2.isLeapMonth, false) - let diff3 = Calendar.current.dateComponents([.weekday], from: date2, to: date1) + let diff3 = calendar.dateComponents([.weekday], from: date2, to: date1) XCTAssertEqual(diff3.weekday, -536) XCTAssertEqual(diff3.isLeapMonth, false) - let diff4 = Calendar.current.dateComponents([.weekday, .weekOfMonth], from: date1, to: date2) + let diff4 = calendar.dateComponents([.weekday, .weekOfMonth], from: date1, to: date2) XCTAssertEqual(diff4.weekday, 4) XCTAssertEqual(diff4.weekOfMonth, 76) XCTAssertEqual(diff4.isLeapMonth, false) - let diff5 = Calendar.current.dateComponents([.weekday, .weekOfYear], from: date1, to: date2) + let diff5 = calendar.dateComponents([.weekday, .weekOfYear], from: date1, to: date2) XCTAssertEqual(diff5.weekday, 4) XCTAssertEqual(diff5.weekOfYear, 76) XCTAssertEqual(diff5.isLeapMonth, false) - let diff6 = Calendar.current.dateComponents([.month, .weekOfMonth], from: date1, to: date2) + let diff6 = calendar.dateComponents([.month, .weekOfMonth], from: date1, to: date2) XCTAssertEqual(diff6.month, 17) XCTAssertEqual(diff6.weekOfMonth, 2) XCTAssertEqual(diff6.isLeapMonth, false) - let diff7 = Calendar.current.dateComponents([.weekOfYear, .weekOfMonth], from: date2, to: date1) + let diff7 = calendar.dateComponents([.weekOfYear, .weekOfMonth], from: date2, to: date1) XCTAssertEqual(diff7.weekOfYear, -76) XCTAssertEqual(diff7.weekOfMonth, 0) XCTAssertEqual(diff7.isLeapMonth, false) - let diff8 = Calendar.current.dateComponents([.era, .quarter, .year, .month, .day, .hour, .minute, .second, .nanosecond, .calendar, .timeZone], from: date2, to: date3) + let diff8 = calendar.dateComponents([.era, .quarter, .year, .month, .day, .hour, .minute, .second, .nanosecond, .calendar, .timeZone], from: date2, to: date3) XCTAssertEqual(diff8.era, 0) XCTAssertEqual(diff8.year, 315) XCTAssertEqual(diff8.quarter, 0) @@ -392,7 +401,7 @@ class TestNSDateComponents: XCTestCase { XCTAssertNil(diff8.calendar) XCTAssertNil(diff8.timeZone) - let diff9 = Calendar.current.dateComponents([.era, .quarter, .year, .month, .day, .hour, .minute, .second, .nanosecond, .calendar, .timeZone], from: date4, to: date3) + let diff9 = calendar.dateComponents([.era, .quarter, .year, .month, .day, .hour, .minute, .second, .nanosecond, .calendar, .timeZone], from: date4, to: date3) XCTAssertEqual(diff9.era, 0) XCTAssertEqual(diff9.year, 0) XCTAssertEqual(diff9.quarter, 0)