PathVerifier should check specifically for invalid use of volume separator and tolerate alt dir separator #327

rkoeninger · 2018-07-16T20:51:15Z

Fixes #283
Fixes #320

^ Lots of information on the issue (#320) about how things are handled in Unix, Mono and the .NET Framework reference implementation of File.Copy/File.Move.

PathVerifier.IsLegalAbsoluteOrRelative now throws NotSupportedException on Windows only when the path contains an invalid use of the drive separator char.
Renamed 8 tests to mention ArgumentException instead of NotSupportedException.
Added 8 tests for invalid drive separator specifically.
Updated 4 Windows only tests to test invalid path chars other than drive separator.

Added 8 tests, updated 4 existing tests

fgreinacher · 2018-07-25T19:16:11Z

TestHelpers.Tests/MockFileCopyTests.cs

@@ -191,6 +197,64 @@ public void MockFile_Copy_ShouldThrowNotSupportedExceptionWhenTargetFileNameCont
            }
        }

+        [Test]
+        [WindowsOnly(WindowsSpecifics.StrictPathRules + "; Mono does not raise this exception")]


Why not WindowsSpecifics.Drives? This way we wouldn't need the additional text. Same for the other new tests.

Sure, used Drives instead.

fgreinacher

Thanks for this. I left a couple of comments.

fgreinacher · 2018-07-25T19:19:58Z

TestHelpers.Tests/MockFileCopyTests.cs

+            var badDestinationPath = XFS.Path(@"^:\elsewhere\demo.txt");
+            var fileSystem = new MockFileSystem();
+            fileSystem.AddFile(sourcePath, new MockFileData("1"));
+            fileSystem.AddDirectory(destinationFolder);


This seems to be unnecessary.

Are you saying the tests themselves are necessary? They're the only place the NotSupportedException throws are tested, which is the impetus for the PR. There's a test for each potential issue with the volume separator (which would cause the NSE), for each of sourcePath and destinationPath, for each of Move and Copy - that's the 8 tests.

Are these 5 particular ones redundant? I thought people would complain about a lack of coverage if I didn't add of these.

I meant just the line above (and the variable), where you add some directory that doesn’t seem to be necessary for the test. Sorry for being unclear on this!

Ah, Ok. I re-visited all 8 of those tests and found that creating the source file and destination folder was not necessary to produce the error, so I removed those parts. They just create a file system and attempt the move/copy and assert the NotSupportedException is raised.

fgreinacher · 2018-07-25T19:20:41Z

TestHelpers.Tests/MockFileMoveTests.cs

+        [WindowsOnly(WindowsSpecifics.StrictPathRules + "; Mono does not raise this exception")]
+        public void MockFile_Move_ShouldThrowNotSupportedExceptionWhenSourcePathContainsInvalidUseOfDriveSeparator()
+        {
+            var sourcePath = XFS.Path(@"C:\something\demo.txt");


This seems to be unnecessary.

fgreinacher · 2018-07-25T19:20:58Z

TestHelpers.Tests/MockFileMoveTests.cs

+        [WindowsOnly(WindowsSpecifics.StrictPathRules + "; Mono does not raise this exception")]
+        public void MockFile_Move_ShouldThrowNotSupportedExceptionWhenSourcePathContainsInvalidDriveLetter()
+        {
+            var sourcePath = XFS.Path(@"C:\something\demo.txt");


This seems to be unnecessary.

fgreinacher · 2018-07-25T19:21:26Z

TestHelpers.Tests/MockFileMoveTests.cs

+        public void MockFile_Move_ShouldThrowNotSupportedExceptionWhenDestinationPathContainsInvalidUseOfDriveSeparator()
+        {
+            var sourcePath = XFS.Path(@"C:\something\demo.txt");
+            var destinationFolder = XFS.Path(@"C:\elsewhere");


This seems to be unnecessary.

fgreinacher · 2018-07-25T19:21:36Z

TestHelpers.Tests/MockFileMoveTests.cs

+        public void MockFile_Move_ShouldThrowNotSupportedExceptionWhenDestinationPathContainsInvalidDriveLetter()
+        {
+            var sourcePath = XFS.Path(@"C:\something\demo.txt");
+            var destinationFolder = XFS.Path(@"C:\elsewhere");


This seems to be unnecessary.

fgreinacher · 2018-07-25T19:23:53Z

TestingHelpers/PathVerifier.cs

+                if (path.Length > 0 && path[0] == Path.VolumeSeparatorChar
+                    || path.Length > 1 && path[1] == Path.VolumeSeparatorChar && !char.IsLetter(path[0])
+                    || path.LastIndexOf(Path.VolumeSeparatorChar) > 1)
+                {


This whole condition is a bit too dense for me. Maybe extract some intermediate variables or helper methods.

Moved to private static method and simplified expression. Almost added a comment, but I think the new code doesn't need it: lastVolSepIndex == -1 || lastVolSepIndex == 1 && char.IsLetter(path[0])

fgreinacher · 2018-07-25T19:24:49Z

TestHelpers.Tests/MockFileCopyTests.cs

+            foreach (var invalidChar in fileSystem.Path.GetInvalidFileNameChars()
+                .Where(x => x != fileSystem.Path.DirectorySeparatorChar
+                    && x != fileSystem.Path.AltDirectorySeparatorChar
+                    && x != fileSystem.Path.VolumeSeparatorChar))


Maybe add a comment that explains why exclude some characters here?

Added comments explaining why they're excluded and moved to local char[] show I could just use .Except() instead of length .Where().

Also handled same case in Move tests

fgreinacher · 2018-07-25T19:25:04Z

TestHelpers.Tests/MockFileCopyTests.cs

+            foreach (var invalidChar in fileSystem.Path.GetInvalidFileNameChars()
+                .Where(x => x != fileSystem.Path.DirectorySeparatorChar
+                    && x != fileSystem.Path.AltDirectorySeparatorChar
+                    && x != fileSystem.Path.VolumeSeparatorChar))


Did same here

rkoeninger · 2018-07-27T00:46:55Z

@fgreinacher Applied suggested changes. Made code simpler and easier to understand. Thanks.

fgreinacher

👍

jpreese · 2018-07-28T15:23:13Z

TestHelpers.Tests/MockFileCopyTests.cs

@@ -106,8 +106,19 @@ public void MockFile_Copy_ShouldThrowNotSupportedExceptionWhenSourceFileNameCont

            var destFilePath = XFS.Path(@"c:\something\demo.txt");
            var fileSystem = new MockFileSystem();
+            var excludeChars = new []


This list, comments and all, is introduced four times throughout the PR. I think there's value in pulling this out into a static constant helper file that we can just pull from.

Added a class called Shared to contain the building of the chars. There isn't a very good name for them though, as they're just chars that don't have ArgumentException to be raised. So it's called SpecialInvalidPathChars.

jpreese · 2018-07-28T15:24:32Z

TestHelpers.Tests/MockFileCopyTests.cs

@@ -120,7 +131,7 @@ public void MockFile_Copy_ShouldThrowNotSupportedExceptionWhenSourceFileNameCont
        }

        [Test]
-        public void MockFile_Copy_ShouldThrowNotSupportedExceptionWhenSourcePathContainsInvalidChars_Message()
+        public void MockFile_Copy_ShouldThrowArgumentExceptionWhenSourcePathContainsInvalidChars_Message()
        {
            if (XFS.IsUnixPlatform())


While not necessarily part of the PR, we're touching the line right above it. It'd be nice to push forward and get rid of these if() checks and use the attribute.

jpreese · 2018-07-28T15:24:39Z

TestHelpers.Tests/MockFileCopyTests.cs

@@ -120,7 +131,7 @@ public void MockFile_Copy_ShouldThrowNotSupportedExceptionWhenSourceFileNameCont
        }

        [Test]
-        public void MockFile_Copy_ShouldThrowNotSupportedExceptionWhenSourcePathContainsInvalidChars_Message()
+        public void MockFile_Copy_ShouldThrowArgumentExceptionWhenSourcePathContainsInvalidChars_Message()


While not necessarily part of the PR, we're touching the line right above it. It'd be nice to push forward and get rid of these if() checks and use the attribute.

Done for this and 3 other similar tests.

jpreese · 2018-07-28T15:29:51Z

TestHelpers.Tests/MockFileCopyTests.cs

+        }
+
+        [Test]
+        [WindowsOnly(WindowsSpecifics.Drives)]


Would this necessarily be a Windows only feature? I may need to look into it more, but the C:\ path will just be converted to a unix path, along the lines of /tmp/some/dir, and I would think /elsewhere:/demo.txt would be a valid case and something to test against a Unix environment.

Tests verify behavior, skipping them on an environment doesn't prevent the behavioral changes from going through, they just make us blind to the side effects.

According to the docs for Path.VolumeSeparatorChar:

The value of this field is a colon (:) on Windows and Macintosh, and a slash (/) on UNIX operating systems. This is most useful for parsing paths such as "c:\windows" or "MacVolume:System Folder".

So on Unix, the test would just be inserting a /, which wouldn't cause any exception to be raised.

Drives isn't exactly the reason why the test is Windows-only, but Unix doesn't have an analog to the : on Windows, so there's no analog for this test on Unix.

I think this approach is fine. @jpreese Any objections left?

jpreese · 2018-07-28T15:33:29Z

TestHelpers.Tests/MockFileReadAllBytesTests.cs

+
+            fileSystem.AddFile(XFS.Path(@"C:\test.txt"), new MockFileData("content"));
+
+            Assert.AreEqual("content", fileSystem.File.ReadAllText(XFS.Path("C:/test.txt")));


The name of the test is ReadAllBytes, but we're calling ReadAllText. It also looks like were hardcoding the forward slash. If AltDirectorySeparator were to change, this test might not make anymore sense. If you want to specifically test forward slash, I'd probably prefer calling out the way that were honestly just testing forwards and backwards slashes.

Used byte array instead and built paths using fileSystem.Path.DirectorySeparatorChar and .AltDirectorySeparatorChar.

jpreese · 2018-07-28T15:35:22Z

TestHelpers.Tests/MockFileCopyTests.cs

+            var destinationPath = XFS.Path(@"C:\elsewhere\demo.txt");
+            var fileSystem = new MockFileSystem();
+
+            Assert.Throws<NotSupportedException>(() => fileSystem.File.Copy(badSourcePath, destinationPath));


Small nit, but it's generally preferred to split up the Act and Assert steps.

TestDelegate action = () => fileSystem.File.Copy(badSourcePath, destinationPath); Assert.Throws<NotSupportedException>(action);

Makes the debugging and readability story a bit better.

Not my style, but sure, done.

Used [WindowsOnly] instead of if (IsUnix) Split delegates out of Assert.Throws onto separate line

...and no longer hard-codes forward slash

rkoeninger · 2018-07-30T02:36:27Z

@jpreese Made suggested changes, except for the volume separator one, which I don't know what to do for.

…stem.IO.Abstractions into copy-move-exception

fgreinacher · 2018-08-27T08:14:04Z

Merging this one. If we want to do something for the volume separator disuccuisb we can do that in a separate PR.

rkoeninger added 3 commits July 16, 2018 16:45

Corrected names of existing File Copy/Move tests

38f60c8

PathVerifier throws NotSupEx instead of ArgEx on invalid use of vol sep

f3614fd

Added 8 tests, updated 4 existing tests

Added spliting by alt dir sep char to PathVerifier

6ecb2f6

rkoeninger changed the title ~~File.Copy/Move should throw NotSupportedException on invalid use of drive separator~~ PathVerifier should check specifically for invalid use of volume separator and tolerate alt dir separator Jul 17, 2018

rkoeninger mentioned this pull request Jul 17, 2018

MockFileSystem's File.ReadAllText throws when given a path using forward slashes #283

Closed

rkoeninger added 4 commits July 17, 2018 17:45

Merge branch 'master' into copy-move-exception

f390a0c

Merge branch 'master' into copy-move-exception

640dd6c

Merge branch 'master' into copy-move-exception

e3ae955

Merge branch 'master' into copy-move-exception

f6064a8

fgreinacher reviewed Jul 25, 2018

View reviewed changes

fgreinacher suggested changes Jul 25, 2018

View reviewed changes

rkoeninger added 5 commits July 25, 2018 15:29

Merge branch 'master' into copy-move-exception

ad5be0c

Used WindowsSpecifics.Drives instead of lengthy message

5b3204e

Extracted and simplified volume separator in path validity check

1b7b739

Comment why certain invalid filename chars not checked in relevant tests

d63b23a

Cleaned up unecessary code in Move/Copy NotSupportedException tests

1ca01ce

fgreinacher approved these changes Jul 27, 2018

View reviewed changes

jpreese requested changes Jul 28, 2018

View reviewed changes

rkoeninger added 3 commits July 28, 2018 15:36

Moved SpecialInvalidChars to single declaration

751d43c

Used [WindowsOnly] instead of if (IsUnix) Split delegates out of Assert.Throws onto separate line

ReadAllBytes_ShouldTolerateAltDirectorySeparatorInPath uses binary data

009718b

...and no longer hard-codes forward slash

Removed redundant use of MockUnixSupport.Path from WindowsOnly tests

22335b7

rkoeninger added 3 commits August 8, 2018 18:05

Merge branch 'master' of https://github.com/System-IO-Abstractions/Sy…

5dc883a

…stem.IO.Abstractions into copy-move-exception

Moved new file to changed project path

2389487

Merge branch 'master' into copy-move-exception

b13843c

rkoeninger mentioned this pull request Aug 27, 2018

Don't allow files and directories with the same name #322

Merged

fgreinacher merged commit 85c51a2 into TestableIO:master Aug 27, 2018

fgreinacher mentioned this pull request Oct 4, 2018

System.IO.DirectoryNotFoundException after upgrade to System.IO.Abstractions.TestHelpers@2.1.0.230 #364

Closed

rkoeninger deleted the copy-move-exception branch December 1, 2018 21:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PathVerifier should check specifically for invalid use of volume separator and tolerate alt dir separator #327

PathVerifier should check specifically for invalid use of volume separator and tolerate alt dir separator #327

rkoeninger commented Jul 16, 2018 •

edited

fgreinacher Jul 25, 2018 •

edited

rkoeninger Jul 25, 2018 •

edited

fgreinacher left a comment

fgreinacher Jul 25, 2018

rkoeninger Jul 25, 2018 •

edited

fgreinacher Jul 26, 2018

rkoeninger Jul 27, 2018

fgreinacher Jul 25, 2018

fgreinacher Jul 25, 2018

fgreinacher Jul 25, 2018

fgreinacher Jul 25, 2018

fgreinacher Jul 25, 2018

rkoeninger Jul 25, 2018

fgreinacher Jul 25, 2018

rkoeninger Jul 25, 2018 •

edited

rkoeninger Jul 25, 2018

fgreinacher Jul 25, 2018

rkoeninger Jul 25, 2018

rkoeninger commented Jul 27, 2018

fgreinacher left a comment

jpreese Jul 28, 2018

rkoeninger Jul 28, 2018

jpreese Jul 28, 2018

jpreese Jul 28, 2018

rkoeninger Jul 28, 2018

jpreese Jul 28, 2018

rkoeninger Jul 28, 2018 •

edited

fgreinacher Aug 8, 2018

jpreese Jul 28, 2018

rkoeninger Jul 28, 2018

jpreese Jul 28, 2018

rkoeninger Jul 28, 2018

rkoeninger commented Jul 30, 2018

fgreinacher commented Aug 27, 2018


		fileSystem.AddFile(XFS.Path(@"C:\test.txt"), new MockFileData("content"));

		Assert.AreEqual("content", fileSystem.File.ReadAllText(XFS.Path("C:/test.txt")));

PathVerifier should check specifically for invalid use of volume separator and tolerate alt dir separator #327

PathVerifier should check specifically for invalid use of volume separator and tolerate alt dir separator #327

Conversation

rkoeninger commented Jul 16, 2018 • edited

fgreinacher Jul 25, 2018 • edited

Choose a reason for hiding this comment

rkoeninger Jul 25, 2018 • edited

Choose a reason for hiding this comment

fgreinacher left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rkoeninger Jul 25, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rkoeninger Jul 25, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rkoeninger commented Jul 27, 2018

fgreinacher left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rkoeninger Jul 28, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rkoeninger commented Jul 30, 2018

fgreinacher commented Aug 27, 2018

rkoeninger commented Jul 16, 2018 •

edited

fgreinacher Jul 25, 2018 •

edited

rkoeninger Jul 25, 2018 •

edited

rkoeninger Jul 25, 2018 •

edited

rkoeninger Jul 25, 2018 •

edited

rkoeninger Jul 28, 2018 •

edited