Skip to content

Adjust tests for Windows 11 changes#57678

Merged
jaredpar merged 2 commits intodotnet:mainfrom
jaredpar:test
Nov 15, 2021
Merged

Adjust tests for Windows 11 changes#57678
jaredpar merged 2 commits intodotnet:mainfrom
jaredpar:test

Conversation

@jaredpar
Copy link
Copy Markdown
Member

Windows 11 appears to have removed the restrictions on legal file names
that was introduced in 1974. For example file names like con, prn, com1,
etc ... More info here.

https://www.howtogeek.com/fyi/windows-10-still-wont-let-you-use-these-file-names-reserved-in-1974/

This broke a number of tests that were verifying we errored correctly in
those scenarios. The verification was done by running Path.GetFullPath
on the file names and then checking them for the escape sequence that
Windows added.

// Windows 10
WriteLine(Path.GetFullPath("aux.txt")); // \\.\aux.txt

// Windows 11
WriteLine(Path.GetFullPath("aux.txt")); // c:\users\jaredpar\aux.txt

This PR adjusts those tests to correctly pass when run on Windows 11.

closes #55730

@jaredpar jaredpar requested a review from a team as a code owner November 10, 2021 15:51
@jaredpar
Copy link
Copy Markdown
Member Author

@dotnet/roslyn-compiler PTAL

Windows 11 appears to have removed the restrictions on legal file names
that was introduced in 1974. For example file names like con, prn, com1,
etc ... More info here.

https://www.howtogeek.com/fyi/windows-10-still-wont-let-you-use-these-file-names-reserved-in-1974/

This broke a number of tests that were verifying we errored correctly in
those scenarios. The verification was done by running `Path.GetFullPath`
on the file names and then checking them for the escape sequence that
Windows added.

```c#
// Windows 10
WriteLine(Path.GetFullPath("aux.txt")); // \\.\aux.txt

// Windows 11
WriteLine(Path.GetFullPath("aux.txt")); // c:\users\jaredpar\aux.txt
```

This PR adjusts those tests to correctly pass when run on Windows 11.

closes dotnet#55730
#endif

public static bool IsWindows => Path.DirectorySeparatorChar == '\\';
public static bool IsWindows11OrGreater => IsWindows && Environment.OSVersion.Version.Build >= 22_000;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Win10 and Win11 are still being updated SxS right? How does the build number work there?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I'm unsure how the Win10 build numbers are being updated. I'm more than open to diff ways to check for Windows 11.

One suggestion I got was to check for the path issue directly. Essentially don't check for windows 11, instead check for aux.txt path being escaped in GetFullPath. That is a more direct test but I worried about about other potential consequences there.

I'm thinking of flipping to that for now and changing if we find such consequnecs

@RikkiGibson
Copy link
Copy Markdown
Member

I don't understand why the failure mode of CommandLineTests.ReservedDeviceNameAsFileName before it was skipped in CI was to time out. Do you have any insight into that?

@jaredpar
Copy link
Copy Markdown
Member Author

I don't understand why the failure mode of CommandLineTests.ReservedDeviceNameAsFileName before it was skipped in CI was to time out. Do you have any insight into that?

No. The best explanation I could come up with is that we hit a temporary bug in .NET core. The timeout was happening on an RC build where now we are on RTM. That is a weak explanation and I have no evidence to back it up. I also don't have a better suggestion.

Now though everything seems to be running fine. If we hit a new timeout we can dig into it.

@RikkiGibson
Copy link
Copy Markdown
Member

Main thing that concerns me about this moving forward is the risk that the test might rot on windows 10 if more changes are made to it in the windows 11 future. I imagine in some reasonable time frame we will have to just agree on a moment to drop windows 10 support for developing this project.

@jaredpar
Copy link
Copy Markdown
Member Author

@RikkiGibson, @chsienki I adjusted the condition to no longer look for Windows11. Instead I have the condition look directly for the odd file behavior. I think it's overall the better approach

Copy link
Copy Markdown
Member

@RikkiGibson RikkiGibson left a comment

Choose a reason for hiding this comment

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

This seems fine. Is the implementation also checking this property in order to decide whether to give the diagnostic?

@jaredpar
Copy link
Copy Markdown
Member Author

@RikkiGibson

This seems fine. Is the implementation also checking this property in order to decide whether to give the diagnostic?

The implementation does the exact same check (albeit with a bit more indirection).

@jaredpar
Copy link
Copy Markdown
Member Author

So the tests are hanging in CI again which is good news I guess cause I can investigate. But they don't fail on my machine CoreCLR or Desktop so that is really surprising. Going to be interested to see what the root cause is here

@jaredpar jaredpar merged commit 08d56b1 into dotnet:main Nov 15, 2021
@ghost ghost added this to the Next milestone Nov 15, 2021
@jaredpar jaredpar deleted the test branch November 15, 2021 19:55
333fred added a commit to 333fred/roslyn that referenced this pull request Nov 17, 2021
…rations

* upstream/main: (3387 commits)
  Fix ValueTracking for index parameters (dotnet#57727)
  Avoid accessing current assembly identity while reporting an accessibility diagnostics for an inaccessible internal symbol. (dotnet#57783)
  Include a type for NoneOperations in VB, print the type in tests (dotnet#57664)
  Don't throw exceptions for file changes after a project is unloaded
  Check up front for being called to remove more than once
  Fix C# language name in spec (dotnet#57427)
  Add test
  Fix null ref in navbars
  Ensure that getting the checksum for a project cone is resilient to its project references being missing
  Check constraints on lifted operator types (dotnet#57050)
  Adjust tests for Windows 11 changes (dotnet#57678)
  Add comment
  Load SVsShellDebugger before calling IVsSolution.CreateSolution
  Remove extra EnsureEditableDocuments  calls (dotnet#57725)
  Don't show nullable annotation in completion items of static field/property
  Don't analyze local function bodies as though they are top level code (dotnet#57623)
  update error code to fix main break (dotnet#57739)
  Error when ref is used on a parameter or return type of an UnmanagedCallersOnly method (dotnet#57043)
  Simplify code from review
  Fix featureflag name for .net 6 host in UI
  ...
@allisonchou allisonchou modified the milestones: Next, 17.1.P2 Nov 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Re-enable test CommandLineTests.ReservedDeviceNameAsFileName

4 participants