Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Make mocked data lazy-loaded #900

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from
Draft

Conversation

hangy
Copy link
Contributor

@hangy hangy commented Oct 26, 2022

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
Copy link
Contributor Author

hangy commented Oct 26, 2022

@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 hangy changed the title chg: Make mocked data lazy-loaded fix: Make mocked data lazy-loaded Oct 26, 2022
Copy link
Contributor

@siprbaum siprbaum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are several aspects not checked or which needs to be changed, see the inline comments

hangy and others added 3 commits October 26, 2022 18:10
Test is renamed from MockFileInfo_Exists_ShouldReturnCachedData to
MockFileInfo_Exists_LazyLoadsData, and now checks that the lazy-loading
in MockFileInfo works correctly.

Co-authored-by: Peter Baumann <peter.baumann@gmail.com>
`Refresh()` needs to be called in the Arrange phase, so that the data is not simply
lazy-loaded in the assertion case. Explicitly loading MockFileData by calling `Refres()`
enforces lazy-loading to be done.

Co-authored-by: Peter Baumann <peter.baumann@gmail.com>
Copy link
Contributor

@siprbaum siprbaum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only some minor changes, leaving some breadcrumbs for the next reader of the test methods.

Co-authored-by: Peter Baumann <peter.baumann@gmail.com>
@siprbaum
Copy link
Contributor

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?

@hangy
Copy link
Contributor Author

hangy commented Oct 27, 2022

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?

@siprbaum
Copy link
Contributor

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

@fgreinacher fgreinacher marked this pull request as draft February 14, 2023 13:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MockFileSystem.AddFile() does not update MockFileInfo.Exists
2 participants