[danbystrom]

…he file content.

Rationale: when mocking a file system, it is commonly populated with a large number of files, where only a few will be relevant for each test suite. By allowing the files content to be lazy loaded, a huge amount of memory and disk reads can be saved.

[manne]

why should you populate the mock file system with files you do not need in a test?

[danbystrom]

If you don't see why - you probably have no need for it, but I do. :-)

I want to have a common ground for populating my mock file system with a number of large bitmap files. Then I want to call this set-up from a number of TestFixtures. Not all of them will need all the files or the content of the files, but most importantly - many of the tests are interested in the availability of the files, but not necessarily then content of the files.

So by adding a lazy loaded file - the code will see that the file exist (which is important) but may choose not to read it's data. And then I have a huge memory and performance save.

Another benefit is that you can check if the file was actually used (if the Func was called).

[danbystrom]

I've added a test class to ensure that the content of a lazy loaded file is not touched by directory browsing or File.Exists. This makes it possible to create mocked files that are being seen by the code to be tested, without having to populate them with real data until needed. (The new commit only ensures this behavior - the feature was already there.)

[MCCoolMoDAue]

I think it would be nice to have the filename in the lazy load function so that we can have the data that's loaded managed in a single point.
That way we could manage all the data (or the data loading) in a single place.
I'm picturing a dictionary like

IDictionary<string, Func<byte[]>>

and in the lazy load Func or function or lambda we can call it on demand:

byte[] LazyLoadFileDate(string filename) { return fileDataDict[filename](); }

I hope you can understand what I mean..

Also, maybe a MockData subclass, or an abstract base class would be helpful, considering that we could get rid of the condition
get { return contents ?? (contents = lazyLoadContent()); }
in here.
Like a LazyMockFileData.
That way you don't break any old behavior.

[fgreinacher]

I love the idea, mostly because I think it enables users to easily simulate special behaviors while reading file contents (as requested in e.g. #226 or #288).

[fgreinacher]

@danbystrom I know this PR is pretty old, but we are currently in the process of bringing this repo back to life. Are you still willing to contribute here? If not, someone else could take this over.

[danbystrom]

Sure, I'm using my fork all the time, so I know it is useful. It would be nice to be able to finally merge it. Just tell me what I can do to help. D 2018-07-11 11:48 GMT+02:00 Florian Greinacher <notifications@github.com>:

…

@danbystrom <https://github.com/danbystrom> I know this PR is pretty old, but we are currently in the process of bringing this library forward. Are you still willing to contribute here? If not, someone else could take this over. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#144 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABEGNXIVLGZ3RReGOk2th-pJtXgL3wVHks5uFcoBgaJpZM4GhE9y> .

[fgreinacher]

First step would be to rebase this on current master - this will then also trigger the new builds. I'd like to have this PR merged before we have to celebrate it's third birthday 🤣 🎉

[fgreinacher]

Can we discuss this separately? I like the lazy loading feature but I'm not sure this new constructor has to be here (instead of in user code).

[danbystrom]

Well, it's closing in on three years since I wrote this, and I realize I have forgotten all about it. :-( 1) What do you propose we should do instead? 2) would it be easier to entirely drop this PR and that I create a new one from the current master, with your suggested changes?

[fgreinacher]

What about this.lazyLoadContext = lazyLoadContent ?? throw new ArgumentNullException(nameof(lazyLoadContent))?

[fgreinacher]

Nit: What about calling this createContents?

[fgreinacher]

See comment below, let's move this to a separate PR

[fgreinacher]

IMO asserting on fileLength does not provide any additional value.

[fgreinacher]

Why use an exception here instead of doing what you do in MockFileDataLazyLoadContentTests. MockFileData_LazyLoadFuncIsCalledOnDemand?

[fgreinacher]

I think the main purpose of this test of to verify that the callback does not get invoked when checking whether the file exists. This assertion should be visible in the test body.

[fgreinacher]

Same as above, I'd like to see the assertion here

[fgreinacher]

Same as above, I'd like to see the assertion here

[fgreinacher]

It doesn't really matter, but what is the value of this ceremony when all you need is some byte array? Just do something like var contents = new byte[] { 1, 2, 3 }.

[fgreinacher]

I'd suggest an additional test case that verifies that the callback is not called multiple times.

[ericnewton76]

I dont agree with this approach. You're supposed to be creating tests that target what you're testing, if you're using some generified common "BuildMyFileSystem" function with the sum total files for all your tests, then you're integration testing, and then why bother with lazy loading things?

This is introducing bit of complexity that'll actually erode confidence in this library when the first bugs start showing up because it was lazy loading files specifically added to the MockFileSystem.

I believe the OP is needing a different solution but can't be sure because he hasn't outlined the problems he's seeing and how he's using the library. We have a proposed PR to make a change without explaining the value and necessity of making this change to the very basic assumptions of the MockFileSystem.

MockFileSystem was not built to be an in-memory file system. There's NTFS on RAM drives for that.

MockFileSystem was built to make file system access tests isolatable and more easily repeatable. I just don't think having MockFileSystem lazy load a whole directory of 1000 entries of massive bitmaps is not its design goal.

And like @manne mentioned, why are you building a huge dataset everytime when the tests aren't testing the whole dataset? Seems like a problem domain isolation problem in the testing methodology.

This PR sat for a long time without a lot of "Gee whiz, would be nice to get this" because most looked at it and probably thought the same as what I'm saying.

[jpreese]

FWIW I agree that this is a little strange. I'm heavily in the camp of your test suites should be such that the minimum amount of data is required to get your test to pass or fail.

Theres no value in having files added to the fake file system that you never interact with.

[jpreese]

What are we thinking on this one @fgreinacher @robkeim ?

[fgreinacher]

For me this feature is symmetrical with the existing MockFileData functionality.

Following your argumentation one could also question whether MockFileData and friends make sense at all. Their main purpose is to get the file system in a specific state/behaivor. You could do the same with the regular file system API, but they are there to make the setup as dense as possible.

So IMO we should accept this one.

[jpreese]

I'll look into MockFileData a bit more. My biggest concern is the current defense being needing to include files that aren't used in tests.

@danbystrom could you provide a concrete example of using this feature? I think this would help with reasoning about this PR.

[robkeim]

I agree with @jpreese that if we could have a concrete example of what's being tested that would help clarify the motivation for being able to lazy load files.

Understanding the why we're doing this will help us review/discuss the what we're doing which we can see in the current implementation.

[danbystrom]

Well, this feels odd. Three years ago I had a problem, I solved it and have been happy with the solution ever since. Now, however, I’m not very happy with arguing whether another solution would have been better. I have no idea what “a problem domain isolation problem in the testing methodology” means. I could probably read that sentence till my eyes started to bleed without being none the wiser. 😊
A couple of typical uses for me:

1. Less important: Static files. There are a number of files that are just supposed to be available at all times. Explicitly including them in each test is just a waste of coding time and loading them into memory in each test is just a waste of memory and processing time. Solution: just lazy load the static file system.
2. More important: code that takes decisions based on the existence of files. Typical scenario: I have four different versions of a portrait:
   a. Original hires jpg with a green background
   b. Hires png with transparent pixels instead of green background
   c. Lores jpg with green background replaced with a gray fund
   d. Lores png
   At various times all files may not yet be available (particularly case b), and at different times it is OK to fallback to a different one and at different times it is not acceptable. A test that wishes to ensure that a particular file version is being used must therefore also populate the file system with other files that is NOT INTENDED TO BE USED, and afterwards inspect a produced result to see on a pixel level that the correct file was being used. Here is a situation when it is wasteful to populate the mock file system with actual file DATA until the code asks for it.

[ericnewton76]

You seem like you're doing a lot of integration tests, and not defined functional tests. Anytime somebody says "not at this time" or similar, then you're basically implying a race condition exists.

Having the simplistic mockfilesystem start lazy loading gets us into all kinds of race conditions that I simply dont see the benefit for. You're better off setting up a RAM drive and running your tests using a real filesystem. Again, for complete isolation, you will definitely need to create isolated instances and therefore I guarantee you'll be copying files around to create a valid virtual reality that you're trying to test against.

All of that is outside of the scope of the (supposed to be simple) MockFileSystem testing helpers. You're probably better off, for those particular tests, in keeping with the current implementation and not lazyloading file data.

In practice, does it matter if the bitmap is 12MB or 1KB? I would argue it doesn't matter. You don't need to run your tests against full 4k HD bitmaps to verify the results. If you do, then again, you're outside the scope of this library.

What happens when the file is deleted by one of your other tests, not isolated properly, and then you try to lazy load the file data? Boom, a race condition that's difficult to detect... when the current code would say "file doesnt exist". That's just one of numerous race conditions lurking with lazy loading the stream.

Even if you were trying to build a simple database storage engine... I would still say you're outside the scope of these MockFileSystem implementations, since there's so many things that aren't being surfaced when running against a real file system like NTFS or FAT like proper locking, file region locking, sparse handling, etc... the list becomes endless.

The whole point of isolation is setting up the bare minimum to satisfy the test conditions. When you find a difficult bug, then you setup the relevant virtual world precisely to isolate the cause. Ironically, most of the time the behavior will vary between a real file system and the MockFileSystem, and that's expected outside of simple use-case scenarios.

I sincerely do not recommend merging this release in. It'll open the door to countless race conditions that will create headaches for all without many benefits.

[jpreese]

Closing.