Skip to content

fix: FileSystemWatcher MoveTests parallel stability with MockFileSystem#966

Merged
vbreuss merged 5 commits intomainfrom
copilot/fix-move-tests-parallel-execution
Mar 14, 2026
Merged

fix: FileSystemWatcher MoveTests parallel stability with MockFileSystem#966
vbreuss merged 5 commits intomainfrom
copilot/fix-move-tests-parallel-execution

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Mar 14, 2026

MoveTests (and EventTests) were marked [NotInParallel] as a workaround for flakiness when running against MockFileSystem. Two distinct bugs caused this:

Root Causes & Fixes

1. CPU busy-wait in FileSystemWatcherMock event loop

The background task consumed channel items with a tight spin loop, starving the ThreadPool under parallel load:

// Before: spins continuously, consuming CPU even when idle
while (!token.IsCancellationRequested)
{
    if (reader.TryRead(out ChangeDescription? c))
        NotifyChange(c);
}

// After: yields the thread when idle, wakes immediately on new item
while (await reader.WaitToReadAsync(token).ConfigureAwait(false))
{
    while (reader.TryRead(out ChangeDescription? c))
        NotifyChange(c);
}

2. Incorrect timeout for expected-to-fire events in MoveTests

All Wait() calls used ExpectTimeout (30 ms) — including assertions where an event should fire. Consistent with NotifyFiltersTests and IncludeSubdirectoriesTests, events that must fire use ExpectSuccess (30 000 ms); only "should not fire" assertions keep ExpectTimeout:

// Before
await That(createdMs.Wait(ExpectTimeout, CancellationToken)).IsTrue();

// After
await That(createdMs.Wait(ExpectSuccess, CancellationToken)).IsTrue();

// Conditional case
await That(deletedMs.Wait(shouldInvokeDeleted ? ExpectSuccess : ExpectTimeout, CancellationToken))
    .IsEqualTo(shouldInvokeDeleted);

Same fix applied in MoveTests.Unix.cs and MoveTests.Windows.cs.

3. Removed [NotInParallel] workarounds

With the above fixes, [NotInParallel] is no longer needed on MoveTests or EventTests.

Original prompt

This section details on the original issue you should resolve

<issue_title>Testably.Abstractions.Tests.FileSystem.FileSystemWatcher.MoveTests don't work in parallel</issue_title>
<issue_description>When running the MoveTests in parallel, they fail.

Workaround was to mark them as NotInParallel in #955, but this should not be necessary when using the MockFileSystem!</issue_description>

<agent_instructions>Check, why these tests fail when they run in parallel and fix it.</agent_instructions>

Comments on the Issue (you are @copilot in this section)


📍 Connect Copilot coding agent with Jira, Azure Boards or Linear to delegate work to Copilot in one click without leaving your project management tool.

Co-authored-by: vbreuss <3438234+vbreuss@users.noreply.github.com>
Copilot AI changed the title [WIP] Fix parallel execution issues in MoveTests fix: FileSystemWatcher MoveTests parallel stability with MockFileSystem Mar 14, 2026
Copilot AI requested a review from vbreuss March 14, 2026 04:45
@github-actions
Copy link
Copy Markdown

github-actions bot commented Mar 14, 2026

Test Results

    102 files  ±0      102 suites  ±0   2h 20m 21s ⏱️ - 39m 10s
112 450 tests  - 5   99 847 ✅  - 5  12 603 💤 ±0  0 ❌ ±0 
267 906 runs  ±0  231 408 ✅ ±0  36 498 💤 ±0  0 ❌ ±0 

Results for commit a7044a4. ± Comparison against base commit 4ed0ce0.

♻️ This comment has been updated with latest results.

@vbreuss
Copy link
Copy Markdown
Member

vbreuss commented Mar 14, 2026

@copilot The unit tests failed on the CI build pipeline. Please check what happened and fix the issue

Copy link
Copy Markdown
Contributor Author

Copilot AI commented Mar 14, 2026

@copilot The unit tests failed on the CI build pipeline. Please check what happened and fix the issue

The CI failures are pre-existing and unrelated to this PR's changes:

  1. macOS (AccessViolationException in DriveInfoFactory.Tests.Wrap_WithSimulatedMockFileSystem_ShouldThrowNotSupportedException): A native crash when calling System.IO.DriveInfo.GetDrives() on macOS. This same failure also occurred on the main branch before my changes (e.g. runs 23059915384 and 23058139243).

  2. Windows net48 (DirectoryNotFoundException in EnumerateFileSystemEntries_SearchOptionAllDirectories_FullPath_ShouldReturnAllFileSystemEntriesWithFullPath): A flaky real-filesystem parallel test where cleanup retries fail due to a locked file handle. This test doesn't use FileSystemWatcher and is unrelated to the mock changes.

All 10,072 tests pass locally on net10.0 (0 failures).

@vbreuss
Copy link
Copy Markdown
Member

vbreuss commented Mar 14, 2026

@copilot The unit tests failed on the CI build pipeline. Please check what happened and fix the issue

The CI failures are pre-existing and unrelated to this PR's changes:

  1. macOS (AccessViolationException in DriveInfoFactory.Tests.Wrap_WithSimulatedMockFileSystem_ShouldThrowNotSupportedException): A native crash when calling System.IO.DriveInfo.GetDrives() on macOS. This same failure also occurred on the main branch before my changes (e.g. runs 23059915384 and 23058139243).

  2. Windows net48 (DirectoryNotFoundException in EnumerateFileSystemEntries_SearchOptionAllDirectories_FullPath_ShouldReturnAllFileSystemEntriesWithFullPath): A flaky real-filesystem parallel test where cleanup retries fail due to a locked file handle. This test doesn't use FileSystemWatcher and is unrelated to the mock changes.

All 10,072 tests pass locally on net10.0 (0 failures).

Please create two separate pull requests to fix the two flaky tests

Copy link
Copy Markdown
Contributor Author

Copilot AI commented Mar 14, 2026

@copilot The unit tests failed on the CI build pipeline. Please check what happened and fix the issue
...

I'm unable to create new pull requests - I can only push changes to this PR's branch. If you'd like, I can fix both issues directly in this branch, or I can document the root causes and suggested fixes for you to open separate PRs manually.

Issue 1 – macOS crash in DriveInfoFactory.Tests: The test calls System.IO.DriveInfo.GetDrives()[0] which triggers a native crash (AccessViolationException) in GetAllMountPoints() on the macOS CI runner. Fix: wrap the call in a try/catch or skip the test when GetDrives() crashes, similar to how other platform-specific behaviors are skipped.

Issue 2 – Windows net48 EnumerateFileSystemEntries test: Two parallel test instances share the same temp subdirectory name (EnumerateFileSystemInfosTests\...) and one instance's cleanup leaves a locked file handle, causing the other instance's file creation to fail. Fix: the test should use a uniquely named per-instance directory to avoid cross-test interference.

Let me know if you'd like me to implement these fixes here.

@vbreuss
Copy link
Copy Markdown
Member

vbreuss commented Mar 14, 2026

@copilot can you create two issues that describe the flaky tests?

@vbreuss vbreuss marked this pull request as ready for review March 14, 2026 09:54
Copilot AI review requested due to automatic review settings March 14, 2026 09:54
Copy link
Copy Markdown

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

Improves parallel stability of FileSystemWatcher tests (notably MoveTests and EventTests) by addressing MockFileSystem watcher scheduling behavior and aligning test wait timeouts with “expected to fire” vs “expected not to fire” semantics.

Changes:

  • Replace the FileSystemWatcherMock channel-consumer busy-spin with an async WaitToReadAsync loop to avoid ThreadPool starvation under parallel load.
  • Update MoveTests* to use ExpectSuccess for events that must fire (keeping ExpectTimeout for “should not fire” assertions).
  • Remove prior [NotInParallel] workarounds from MoveTests and EventTests.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
Tests/Testably.Abstractions.Tests/FileSystem/FileSystemWatcher/MoveTests.cs Removes parallel workaround and fixes “must fire” waits to use ExpectSuccess.
Tests/Testably.Abstractions.Tests/FileSystem/FileSystemWatcher/MoveTests.Windows.cs Adjusts event waits to use success vs timeout depending on expected behavior.
Tests/Testably.Abstractions.Tests/FileSystem/FileSystemWatcher/MoveTests.Unix.cs Adjusts event waits to use success vs timeout depending on expected behavior.
Tests/Testably.Abstractions.Tests/FileSystem/FileSystemWatcher/EventTests.cs Removes [NotInParallel] now that underlying causes are addressed.
Source/Testably.Abstractions.Testing/FileSystem/FileSystemWatcherMock.cs Reworks event loop to avoid CPU spinning and refactors some rename/event arg helpers.

vbreuss and others added 2 commits March 14, 2026 11:10
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud bot commented Mar 14, 2026

@vbreuss vbreuss merged commit 81692db into main Mar 14, 2026
14 checks passed
@vbreuss vbreuss deleted the copilot/fix-move-tests-parallel-execution branch March 14, 2026 11:17
@github-actions
Copy link
Copy Markdown

This is addressed in release v6.0.0.

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.

Testably.Abstractions.Tests.FileSystem.FileSystemWatcher.MoveTests don't work in parallel

3 participants