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

Switch DateTimeKind from Unspecified to Local #90149

Merged
merged 3 commits into from
Aug 14, 2023

Conversation

Maximys
Copy link
Contributor

@Maximys Maximys commented Aug 8, 2023

Fix #30128

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Aug 8, 2023
@ghost
Copy link

ghost commented Aug 8, 2023

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

Issue Details

Fix #30128

Author: Maximys
Assignees: -
Labels:

area-System.Management

Milestone: -

@Maximys
Copy link
Contributor Author

Maximys commented Aug 9, 2023

@ericstj, could you look to current PR and failed test System.Management.Tests.ManagementDateTimeConverterTests.DateTime_MinValue_RoundTrip?
If I correctly understand, .NET Framework 4.8 currently use DateTime(Int32, Int32, Int32, Int32, Int32, Int32, Int32) inside ToDateTime method.

@ericstj
Copy link
Member

ericstj commented Aug 10, 2023

That test cannot expect the behavior to change on .NETFramework - since we don't build System.Management for .NETFramework here.

If I correctly understand, .NET Framework 4.8 currently use DateTime(Int32, Int32, Int32, Int32, Int32, Int32, Int32) inside ToDateTime method.

Yes, that's correct.

So you'd need to make this test expect different behavior on .NETFramework.

@tarekgh @dotnet/area-system-management do you see a problem with this bug being fixed here?

@tarekgh
Copy link
Member

tarekgh commented Aug 10, 2023

I expressed in my old comment #30128 (comment) that the behavior is not matching the docs which expect the fixed behavior. So, I am fine with the fix but I prefer filing a breaking change issue for this one https://github.com/dotnet/docs/issues/new?assignees=gewarren&labels=breaking-change%2CPri1%2Cdoc-idea&projects=&template=breaking-change.yml&title=%5BBreaking+change%5D%3A+.

So you'd need to make this test expect different behavior on .NETFramework.

Correct, the test is already failing and should be fixed to return different result on NETFX. https://helixre107v0xdcypoyl9e7f.blob.core.windows.net/dotnet-runtime-refs-pull-90149-merge-039e8e43d9964292b0/System.Management.Tests/1/console.7246bcff.log?helixlogtype=result

Copy link
Contributor

@IDisposable IDisposable left a comment

Choose a reason for hiding this comment

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

:shipit:

@Maximys Maximys force-pushed the bugfix/30128-invalid-datetime-kind branch from 079948c to 59e5892 Compare August 11, 2023 07:05
@Maximys
Copy link
Contributor Author

Maximys commented Aug 11, 2023

@tarekgh , test for .NET Framework was skipped by the 59e5892 commit.
Breaking change issue is here

@tarekgh
Copy link
Member

tarekgh commented Aug 11, 2023

@Maximys thanks a lot for your response and opening the breaking change tracking. Would it make sense to not skip the test on the NETFX and instead validate the expected value there? you can use

public static bool IsNetFramework => RuntimeInformation.FrameworkDescription.StartsWith(".NET Framework", StringComparison.OrdinalIgnoreCase);
to do this check.

…entDateTimeConverter.ToDateTime(string) of NetFX.
@Maximys
Copy link
Contributor Author

Maximys commented Aug 14, 2023

@tarekgh , I had fixed highlighted problem, but there are some unrelated test failures. Could you look please?

@tarekgh tarekgh merged commit a666f59 into dotnet:main Aug 14, 2023
102 of 105 checks passed
@tarekgh
Copy link
Member

tarekgh commented Aug 14, 2023

I tracked the failed test with #90521

@ghost ghost locked as resolved and limited conversation to collaborators Sep 13, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Management community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ManagementDateTimeConverter.ToDateTime returns the wrong DateTimeKind
4 participants