Enable MockDirectory.Move to also move files #438
Conversation
|
👀 anyone? |
| var fileSystem = new MockFileSystem(new Dictionary<string, MockFileData> | ||
| { | ||
| { sourceFilePath, new MockFileData(sourceFileContent) }, | ||
| { XFS.Path(@"c:\somethingelse\dummy.txt"), MockFileData.NullObject } |
There was a problem hiding this comment.
I think this test could be simplified a bit. The focus here is files, so we could probably just do something like C:\demo.txt, and drop the directory. There also more than likely isn't a need to include two files (I'm seeing dummy.txt and demo.txt). We just need to make sure the file gets from Point A to Point B. A rename of the file could achieve the same effect as moving directories.
There was a problem hiding this comment.
Yeah, the second file was there to ensure the destination directory exists. I agree there was too much going on, so I'll simplify this as suggested. I won't rename the file because the focus of this test is the special behavior of MockDirectory.Move.
|
|
||
| Assert.That(fileSystem.FileExists(destFilePath), Is.True); | ||
| Assert.That(fileSystem.GetFile(destFilePath).TextContents, Is.EqualTo(sourceFileContent)); | ||
| Assert.That(fileSystem.FileExists(sourceFilePath), Is.False); |
There was a problem hiding this comment.
I'm usually against multiple asserts, but since this is more about state after a move, it seems OK. I had a slight issue with this because it was initially kinda confusing. Maybe we move the FileExists check to the top so we're validating state together? i.e. FileExists destFile (true) and FileExists sourceFile (false) need to be together to validate the file moved on the file system. The GetFile makes sure the source and dest were the same.
There was a problem hiding this comment.
Good catch, will reorder!
|
|
||
| //if we're moving a file, not a directory, call the appropriate file moving function. | ||
| var fileData = mockFileDataAccessor.GetFile(fullSourcePath); | ||
| if (fileData != null && (fileData.Attributes & FileAttributes.Directory) == 0) |
There was a problem hiding this comment.
I think a safe navigation operator (?.) might simplify this conditional a little?
There was a problem hiding this comment.
Sure, changed this line to use safe navigation operator and Enum.HasFlag. Reads a lot easier now.
|
Overall I think it's fine, just a couple ease of understanding changes. |
|
Feedback addressed with e74649d |
2 participants
Fixes #187
Supersedes #188