[libs][Unix] Fix UTC alias lookup

[RenderMichael]

• Fix UTC -> UCT typo
• Re-order lookup to match source in the comment linked above

I noticed #88368 introduced this bug.

Not sure if this should have a regression test, or where such a test would go.

[ghost]

Tagging subscribers to this area: @dotnet/area-system-datetime
See info in area-owners.md if you want to be subscribed.

Issue Details

---

• Fix UTC -> UCT typo
• Re-order lookup to match source in the comment linked above

I noticed #88368 introduced this bug.

Not sure if this should have a regression test, or where such a test would go.

Author:	RenderMichael
Assignees:	-
Labels:	area-System.DateTime
Milestone:	-

[danmoseley]

@mdh1418

[lewing]

Etc/UCT is clearly a valid alias @mdh1418 can you point to the right place for a test?

[mdh1418]

Thanks for the catch @RenderMichael! I believe TimeZoneInfoTests would be a good place for a test. Perhaps we can all of the aliases to the test data (sans UTC which should be in GetSystemTimeZones())

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

Lines 2309 to 2338 in 4e48d2d

	public static IEnumerable<object[]> SystemTimeZonesTestData()
	{
	foreach (TimeZoneInfo tz in TimeZoneInfo.GetSystemTimeZones())
	{
	yield return new object[] { tz };
	}
	// Include fixed offset IANA zones in the test data when they are available.
	if (!PlatformDetection.IsWindows)
	{
	for (int i = -14; i <= 12; i++)
	{
	TimeZoneInfo tz = null;
	try
	{
	string id = $"Etc/GMT{i:+0;-0}";
	tz = TimeZoneInfo.FindSystemTimeZoneById(id);
	}
	catch (TimeZoneNotFoundException)
	{
	}
	if (tz != null)
	{
	yield return new object[] { tz };
	}
	}
	}
	}

. That way atleast TimeZoneDisplayNames_Unix can catch if they will have the correct DisplayName, StandardName, and DaylightName.

[tarekgh]

Hardcoding all UTC aliases in the test that @mdh1418 is a good idea. Just ensure adding them under the if (!PlatformDetection.IsWindows) condition.
Thanks @RenderMichael for the catch.

[tarekgh]

@RenderMichael please add the test and we should be good to go. Thanks!

[RenderMichael]

@dotnet-policy-service agree

[tarekgh]

Thanks @RenderMichael

[tarekgh]

The tests are failing with

[FAIL] System.Tests.TimeZoneInfoTests.UtcAliases_MapToUtc
Assert.Equal() Failure
Expected: TimeZoneInfo { }
Actual:   TimeZoneInfo { }
   at System.Tests.TimeZoneInfoTests.UtcAliases_MapToUtc() + 0xb4
   at System.Runtime!<BaseAddress>+0x2ee348c
   at System.Reflection.DynamicInvokeInfo.Invoke(Object, IntPtr, Object[], BinderBundle, Boolean) + 0xe0

May be you need to change the equality with checking the time zone Id instead.

[RenderMichael]

May be you need to change the equality with checking the time zone Id instead.

@tarekgh Sorry I wasn't on my Linux box at the time and use GitHub's text editor. Looks like two TimeZoneInfos constructed with different UTC aliases have different Id's and thus equality fails.

However, their DisplayNames are equal. That should be enough right?

[tarekgh]

However, their DisplayNames are equal. That should be enough right?

Maybe it is better to check the BaseUtcOffset is zero. names can change according to the UI language and never guarantee which UI culture is used when creating TimeZoneInfo.Utc and when we create the other variants. The good thing is your test ensure creating time zones with the different aliases.

[RenderMichael]

The test failed for some reason. For some reason, the following code is false on Linux but true on Windows:

TimeZoneInfo actualUtc = TimeZoneInfo.FindSystemTimeZoneById("UCT");
bool result = TimeZoneInfo.Utc.HasSameRules(actualUtc);

Is this an issue or am I missing something?

[tarekgh]

The test failed for some reason. For some reason, the following code is false on Linux but true on Windows

On Linux the internal adjustment rules array of TimeZoneInfo.Utc is stored as null and not an empty array. This will make the comparison fail. Just check the BaseUtcOffset and ensure calling TimeZoneInfo.GetAdjustmentRules() return empty array. This should be enough for this test.

[RenderMichael]

On Linux the internal adjustment rules array of TimeZoneInfo.Utc is stored as null and not an empty array. This will make the comparison fail.

Seems like a pit of failure to me, a pit I tripped into writing this PR.

Either way, the feedback has been addressed, PTAL

[mdh1418]

Can we also have the other Utc Aliases added to the test data SystemTimeZonesTestData?
I believe that the BaseUtcOffset and AdjustmentRules were still correct before the typo was fixed (atleast on Android), the thing that slipped through were the DisplayName, StandardName, and DaylightName, and TimeZoneDisplayNames_Unix would check the values of those three

I believe the public getter GetAdjustmentRules() for the internal field _adjustmentRules would still return the Array.Empty<AdjustmentRule>() even if the value is null.

runtime/src/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Unix.cs

Lines 142 to 145 in a3c21b9

	if (_adjustmentRules == null)
	{
	return Array.Empty<AdjustmentRule>();
	}

, so the test being added would still pass even if they typo wasn't fixed

[tarekgh]

I believe that the BaseUtcOffset and AdjustmentRules were still correct before the typo was fixed (atleast on Android), the thing that slipped through were the DisplayName, StandardName, and DaylightName, and TimeZoneDisplayNames_Unix would check the values of those three

@mdh1418 before this change, could you create the time zone in the first place? or we get some failure? Did you try it?

I believe the public getter GetAdjustmentRules() for the internal field _adjustmentRules would still return the Array.Empty() even if the value is null.

Yes, what is the point here?

[RenderMichael]

Can we also have the other Utc Aliases added to the test data SystemTimeZonesTestData?

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

Lines 2323 to 2330 in 4e48d2d

	try
	{
	string id = $"Etc/GMT{i:+0;-0}";
	tz = TimeZoneInfo.FindSystemTimeZoneById(id);
	}
	catch (TimeZoneNotFoundException)
	{
	}

I see the missing values in this loop are ignored, should we do the same for the UTC aliases @mdh1418 ?

[mdh1418]

@mdh1418 before this change, could you create the time zone in the first place? or we get some failure? Did you try it?

Yes, I ran a test like

        [Fact]
        public static void EnsureUtcAliases()
        {
            foreach (var tz in s_UtcAliases)
            {
                Console.WriteLine($"ID: {tz}");
                TimeZoneInfo etcUtcObject = TimeZoneInfo.FindSystemTimeZoneById(tz);
                Assert.True(etcUtcObject.BaseUtcOffset == TimeSpan.Zero);
                Assert.True(etcUtcObject.GetAdjustmentRules().Length == 0);
                Console.WriteLine($"{etcUtcObject.DisplayName}");
                Console.WriteLine($"{etcUtcObject.StandardName}");
                Console.WriteLine($"{etcUtcObject.DaylightName}");
            }
        }

and it gave something like

07-12 12:22:09.050  3040  3080 I DOTNET  : (UTC) Coordinated Universal Time
07-12 12:22:09.050  3040  3080 I DOTNET  : Coordinated Universal Time
07-12 12:22:09.050  3040  3080 I DOTNET  : Coordinated Universal Time
07-12 12:22:09.058  3040  3080 I DOTNET  : (UTC+00:00) GMT
07-12 12:22:09.058  3040  3080 I DOTNET  : Coordinated Universal Time
07-12 12:22:09.058  3040  3080 I DOTNET  : GMT
07-12 12:22:09.059  3040  3080 I DOTNET  : (UTC) Coordinated Universal Time
07-12 12:22:09.059  3040  3080 I DOTNET  : Coordinated Universal Time
07-12 12:22:09.059  3040  3080 I DOTNET  : Coordinated Universal Time
07-12 12:22:09.059  3040  3080 I DOTNET  : (UTC) Coordinated Universal Time
07-12 12:22:09.059  3040  3080 I DOTNET  : Coordinated Universal Time
07-12 12:22:09.059  3040  3080 I DOTNET  : Coordinated Universal Time
07-12 12:22:09.059  3040  3080 I DOTNET  : (UTC) Coordinated Universal Time
07-12 12:22:09.059  3040  3080 I DOTNET  : Coordinated Universal Time
07-12 12:22:09.059  3040  3080 I DOTNET  : Coordinated Universal Time
07-12 12:22:09.059  3040  3080 I DOTNET  : (UTC) Coordinated Universal Time
07-12 12:22:09.059  3040  3080 I DOTNET  : Coordinated Universal Time
07-12 12:22:09.059  3040  3080 I DOTNET  : Coordinated Universal Time
07-12 12:22:09.059  3040  3080 I DOTNET  : (UTC) Coordinated Universal Time
07-12 12:22:09.059  3040  3080 I DOTNET  : Coordinated Universal Time
07-12 12:22:09.059  3040  3080 I DOTNET  : Coordinated Universal Time
07-12 12:22:09.059  3040  3080 I DOTNET  : (UTC) Coordinated Universal Time
07-12 12:22:09.059  3040  3080 I DOTNET  : Coordinated Universal Time
07-12 12:22:09.059  3040  3080 I DOTNET  : Coordinated Universal Time

So the TimeZoneInfo was created for Etc/UCT but with the wrong names, yet correct BaseUtcOffset and AdjustmentRules

Yes, what is the point here?

Just pointing out that If one were to typo in the future, the test would pass (because we are checking for GetAdjustmentRules instead of the true value of the internal field being Array.Empty<AdjustmentRule>() instead of null) despite the DisplayName DaylightName and StandardName being incorrect.

[mdh1418]

Can we also have the other Utc Aliases added to the test data SystemTimeZonesTestData?

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

Lines 2323 to 2330 in 4e48d2d

	try
	{
	string id = $"Etc/GMT{i:+0;-0}";
	tz = TimeZoneInfo.FindSystemTimeZoneById(id);
	}
	catch (TimeZoneNotFoundException)
	{
	}

I see the missing values in this loop are ignored, should we do the same for the UTC aliases @mdh1418 ?

We could, but maybe its unnecessary if all of the UTC aliases TimeZoneInfo objects can be created via FindSystemTimeZoneById?

[mdh1418]

I think the other test you proposed was still a good idea cause there doesn't seem to be any other tests checking for the BaseUtcOffset and GetAdjustmentRules() for UTC and its aliases. So together all 5 fields BaseUtcOffset GetAdjustmentRules() DisplayName StandardName DaylightName will be as expected for the Utc aliases!

Thanks for your patience!

[RenderMichael]

I think the other test you proposed was still a good idea cause there doesn't seem to be any other tests checking for the BaseUtcOffset and GetAdjustmentRules() for UTC and its aliases. So together all 5 fields BaseUtcOffset GetAdjustmentRules() DisplayName StandardName DaylightName will be as expected for the Utc aliases!

That's what I tried to do with HasSameRules() but that method checks the underlying adjustment rules, which fails on the null == GetAdjustmentRules(). Since TimeZoneDisplayNames_Unix covers the last 3 you mentioned, I'll add back in the test for the offset and adjustment rules

[tarekgh]

@mdh1418 @RenderMichael if we need to, we can try to fix HasSameRules to make it compare the null adjustment rules equal to the empty adjustment rule array. I am ok with the current changes though.

[RenderMichael]

Failures are related (some of them)
On tvOS and iOS, when running against for example the UCT alias, we get the error

Id: "UCT", Expected DisplayName: "(UTC) UTC", Actual DisplayName: "(UTC) UCT"

An issue like this happens for browser but I found that locally and disabled the aliases from SystemTimeZonesTestData() on browser.

@tarekgh @mdh1418 Assuming this isn't a real issue, should we really just gate the alises behind "if not Browser and not tvOS and not iOS"? I'm not familiar with how .NET works on platforms besides Windows and Linux.

[tarekgh]

Assuming this isn't a real issue, should we really just gate the alises behind "if not Browser and not tvOS and not iOS"? I'm not familiar with how .NET works on platforms besides Windows and Linux.

CC @lewing @steveisok

Consistency is good here if we can return the same data in other OSs. But we have differences anyway on such OSs because they carry different sets of data. So, I am not super worried about this specific case.

[tarekgh]

Thanks @RenderMichael for your effort getting this done!

[RenderMichael]

Always happy to contribute to the software I use every day!

[danmoseley]

@RenderMichael if you're interested in another contribution, we'd welcome it. There's some approved API's that just need implementation, eg.
https://github.com/dotnet/runtime/issues?q=is%3Aopen+is%3Aissue+label%3A%22help+wanted%22

[akoeplinger]

@tarekgh the tests mentioned in #88641 (comment) are failing in main now.

I assume this affects all platforms where we use our own ICU (currently Browser, iOS, tvOS, MacCatalyst) so we should disable the tests there as well, I opened #88909

[tarekgh]

Thanks @akoeplinger for the quick fix.