Skip to content

Conversation

@stogle
Copy link
Contributor

@stogle stogle commented Sep 17, 2025

This PR attempts to fix missing notifications from FileSystemWatcherMock on Windows, when moving directories into or out of the watched path, as described in #865.

On Windows, moving a directory from a location outside the watched path to a location inside the watched path should trigger a Created event. Similarly, moving a directory from a location inside the watched path to a location outside the watched path should trigger a Deleted event. On other platforms, these moves should trigger a Rename event instead.

Copy link
Member

@vbreuss vbreuss left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, @stogle. I really like the general approach and have some suggestions for improvement.
I also explained the change I made in #866 that you deleted that currently fail the build on the real file system.

@stogle
Copy link
Contributor Author

stogle commented Sep 18, 2025

Thanks for the review @vbreuss. I'm not surprised that some changes are needed. I will look at it again tomorrow. I just saw the build failure, but I'm sure that all the tests passed locally before I pushed? Anyway, I'll figure it out.

@vbreuss
Copy link
Member

vbreuss commented Sep 18, 2025

Thanks for the review @vbreuss. I'm not surprised that some changes are needed. I will look at it again tomorrow. I just saw the build failure, but I'm sure that all the tests passed locally before I pushed? Anyway, I'll figure it out.

@stogle: Please note the TestSettings project:
It allows some tests to be skipped automatically during local development (per default no tests against the real file system are executed locally). In order for your tests to run locally also against the real file system you have to either switch to Release mode or execute the Testably.Abstractions.TestSettings.RealFileSystemTests.EnableAlways test once explicitely.
The NotifyFiltersTests even then are skipped due to the SkipIfLongRunningTestsShouldBeSkipped(), so you also need to enable them in the settings the same way!

@stogle
Copy link
Contributor Author

stogle commented Sep 18, 2025

@vbreuss I've reinstated the code that I removed, refactored the logic in MatchesFilter and TriggerRenameNotification to improve readability, and added tests for moving files in to, and out of the watched path. But I'm having trouble getting all the "move in" and "move out" tests to pass on all file systems, and it seems to be related to the issue you found with relative paths. Could you help me with this?

@vbreuss
Copy link
Member

vbreuss commented Sep 19, 2025

But I'm having trouble getting all the "move in" and "move out" tests to pass on all file systems, and it seems to be related to the issue you found with relative paths. Could you help me with this?

I completely understand the difficulties and I am working on finding and fixing all edge cases.
I will update this PR accordingly...

@vbreuss vbreuss requested a review from Copilot September 19, 2025 12:41
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Fixes FileSystemWatcherMock notifications for directory moves across the watched boundary: on Windows emit Created/Deleted when moving into/out of the watched path, and on non-Windows emit Renamed.

  • Refactors watcher path matching and rename handling to correctly emit Created/Deleted on Windows for cross-boundary moves
  • Adjusts event args construction to override FullPath via reflection; updates tests and adds Windows-specific cases for moves into/out of the watched path
  • Minor test fix to use absolute watcher paths for reliable separator handling

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.

File Description
Tests/Testably.Abstractions.Tests/FileSystem/FileSystemWatcher/NotifyFiltersTests.cs Adds Windows-specific tests for file and directory moves into/out of the watched path; renames methods to clarify Renamed semantics; parameterizes IncludeSubdirectories.
Tests/Testably.Abstractions.Testing.Tests/FileSystem/FileSystemWatcherMockTests.cs Ensures watcher is created with an absolute path to validate FullPath/Name behavior consistently.
Source/Testably.Abstractions.Testing/FileSystem/FileSystemWatcherMock.cs Refactors path matching (MatchesWatcherPath), adjusts rename handling to Created/Deleted on Windows, and overrides FileSystemEventArgs/RenamedEventArgs FullPath via reflection.

@vbreuss vbreuss merged commit 4bf0d84 into Testably:main Sep 19, 2025
11 checks passed
@github-actions
Copy link

This is addressed in release v4.3.9.

@stogle
Copy link
Contributor Author

stogle commented Sep 19, 2025

@vbreuss I have a theory about these hacks around constructing FileSystemEventArgs, although I'm very new to this codebase so I may be wrong. In System.IO.FileSystemWatcher, when they construct FileSystemEventArgs or RenameEventArgs, they pass in the watcher's directory, not the directory of the file that changed. The constructor then combines them to set the FullPath property.

If FileSystemWatcherMock did the same thing in ToFileSystemEventArgs and TryMakeRenamedEventArgs could some of the hacks be avoided?

@vbreuss
Copy link
Member

vbreuss commented Sep 19, 2025

@vbreuss I have a theory about these hacks around constructing FileSystemEventArgs, although I'm very new to this codebase so I may be wrong. In System.IO.FileSystemWatcher, when they construct FileSystemEventArgs or RenameEventArgs, they pass in the watcher's directory, not the directory of the file that changed. The constructor then combines them to set the FullPath property.

If FileSystemWatcherMock did the same thing in ToFileSystemEventArgs and TryMakeRenamedEventArgs could some of the hacks be avoided?

@stogle: You are right 😄 :
The hacks can be avoided when simulating the native operating system, but not when simulating a different operating system, as it then uses an incorrect directory separator.
But thanks to your input I created another refactor PR (#871) that will eliminate the hacks unless you simulate another OS.

@stogle
Copy link
Contributor Author

stogle commented Sep 19, 2025

Ah, I get it now. That explains the failures I was seeing when I tried removing them myself. And your new PR not only improves performance but makes it clearer when and why the hacks are needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants