Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Fix Time Zones Adjustment Rules on Linux #49733

Merged
merged 6 commits into from
Mar 20, 2021

Conversation

tarekgh
Copy link
Member

@tarekgh tarekgh commented Mar 17, 2021

Fixes #20626 and #20624

When running on Linux based systems, we store the time zone adjustment rules differently than we store for Windows. When users ask for the adjustment rules for any zone, we try to convert the internal stored rules into the form that match Windows rules. We had some bugs in this conversion and this change is fixing that. The change in the code has some comment describing what we are doing during this conversion.

I tested this manually for some time zones, but I am going to add some test later to this PR.

Note, I am planning to export a new property in the adjustment rule to report the UTC offset for the rule. So, the list of the reported rules would be more useful even on Windows.

@tarekgh tarekgh added this to the 6.0.0 milestone Mar 17, 2021
@tarekgh tarekgh self-assigned this Mar 17, 2021
@tarekgh
Copy link
Member Author

tarekgh commented Mar 17, 2021

@eerhardt could you please have a look at this change? Thanks!

@tarekgh tarekgh requested a review from eerhardt March 17, 2021 00:57
@lewing
Copy link
Member

lewing commented Mar 17, 2021

does this also fix #49491 ?

Copy link
Member

@lewing lewing left a comment

Choose a reason for hiding this comment

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

Are there any tests for these paths?

@tarekgh
Copy link
Member Author

tarekgh commented Mar 17, 2021

@lewing

does this also fix #49491 ?

No, this is different issue talking about the serialization. We already have another issue tracking same issue, so I closed #49491. Also, this is old functionality, and we are not recommending it. In other word we should obsolete it.

Are there any tests for these paths?

What paths? we have many time zone tests in general under the System.Runtime. If you are talking about the serialization, it is broken functionality on Linux Systems and as I mentioned this should be obsoleted.

@tarekgh
Copy link
Member Author

tarekgh commented Mar 18, 2021

@safern could you look too?

@eerhardt did you have a chance to take a quick look at this one?

@eerhardt
Copy link
Member

but I am going to add some test later to this PR.

Do you plan on adding tests before merging?

@tarekgh
Copy link
Member Author

tarekgh commented Mar 18, 2021

Do you plan on adding tests before merging?

I started to write the test but found would be better to write the test after exposing the rule UTC offset. Is it ok we can postpone the test till we expose the UTC offset?

@eerhardt
Copy link
Member

Is it ok we can postpone the test till we expose the UTC offset?

I would think we could at least write the test for an adjustment rule spanning multiple years, and verifying the data returned is correct. When the UTC offset API gets added, we can modify the test to verify the new UTC offset values are correct.

@tarekgh
Copy link
Member Author

tarekgh commented Mar 19, 2021

I would think we could at least write the test for an adjustment rule spanning multiple years, and verifying the data returned is correct. When the UTC offset API gets added, we can modify the test to verify the new UTC offset values are correct.

I was trying to avoid assuming any rule in any time zone, but I am ok to add a test for like for America/Los_Angeles looking at the reported rules matching the original rule spanning multiple years.

@eerhardt
Copy link
Member

I was trying to avoid assuming any rule in any time zone

We already have tests like this, which assume the exact rules of the Africa/Casablanca zone.

[Theory]
[PlatformSpecific(TestPlatforms.AnyUnix)] // Linux will use local mean time for DateTimes before standard time came into effect.
[InlineData("1940-02-24T23:59:59.0000000Z", false, "0:00:00")]
[InlineData("1940-02-25T00:00:00.0000000Z", true, "1:00:00")]
[InlineData("1940-11-20T00:00:00.0000000Z", true, "1:00:00")]
[InlineData("1940-12-31T23:59:59.0000000Z", true, "1:00:00")]
[InlineData("1941-01-01T00:00:00.0000000Z", true, "1:00:00")]
[InlineData("1945-02-24T12:00:00.0000000Z", true, "1:00:00")]
[InlineData("1945-11-17T01:00:00.0000000Z", true, "1:00:00")]
[InlineData("1945-11-17T22:59:59.0000000Z", true, "1:00:00")]
[InlineData("1945-11-17T23:00:00.0000000Z", false, "0:00:00")]
public static void IsDaylightSavingTime_CasablancaMultiYearDaylightSavings(string dateTimeString, bool expectedDST, string expectedOffsetString)
{
// Africa/Casablanca had DST from
// 1940-02-25T00:00:00.0000000Z {+01:00:00 DST=True}
// 1945-11-17T23:00:00.0000000Z { 00:00:00 DST=False}
DateTime dt = DateTime.ParseExact(dateTimeString, "o", CultureInfo.InvariantCulture, DateTimeStyles.AdjustToUniversal);
VerifyDST(s_casablancaTz, dt, expectedDST);
TimeSpan offset = TimeSpan.Parse(expectedOffsetString, CultureInfo.InvariantCulture);
Assert.Equal(offset, s_casablancaTz.GetUtcOffset(dt));
}

@tarekgh
Copy link
Member Author

tarekgh commented Mar 19, 2021

We already have tests like this, which assume the exact rules of the Africa/Casablanca zone.

That is exactly the point I want to avoid. These tests fail on the platforms which has Julian rules we don't support.

Copy link
Member

@eerhardt eerhardt left a comment

Choose a reason for hiding this comment

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

This looks good to me. Only a couple minor/nit comments.

Thanks @tarekgh!

@tarekgh tarekgh merged commit 8851b5d into dotnet:main Mar 20, 2021
@tarekgh tarekgh deleted the FixTimeZoneRulesOnLinux branch March 20, 2021 04:29
@ghost ghost locked as resolved and limited conversation to collaborators Apr 19, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TimeZoneInfo.AdjustmentRule information is not correct on Unix
6 participants