fix: correct MockFileInfo and MockDirectoryInfo for non existing files/directories #834
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
Author
|
This is best reviewed commit by commit (and also reading the commit messages which contain quite a lot of information). |
siprbaum
changed the title
Contributor
|
@siprbaum I agree that we should make |
The `MockFileData.NullObject` was introduced in commit 0d85da3 as a way to represent the data of a non-existing file. Nevertheless, the issue TestableIO#830 shows that at least one person used it as "some arbitrary MockFileData of an existing file I don't care about". BREAKING CHANGE: Make `MockFiledata.NullObject` an internal implementation detail. Reason: As we are going to really make this the data representing a non-existing file by fixing/changing the `Attribute` property in one of the next commits, make this property internal and thereby breaking to force all consumers to really think about their tests and if they really ment a non-existing file or simply some arbirary data. Otherwise, the upcoming change would have nasty side effects if the NullObject is used for arbitrary data of an existing file. An alternative considered would be to rename this static property (which is also breaking) to any users, but decided against it as I couldn't think of a usecase where explicitly creating a file or directory which doesn't exist instead of relying on what the library will for you when being asked for a non-existing file. E.g. it is the difference of var fs = new MockFileSystem(); var nonExistingPath = XFS.Path(@"c:\some\non\existing\file.txt"); fs.AddFile(nonExistingPath, MockFileData.NullObject); var fi = fs.FileInfo.FromFilename(nonExistingPath); vs simply relying on the library to return the correct data for the non-existing file: var fs = new MockFileSystem(); var nonExistingPath = XFS.Path(@"c:\some\non\existing\file.txt"); var fi = fs.FileInfo.FromFilename(nonExistingPath); All tests are adapted to no longer use the MockFileData.NullObject.
According to the Microsoft documentation[1] the `Attributes` property of
FileSystemInfo (base of FileInfo and DirectoryInfo) returns
(FileAttributes)(-1) for a file/directory which doesn't exist when the
object instance is created. The on disk content is not looked at when
calling the `Attributes` property, only a pre-cached value is returned.
Only calling `Refresh` will read from disk.
To be on the safe side, I checked the documented behaviour with the real
implemenation with the below code snipped and it indeed shows the
documented behaviour.
var fs = new FileSystem();
var expected = (FileAttributes)(-1);
var fi = fs.FileInfo.FromFileName(@"x:\not-existing\path");
Assert.That(fi.Attributes, Is.EqualTo(expected));
This lead to quite some needed code adaptions, especially in the area of
the `GetMockFileDataForRead` methods, as those now no longer throw an
exception. Furthermore, as cachedMockFileData is always a non-null instance,
get rid of the logic checking for null in `GetMockFileDataForRead` and
`MockFileInfo.CopyTo(string destFileName)` method.
Also ensure that the used MockFileData is always `Clone`ed (and therefore
even the MockFileData.NullObject) so that we operate on our own copy
and don't get any updates done in the FileSystem.
With this new semantics the `MockDiretory.Exists` method requires special
treatment, as we now must check for a valid MockFileData by checking
if Attributes != -1.
[1]: https://docs.microsoft.com/en-us/dotnet/api/system.io.filesysteminfo.attributes?view=net-6.0#remarks
Ensure that when setting the Attributes property on a non existing file the appropriate FileNotFoundException is thrown, as documented in [1]. [1]: https://docs.microsoft.com/en-us/dotnet/api/system.io.filesysteminfo.attributes?view=net-6.0#remarks
Add tests that when trying to retrieve the time properties of a non existing file returns 12:00 midnight, January 1, 1601 A.D. (C.E.) Coordinated Universal Time (UTC) as documented in [1] [1]: https://docs.microsoft.com/en-us/dotnet/api/system.io.filesysteminfo.lastaccesstime?view=net-6.0#remarks
Add tests that when trying to retrieve the time properties of a non existing file returns 12:00 midnight, January 1, 1601 A.D. (C.E.) Coordinated Universal Time (UTC) as documented in [1] [1]: https://docs.microsoft.com/en-us/dotnet/api/system.io.filesysteminfo.lastaccesstime?view=net-6.0#remarks
When setting one of the Time properties or the `Attributes` property on a non-existing directory, a DirectoryNotFoundException should be thrown. Fix the code by throwing the correct exception (it did throw a FileNotFoundExecption before) and add the missing tests.
MockFileData.NullObject was made internal, which is a breaking change, therefore increase the major version.
Contributor
Author
|
Rebased and added a commit on top which inreases the major version number. It is not clear if increasing the version per PR is fine or if we want to do it per commit, as I believe keeping the history |
Contributor
Thanks a lot, we always squash here, so the extra commit for the version is fine. I will keep the commit messages and we have the link to this PR in case somebody wants to see details! |
fgreinacher
approved these changes
Apr 25, 2022
Closed
1 task
This was referenced Apr 28, 2022
This was referenced May 4, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
fixes #829
closes #830