[Libraries] Fix TimeZoneInfoTests IsIanaIdTest for iOS/MacCatalyst/tvOS #58562

mdh1418 · 2021-09-02T14:36:05Z

On iOS/MacCatalyst/tvOS there are currently no time zones found that are not Iana Ids. As such, the original change to IsIanaIdTest in #57732 was incorrect.

Browser, iOS, MacCatalyst, and tvOS all do not support Iana name conversions as a result of some ICU changes. For the tests, we opt to align Android along with iOS/MacCatalyst/tvOS for ICU support.

This PR does the following:
remove mobile from SupportIanaNamesConversion
revert some changes from #57732

dotnet-issue-labeler · 2021-09-02T14:36:09Z

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

filipnavara · 2021-09-02T14:48:37Z

src/libraries/System.Runtime/tests/System/TimeZoneInfoTests.cs

@@ -2664,7 +2664,7 @@ public static void IsIanaIdTest()
        {
            bool expected = !s_isWindows;

-            Assert.Equal((expected || TimeZoneInfo.Local.Id.Equals("Utc", StringComparison.OrdinalIgnoreCase)), TimeZoneInfo.Local.HasIanaId);
+            Assert.Equal((expected || PlatformDetection.IsiOS || PlatformDetection.IstvOS || TimeZoneInfo.Local.Id.Equals("Utc", StringComparison.OrdinalIgnoreCase)), TimeZoneInfo.Local.HasIanaId);


That doesn't make sense to me. I'd assume that !s_isWindows is true thus expected == true and this condition would not need the change.

Ah oops, I got the order wrong when I checked the failure

eerhardt · 2021-09-02T15:49:05Z

On iOS/MacCatalyst/tvOS, TimeZoneInfo.Local.Id is found to be America/New_York

It should still have HasIanaId set to true though. If it doesn't, there's a bug somewhere. America/New_York is an IANA time zone ID.

tarekgh · 2021-09-02T16:34:54Z

The behavior of PlatformDetection.IsiOS || PlatformDetection.IstvOS looks unclear to me. Why America/New_York should not be IANA Id on such platforms? Also I am seeing some code in the tests has the following

        string timeZoneIdNotIANAId = PlatformDetection.IsiOS || PlatformDetection.IstvOS ? "America/Buenos_Aires" : "Pacific Standard Time";
        Assert.False(TimeZoneInfo.FindSystemTimeZoneById(timeZoneIdNotIANAId).HasIanaId, $" should not be IANA Id.");
        Assert.True(TimeZoneInfo.FindSystemTimeZoneById("America/Los_Angeles").HasIanaId, $"'America/Los_Angeles' should be IANA Id");

Why America/Buenos_Aires is not IANA Id there too?

Some more explanation would help here.

mdh1418 · 2021-09-02T18:30:46Z

Ah I think I know what happened. In #57732, the test failed because it would throw when looking up the timezone Pacific Standard Time (because it does not exist) instead of returning false for iOS/MacCatalyst/tvOS.

To maintain the purpose of the test check, I had evaluated all the timezones "America/Los_Angeles" "Australia/Sydney" "Europe/London" "Pacific/Tongatapu" "America/Sao_Paulo" "Australia/Perth" "America/Sao_Paulo" "Africa/Nairobi" "Europe/Berlin" "Europe/Moscow" "Africa/Tripoli" "Africa/Johannesburg" "Africa/Casablanca" "America/Argentina/Catamarca" "Europe/Lisbon" "America/St_Johns" "Asia/Tehran" for which ones had HasIanaId to be true and which to be false.

At the time, I wasn't aware that they are all considered Iana Ids (I didn't find the right documentation that compelled me to think that there was a bug when some of these returned false. As a result, I chose the first false for HasIanaId which happened to be America/Buenos_Aires.

To my other mistake, I didn't make a clean build with recent main, and so my test failure was on
Assert.Equal((expected || TimeZoneInfo.Local.Id.Equals("Utc", StringComparison.OrdinalIgnoreCase)), TimeZoneInfo.Local.HasIanaId);, but now after buliding from clean, it is on
Assert.False(TimeZoneInfo.FindSystemTimeZoneById(timeZoneIdNotIANAId).HasIanaId, $" should not be IANA Id.");

Are there any TimeZoneIds that are found on iOS/MacCatalyst/tvOS that are not Iana Id? I believe any timezone that are not found will throw a timezonenotfound exception. Instead, I could perhaps tweak that part of the test that would Assert.Throw instead of Assert.False for iOS/MacCatalyst/tvOS

filipnavara · 2021-09-02T18:44:40Z

Are there any TimeZoneIds that are found on iOS/MacCatalyst/tvOS that are not Iana Id?

There should not be any.

On desktop there's a translation table between IANA IDs and Windows IDs (provided through libicu), so creating Pacific Standard Time will map to the IANA time zone but keep the original ID and HasIanaId == false. That cannot be replicated on iOS at the moment unless completely custom time zone is defined.

tarekgh · 2021-09-02T19:03:58Z

I wasn't aware that they are all considered Iana Ids (I didn't find the right documentation that compelled me to think that there was a bug when some of these returned false

You can always refer to IANA timezones and can download the data and look at it. The data are just a text files. Or you can look at NodaTime page for quick reference.

tarekgh · 2021-09-02T19:35:09Z

src/libraries/System.Runtime/tests/System/TimeZoneInfoTests.cs

+            }
+            else
+            {
+                Assert.Throws<TimeZoneNotFoundException>(() => TimeZoneInfo.FindSystemTimeZoneById("Pacific Standard Time").HasIanaId);


HasIanaId

nit: Maybe you don't need calling this property here? it wouldn't hurt if you keep it though.

I was thinking similarly since it is unnecessary, but was wondering if keeping the HasIanaId would be in the spirit of the test purpose of checking if it was Iana Id.

filipnavara · 2021-09-02T20:00:11Z

The test is conditioned on SupportIanaNamesConversion. Should not that be false on iOS?

It is actually patching a test that should not run in the first place.

mdh1418 · 2021-09-02T20:14:35Z

Should not that be false on iOS?

It can be changed to be false on iOS/MacCatalyst/tvOS. That would help remove a bunch of the manual SkipOnPlatform attributes, but some of the tests dont specifically seem to test for conversions? This test (IsIanaIdTest) in particular seems to be fine to just check that HasIanaId works and not necessarily need to test the conversion?

filipnavara · 2021-09-02T20:20:40Z

This test (IsIanaIdTest) in particular seems to be fine

The test (before your changes in this PR) was actually testing the implicit conversion when Windows name is passed on Unix platform. It would make more sense to me to just disable the whole set since it's actually identical situation to browser (app local ICU with trimmed data needed for the conversions).

…ile from iana conversion support

… casing

tarekgh · 2021-09-03T16:59:42Z

I am preferring to test the throwing on iOS/tvOS as originally written. The reason is in the future if this functionality started to work on such platforms, we can get the test failures and can update the test by then.

My comment shouldn't block this PR if you don't want considering it now.

filipnavara · 2021-09-03T17:07:43Z

I am preferring to test the throwing on iOS/tvOS as originally written

I can submit a PR with a separate test for that... but it makes sense to keep the browser and iOS test situation consistent because at the moment the implementation is identical.

…cCatalyst_tvOS

…OS (dotnet#58562) * [Libraries] Fix TimeZoneInfoTests IsIanaIdTest for iOS/MacCatalyst/tvOS * [Libraries] Fix IsIanaIdTest to assert throw for iOS/MacCatalyst/tvOS * Readd new line * [Mobile][Libraries] System.Runtime.Tests TimeZoneInfoTests remove mobile from iana conversion support * [libraries] System.Runtime.Tests revert IsIanaIdTest iOS tvOS special casing * Remove active issue added in recently merged PR Co-authored-by: Mitchell Hwang <[email protected]>

…t and test fixes for Android (#58841) Manual backport of #57208, #58519, #58562, #58210, #57732, #58428, #58586, #58745, #57687 to release/6.0 Numerous test suites have been failing for iOS/tvOS/MacCatalyst consistently on CI without useful logs as to why. Moreover, some of these suites pass locally. This PR looks to reduce the failures on CI by skipping the problematic suites Skips test suites logged in #53624 ActiveIssues #58440 #58418 #58367 #58584 Co-authored-by: Mitchell Hwang <[email protected]> Co-authored-by: Alexander Köplinger <[email protected]> Co-authored-by: Jo Shields <[email protected]>

mdh1418 requested a review from tarekgh September 2, 2021 14:36

[Libraries] Fix TimeZoneInfoTests IsIanaIdTest for iOS/MacCatalyst/tvOS

d7ee4e7

mdh1418 requested a review from filipnavara September 2, 2021 14:39

filipnavara reviewed Sep 2, 2021

View reviewed changes

Mitchell Hwang added 2 commits September 2, 2021 15:25

[Libraries] Fix IsIanaIdTest to assert throw for iOS/MacCatalyst/tvOS

4aa8873

Readd new line

471ddd9

tarekgh reviewed Sep 2, 2021

View reviewed changes

tarekgh approved these changes Sep 2, 2021

View reviewed changes

Mitchell Hwang added 2 commits September 3, 2021 10:39

[Mobile][Libraries] System.Runtime.Tests TimeZoneInfoTests remove mob…

a7c9291

…ile from iana conversion support

[libraries] System.Runtime.Tests revert IsIanaIdTest iOS tvOS special…

694989e

… casing

mdh1418 requested a review from akoeplinger September 3, 2021 14:43

filipnavara approved these changes Sep 3, 2021

View reviewed changes

akoeplinger approved these changes Sep 3, 2021

View reviewed changes

Mitchell Hwang added 2 commits September 3, 2021 13:52

Merge remote-tracking branch 'upstream/main' into fix_IsIanaId_iOS_ma…

20a55b7

…cCatalyst_tvOS

Remove active issue added in recently merged PR

67039ab

marek-safar merged commit db04259 into dotnet:main Sep 6, 2021

filipnavara mentioned this pull request Sep 6, 2021

Add test for implicit time zone conversion for mobile platforms #58717

Merged

mdh1418 deleted the fix_IsIanaId_iOS_macCatalyst_tvOS branch September 7, 2021 13:13

mdh1418 mentioned this pull request Sep 9, 2021

[release/6.0] Skip test suites crashing on CI for iOS/tvOS/MacCatalyst and test fixes for Android #58841

Merged

ghost locked as resolved and limited conversation to collaborators Oct 7, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Libraries] Fix TimeZoneInfoTests IsIanaIdTest for iOS/MacCatalyst/tvOS #58562

[Libraries] Fix TimeZoneInfoTests IsIanaIdTest for iOS/MacCatalyst/tvOS #58562

mdh1418 commented Sep 2, 2021 •

edited

Loading

dotnet-issue-labeler bot commented Sep 2, 2021

filipnavara Sep 2, 2021

mdh1418 Sep 2, 2021

eerhardt commented Sep 2, 2021

tarekgh commented Sep 2, 2021 •

edited

Loading

mdh1418 commented Sep 2, 2021

filipnavara commented Sep 2, 2021

tarekgh commented Sep 2, 2021

tarekgh Sep 2, 2021

mdh1418 Sep 2, 2021

filipnavara commented Sep 2, 2021 •

edited

Loading

mdh1418 commented Sep 2, 2021

filipnavara commented Sep 2, 2021 •

edited

Loading

tarekgh commented Sep 3, 2021

filipnavara commented Sep 3, 2021

[Libraries] Fix TimeZoneInfoTests IsIanaIdTest for iOS/MacCatalyst/tvOS #58562

[Libraries] Fix TimeZoneInfoTests IsIanaIdTest for iOS/MacCatalyst/tvOS #58562

Conversation

mdh1418 commented Sep 2, 2021 • edited Loading

dotnet-issue-labeler bot commented Sep 2, 2021

filipnavara Sep 2, 2021

Choose a reason for hiding this comment

mdh1418 Sep 2, 2021

Choose a reason for hiding this comment

eerhardt commented Sep 2, 2021

tarekgh commented Sep 2, 2021 • edited Loading

mdh1418 commented Sep 2, 2021

filipnavara commented Sep 2, 2021

tarekgh commented Sep 2, 2021

tarekgh Sep 2, 2021

Choose a reason for hiding this comment

mdh1418 Sep 2, 2021

Choose a reason for hiding this comment

filipnavara commented Sep 2, 2021 • edited Loading

mdh1418 commented Sep 2, 2021

filipnavara commented Sep 2, 2021 • edited Loading

tarekgh commented Sep 3, 2021

filipnavara commented Sep 3, 2021

mdh1418 commented Sep 2, 2021 •

edited

Loading

tarekgh commented Sep 2, 2021 •

edited

Loading

filipnavara commented Sep 2, 2021 •

edited

Loading

filipnavara commented Sep 2, 2021 •

edited

Loading