Vstest: Test Console fails if path contains forward slash

[tannergooding]

Description

When using the latest SDK, you get an error if your VSTestResultsDirectory contains a forward slash (on Windows).

Microsoft (R) Test Execution Command Line Tool Version 16.3.0-preview-20190808-04
Copyright (c) Microsoft Corporation.  All rights reserved.

The path 'C:\Users\tagoo\repos\clangsharp\artifacts/tst/Debug/' specified in the 'ResultsDirectory' is invalid. Error: Illegal characters in path.

Steps to reproduce

1. Clone https://github.com/microsoft/clangsharp
2. Run .\scripts\cibuild.cmd

Expected behavior

Everything succeeds (as with previous versions)

Actual behavior

The Test Execution Command Line Tool fails with the above diagnostics.

[tannergooding]

This does not repro on preview 7 (3.0.100-preview7-012821)

[ShreyasRmsft]

@tannergooding i took a look a have an idea what is causing the issue.

https://github.com/microsoft/vstest/blob/03cb97c1e7a57dfed561a872e5797110524a1d70/src/vstest.console/Processors/ResultsDirectoryArgumentProcessor.cs#L178

this methods "IsValidPath"

is first splitting the given path using Path.DirectorySeparatorChar which returns '\' for windows.

Then for each folder (results of the split) we are checking if it contains any of Path.GetInvalidFileNameChars one of which is a '/'

The fix would be to normalize (replace '/' and '\' with ) the filepath before calling isvalid on it

Thank you for reporting this. We would be delighted if you could contribute this fix. And i will provide any help needed. Otherwise i will pick this up sometime soon. Thanks again.

[tannergooding]

@ShreyasRmsft, why does the path need to be validated at all? Why aren't we just passing the path (as given) down to the right APIs and allowing the file operation to fail if it determines it should?

There are a range of characters that may or may not be valid based on where the destination exists and the file system it falls under. For example, it could be a network path in which case normalizing / or \ would be incorrect. You could be targeting a file system (say FAT16, FAT32, or a Unix file system) which has a different set of allowed characters than NTFS. You could be targeting a virtual file system, etc

@JeremyKuhne, @carlossanlop. Could you possibly comment on "best practice" here, with regards to taking in user specified file paths and having them "just work" for cross-platform code?

[ShreyasRmsft]

@tannergooding the main motive was to fail fast with a helpful error message upfront rather than failing to say create the attachment after an hour long test run. And i would stand by this and not change my stance here.

As for the normalization , the normalization happens on the target machine (there is no other target machine as such, the testhost runs on the same machine as vstest.console.exe). The only case that could remotely affect us is network paths. The only bit of knowledge i lack at moment and will probably will need read up on is whether you can refer to network paths from one OS to another (linux to windows or vise-versa).

Also the .Net Path apis we are using are OS specific and storage agnostic afaik. Please do correct me if i am wrong.

[tannergooding]

the main motive was to fail fast with a helpful error message upfront rather than failing to say create the attachment after an hour long test run

Why not validate this with something as simple as validating that the directory exists or can be created upfront?

[JeremyKuhne]

As a fundamental design principle, don't try and validate paths. You'll inevitably get it wrong. Let Path.GetFullPath() normalize it for you (which it does through the OS) and let the OS tell you if the path isn't usable when you try to use it.

Note that even if you do validate a path, there is no way to ensure that the path will be the same next time you access it. Files turn into directories and vice versa, paths get deleted, etc.

[JeremyKuhne]

Note that we explicitly took "pre validation" out of .NET as we blocked valid scenarios. The _only_ thing we check up front now is that you don't have an embedded null in a path as that can result in very hard to reason failures. If you need to try to assert something don't hesitate to contact me and I can give you feedback.

[eerhardt]

Note this is also breaking ML.NET consuming 3.0 preview8 SDK.

https://github.com/dotnet/machinelearning/pull/4115

Are we going to get a fix for preview9?

[eerhardt]

Should this issue be closed now that #2133 is merged?

Also, will the fix make it into 3.0, since this is a regression?

[vagisha-nidhi]

@eerhardt Yes the issue should be closed. We have made a release as well with this fix.
The fix will go into dotnet cli 3.0 also.

[eerhardt]

This fix didn't make 3.0 preview9. Will it make the final 3.0 release?