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

rkoeninger · 2018-07-12T03:01:07Z

Fixes #208
Fixes #223
Fixes #318

Effects of this PR:

Can no longer have a file and a directory with the same name exist at the same time.
Trailing slashes are acceptable but not required.
Directories can be created, removed and queried with a trailing slash without any difference in behavior.
Tests that assumed directories would have trailing slashes have been fixed.
Keys in the dictionary in MockFileSystem no longer have trailing slashes unless:
- It's a Windows drive root.
- Or the Unix file system root.
Files enumerated by Directory.GetFiles() won't have trailing slashes.
Incidentally, UnixOnlyAttribute/UnixSpecifics were added to mirror WindowsOnlyAttribute/WindowsSpecifics.
14 unit tests added to cover the TrimSlashes helper, increase coverage of existing functionality and confirm that issues MockFileSystem should not require trailing slash for directories #208 and MockFileSystem.File.Exists throws NullReferenceException #233 have been addressed as well.

jpreese · 2018-07-12T23:48:19Z

TestHelpers.Tests/StringExtensionsTests.cs

+        [Test]
+        public void CleanPath_DriveRoot_PreserveTrailingSlash()
+        {
+            Assert.AreEqual(@"c:\", @"c:\".CleanPath());


It may seem a little silly / overly verbose, but I think I'd prefer if this was broken up into three lines for AAA (no comments for each section though).

You'll also need XFS.Path for Linux shenanigans.

AAA really only makes sense in situations where you're testing mutable state. Arrange the state. Act on state with impure operation. Assert result.

This is the closest approximation I can make:

// Arrange var x = @"c:\"; // Act var y = x.CleanPath(); // Assert Assert.AreEqual(@"c:\", y);

XFS.Path noted.

jpreese · 2018-07-12T23:50:07Z

TestingHelpers/MockFileSystem.cs


+        private void SetEntry(string path, MockFileData mockFile)
+        {
+            path = FixPath(path, true);


I'm personally a fan of pure/functional methods where possible, so I think I'd rather see this be immutable.

You mean overwriting the value of the local variable? I'm not a fan of that either. I've inline-ed those in places and will continue to.

jpreese · 2018-07-12T23:53:29Z

Thanks for working on this @rkoeninger. I'm really curious what you come up for this one. In the spirit of rapid feedback, I left a couple comments.

…'t end with slashes

…s assumed

rkoeninger · 2018-07-13T03:23:48Z

Re: 0c6879a
Did this test make sense originally? It was making a directory with the name of the destination we want to rename/move a file to.

MockFileSystem.FileExists trims trailing slashes before checking

…ith the same name

…stem.IO.Abstractions into remove-trailing-slashes

rkoeninger · 2018-07-13T04:25:19Z

Got the tests passing on Windows, but plenty of failures on Linux.

How does one normally work on Unix-specific failures? Startup a VM?

I'll try that tomorrow.

jpreese · 2018-07-13T14:09:11Z

Re: 0c6879a
Did this test make sense originally? It was making a directory with the name of the destination we want to rename/move a file to.

I think the test is OK. Given what it's doing, and the name of the test, it's verifying that MoveTo does a replace, which is a valuable test.

I do not however see a test that just simply test moving a file to another directory where the file does not already exist.

How does one normally work on Unix-specific failures? Startup a VM?

I have always used a docker container to text Linux tests locally. You can mount the System.IO.Abstractions source folder and run dotnet test.

Added UnixOnlyAttribute, UnixSpecifics to mirror WindowsOnlyAttribute/Specifics

rkoeninger · 2018-07-13T17:10:15Z

@jpreese About that test (0c6879a): I wasn't questioning the existence of the test, but the way it was written. It started to fail after I made my changes, I believe because it was depending on the ability to have a file and directory with the same name exist at the same time. That commit contains the one-line fix that fixes what may have just been a typo.

I get worried when I have to change tests to make them pass, and they're not changes I expected to have to make.

jpreese · 2018-07-13T21:10:00Z

For that test, yeah I was agreeing with the name and how it was written.

Add a file at c:\temp\file.txt
Add a directory at c:\temp2\file.txt
Move c:\temp\file.txt to c:\temp2\file.txt
Validate the move overwrote c:\temp2\file.txt

So if I'm understanding this right, the fault here is that we're doing .AddDirectory and not adding a File. The test is validating that we can replace a file, and the way its written now (in the PR), does not do that since destinationFolder is empty.

The test you've written validates that we can Move a file (which I think we need, because for some reason we don't have it).

There should be a test for move to an empty directory, and a test for a move to a source that already exists.

…xisting target dir

rkoeninger · 2018-07-14T15:40:17Z

@jpreese Ok, so the original test was split into 3 tests, and added Copy/Move tests for MockFile.

rkoeninger · 2018-07-15T01:05:49Z

I think this is pretty much ready. Does anyone have any other comments/concerns?

…stem.IO.Abstractions into remove-trailing-slashes

fgreinacher

Some minor nits – otherwise LGTM

fgreinacher · 2018-07-15T18:58:25Z

TestHelpers.Tests/MockFileSystemTests.cs

+            var fileSystem = new MockFileSystem();
+            var path2 = XFS.Path(path);
+            fileSystem.AddDirectory(path2);
+            fileSystem.FileExists(path2);


Assert on return value here?

Whoops, assert added

fgreinacher · 2018-07-15T19:00:55Z

TestHelpers.Tests/StringExtensionsTests.cs

+    using XFS = MockUnixSupport;
+
    [TestFixture]
    public class StringExtensions


Add Tests suffix?

fgreinacher · 2018-07-15T19:04:46Z

TestingHelpers/StringExtensions.cs

+
+            if (MockUnixSupport.IsUnixPlatform()
+                && (path[0] == Path.DirectorySeparatorChar || path[0] == Path.AltDirectorySeparatorChar)
+                && trimmed == "")


string.Empty?

Eh, I prefer "" to string.Empty. 2 chars vs 12.

fgreinacher · 2018-07-16T15:43:14Z

@jpreese for a last pair of eyes 👀

Used WindowsOnly on existing tests

rkoeninger · 2018-07-16T17:05:51Z

I had copy/paste/modified the MockFileInfo tests for Copy/Move over to MockFile because I thought they were missing, but just noticed while looking at the next issue that they already existed in MockFileCopyTests.cs and MockFileMoveTests.cs.

Minor improvement would be to name the files like MockFile.Copy.cs so they get sorted in proper alpha order. Or in nested classes, not sure what the best approach would be.

jpreese · 2018-07-16T21:48:37Z

TestHelpers.Tests/MockDirectoryTests.cs

            fileSystem.AddFile(additionalFilePath, new MockFileData(string.Empty));
            fileSystem.AddFile(XFS.Path(@"c:\a\a\c.gifx.xyz"), new MockFileData(string.Empty));
-            fileSystem.AddFile(XFS.Path(@"c:\a\a\c.gifx\xyz"), new MockFileData(string.Empty));
+            fileSystem.AddFile(XFS.Path(@"c:\a\a\c.gifz\xyz"), new MockFileData(string.Empty));


Why did we change the test data for this?

That test depended on the ability to have a file and directory with the same name, perhaps unintentionally (?) But that's no longer allowed, per this PR. Specific commit: 9fad978

jpreese · 2018-07-16T21:52:34Z

TestHelpers.Tests/MockFileCopyTests.cs

        }

        [Test]
+        [WindowsOnly(WindowsSpecifics.StrictPathRules + "; Path.GetInvalidChars() does not return anything on Mono")]


I think the StrictPathRules const is good enough here (and the others)

Ok, I'll cut of the rest.

jpreese · 2018-07-16T21:57:49Z

Well, I started a review but then realized this PR is pretty large.. and I'm a little uneasy with how many tests were changing. It's going to take me a bit to look this over.

…stem.IO.Abstractions into remove-trailing-slashes

rkoeninger · 2018-08-27T00:23:19Z

@jpreese @fgreinacher I understand if the size of this PR is discomforting, but it's been 6 weeks and I've had to back merge 6 times. If we're not going to merge it soon, it will likely never get merged and we might as well close it. Same for #327.

I don't want to throw away my work, but there's a point where I'd rather just not be hanging.

fgreinacher

I don’t like that we have to adapt so many tests, but all of the adaptations seem to be OK to me.

fgreinacher · 2018-08-27T08:24:13Z

Sorry @rkoeninger and thanks for the trigger :) @jpreese I’m fine merging this as it simplifies and fixes several things.

…stem.IO.Abstractions into remove-trailing-slashes

fgreinacher · 2018-09-05T15:18:37Z

@jpreese ?

rkoeninger · 2018-09-19T19:30:51Z

@fgreinacher @jpreese 🚢 ?

fgreinacher · 2018-09-19T22:08:48Z

OK, I’m gonna merge this now - we have no negative feedback except the amount of tests this impacts - but as I mentioned above all the changes look unproblematic to me.

Thanks again for working on this @rkoeninger!

rkoeninger · 2018-09-20T00:13:40Z

@fgreinacher No prob, thanks for getting this merged

rkoeninger added 2 commits July 11, 2018 21:44

Replace both EnsurePathEnds... with .CleanPath extension

6fbaf50

Used GetFile, SetEntry in MockFileSystem

2376e3f

rkoeninger changed the title ~~Don't allow files and directories with the same name~~ [WIP] Don't allow files and directories with the same name Jul 12, 2018

Simplified CleanPath to remove trailing slashes, except on drive root

6316112

jpreese reviewed Jul 12, 2018

View reviewed changes

rkoeninger added 5 commits July 12, 2018 20:37

Fixed GetPathWithCorrectDirectoryCapitalization to consider paths don…

0cf5fbf

…'t end with slashes

Fixed issue in MockDirectory.GetFilesInternal where trailing slash wa…

d7d522c

…s assumed

Fixed tests by removing expectation of trailing slashes

8de84a4

Used MockUnixSupport in StringExtensionsTests

9a59f87

Fixed (?) file move test (did this test make sense before?)

0c6879a

rkoeninger added 7 commits July 12, 2018 23:32

Fixed some tests expecting trailing slashes

054862a

MockFile.Exists no longer depends on matching path in AllDirectories

392634c

MockFileSystem.FileExists trims trailing slashes before checking

Fixed more tests that expected trailing slashes

3b27061

Fixed test that depended on being able to have a directory and file w…

9fad978

…ith the same name

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

f0ba080

…stem.IO.Abstractions into remove-trailing-slashes

Removed trailing slash from test

1cde294

Made use of WindowsOnly in TrimSlashes_DriveRoot tests

d86720d

rkoeninger added 3 commits July 13, 2018 12:32

Added test from #223 to confirm that issue is fixed

6511375

Added a couple of tests to confirm #208 is fixed

aaae5d2

Made TrimSlashes() handle Unix slash root better

38b55ca

Added UnixOnlyAttribute, UnixSpecifics to mirror WindowsOnlyAttribute/Specifics

Removed some Windows-specific code introduced into GetFilesInternal

3f5fee0

rkoeninger added 2 commits July 14, 2018 10:48

Replaced MoveTo test with 1 with pre-existing target and 1 with pre-e…

71be7f1

…xisting target dir

Added Copy/Move tests for MockFile

f7e3929

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

73d409b

…stem.IO.Abstractions into remove-trailing-slashes

rkoeninger changed the title ~~[WIP] Don't allow files and directories with the same name~~ Don't allow files and directories with the same name Jul 15, 2018

fgreinacher approved these changes Jul 15, 2018

View reviewed changes

Added Assert, added Tests suffix to test class

d144e8c

Removed redundant tests

b4cd78a

Used WindowsOnly on existing tests

jpreese reviewed Jul 16, 2018

View reviewed changes

rkoeninger added 9 commits July 16, 2018 18:29

Removed expanded WindowsOnly reason message from 6 File Copy/Move tests

a93ceb9

Merge branch 'master' into remove-trailing-slashes

3acd9b8

Merge branch 'master' into remove-trailing-slashes

a916142

Merge branch 'master' into remove-trailing-slashes

fe40ac7

Merge branch 'master' into remove-trailing-slashes

83e7131

Merge branch 'master' into remove-trailing-slashes

7631b2d

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

fdaae51

…stem.IO.Abstractions into remove-trailing-slashes

Moved new files to changed project path

f1be1a0

Merge branch 'master' into remove-trailing-slashes

27ac2c5

fgreinacher approved these changes Aug 27, 2018

View reviewed changes

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

6a6c7b1

…stem.IO.Abstractions into remove-trailing-slashes

Merge branch 'master' into remove-trailing-slashes

1f94f40

fgreinacher merged commit 55a82b0 into TestableIO:master Sep 19, 2018

rkoeninger deleted the remove-trailing-slashes branch December 1, 2018 21:05

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

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

Uh oh!

Conversation

rkoeninger commented Jul 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rkoeninger Jul 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rkoeninger Jul 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jpreese commented Jul 12, 2018

Uh oh!

rkoeninger commented Jul 13, 2018

Uh oh!

rkoeninger commented Jul 13, 2018

Uh oh!

jpreese commented Jul 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rkoeninger commented Jul 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jpreese commented Jul 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rkoeninger commented Jul 14, 2018

Uh oh!

rkoeninger commented Jul 15, 2018

Uh oh!

fgreinacher left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fgreinacher commented Jul 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rkoeninger commented Jul 16, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rkoeninger Jul 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jpreese commented Jul 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rkoeninger commented Aug 27, 2018

Uh oh!

fgreinacher left a comment

Choose a reason for hiding this comment

Uh oh!

fgreinacher commented Aug 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rkoeninger commented Jul 12, 2018 •

edited

Loading

rkoeninger Jul 13, 2018 •

edited

Loading

rkoeninger Jul 13, 2018 •

edited

Loading

jpreese commented Jul 13, 2018 •

edited

Loading

rkoeninger commented Jul 13, 2018 •

edited

Loading

jpreese commented Jul 13, 2018 •

edited

Loading

fgreinacher left a comment •

edited

Loading

fgreinacher commented Jul 16, 2018 •

edited

Loading

rkoeninger Jul 16, 2018 •

edited

Loading

jpreese commented Jul 16, 2018 •

edited

Loading

fgreinacher commented Aug 27, 2018 •

edited

Loading