feat: Add support for event-driven behaviours in MockFileSystem.#1313
feat: Add support for event-driven behaviours in MockFileSystem.#1313mistial-dev wants to merge 4 commits intoTestableIO:mainfrom
Conversation
Added unit tests including various event handling scenarios, such as tracking file operations, validating timestamps, monitoring access patterns, simulating controlled failures, and profiling performance metrics.
vbreuss
left a comment
vbreuss
left a comment
There was a problem hiding this comment.
I appreciate your contribution, but as it seems to be quite heavily AI-generated, it will take time to make a thorough review. I expect it to take multiple iterations, so please be patient!
| public enum FileOperation | ||
| { | ||
| /// <summary> | ||
| /// File or directory creation operation. |
There was a problem hiding this comment.
It would help to have a list of operations on the file system in the XML-Comment, that trigger the corresponding FileOperations.
| /// </summary> | ||
| [NonSerialized] | ||
| #endif | ||
| private readonly List<Subscription> subscriptions = []; |
There was a problem hiding this comment.
You could use a ConcurrentDictionary (or another thread-safe collection), to avoid using the LockObject...
| /// Tracks the current version of active subscriptions, allowing for the detection of changes | ||
| /// such as additions or removals of subscription handlers. | ||
| /// </summary> | ||
| private volatile int subscriptionVersion; |
There was a problem hiding this comment.
I don't understand why you need the subscriptionVersion, as you only ever increment it, but never read it. Or did I overlook something?
| } | ||
|
|
||
| var args = new FileSystemOperationEventArgs(path, operation, resourceType, phase); | ||
| Subscription[] currentSubscriptions; |
There was a problem hiding this comment.
For thread-safe collections you should be able to directly iterate over the subscriptions and can avoid introducing a temporary copy of the collection.
| /// <summary> | ||
| /// Gets a value indicating whether this subscription has been disposed. | ||
| /// </summary> | ||
| public bool IsDisposed |
There was a problem hiding this comment.
As you remove the Subscription from the collection, it seems unnecessary to provide a flag and check for it.
What are your reasons for doing so?
|
|
||
| using (fileSystem.Events.Subscribe(args => Interlocked.Increment(ref eventCount))) | ||
| { | ||
| const int operationCount = 100; |
There was a problem hiding this comment.
100 parallel tasks seems a bit excessive for a unit test. Wouldn't e.g. 10 be sufficient?
| { | ||
| var fileSystem = new MockFileSystem(new MockFileSystemOptions { EnableEvents = true }); | ||
| var eventCount = 0; | ||
| var exceptions = new List<Exception>(); |
There was a problem hiding this comment.
If you use a thread-safe collection, you can avoid locking on it (see line 588).
| public void Events_SubscriptionDisposal_DuringEventFiring_ShouldNotFail() | ||
| { | ||
| var fileSystem = new MockFileSystem(new MockFileSystemOptions { EnableEvents = true }); | ||
| var exceptions = new List<Exception>(); |
There was a problem hiding this comment.
If you use a thread-safe collection, you can avoid locking on it (see line 620).
| public void Events_ConcurrentSubscriptionAndUnsubscription_ShouldBeThreadSafe() | ||
| { | ||
| var fileSystem = new MockFileSystem(new MockFileSystemOptions { EnableEvents = true }); | ||
| var exceptions = new List<Exception>(); |
There was a problem hiding this comment.
If you use a thread-safe collection, you can avoid locking on it (see line 694 and 714).
| [Test] | ||
| public void Events_FileSystemOperationEventArgs_ShouldNotBeSerializable() | ||
| { |
There was a problem hiding this comment.
We don't need a test to verify, that a class is NOT serializable.
|
@mistial-dev : I will close this PR, as you seem to no longer work on it... |
2 participants
Added unit tests including various event handling scenarios, such as tracking file operations, validating timestamps, monitoring access patterns, simulating controlled failures, and profiling performance metrics.
Potential resolution for #805