System.IO.Abstractions icon indicating copy to clipboard operation
System.IO.Abstractions copied to clipboard

fix: Make mocked data lazy-loaded

Open hangy opened this issue 2 years ago • 4 comments

This moves the initialization of cachedMockFileData to the first time that it is actually used. Apparently, this is more closely to how it works in .NET 6.

Closes #899

hangy avatar Oct 26 '22 08:10 hangy

@fgreinacher this partly implicitly reverts a change that you made here. In #899, we noticed that the behaviour of FileInfo in .NET 6 is a bit different to the current behaviour of the MockFileSystem. This aligns the mock's behaviour a bit better to CoreCLR, but is probably not perfect, either.

hangy avatar Oct 26 '22 08:10 hangy

Thinking aloud, now that I have a little more time for this PR:

With this change, we would actually regress in regards to the DirectoryInfo.GetFiles, DirectoryInfo.EnumerateFiles, ... (see here for details) methods initialization behavior, as those methods no longer return an initalized MockFileInfo. Remember, before it was automatically initialized by the Refresh() call in MockFileInfo ctor!

Can't we fix this regression by simply calling Refresh in the corresponding MockDirectoryInfo methods, e.g. here https://github.com/TestableIO/System.IO.Abstractions/blob/main/src/System.IO.Abstractions.TestingHelpers/MockDirectoryInfo.cs#L311-L368 when we are constructing the MockFileInfo object?

We don't have any tests for the "initalize FileInfo when going through these methods", that is why this change in behavior is not caught by the tests, but propably need them. Willing to go the extra mile?

siprbaum avatar Oct 27 '22 17:10 siprbaum

With this change, we would actually regress in regards to the DirectoryInfo.GetFiles, DirectoryInfo.EnumerateFiles, ... (see here for details) methods initialization behavior, as those methods no longer return an initalized MockFileInfo. Remember, before it was automatically initialized by the Refresh() call in MockFileInfo ctor!

If we're speaking .NET 5's System.IO.DirectoryInfo, I don't think that the FileInfo returned by GetFiles etc. are actually initialized by the framework.

  1. GetFiles calls InternalEnumerateInfos
  2. InternalEnumerateInfos returns a new FileSystemEnumerable<FileInfo> (via FileSystemEnumerableFactory)
  3. FileSystemEnumerable, through a delegate and via FileSystemEntry returns a FileSystemInfo from the Create method
  4. Since that uses the normal FileInfo constructor internally, it's not initialized

Sooo … the changed behaviour caused by this PR would be a regression to the current System.IO.Abstractions behaviour, but closer to the current .NET behaviour?

We don't have any tests for the "initalize FileInfo when going through these methods", that is why this change in behavior is not caught by the tests, but propably need them. Willing to go the extra mile?

Therefore, we possibly don't need this?

hangy avatar Oct 27 '22 20:10 hangy

With this change, we would actually regress in regards to the DirectoryInfo.GetFiles, DirectoryInfo.EnumerateFiles, ... (see here for details) methods initialization behavior, as those methods no longer return an initalized MockFileInfo. Remember, before it was automatically initialized by the Refresh() call in MockFileInfo ctor!

If we're speaking .NET 5's System.IO.DirectoryInfo, I don't think that the FileInfo returned by GetFiles etc. are actually initialized by the framework.

  1. GetFiles calls InternalEnumerateInfos
  2. InternalEnumerateInfos returns a new FileSystemEnumerable<FileInfo> (via FileSystemEnumerableFactory)
  3. FileSystemEnumerable, through a delegate and via FileSystemEntry returns a FileSystemInfo from the Create method

You need to follow the reading of this method, a few lines below, the FileSystemInfo.Init method is called, see here https://github.com/dotnet/runtime/blob/13f648fa15c2989455415f4941dad2abfbaa6bba/src/libraries/System.Private.CoreLib/src/System/IO/FileSystemInfo.Windows.cs#L34

To my understanding, this does initialize the cache, following the calls into FileSystemInfo.Init where it sets the _dataInitialized=0

siprbaum avatar Oct 28 '22 07:10 siprbaum