Skip to content

Conversation

spevans
Copy link
Contributor

@spevans spevans commented Mar 8, 2019

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.

(cherry picked from commit 9f3e050)

Fix Off-By-One Error With System TimeZone

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.

(cherry picked from commit 827e842)

Cleanup SystemTimeZone tests

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.

(cherry picked from commit 2219bd1)

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.

(cherry picked from commit 5b1bfc9)

Disable test_expectedTimeZone Case 1

This is hitting an issue on GMT time zones where it shows up as UTC when formatted.

(cherry picked from commit e826252)

@spevans
Copy link
Contributor Author

spevans commented Mar 8, 2019

@swift-ci test 4.2

@weissi
Copy link
Contributor

weissi commented Mar 11, 2019

@spevans this one failed (and does conflict now, possibly because of me merging OOO)

@spevans
Copy link
Contributor Author

spevans commented Mar 11, 2019

@weissi I know the reason for failur,e it should be fixed by #1985. I will rebase.

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.

(cherry picked from commit 9f3e050)

Fix Off-By-One Error With System TimeZone

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.

(cherry picked from commit 827e842)

Cleanup SystemTimeZone tests

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.

(cherry picked from commit 2219bd1)

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.

(cherry picked from commit 5b1bfc9)

Disable test_expectedTimeZone Case 1

This is hitting an issue on GMT time zones where it shows up as UTC when formatted.

(cherry picked from commit e826252)
@spevans
Copy link
Contributor Author

spevans commented Mar 11, 2019

@swift-ci test 4.2

1 similar comment
@spevans
Copy link
Contributor Author

spevans commented Mar 11, 2019

@swift-ci test 4.2

@spevans
Copy link
Contributor Author

spevans commented Mar 11, 2019

@weissi looks good now

Copy link
Contributor

@weissi weissi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thank you!

@weissi weissi merged commit 2a5f7bc into swiftlang:swift-4.2-branch Mar 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants