Uh oh!
There was an error while loading. Please reload this page.
Improve FileSystem tests on FAT32 temp volumes - #130369
Conversation
Sahithbasani
commented
Jul 9, 2026
@dotnet-policy-service agree |
jeffhandley
left a comment
There was a problem hiding this comment.
Note
This review was generated by the holistic code review workflow being iterated on as part of #130339. Please treat its findings as assistive input for human review.
Holistic Review
Motivation: The linked issue is a real test-infrastructure gap: System.IO.FileSystem.Tests has explicit FAT32 coverage intent, and the existing tests assume NTFS-like ADS, large-file, delete-pending, and timestamp behavior. Improving these tests is justified as long as the changes preserve coverage on capable file systems.
Approach: Centralizing temp-volume capability probes in FileSystemTest is a good direction, and the ADS / large-file / delete-open-file adjustments are narrowly targeted to the temp volume actually used by FileCleanupTestBase. I have one unresolved FAT timestamp concern that needs someone with a FAT32 run (or area expertise) to confirm.
Summary: MountHelper.CanCreateSymbolicLinks is evaluated in elevated Windows environments.
Detailed Findings
⚠️ Test Quality — FAT32 last-access timestamp granularity may still be over-asserted
BaseGetSetTimes<T>.SettingUpdatesPropertiesCore now chooses an even second on FAT32 (Base/BaseGetSetTimes.cs:20-24, :78), which addresses FAT write-time's 2-second resolution, and File/GetSetTimes.cs:116-127 still includes both local and UTC last-access time setters/getters in the exact round-trip assertions. However, the Win32 file-time documentation says FAT write time has 2-second resolution while access time has 1-day resolution (it is really the access date): https://learn.microsoft.com/windows/win32/sysinfo/file-times.
If FAT32 returns last-access times with the time-of-day truncated, SettingUpdatesProperties, SettingUpdatesPropertiesAfterAnother, and the symlink variants will still fail on FAT32 despite the new MilliSecondTemporalResolution/even-second handling. I did not run against a FAT32 temp volume, so this is a human-review item rather than a firm blocker, but the central FAT32 timestamp assumption should be verified before relying on this PR to make the test suite pass there.
⚠️ Test Scope — symlink capability may still be filesystem-sensitive on FAT32
The changed timestamp base class still runs SettingPropertiesOnSymlink under [ConditionalTheory(typeof(MountHelper), nameof(MountHelper.CanCreateSymbolicLinks))] (Base/BaseGetSetTimes.cs:119-123). MountHelper.CanCreateSymbolicLinks probes by attempting to create links under Path.GetTempPath(), but its Windows failure path asserts that a privileged process should not fail (src/libraries/Common/tests/System/IO/ReparsePointUtilities.cs:62-80), while the linked issue discussion notes FAT32 itself does not support symlinks.
This may already skip correctly for normal unelevated FAT32 runs, but it is worth a human check for elevated Windows / Developer Mode scenarios: unsupported filesystem capability should ideally skip tests, not fail during condition evaluation.
✅ Test Quality — capability skips are mostly narrow and preserve existing coverage elsewhere
The new SupportsAlternateDataStreams, TempPathSupportsLargeFiles, IsTempPathOnFat32, and DeletesOpenFileNameImmediately helpers are based on the same Path.GetTempPath() root used by FileCleanupTestBase, so the conditions track the volume under test rather than hardcoding a drive. ADS tests remain active when a live probe succeeds, large-file tests are skipped only for FAT32, and delete-open-file expectations remain unchanged for non-FAT32 Windows versions.
Workflow state for the Holistic Review Orchestrator. {
"version": 5,
"last_dispatched_commit": "346a22802976bfa6c9749850b1329923b2cd1ac2",
"last_dispatched_base_ref": "main",
"last_dispatched_base_sha": "7abe5f60c079129a57c358451cfe01ee198fa40f",
"last_reviewed_commit": "346a22802976bfa6c9749850b1329923b2cd1ac2",
"last_reviewed_base_ref": "main",
"last_reviewed_base_sha": "7abe5f60c079129a57c358451cfe01ee198fa40f",
"last_recorded_worker_run_id": "29686398719",
"review_attempt_commit": "",
"review_attempt_base_ref": "",
"review_attempt_count": 0,
"max_review_attempts": 5,
"review_history_format": "holistic-review-disclosure-v1",
"review_history": [
{
"commit": "346a22802976bfa6c9749850b1329923b2cd1ac2",
"review_id": 4730735805
}
]
} |
There was a problem hiding this comment.
Holistic Review
Motivation: Fixes #66544. The System.IO.FileSystem tests generate hundreds of false failures when the temp path resides on a FAT32 volume, because they assume capabilities FAT32 lacks: alternate data streams (ADS), sub-second timestamp resolution, files larger than 4 GB, and immediate deletion of files with open handles. The goal is to make these tests pass on FAT32 while keeping the assertions active on file systems that do support those capabilities.
Approach: The PR centralizes temp-volume capability detection into FileSystemTest via lazily-evaluated static helpers (TempDriveFormat, IsTempPathOnFat32, TempPathSupportsLargeFiles, DeletesOpenFileNameImmediately, SupportsAlternateDataStreams). It then replaces scattered, local FAT32/Windows detection (notably the duplicated P/Invoke GetVolumeInformation block in ctor_options.Windows.cs) with these shared helpers. ADS-dependent theories are converted to ConditionalTheory/ConditionalFact gated on SupportsAlternateDataStreams; timestamp tests widen tolerance and use a FAT32-safe seconds value; large-file tests are gated on TempPathSupportsLargeFiles; and open-handle delete tests use DeletesOpenFileNameImmediately. This is a focused, test-only change with no product code impact.
Summary: This is a well-scoped, correct improvement that directly addresses the four failure categories enumerated in the issue and removes duplicated interop code in favor of a single shared detection point. SupportsAlternateDataStreams is a sound design choice: it probes actual behavior (attempt to write/read an ADS) rather than inferring from the volume format, which is more robust than a pure FAT32 check. The lazy caching avoids repeated DriveInfo/probe cost, and the probe cleans up its temp file in a finally. I have one non-blocking inline note about exception-type exactness in InvalidPath_Core. Overall this looks good to merge; verify via CI that the affected test assemblies still build and pass on the standard (non-FAT32) NTFS/Unix legs, since the change touches many [Theory]→[ConditionalTheory] attribute conversions.
Detailed Findings
No blocking issues. One minor, non-actionable observation left inline on Directory/GetFileSystemEntries_str.cs regarding Assert.Throws<IOException> matching the exact type rather than derived types when SupportsAlternateDataStreams is false.
Note
This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.
Generated by Holistic Review · 92 AIC · ⌖ 10.7 AIC · ⊞ 10K
Uh oh!
There was an error while loading. Please reload this page.
7687e75 to
bd7a21fCompare
Fixes#66544
Summary
Impact
This should reduce false failures when the test temp path is on FAT32, while keeping the existing assertions active on file systems that support the required capabilities.
Validation
git diff --checkSystem.IO.FileSystem.Testsproject builds, but the builds did not finish within the local time budget on this machine