Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

TimeZoneInfo.TryConvertWindowsIdToIanaId() difference compared to TZConvert.TryWindowsToIana() #53279

Closed
martincostello opened this issue May 26, 2021 · 17 comments · Fixed by #53315

Comments

@martincostello
Copy link
Member

Description

As part of trying out .NET 6 preview 4 on an existing .NET 5 application, I tried swapping out usage of the TimeZoneConverter NuGet package with TimeZoneInfo.TryConvertWindowsIdToIanaId as described in the blog post.

Upon doing this, some existing unit tests for the existing application failed due to different IANA time zones being returned. The three specific differences I found when using TimeZoneInfo.TryConvertWindowsIdToIanaId(string, string, out string) compared to TZConvert.TryWindowsToIana(string, string, out string) are:

  • "Romance Standard Time" and "es" returns "Europe/Paris" instead of "Europe/Madrid"
  • "Romance Standard Time" and "ie" returns "Europe/London" instead of "Europe/Dublin"
  • "Romance Standard Time" and "it" returns "Europe/Berlin" instead of "Europe/Rome"

This can be replicated with the unit test class below.

The blog post seems to imply that this usage would be a supported like-for-like replacement, but as it's behaving differently I've made the assumption this is a bug and not the intended behaviour.

Removes need to use TimeZoneConverter OSS library. The functionality is now built-in.

However if this is not a supported scenario for the new APIs, then it would just be the case that for my specific use case I would need to retain the use of the TimeZoneConverter package.

using System.Collections.Generic;
using TimeZoneConverter;
using Xunit;

namespace System
{
    public static class TimeZoneTests
    {
        public static IEnumerable<object[]> TestCases()
        {
            yield return new object[] { "au", "Cen. Australia Standard Time", "Australia/Adelaide" };
            yield return new object[] { "au", "AUS Central Standard Time", "Australia/Darwin" };
            yield return new object[] { "au", "E. Australia Standard Time", "Australia/Brisbane" };
            yield return new object[] { "au", "AUS Eastern Standard Time", "Australia/Sydney" };
            yield return new object[] { "au", "Tasmania Standard Time", "Australia/Hobart" };
            yield return new object[] { "es", "Romance Standard Time", "Europe/Madrid" };
            yield return new object[] { "gb", "GMT Standard Time", "Europe/London" };
            yield return new object[] { "ie", "GMT Standard Time", "Europe/Dublin" };
            yield return new object[] { "it", "W. Europe Standard Time", "Europe/Rome" };
            yield return new object[] { "nz", "New Zealand Standard Time", "Pacific/Auckland" };
        }

        [Theory]
        [MemberData(nameof(TestCases))]
        public static void TZConvert_TryWindowsToIana(string territoryCode, string windowsTimeZoneId, string expected)
        {
            // Act
            bool result = TZConvert.TryWindowsToIana(windowsTimeZoneId, territoryCode, out string actual);

            // Assert
            Assert.True(result);
            Assert.Equal(expected, actual);
        }

        [Theory]
        [MemberData(nameof(TestCases))]
        public static void TimeZoneInfo_TryConvertWindowsIdToIanaId(string territoryCode, string windowsTimeZoneId, string expected)
        {
            // Act
            bool result = TimeZoneInfo.TryConvertWindowsIdToIanaId(windowsTimeZoneId, territoryCode, out string actual);

            // Assert
            Assert.True(result);
            Assert.Equal(expected, actual);
        }
    }
}

Configuration

.NET SDK 6.0.100-preview.4.21255.9 on Windows 10 (Version 2004 (OS Build 19041.985)).

Regression?

No, this functionality is new in .NET 6 preview 4, but it is behaving differently to TimeZoneConverter which the blog post implies it is a replacement for. If this is a use case which is supported as a replacement, then I would expect it to behave the same.

@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged New issue has not been triaged by the area owner label May 26, 2021
@dotnet-issue-labeler
Copy link

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

@ghost
Copy link

ghost commented May 26, 2021

Tagging subscribers to this area: @tarekgh, @safern
See info in area-owners.md if you want to be subscribed.

Issue Details

Description

As part of trying out .NET 6 preview 4 on an existing .NET 5 application, I tried swapping out usage of the TimeZoneConverter NuGet package with TimeZoneInfo.TryConvertWindowsIdToIanaId as described in the blog post.

Upon doing this, some existing unit tests for the existing application failed due to different IANA time zones being returned. The three specific differences I found when using TimeZoneInfo.TryConvertWindowsIdToIanaId(string, string, out string) compared to TZConvert.TryWindowsToIana(string, string, out string) are:

  • "Romance Standard Time" and "es" returns "Europe/Paris" instead of "Europe/Madrid"
  • "Romance Standard Time" and "ie" returns "Europe/London" instead of "Europe/Dublin"
  • "Romance Standard Time" and "it" returns "Europe/Berlin" instead of "Europe/Rome"

This can be replicated with the unit test class below.

The blog post seems to imply that this usage would be a supported like-for-like replacement, but as it's behaving differently I've made the assumption this is a bug and not the intended behaviour.

Removes need to use TimeZoneConverter OSS library. The functionality is now built-in.

However if this is not a supported scenario for the new APIs, then it would just be the case that for my specific use case I would need to retain the use of the TimeZoneConverter package.

using System.Collections.Generic;
using TimeZoneConverter;
using Xunit;

namespace System
{
    public static class TimeZoneTests
    {
        public static IEnumerable<object[]> TestCases()
        {
            yield return new object[] { "au", "Cen. Australia Standard Time", "Australia/Adelaide" };
            yield return new object[] { "au", "AUS Central Standard Time", "Australia/Darwin" };
            yield return new object[] { "au", "E. Australia Standard Time", "Australia/Brisbane" };
            yield return new object[] { "au", "AUS Eastern Standard Time", "Australia/Sydney" };
            yield return new object[] { "au", "Tasmania Standard Time", "Australia/Hobart" };
            yield return new object[] { "es", "Romance Standard Time", "Europe/Madrid" };
            yield return new object[] { "gb", "GMT Standard Time", "Europe/London" };
            yield return new object[] { "ie", "GMT Standard Time", "Europe/Dublin" };
            yield return new object[] { "it", "W. Europe Standard Time", "Europe/Rome" };
            yield return new object[] { "nz", "New Zealand Standard Time", "Pacific/Auckland" };
        }

        [Theory]
        [MemberData(nameof(TestCases))]
        public static void TZConvert_TryWindowsToIana(string territoryCode, string windowsTimeZoneId, string expected)
        {
            // Act
            bool result = TZConvert.TryWindowsToIana(windowsTimeZoneId, territoryCode, out string actual);

            // Assert
            Assert.True(result);
            Assert.Equal(expected, actual);
        }

        [Theory]
        [MemberData(nameof(TestCases))]
        public static void TimeZoneInfo_TryConvertWindowsIdToIanaId(string territoryCode, string windowsTimeZoneId, string expected)
        {
            // Act
            bool result = TimeZoneInfo.TryConvertWindowsIdToIanaId(windowsTimeZoneId, territoryCode, out string actual);

            // Assert
            Assert.True(result);
            Assert.Equal(expected, actual);
        }
    }
}

Configuration

.NET SDK 6.0.100-preview.4.21255.9 on Windows 10 (Version 2004 (OS Build 19041.985)).

Regression?

No, this functionality is new in .NET 6 preview 4, but it is behaving differently to TimeZoneConverter which the blog post implies it is a replacement for. If this is a use case which is supported as a replacement, then I would expect it to behave the same.

Author: martincostello
Assignees: -
Labels:

area-System.Globalization, untriaged

Milestone: -

@Clockwork-Muse
Copy link
Contributor

Probably also want to add @mattjohnsonpint

@mattjohnsonpint
Copy link
Contributor

Thanks. I'm thinking this could be an ICU data issue. I'll dig further.

@ghost
Copy link

ghost commented May 26, 2021

Tagging subscribers to this area: @tannergooding
See info in area-owners.md if you want to be subscribed.

Issue Details

Description

As part of trying out .NET 6 preview 4 on an existing .NET 5 application, I tried swapping out usage of the TimeZoneConverter NuGet package with TimeZoneInfo.TryConvertWindowsIdToIanaId as described in the blog post.

Upon doing this, some existing unit tests for the existing application failed due to different IANA time zones being returned. The three specific differences I found when using TimeZoneInfo.TryConvertWindowsIdToIanaId(string, string, out string) compared to TZConvert.TryWindowsToIana(string, string, out string) are:

  • "Romance Standard Time" and "es" returns "Europe/Paris" instead of "Europe/Madrid"
  • "Romance Standard Time" and "ie" returns "Europe/London" instead of "Europe/Dublin"
  • "Romance Standard Time" and "it" returns "Europe/Berlin" instead of "Europe/Rome"

This can be replicated with the unit test class below.

The blog post seems to imply that this usage would be a supported like-for-like replacement, but as it's behaving differently I've made the assumption this is a bug and not the intended behaviour.

Removes need to use TimeZoneConverter OSS library. The functionality is now built-in.

However if this is not a supported scenario for the new APIs, then it would just be the case that for my specific use case I would need to retain the use of the TimeZoneConverter package.

using System.Collections.Generic;
using TimeZoneConverter;
using Xunit;

namespace System
{
    public static class TimeZoneTests
    {
        public static IEnumerable<object[]> TestCases()
        {
            yield return new object[] { "au", "Cen. Australia Standard Time", "Australia/Adelaide" };
            yield return new object[] { "au", "AUS Central Standard Time", "Australia/Darwin" };
            yield return new object[] { "au", "E. Australia Standard Time", "Australia/Brisbane" };
            yield return new object[] { "au", "AUS Eastern Standard Time", "Australia/Sydney" };
            yield return new object[] { "au", "Tasmania Standard Time", "Australia/Hobart" };
            yield return new object[] { "es", "Romance Standard Time", "Europe/Madrid" };
            yield return new object[] { "gb", "GMT Standard Time", "Europe/London" };
            yield return new object[] { "ie", "GMT Standard Time", "Europe/Dublin" };
            yield return new object[] { "it", "W. Europe Standard Time", "Europe/Rome" };
            yield return new object[] { "nz", "New Zealand Standard Time", "Pacific/Auckland" };
        }

        [Theory]
        [MemberData(nameof(TestCases))]
        public static void TZConvert_TryWindowsToIana(string territoryCode, string windowsTimeZoneId, string expected)
        {
            // Act
            bool result = TZConvert.TryWindowsToIana(windowsTimeZoneId, territoryCode, out string actual);

            // Assert
            Assert.True(result);
            Assert.Equal(expected, actual);
        }

        [Theory]
        [MemberData(nameof(TestCases))]
        public static void TimeZoneInfo_TryConvertWindowsIdToIanaId(string territoryCode, string windowsTimeZoneId, string expected)
        {
            // Act
            bool result = TimeZoneInfo.TryConvertWindowsIdToIanaId(windowsTimeZoneId, territoryCode, out string actual);

            // Assert
            Assert.True(result);
            Assert.Equal(expected, actual);
        }
    }
}

Configuration

.NET SDK 6.0.100-preview.4.21255.9 on Windows 10 (Version 2004 (OS Build 19041.985)).

Regression?

No, this functionality is new in .NET 6 preview 4, but it is behaving differently to TimeZoneConverter which the blog post implies it is a replacement for. If this is a use case which is supported as a replacement, then I would expect it to behave the same.

Author: martincostello
Assignees: -
Labels:

area-System.Runtime, untriaged

Milestone: -

@tarekgh tarekgh removed the untriaged New issue has not been triaged by the area owner label May 26, 2021
@tarekgh tarekgh added this to the Future milestone May 26, 2021
@tarekgh
Copy link
Member

tarekgh commented May 26, 2021

This is a bug in our code as we use ICU and ICU require the regions be uppercased. i.e. use ES instead of es. I'll track fixing this issue. till you get the fix, you may work around this by casing the region.

thanks for reporting.

https://github.com/unicode-org/cldr/blob/85885edfb64a49d6ad903f639e9bfbb00a15ea0c/common/supplemental/windowsZones.xml#L341

@tarekgh tarekgh modified the milestones: Future, 6.0.0 May 26, 2021
@martincostello
Copy link
Member Author

Thanks for the workaround, that's fixed the tests locally on my Windows 10 machine.

However after pushing the change up to our TeamCity build servers which run Windows Server 2016, I've noticed some other issues that I'd not dug into further until now as I knew the tests were broken anyway.

On the build server, the tests are failing with TryConvertWindowsIdToIanaId() returning false.

In another application that had the same issue with incorrect IANA IDs, I've also found it throws the following exception for this piece of code which works correctly on my Windows 10 machine:

TimeZoneInfo londonTimeZone = TimeZoneInfo.FindSystemTimeZoneById("Europe/London");
System.TimeZoneNotFoundException : The time zone ID 'Europe/London' was not found on the local computer.

Similar code throws exceptions with other IANA time zones:

System.TimeZoneNotFoundException : The time zone ID 'Australia/Sydney' was not found on the local computer.

Are these likely to be the same underlying issue, or should I spin up another issue for one or both of these? Or is it that this scenario is not supported with Windows Server 2016?

@filipnavara
Copy link
Member

I don't think the scenario is supported on Windows Server 2016 because it doesn't ship ICU and the data with the operating system. It may be possible to use app-local ICU but I am not sure if there's documentation for that option.

@martincostello
Copy link
Member Author

OK cool. For my specific use case I'll just need to revert back to using the TimeZoneConverter NuGet package instead then (getting our build servers moved to Windows 2019 or newer is outside of my team's control, though I can at least ask).

I realise the blog posts err on the side of brevity so wouldn't have called this out specifically, but if it's not already it should definitely be documented somewhere as a limitation before the final release.

@tarekgh
Copy link
Member

tarekgh commented May 26, 2021

@martincostello we'll ensure the docs will be clear about that.

For my specific use case I'll just need to revert back to using the TimeZoneConverter NuGet package

I would recommend having a fallback path when ICU is not enabled. So, you need to call the new TimeZoneInfo api and then fallback to TimeZoneConverter if needed. The reason is moving forward, I would expect in the future ICU will be enabled everywhere when stop supporting old Windows server versions. And I expect at some point TimeZoneConverter will be deprecated.

@tarekgh
Copy link
Member

tarekgh commented May 26, 2021

@martincostello another question, would using app-local ICU be feasible to you too? I am asking because this will ensure behavior consistency when running with and without ICU.

@filipnavara
Copy link
Member

For my specific use case I'll just need to revert back to using the TimeZoneConverter NuGet package instead then

We are kinda in the same boat. Actually we shipped our own data and converters until recently. I only discovered TimeZoneConverter thanks to the changes to .NET runtime and we migrated to it immediately. In our case we already ship ICU as part of Chromium Embedded Framework with our app so using the app-local ICU would be a cheap option for us going forward.

@martincostello
Copy link
Member Author

I think that's fair for a general use case, but our web/API apps are just deployed to our own servers where we know if ICU is available or not in advance.

It's less code to maintain and test to just do one or the other. Until our build agents have 2019, I'll stick with our current code using the NuGet package, then if/when they're updated to 2019 (or we switch to Linux) we can use the new .NET 6 in-box support.

Installing the ICU data is another possibility for the build servers though, what's the process for doing that?

@tarekgh
Copy link
Member

tarekgh commented May 26, 2021

You may look at App-local ICU for more info. Also, Microsoft ships ICU nuget packages which you can use too https://github.com/microsoft/icu.

@martincostello
Copy link
Member Author

martincostello commented May 26, 2021

Thanks. I fixed the tests on Windows Server 2016 with the following:

<ItemGroup Condition="$([MSBuild]::IsOSPlatform('Windows'))">
  <PackageReference Include="Microsoft.ICU.ICU4C.Runtime" Version="68.2.0.6" />
  <RuntimeHostConfigurationOption Include="System.Globalization.AppLocalIcu" Value="68.2" />
</ItemGroup>

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label May 26, 2021
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label May 28, 2021
@martincostello
Copy link
Member Author

Did this fix make it into 6.0.100-preview.5.21302.13? If I remove the manual workaround in my repos to normalize the country code I can still repro the original issue.

@tarekgh
Copy link
Member

tarekgh commented Jun 17, 2021

@martincostello the fix will be in preview 6.0. If you want to try the fix, you may try install the preview 6 SDK from https://github.com/dotnet/installer.

@ghost ghost locked as resolved and limited conversation to collaborators Jul 17, 2021
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants