feat: Support event callbacks and custom exceptions in MockFileSystem

[vbreuss]

Observing Changes Implements #805 Allows registering a callback to be executed whenever a file or directory event is triggered in the file system:

    var fs = new MockFileSystem()
        .OnFileEvent(f => { /* Do something */ })
        .OnDirectoryEvent(d => { /* Do something */ });

The callback parameter provides the path of the file or directory and the event type.

Throw custom exceptions Implements #877 Additionally the callback allows throwing a custom exception. This callback is executed before the actual change in the MockFileSystem, thus allowing simulating e.g. OutOfMemoryException or other edge cases...

[vbreuss]

This draft is to gather feedback about the suggested approach in general. Currently no event is triggered, when the file is only accessed. This could be quite easily added after #875 by executing the callback when the AccessTime is changed...

[fgreinacher]

I like the general direction here 👍

Any reason to not expose this as regular events, as e.g. suggested here?

This callback is executed before the actual change

Maybe we could use the *ing/*ed scheme e.g. (FileCreating/FileCreated) for naming the events to support callbacks both before and after the actual change is made?

[vbreuss]

@fgreinacher

Any reason to not expose this as regular events, as e.g. suggested https://github.com/TestableIO/System.IO.Abstractions/issues/168#issuecomment-263130564?

It probably boils down to personal preference :-) I think callbacks are easier and for unit testing I don't see the requirement to support registering multiple event handlers. Also I think it is clearer for callbacks to allow throwing an exception. With events I wouldn't be sure how exceptions are treated, as I could have registered multiple event handlers and only some of them throw exceptions.

Maybe we could use the *ing/*ed scheme e.g. (FileCreating/FileCreated) for naming the events to support callbacks both before and after the actual change is made?

I think this is a good idea, but I am not sure about the naming. If I split it into too many different events ("FileCreating", "FileUpdating", "FileDeleting", "FilePermissionChanging", ...) and its counterparts it gets hard to register to all relevant events. That's why I only suggested OnFileEvent and OnDirectoryEvent and then I can filter on the events I want to handle.

What are your thoughts on this? Do you think the use cases are more specific and I should add multiple different events? Or what about OnFileEventOccuring and OnFileEventOccured as the "general" name?

[fgreinacher]

Ah sorry @vbreuss, I got derailed by other topics 🙇

The main problem I see with the callback approach is that it's not easily possible to unregister the handlers.

I think we should for something like this:

var fs = new MockFileSystem();
fs.Events.DirectoryCreating += (sender, args) => {}
fs.Events.FileCreated += (sender, args) => {}
...

Yes, it's a bit verbose if you want to register the same handler to different events. But on the other hand it's quite intuitive and future-proof (no breaking change when adding new events).

WDYT?

[vbreuss]

@fgreinacher : I think in the end it boils down to preference. Especially in unit tests I like the fluent syntax (I also use FluentAssertions in my xUnit tests). Regarding the unregistering, you could return an IDisposable which would be quite easy to handle.

Unfortunately, I won't be able to continue working on this pull request! In my application, where I used this library I had quite some inconsistencies, that were not detected by the unit test suite due to incorrect handling of times on different operating systems. Even after my previous contribution here, I stumbled on differences between Linux, Mac and Windows. That was the reason, why I started my own implementation of a broader approach to testing abstractions including a FileSystemMock where I build an extensive test suite that also runs on a real file system to ensure, that the behaviour matches the real file system.

There I already implemented notification handling as follows:

var fileSystem = new FileSystemMock();
fileSystem.Intercept.Creating(FileSystemTypes.Directory, _ => throw new Exception("custom exception"));
fileSystem.Notify.OnCreated(FileSystemTypes.Directory, c => DirectoryWasCreated(c.Path));

Maybe a similar approach would also work for you?

[fgreinacher]

@vbreuss All good, fully understood! I'm maintaining this at super low capacity and want to keep changes as non-invasive as possible. I personally also like the fluent syntax a lot, but none of the existing APIs use it, and if in doubt I favor consistency over anything else.

I like your library a lot! I'm wondering whether it would make sense if you implement the System.IO.Abstractions interfaces instead of duplicating them. In that case I'd be happy to link your lib from the README here, as an alternative, way of mocking things. Let me know what you think 🙇

[vbreuss]

@fgreinacher : In the beginning I started doing exactly that, however the interfaces are not completely identical:

• I separated the interfaces from the default implementation with wrappers in separate projects. In this project they are both in the main DLL, so I can't just use the interfaces, but would always get the implementations as well.
• You included the ACL-related functionality directly in the interfaces. I extracted them to a separate DLL (Testably.Abstractions.AccessControl), so if they are not required, I don't need a reference to System.IO.FileSystem.AccessControl
• I replaced the Stream in the IFileStreamFactory with a custom stream class FileSystemStream (similar to #793)
• I added and used a ITimeSystem interface to simulate the system time, which I need for determining the correct time. This interface is not available in "System.IO.Abstraction" (and wouldn't really fit, as it is not IO-related)
• You didn't replace the WaitForChangedResult as return value in the IFileSystemWatcher interface. As this struct only has internal constructors, it is not possible to mock. Changing these now, would unfortunately be a breaking change...
• I would also be sceptical of using two completely distinct namespaces ("System.IO.Abstractions" and "Testably.Abstractions.Testing") and adding more projects under the "System.IO.*" namespace would not be possible, if I understand this comment correctly...

All in all I really like the idea, but my goal with Testably.Abstractions is for a broader approach to unit testing helpers and I don't see, how we could make these two libraries 100% compatible :-(

Do you have an idea how to overcome these challenges?

[fgreinacher]

@fgreinacher : In the beginning I started doing exactly that, however the interfaces are not completely identical:

• I separated the interfaces from the default implementation with wrappers in separate projects. In this project they are both in the main DLL, so I can't just use the interfaces, but would always get the implementations as well.

I would have no problem changing that so that the interface are separate.

• You included the ACL-related functionality directly in the interfaces. I extracted them to a separate DLL (Testably.Abstractions.AccessControl), so if they are not required, I don't need a reference to System.IO.FileSystem.AccessControl

I tried to mirror what .NET does and I think on modern versions we don’t actually need that reference. Or at least it is a no-op.

• I replaced the Stream in the IFileStreamFactory with a custom stream class FileSystemStream (similar to feat: Add Name to IFile.Create (and others) #793)

As said the PR I like this, just a matter of getting it merged IMO 😀

• I added and used a ITimeSystem interface to simulate the system time, which I need for determining the correct time. This interface is not available in "System.IO.Abstraction" (and wouldn't really fit, as it is not IO-related)

Agreed, this is not something I’d like to have in SIOA. But is this really a problem at the Interfaxe level?

• You didn't replace the WaitForChangedResult as return value in the IFileSystemWatcher interface. As this struct only has internal constructors, it is not possible to mock. Changing these now, would unfortunately be a breaking change...

I would have no problem with changing this. Yes, it’s breaking, but easy to adapt.

• I would also be sceptical of using two completely distinct namespaces ("System.IO.Abstractions" and "Testably.Abstractions.Testing") and adding more projects under the "System.IO.*" namespace would not be possible, if I understand this comment correctly...

Yes, new things need to go with the TestableIO prefix. Any option would be fine for me, either you stay on your own or you move to this org.

All in all I’d say nothing really blocking. Let me know what you think @vbreuss

[vbreuss]

@fgreinacher

@fgreinacher : In the beginning I started doing exactly that, however the interfaces are not completely identical:

• I separated the interfaces from the default implementation with wrappers in separate projects. In this project they are both in the main DLL, so I can't just use the interfaces, but would always get the implementations as well.

I would have no problem changing that so that the interface are separate.

Would this be possible? I think you can't create new projects in the System.IO.Abstractions namespace? Or do you suggest to move the interface to e.g. TestableIO.Abstractions.Interface and use them in the System.IO.Abstractions library? This would also mean a breaking change to consumers, as they all need to adjust their using-statements...

And I fear this would become quite confusing, especially in my library:

• TestableIO.Abstractions.Interface Defines the IO-related interfaces
• System.IO.Abstractrions Contains the default implementation of the IO-related interfaces
• Testably.Abstractions.Interface Defines the time and random related interfaces
• Testably.Abstractions Contains the default implementation of the time and random related interfaces
• Testably.Abstractions.Testing Testing helper

If I were to split my interface DLL, so that one only contains the file-related interfaces (e.g. Testably.Abstractions.Interface.IO), would it be possible for you to use these interfaces? If you think this approach feasible, I could create a draft-PR...

• You included the ACL-related functionality directly in the interfaces. I extracted them to a separate DLL (Testably.Abstractions.AccessControl), so if they are not required, I don't need a reference to System.IO.FileSystem.AccessControl

I tried to mirror what .NET does and I think on modern versions we don’t actually need that reference. Or at least it is a no-op.

In modern versions it was extracted to extension methods. I implemented the same behaviour in Testably.Abstractions.AccessControl. For this to work, I had to implement the IFileSystemExtensionContainer interface, but this would be quite a trivial change in "System.IO.Abstractions", so no blocker...

• I replaced the Stream in the IFileStreamFactory with a custom stream class FileSystemStream (similar to feat: Add Name to IFile.Create (and others) #793)

As said the PR I like this, just a matter of getting it merged IMO 😀

Agreed :-)

• I added and used a ITimeSystem interface to simulate the system time, which I need for determining the correct time. This interface is not available in "System.IO.Abstraction" (and wouldn't really fit, as it is not IO-related)

Agreed, this is not something I’d like to have in SIOA. But is this really a problem at the Interfaxe level?

• You didn't replace the WaitForChangedResult as return value in the IFileSystemWatcher interface. As this struct only has internal constructors, it is not possible to mock. Changing these now, would unfortunately be a breaking change...

I would have no problem with changing this. Yes, it’s breaking, but easy to adapt.

Agreed, as said above, I think I could split my interface to only contain the file-related interfaces. However the ITimeSystem was quite handy, especially when implementing the "CreationTime", "LastAccessTime" and "LastWriteTime"...

[fgreinacher]

Would this be possible? I think you can't create new projects in the System.IO.Abstractions namespace? Or do you suggest to move the interface to e.g. TestableIO.Abstractions.Interface and use them in the System.IO.Abstractions library? This would also mean a breaking change to consumers, as they all need to adjust their using-statements...

True, we cannot upload new NuGet packages with the System.IO prefix. But we can keep using the System.IO prefix for the namespaces.

I think a simple way would be keep the namespaces as-is and change the NuGet package structure a bit:

• Add TestableIO.System.IO.Abstractions with just the interfaces
• Add TestableIO.System.IO.Abstractions.Wrappers with the wrapper implementations, referencing TestableIO.System.IO.Abstractions
• Keep System.IO.Abstractions for backward compatibility, referencing TestableIO.System.IO.Abstractions.Wrappers

You could then reference just TestableIO.System.IO.Abstractions from your Testably.Abstractions.Interface and plug in in your implementations.

[fgreinacher]

In modern versions it was extracted to extension methods. I implemented the same behaviour in Testably.Abstractions.AccessControl. For this to work, I had to implement the IFileSystemExtensionContainer interface, but this would be quite a trivial change in "System.IO.Abstractions", so no blocker...

I'm open to other solutions as long as the breaking change is minimal. From a gut feeling I don't think it's worth the refactoring effort though - I don't see the big problem with an extra reference, and up to now nobody complained.

[fgreinacher]

Agreed, as said above, I think I could split my interface to only contain the file-related interfaces. However the ITimeSystem was quite handy, especially when implementing the "CreationTime", "LastAccessTime" and "LastWriteTime"...

Agreed that it helps with the implementation, but I'd like to keep it out of the interfaces and implementation here. But that should be fine, right?

[vbreuss]

I think a simple way would be keep the namespaces as-is and change the NuGet package structure a bit:

• Add TestableIO.System.IO.Abstractions with just the interfaces
• Add TestableIO.System.IO.Abstractions.Wrappers with the wrapper implementations, referencing TestableIO.System.IO.Abstractions
• Keep System.IO.Abstractions for backward compatibility, referencing TestableIO.System.IO.Abstractions.Wrappers

You could then reference just TestableIO.System.IO.Abstractions from your Testably.Abstractions.Interface and plug in in your implementations.

@fgreinacher : I created #905 to implement the split into the two libraries. Is this how you envisioned the change? I would suggest to implement any adaptions to the interfaces in a separate PR, so that it is easier for GIT to detect the renamed files and preserve the history. Or would you prefer for me to add the changes into the same PR? Do you need to change the build pipelines/deployments in any way?

[vbreuss]

I like your library a lot! I'm wondering whether it would make sense if you implement the System.IO.Abstractions interfaces instead of duplicating them. In that case I'd be happy to link your lib from the README here, as an alternative, way of mocking things. Let me know what you think 🙇

I now released v2.0.0 which references the interfaces from TestableIO.System.IO.Abstractions (see https://github.com/Testably/Testably.Abstractions/pull/225) and also mentioned the System.IO.Abstractions in the Readme.md... What do you think, @fgreinacher ?

Would it make sense to merge the two projects further together?

[fgreinacher]

I now released v2.0.0 which references the interfaces from TestableIO.System.IO.Abstractions (see https://github.com/Testably/Testably.Abstractions/pull/225) and also mentioned the System.IO.Abstractions in the Readme.md... What do you think, @fgreinacher ?

Would it make sense to merge the two projects further together?

Looks great @vbreuss, I think we're good then. I'll prepare a small PR to mention the Testably library.