-
Notifications
You must be signed in to change notification settings - Fork 261
fix: handle case-sensitive file overwrite on Windows and add tests #1259
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
fix: handle case-sensitive file overwrite on Windows and add tests #1259
Conversation
test: add tests for Windows file copy behavior
1277a9c
to
fc24241
Compare
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.
Thanks @ana1250, looks really good.
I just have some minor suggestions on improving the tests...
tests/TestableIO.System.IO.Abstractions.TestingHelpers.Tests/MockFileTests.cs
Outdated
Show resolved
Hide resolved
tests/TestableIO.System.IO.Abstractions.TestingHelpers.Tests/MockFileTests.cs
Outdated
Show resolved
Hide resolved
tests/TestableIO.System.IO.Abstractions.TestingHelpers.Tests/MockFileTests.cs
Outdated
Show resolved
Hide resolved
tests/TestableIO.System.IO.Abstractions.TestingHelpers.Tests/MockFileMoveTests.cs
Outdated
Show resolved
Hide resolved
tests/TestableIO.System.IO.Abstractions.TestingHelpers.Tests/MockFileMoveTests.cs
Outdated
Show resolved
Hide resolved
tests/TestableIO.System.IO.Abstractions.TestingHelpers.Tests/MockFileCopyTests.cs
Outdated
Show resolved
Hide resolved
This is addressed in release v22.0.13. |
Thanks! Looking forward to contributing more. |
@@ -163,6 +163,11 @@ public override void Copy(string sourceFileName, string destFileName, bool overw | |||
throw CommonExceptions.FileAlreadyExists(destFileName); | |||
} | |||
|
|||
if (string.Equals(sourceFileName, destFileName, StringComparison.OrdinalIgnoreCase) && XFS.IsWindowsPlatform()) |
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.
in all the other cases we use
mockFileDataAccessor.StringOperations.Equals(sourceFileName, destinationFileName)
instead of relying on
string.Equals(..., ..., StringComparison.OrdinalIgnoreCase)
Is intentional or an oversight?
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.
This seems to be an oversight.
We had one case here where we used string.Equals
on purpose, as it should compare case-sensitive in all operating systems, but here it seems I overlooked it.
This PR fixes an issue (#1140) where copying a file with the same name but different casing on Windows resulted in an "already in use" error. It aligns the behavior with the native file system.
Changes
System.IO.File.Copy
System.IO.File.Move
andSystem.IO.File.Replace
behavior.Testing