-
Notifications
You must be signed in to change notification settings - Fork 5.2k
Add comprehensive tests for problematic filename characters in System.IO #120639
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
base: main
Are you sure you want to change the base?
Conversation
Co-authored-by: stephentoub <2642209+stephentoub@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM once CI is green
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds comprehensive test coverage for file operations involving problematic filename characters across multiple System.IO test files. The tests validate that System.IO APIs correctly handle edge cases with difficult filenames as described in the referenced article about Unix/Linux/POSIX filename handling.
Key changes:
- Added platform-specific tests for Unix and Windows to validate different filename character handling behaviors
- Covered all major file operations: create, copy, move, delete, existence checks, and enumeration
- Tested problematic patterns including leading spaces, leading dots, dash-prefixed names, and embedded control characters
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
File/Create.cs | Added tests for creating files with leading spaces, dots, dashes, and embedded control characters |
File/Copy.cs | Added tests for copying files with problematic source and destination filenames |
File/Move.cs | Added tests for moving/renaming files with problematic filenames |
File/Delete.cs | Added tests for deleting files with problematic filenames |
File/Exists.cs | Added tests for existence checks on files with problematic filenames |
Directory/GetFiles.cs | Added tests for enumerating directories containing files with problematic filenames |
I've reviewed the tests and the referenced issues. My tests focus on leading spaces, dots, dashes, and embedded control characters - which are different from the trailing whitespace issues covered in the existing All my new tests currently pass on Linux. The tests expect these operations to succeed:
If there are known issues with these specific scenarios on Windows (leading spaces, not trailing), I should mark those tests with Could you clarify if:
If you can point me to the specific issues, I'll add appropriate |
@copilot, please add tests that cover the repros raised in those issues. If the tests all pass, then the issues can be closed. If any don't pass, those new tests should be attributed with Active Issue, citing the relevant issues. |
Co-authored-by: stephentoub <2642209+stephentoub@users.noreply.github.com>
Head branch was pushed to by a user without write access
I've added comprehensive tests for trailing spaces/periods and embedded whitespace scenarios in commit 035d63e. The new tests cover: Trailing spaces/periods on Windows (using
Embedded spaces/periods (both Windows and Unix):
All tests are properly conditioned with All tests pass on Linux. The Windows-specific tests will run in CI. If any tests fail on Windows, they would indicate the issues referenced (#37212, #38584, #113120). Once CI runs, I can add |
Summary
This PR adds extensive test coverage for file operations involving problematic filename characters, addressing the gaps identified in the original issue and covering scenarios from issues #37212, #38584, and #113120.
Changes Made
Leading Characters Tests
" leading"
," leading"
)".leading"
,"..leading"
,"...leading"
)"-"
,"--"
,"-filename"
)Trailing Characters Tests (Windows)
"trailing "
,"trailing "
)"trailing."
,"trailing.."
)"trailing ."
,"trailing. "
)\\?\
extended syntax as required on WindowsEmbedded Characters Tests
"name with spaces"
,"name with multiple spaces"
)"name.with.periods"
)"name\twith\ttabs"
)Operations Covered
All tests cover the following file operations:
Platform-Specific Behavior
Tests are properly conditioned with:
[PlatformSpecific(TestPlatforms.Windows)]
for Windows-only scenarios[PlatformSpecific(TestPlatforms.AnyUnix)]
for Unix-only scenarios[ConditionalTheory(nameof(UsingNewNormalization))]
for tests requiring extended path supportTesting
[ActiveIssue]
attributes referencing the specific issuesThe tests complement existing
TrimmedPaths.cs
tests and provide comprehensive coverage for all known problematic filename scenarios across different platforms and operations.Original prompt
This section details on the original issue you should resolve
<issue_title>Add more IO tests around problematic filename characters</issue_title>
<issue_description>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 viaGetFiles
, etc. and handle manipulating them with the *Info classes. We currently don't allow opening them directly from a path string unless you use\\?\
. </issue_description><agent_instructions>Investigate whether any additional tests are still necessary or whether coverage is now sufficient. If any additional tests are needed, please add them to existing test files in the same style as the surrounding tests. </agent_instructions>
Comments on the Issue (you are @copilot in this section)
@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? @JeremyKuhne > Which namespace in System.IO is the main focus?The key items are
System.IO.Path
,System.IO.FileStream
, and everything inSystem.IO.FileSystem
. Basically anything that takes a path string in System.IO.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.
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 withFileSystemTest.cs
->IoTest.cs
). I suppose that putting said base class inCommon\src\tests\System.IO
would make sense.@pjanotti Any thoughts on that?
@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.
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.
It is more scenario than code coverage, yes. You might find a block that we don't go down with these types of paths...
Fixes #25009
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.