Skip to content

Conversation

@GrabYourPitchforks
Copy link
Member

@GrabYourPitchforks GrabYourPitchforks commented May 5, 2021

Resolves #20986.

Recent versions of Win10 relax the restriction on creating filesystem objects named COM1, LPT1, AUX, and similar. Some of our tests attempt to create these "illegal" filesystem objects and fail, as they're expecting an exception, but the directory is instead created successfully.

This PR skips all such tests on Win10, since we shouldn't test behavior which is an OS implementation detail subject to change. The tests still run on Win7 and Win8 to ensure that we're throwing friendly exception messages on those platforms.

I also took this opportunity to clean up some of the logic in PlatformDetection.cs. The Windows version checks now mostly filter down to a single helper, except where very specific version checks must take place.

There are no changes to shipping code in this PR. This PR only affects unit tests.

@GrabYourPitchforks GrabYourPitchforks added area-System.IO test-enhancement Improvements of test source code labels May 5, 2021
@GrabYourPitchforks GrabYourPitchforks added this to the 6.0.0 milestone May 5, 2021
@ghost
Copy link

ghost commented May 5, 2021

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

Issue Details

Resolves #20986.

Recent versions of Win10 relax the restriction on creating filesystem objects named COM1, LPT1, AUX, and similar. Some of our tests attempt to create these "illegal" filesystem objects and fail, as they're expecting an exception, but the directory is instead created successfully.

This PR skips all such tests on Win10, since we shouldn't test behavior which is an OS implementation detail subject to change. The tests still run on Win7 and Win8 to ensure that we're throwing friendly exception messages on those platforms.

I also took this opportunity to clean up some of the logic in PlatformDetection.cs. The Windows version checks now mostly filter down to a single helper, except where very specific version checks must take place.

Author: GrabYourPitchforks
Assignees: -
Labels:

area-System.IO, test enhancement

Milestone: 6.0.0

private static extern int GetCurrentApplicationUserModelId(ref uint applicationUserModelIdLength, byte[] applicationUserModelId);

internal static uint GetWindowsVersion()
private static volatile Version s_windowsVersionObject;
Copy link
Member

Choose a reason for hiding this comment

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

Volatile to avoid torn write?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, want the object ref to be published after all its fields are set.

Copy link
Member

Choose a reason for hiding this comment

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

Why don't we just use Lazy for it?

@danmoseley danmoseley merged commit 65020c9 into dotnet:main May 5, 2021
@GrabYourPitchforks GrabYourPitchforks deleted the win10fs branch May 5, 2021 06:17
@GrabYourPitchforks
Copy link
Member Author

Area owners - if there's any other feedback, leave it here (or email me) and we can get a followup PR posted, no problem.

Copy link
Member

@adamsitnik adamsitnik left a comment

Choose a reason for hiding this comment

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

@GrabYourPitchforks thank you for helping us to get stable tests!

private static extern int GetCurrentApplicationUserModelId(ref uint applicationUserModelIdLength, byte[] applicationUserModelId);

internal static uint GetWindowsVersion()
private static volatile Version s_windowsVersionObject;
Copy link
Member

Choose a reason for hiding this comment

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

Why don't we just use Lazy for it?

@ghost ghost locked as resolved and limited conversation to collaborators Jun 4, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-System.IO test-enhancement Improvements of test source code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Test PathWithReservedDeviceNameAsPath_ThrowsDirectoryNotFoundException fails on windows machine

3 participants