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

Test Console fails if path contains forward slash #2126

Open
tannergooding opened this issue Aug 9, 2019 · 7 comments

Comments

@tannergooding
Copy link
Member

commented Aug 9, 2019

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 comment has been minimized.

Copy link
Member Author

commented Aug 9, 2019

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

@ShreyasRmsft

This comment has been minimized.

Copy link
Member

commented Aug 12, 2019

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

private bool IsValidPath(string argument)

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.

@ShreyasRmsft ShreyasRmsft self-assigned this Aug 12, 2019

@ShreyasRmsft ShreyasRmsft added the bug label Aug 12, 2019

@tannergooding

This comment has been minimized.

Copy link
Member Author

commented Aug 12, 2019

@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

This comment has been minimized.

Copy link
Member

commented Aug 12, 2019

@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

This comment has been minimized.

Copy link
Member Author

commented Aug 12, 2019

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

This comment has been minimized.

Copy link
Member

commented Aug 12, 2019

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

This comment has been minimized.

Copy link
Member

commented Aug 12, 2019

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.