Add more IO tests around problematic filename characters

[JeremyKuhne]

While we have some tests, it would be good to add more with problematic filenames and filename characters.

Here is a good article describing difficult names for reference:

https://www.dwheeler.com/essays/fixing-unix-linux-filenames.html

Creating, copying, moving, enumerating, deleting, etc.

Trailing spaces and periods on Windows are legal, but difficult to create and access. Calling Win32 APIs with paths normally will strip these. The exception is when prepended with \\?\. We purposefully do not handle creating these unless you use \\?\ as creating these sort of paths frequently breaks apps. We should (and do) handle finding them via GetFiles, etc. and handle manipulating them with the *Info classes. We currently don't allow opening them directly from a path string unless you use \\?\.

[eriawan]

@karelz

Does this mean adding more tests as unit tests? Which namespace in System.IO is the main focus?

I would like to try to take this one,

[ViktorHofer]

@JeremyKuhne I pointed @eriawan to https://github.com/dotnet/corefx/tree/master/src/System.IO.FileSystem/tests/File as a starting point to add tests. You probably want to be more precise where you expected these tests to be added. Do you have a file with TestData that is shared across test files and potentially also test projects?

[eriawan]

@ViktorHofer

Yes, that would be great to know files to work directly. Thanks!

@JeremyKuhne

I have looked at https://github.com/dotnet/corefx/tree/master/src/System.IO.FileSystem/tests/File especially TestData.cs file. As far as I know, the test data of valid directory is mostly for Windows, although there's no explicit indicator that it's for Windows.

Compared to the reference article of defining testing of valid filename you have mentioned, there should be test data for Linux valid filenames/path.

Also I'm curious about the validation, because Path class has method of GetInvalidFileNameChars: https://docs.microsoft.com/en-us/dotnet/api/system.io.path.getinvalidfilenamechars

Should we include this method to test also? CMIIW, Path is not in the System.IO.FileSystem folder, it's in CoreLib. How to aggregate with tests in System.IO.FileSystem?

[JeremyKuhne]

Which namespace in System.IO is the main focus?

The key items are System.IO.Path, System.IO.FileStream, and everything in System.IO.FileSystem. Basically anything that takes a path string in System.IO.

Does this mean adding more tests as unit tests?

The idea is to use the linked post as a hint for creating paths that are known to be problematic. Spaces are one example. Leading, trailing, embedded- they're easy to mess up.

Here is one PR where I was expanding/cleaning up in this vein: dotnet/corefx#27449
Here is another PR where I was fixing trailing space handling when enumerating on Windows: dotnet/corefx#27809

Some things will not work that should (#27809). Having tests and assigning issues to them we can fix them and track where they're still "broken" (e.g. on desktop .NET). Some things will not work and will be technically difficult to fix and/or shouldn't be fixed by design (e.g. we don't want to allow creating files on Windows with trailing spaces or periods without using device syntax as discussed above). They'll sometimes point out gaps in documentation that we can also follow up on.

Some things will work that we don't have coverage for and we'll be less likely to break them in the future. One example of this is that I added a bunch of tests that tried working with paths with and without trailing separators. I both found existing issues and prevented introducing new ones when making a variety of the perf related changes.

How to aggregate with tests in System.IO.FileSystem?

Tests have to stay local to the reference assembly where they're exposed (i.e. where they show up in \ref). Despite that, cleaning up and pulling together IO test data is a worthwhile endeavor. All System.IO tests should probably derive from the same base class to get common test data in typical xunit style (probably starting with FileSystemTest.cs -> IoTest.cs). I suppose that putting said base class in Common\src\tests\System.IO would make sense.

@pjanotti Any thoughts on that?

[eriawan]

@JeremyKuhne

The idea is to use the linked post as a hint for creating paths that are known to be problematic. Spaces are one example. Leading, trailing, embedded- they're easy to mess up.

Here is one PR where I was expanding/cleaning up in this vein: dotnet/corefx#27449
Here is another PR where I was fixing trailing space handling when enumerating on Windows: dotnet/corefx#27809

Sorry, I could only follow some of your explanation. Also I'm still confused at the references that's mostly for Linux/Unix bad filenames in relations to which tests and what would be the concrete steps to do.

Ok, my questions are:

1. Your PR Path tests overhaul corefx#27449 focused on tests on Path on System.Runtime.Extensions. So is this will be the first implementation focus of my ongoing PR (for this issue) to add more tests?
2. What about tests in System.IO.FileSystem? Imho, it is related, so we need a way to integrate, and using an abstract test class would be good idea.
3. Since the trailing space handling is fixed on Windows, then how about on Linux? I couldn't see how to test when the Path and other IO classes used to test bad filenames on Linux/Unix, or maybe I miss something in the codebase.

Some things will work that we don't have coverage for and we'll be less likely to break them in the future.

4. Does this mean more like scenario coverage, not code coverage? CMIIW, because if this is a code coverage, then adding more code coverage should be in separate issue. If it is about additional coverage of scenarios, then I'll start to work on this too, within the scope of this issue.
5. For the initial work to add more IO tests, which codebase I should focus first? also looking on your PRs, I think I should also add conditional check on filename validity, based on platform. Please CMIIW on this too,

Thanks in advance

[JeremyKuhne]

System.Runtime.Extensions. So is this will be the first implementation focus of my ongoing PR (for this issue) to add more tests?

There or System.IO.FileSystem. Doesn't matter. I know it is a little bit confusing. In theory Path should be in System.IO.FileSystem. It can't be, however, as assemblies that are lower in the stack of dependencies need it. The source code is actually in System.Private.CoreLib in CoreCLR for path. System.Runtime.Extensions was the lowest assembly where we could expose the type publicly.

Since the trailing space handling is fixed on Windows, then how about on Linux?

I fixed some things. I may not have fixed all of them. The key is to make sure we have all APIs that take paths on all platforms with tests that validate behavior with "weird" paths. "Weird" paths are more likely to either be broken or get broken with future source code changes.

Does this mean more like scenario coverage, not code coverage?

It is more scenario than code coverage, yes. You might find a block that we don't go down with these types of paths, but I think it is probably unlikely.

For the initial work to add more IO tests, which codebase I should focus first?

System.IO.FileSystem may be easier as it doesn't have the actual source code in another repository. :)

I think I should also add conditional check on filename validity, based on platform.

For the most part we want to check the same input data on both platforms. For example: Even though C:\foo has no special meaning for Unix, there is value in making sure we don't treat it like it does.

[eriawan]

@JeremyKuhne

Thanks! I'll assess this first.
Yes, it is quite confusing from start, because Path is not in System.IO.FileStream nor in System.IO.
I'll check your PR dotnet/corefx#27449 and dotnet/corefx#27809 again.

[deeprobin]

It would make sense to test reserved file names/devices and reserved characters on windows (See https://docs.microsoft.com/en-us/windows/win32/fileio/naming-a-file?redirectedfrom=MSDN).