Repository navigation
Conversation
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
There are a few correctness and coverage issues in the new handle registry/tests/docs (e.g., empty-path FileNotFound, missing closed-handle guard for foreign handles, and an “async” test that doesn’t exercise the async path).
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 3
Open (4)
What changed in this PR
This PR completes the SafeFileHandle/RandomAccess follow-up by removing stale, never-enabled SafeFileHandle test scaffolding and replacing it with cross-platform tests that obtain handles from the abstraction, while also lifting code-coverage exclusions that are now genuinely testable.
Changes:
- Adds
IRandomAccessfacade wiring (real + mock) and introduces extensive parity/behavior tests for RandomAccess reads/writes/length/append/identity semantics. - Replaces (and removes) the old
EXECUTE_SAFEFILEHANDLE_TESTS+ Win32 P/Invoke helper approach with tests that useIFile.OpenHandleand exercise SafeFileHandle-taking overloads. - Updates API baselines, feature flags, statistics tracking, and documentation to reflect the new handle-based surface.
| File | Description |
|---|---|
| Tests/Testably.Abstractions.Tests/TestHelpers/UnmanagedFileLoader.cs | Removes dead Win32 P/Invoke helper behind an unused compile symbol. |
| Tests/Testably.Abstractions.Tests/Testably.Abstractions.Tests.csproj | Switches Interface dependency to a project reference for stacked changes. |
| Tests/Testably.Abstractions.Tests/FileSystem/RandomAccess/WriteTests.cs | Adds RandomAccess write behavior tests (offset, growth, gather, async, visibility). |
| Tests/Testably.Abstractions.Tests/FileSystem/RandomAccess/Tests.cs | Adds baseline RandomAccess wiring + disposed-handle behavior tests (+ FlushToDisk gated tests). |
| Tests/Testably.Abstractions.Tests/FileSystem/RandomAccess/ReadTests.cs | Adds RandomAccess read behavior tests (offsets, short reads, scatter, async, access checks). |
| Tests/Testably.Abstractions.Tests/FileSystem/RandomAccess/LengthTests.cs | Adds GetLength + SetLength behavior tests (including net7+ gated SetLength). |
| Tests/Testably.Abstractions.Tests/FileSystem/RandomAccess/HandleIdentityTests.cs | Adds handle identity tests (rename behavior, DeleteOnClose semantics), with Windows limitations noted. |
| Tests/Testably.Abstractions.Tests/FileSystem/RandomAccess/AppendTests.cs | Adds platform-differentiated append-handle behavior tests (Linux vs Windows/macOS). |
| Tests/Testably.Abstractions.Tests/FileSystem/FileStreamFactory/SafeFileHandleTests.cs | Removes compiled-out legacy tests that depended on unmanaged handle creation. |
| Tests/Testably.Abstractions.Tests/FileSystem/FileStreamFactory/OpenHandleStreamTests.cs | Adds new tests covering IFileStreamFactory.New(SafeFileHandle, ...) using abstraction-created handles. |
| Tests/Testably.Abstractions.Tests/FileSystem/File/SafeFileHandleTests.cs | Adds tests covering IFile SafeFileHandle overloads now that handles can be produced. |
| Tests/Testably.Abstractions.Tests/FileSystem/File/OpenHandleTests.cs | Adds tests for IFile.OpenHandle behaviors (modes, sharing, DeleteOnClose, disposal). |
| Tests/Testably.Abstractions.Testing.Tests/TestHelpers/UnmanagedFileLoader.cs | Removes duplicate dead P/Invoke helper behind unused compile symbol. |
| Tests/Testably.Abstractions.Testing.Tests/Testably.Abstractions.Testing.Tests.csproj | Switches Interface dependency to a project reference for stacked changes. |
| Tests/Testably.Abstractions.Testing.Tests/Statistics/FileSystem/FileStatisticsTests.cs | Adds statistics assertion coverage for IFile.OpenHandle(...) call tracking. |
| Tests/Testably.Abstractions.Testing.Tests/FileSystem/FileStreamFactoryMockTests.SafeFileHandle.cs | Removes compiled-out legacy tests for SafeFileHandle strategy behavior. |
| Tests/Testably.Abstractions.Parity.Tests/TestHelpers/Parity.cs | Adds a parity check bucket for RandomAccess under feature flag. |
| Tests/Testably.Abstractions.Parity.Tests/ParityTests.cs | Adds parity test ensuring IRandomAccess surface matches System.IO.RandomAccess. |
| Tests/Testably.Abstractions.Parity.Tests/Net8ParityTests.cs | Removes now-obsolete “missing method” exclusion for File.OpenHandle. |
| Tests/Testably.Abstractions.Parity.Tests/Net9ParityTests.cs | Removes now-obsolete “missing method” exclusion for File.OpenHandle. |
| Tests/Testably.Abstractions.Parity.Tests/Net10ParityTests.cs | Removes now-obsolete “missing method” exclusion for File.OpenHandle. |
| Tests/Helpers/Testably.Abstractions.TestHelpers/Testably.Abstractions.TestHelpers.csproj | Switches test helper dependencies to project references for stacked changes. |
| Tests/Api/Testably.Abstractions.Core.Api.Tests/Expected/Testably.Abstractions.FileSystem.Interface_net6.0.txt | Updates interface API baseline for OpenHandle + IRandomAccess (net6). |
| Tests/Api/Testably.Abstractions.Core.Api.Tests/Expected/Testably.Abstractions.FileSystem.Interface_net8.0.txt | Updates interface API baseline for OpenHandle + IRandomAccess (net8). |
| Tests/Api/Testably.Abstractions.Core.Api.Tests/Expected/Testably.Abstractions.FileSystem.Interface_net9.0.txt | Updates interface API baseline for OpenHandle + IRandomAccess (net9). |
| Tests/Api/Testably.Abstractions.Core.Api.Tests/Expected/Testably.Abstractions.FileSystem.Interface_net10.0.txt | Updates interface API baseline for OpenHandle + IRandomAccess (+ FlushToDisk) (net10). |
| Tests/Api/Testably.Abstractions.Api.Tests/Expected/Testably.Abstractions.Testing_net6.0.txt | Updates Testing API baseline to include RandomAccess + statistics (net6). |
| Tests/Api/Testably.Abstractions.Api.Tests/Expected/Testably.Abstractions.Testing_net8.0.txt | Updates Testing API baseline to include RandomAccess + statistics (net8). |
| Tests/Api/Testably.Abstractions.Api.Tests/Expected/Testably.Abstractions.Testing_net9.0.txt | Updates Testing API baseline to include RandomAccess + statistics (net9). |
| Tests/Api/Testably.Abstractions.Api.Tests/Expected/Testably.Abstractions.Testing_net10.0.txt | Updates Testing API baseline to include RandomAccess + statistics (net10). |
| Tests/Api/Testably.Abstractions.Api.Tests/Expected/Testably.Abstractions_net6.0.txt | Updates core API baseline to include IFileSystem.RandomAccess (net6). |
| Tests/Api/Testably.Abstractions.Api.Tests/Expected/Testably.Abstractions_net8.0.txt | Updates core API baseline to include IFileSystem.RandomAccess (net8). |
| Tests/Api/Testably.Abstractions.Api.Tests/Expected/Testably.Abstractions_net9.0.txt | Updates core API baseline to include IFileSystem.RandomAccess (net9). |
| Tests/Api/Testably.Abstractions.Api.Tests/Expected/Testably.Abstractions_net10.0.txt | Updates core API baseline to include IFileSystem.RandomAccess (net10). |
| Source/Testably.Abstractions/RealFileSystem.cs | Wires IRandomAccess into the real file system under feature flag. |
| Source/Testably.Abstractions/FileSystem/RandomAccessWrapper.cs | Adds real wrapper delegating to System.IO.RandomAccess. |
| Source/Testably.Abstractions/FileSystem/FileWrapper.cs | Adds IFile.OpenHandle(...) forwarding + removes SafeFileHandle coverage exclusions. |
| Source/Testably.Abstractions/FileSystem/FileStreamFactory.cs | Removes SafeFileHandle coverage exclusions now that tests exist. |
| Source/Testably.Abstractions.Testing/Testably.Abstractions.Testing.csproj | Switches Interface reference to project reference to implement new members in-stack. |
| Source/Testably.Abstractions.Testing/Statistics/IFileSystemStatistics.cs | Adds statistics surface for IFileSystem.RandomAccess. |
| Source/Testably.Abstractions.Testing/Statistics/FileSystemStatistics.cs | Implements RandomAccess statistics bucket. |
| Source/Testably.Abstractions.Testing/MockFileSystem.cs | Adds RandomAccess mock wiring and handle registry sweeping hook. |
| Source/Testably.Abstractions.Testing/Helpers/FileModeHelper.cs | Extracts shared mode/access validation logic. |
| Source/Testably.Abstractions.Testing/Helpers/ExceptionFactory.cs | Adds helper exceptions for non-negative requirements + closed handle reporting. |
| Source/Testably.Abstractions.Testing/FileSystem/RandomAccessMock.cs | Implements IRandomAccess behavior for the mock with statistics and platform-specific append rules. |
| Source/Testably.Abstractions.Testing/FileSystem/MockSafeFileHandleRegistry.cs | Adds registry to create/resolve synthetic SafeFileHandles and enforce sharing/DeleteOnClose behavior. |
| Source/Testably.Abstractions.Testing/FileSystem/FileStreamMock.cs | Uses extracted FileMode/access validation helper. |
| Source/Testably.Abstractions.Testing/FileSystem/FileStreamFactoryMock.cs | Prefers registry mapping for mock-created handles; keeps strategy fallback. |
| Source/Testably.Abstractions.Testing/FileSystem/FileMock.cs | Implements IFile.OpenHandle(...) via registry and routes SafeFileHandle container resolution through it. |
| Source/Testably.Abstractions.FileSystem.Interface/IRandomAccess.cs | Introduces IRandomAccess interface abstraction (feature-flagged). |
| Source/Testably.Abstractions.FileSystem.Interface/IFileSystem.cs | Adds IFileSystem.RandomAccess (feature-flagged). |
| Source/Testably.Abstractions.FileSystem.Interface/IFile.cs | Adds IFile.OpenHandle(...) (feature-flagged). |
| Feature.Flags.props | Adds FEATURE_FILESYSTEM_RANDOMACCESS and FEATURE_RANDOMACCESS_FLUSHTODISK. |
| Docs/pages/docs/file-system/safe-file-handles.mdx | Updates documentation to lead with abstraction-created handles and retain strategy guidance for foreign handles. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| internal (IStorageContainer Container, FileAccess Access, FileMode Mode) GetContainer( | ||
| SafeFileHandle handle) | ||
| { | ||
| if (Resolve(handle) is { } entry) | ||
| { |
| .ThrowExceptionIfNotFound(_fileSystem)); | ||
| if (container is NullContainer) | ||
| { | ||
| throw ExceptionFactory.FileNotFound(""); |
| using SafeFileHandle handle = FileSystem.File.OpenHandle(path, | ||
| FileMode.Open, FileAccess.Read, FileShare.ReadWrite); | ||
| using FileSystemStream stream = | ||
| FileSystem.FileStream.New(handle, FileAccess.Read, 1024, false); |
| When a handle comes from somewhere the mock knows nothing about - platform invocation, or a library that hands you one - you still have to tell it which file that handle stands for. | ||
|
|
||
| `MockFileSystem.WithSafeFileHandleStrategy(ISafeFileHandleStrategy)` registers the translation. The strategy maps a `SafeFileHandle` to a `SafeFileHandleMock` that points at a location inside the mock, and is consulted for any handle the mock did not create itself. | ||
|
|
||
| The default strategy (`NullSafeFileHandleStrategy`) is registered automatically and throws `NotSupportedException` for any handle - install a custom strategy as soon as your code under test reaches for a handle from outside. |
72deddb to
3c62cc7
Compare
|
Pushed fixes for all four findings, and rebased onto the updated #1082. The async overload test did not test the async path — a fair hit: it passed The docs still reached for The other two findings were about code introduced in #1082 rather than here, so I fixed them on that branch: a disposed or null handle from outside the mock now throws before reaching 10506 tests pass; Still happy to keep and port the deleted test files instead of removing them, if you would rather — the offer in the description stands. |
3c62cc7 to
d23c9b9
Compare
Implements the abstractions added alongside for `MockFileSystem`, so the `SafeFileHandle` surface can be exercised against the mock. `SafeFileHandle` is sealed and wraps an operating system handle, so the mock cannot create one a real syscall would accept. It hands out a handle with a synthetic value and remembers which file that value stands for — the same indirection `ISafeFileHandleStrategy` already provides for handles created elsewhere, which continues to work unchanged. A handle resolves to the file it was opened on rather than to its name, so it keeps working across a rename and does not start referring to a file later created under the original path. File share locks are released and `FileOptions.DeleteOnClose` applies when the last handle to a file closes, noticed on the next file system operation because a sealed `SafeFileHandle` gives no disposal callback. Tests run against both the real and the mocked file system, covering every member that takes a handle. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
- Serialise read-modify-write sequences per file. `RandomAccess` permits concurrent writes at distinct offsets, and rebuilding the whole buffer without a gate let one of them discard the other. - Copy only what the destination asks for. Reading a single byte at the start of a large file materialised the entire tail first. - Follow the file rather than the path when applying `FileOptions.DeleteOnClose`, so a rename while the handle is open deletes the right file and leaves a replacement at the old path alone. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A handle the `MockFileSystem` did not create is resolved by the registered `ISafeFileHandleStrategy`, but was otherwise unchecked: a disposed or null handle still reached the strategy, where the real implementation throws. Also name the file in the exception when the mapped path no longer exists, instead of reporting an empty path. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Follows the interface change: one flag now limits the random-access surface that needs more than .NET 6. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Rebased onto `main` now that the interface ships in 10.4.0-pre.1: - the project references added while the interface was unreleased go back to package references, - `Directory.Packages.props` moves to `[10.4.0-pre.1,10.5.0)`, - `BuildScope` returns to `Default`. Two of the tests that skipped the Windows-simulating variant now say why more precisely: moving a file held open by a handle needs `ignoreFileShare`, which is inert on Windows (Testably#1086), and two openers that both permit `FileShare.ReadWrite | FileShare.Delete` are refused (Testably#1090). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
d23c9b9 to
c123e40
Compare
Drops the summaries on private and internal members that restate the member name, the duplicated rationale on `IStorage.GetLocation` in favour of `<inheritdoc />`, and the prose repeated at both ends of the handle sweep. What stays is the handful of decisions the code cannot show on its own: why synthetic handle values start where they do, why `deleteAccess` is not what `FileOptions.DeleteOnClose` means, why a closed handle is noticed late, why writes are serialised, and the Linux `pwrite(2)` deviation for append handles. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
c123e40 to
115872b
Compare
Running in Release, which is what adds the real file system to the test matrix, showed two places where the mock had drifted from it on Unix. `FileOptions.DeleteOnClose` unlinks the name that was opened, as soon as that handle closes: it does not wait for other handles, a file renamed since then survives, and a replacement under the old name does not. The delete-on-last-close behaviour that follows the file across a rename is Windows', and is now gated on that. `RandomAccess.SetLength` on a read-only handle fails with `EINVAL` from `ftruncate`, reported as an `IOException`, where Windows denies access. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
115872b to
d04b0c8
Compare
|
Rebased onto the updated #1082, which carries the I have also corrected the verification note in the description. It previously quoted Debug numbers, and Debug leaves the real file system out of the test matrix entirely — |
I had the mock append on Linux, on the strength of the `pwrite(2)` note that `O_APPEND` overrides the offset. The Linux CI run disagrees: the offset is honoured there exactly as it is on Windows and macOS, so the special case and the platform-split tests are gone. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
d04b0c8 to
c865c2c
Compare
- `safe-file-handles.mdx` opened by saying the mock has no handle to give you and sending you to `ISafeFileHandleStrategy`, which this change makes untrue. It now leads with `IFile.OpenHandle`, says what the handle carries and how `FileOptions.DeleteOnClose` differs between Unix and Windows, and keeps the strategy for handles from elsewhere. - The statistics table gains `RandomAccess`, which it introduces as one row per `IFileSystem` sub-property. - A new page for `RandomAccess` itself: positionless reads and writes, what each framework offers, the behaviour worth knowing, and why `FlushToDisk` is worth asserting against the mock. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The tests for the `SafeFileHandle` overloads were compiled out behind `EXECUTE_SAFEFILEHANDLE_TESTS`, which is defined nowhere, because obtaining a handle required platform invocation. The implementations carried `[ExcludeFromCodeCoverage(Justification = "SafeFileHandle cannot be unit tested.")]` for the same reason. `IFile.OpenHandle` removes that constraint, so: - the 22 exclusions on members that the new tests exercise are removed; the two `ISafeFileHandleStrategy` implementations keep theirs, as nothing covers a handle that originates outside the mock, - the dead tests and their platform invocation helpers are removed, superseded by tests that obtain a handle from the abstraction and run against both the real and the mocked file system, - the documentation shows the new way in, and keeps the strategy for handles that come from elsewhere. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The test for `New(SafeFileHandle, FileAccess, int, bool)` passed `isAsync: false`, so it never covered the asynchronous path its name claimed. It now opens the handle with `FileOptions.Asynchronous`, asserts `IsAsync` and reads asynchronously, with a second test for the synchronous case. The documentation still reached for `UnmanagedFileLoader` further down the page, which this change removes; the example now opens the handle through the real file system. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
c865c2c to
a94fae0
Compare
The only remaining difference was whitespace before `/>`, left over from switching these references to projects and back. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…dle` `FileModeHelper.GetFileContainer` now holds what both did twice: a missing file for `Open`/`Truncate`, creating it otherwise, a directory at the path, `CreateNew` on an existing file and a read-only file opened for writing. `FileStreamMock` calls `FileModeHelper` directly and loses the forwarding method. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Handle lifetime - Closed handles are no longer released from the `MockFileSystem.Storage` getter. `InMemoryStorage` calls `ReleaseClosedHandles()` explicitly where a closed handle becomes observable: `GetContainer`, `EnumerateLocations` and `TryGetFileAccess`. - The sweep no longer throws. A delete-on-close that fails (the parent is gone, a directory took the name) is ignored, as the OS ignores it. - On Windows a pending delete waits until nothing holds the file, streams included, instead of being dropped when a stream still has it open. - The Windows branch no longer follows a renamed file: moving a file a handle holds open is not possible in the mock on Windows yet (Testably#1086), so that behaviour could not be observed. `IStorage.GetLocation( IStorageContainer)` goes with it. - The registry holds handles weakly, so one that is dropped without `Dispose()` releases its share lock once it is collected, as a real handle does when it is finalized. `OpenHandle` - Arguments are validated in the order of the runtime's `FileStreamHelpers.ValidateArguments`: path, then the enum ranges, options, `preallocationSize`, the mode/access combination, and finally preallocation on a non-creating mode or without write access. - `Map` and the handle lookup guard against `null`, and a closed foreign handle is rejected on every path. `RandomAccessMock` - Writes go through `IStorageContainer.WriteRange`, so an open `FileStreamMock` gets a range update and a vetoing interception sees the content unchanged. - An offset beyond what the content can hold throws `IOException` instead of overflowing. - `FileMode` is no longer threaded through; `GetContainerForResize` is behind `FEATURE_RANDOMACCESS_FLUSHTODISK`; the buffers are recorded in the statistics like the rest of the mock records them. Also: `Entry` is a record, `IsKnown` and the redundant `NullContainer` check are gone, `ExceptionFactory` additions are in alphabetical order, the redundant `AllHandleOverloads_ShouldAgreeWithThePathOverloads` test is removed and the skip reasons point at Testably#1086 again. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
`IRandomAccess` joins the data-driven list in `StatisticsTests`, with a registration test for every member. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Windows no longer claims that a delete-on-close follows a renamed file, and the page says when a closed or dropped handle is noticed. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Merged the updated #1082 into this branch (no rebase, so the history here is unchanged). Nothing in this PR's own changes needed adjusting. In Release on net10.0, merged with main: |
The `(T1, ReadOnlySpan<T2>, T3)` registration overload was compiled only with `FEATURE_FILE_SPAN` (.NET 9+), so on .NET 6 and 8 the buffers bound to the plain generic overload, which cannot take a span. It is a generic helper with nothing file-specific about it, so it now needs only `FEATURE_SPAN`, like its counterpart in the test helpers. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>


Summary
The follow-up you asked for on #1081: now that a
SafeFileHandlecan be obtained from the abstraction, test the surface that takes one and retire the scaffolding that existed because it could not be.Stacked on #1081 and #1082 — the diff shows those branches too, and shrinks to this one commit once they merge.
What was there
Tests/.../FileStreamFactory/SafeFileHandleTests.csandFileStreamFactoryMockTests.SafeFileHandle.cswere compiled out behindEXECUTE_SAFEFILEHANDLE_TESTS, which is defined nowhere in the repository. They neededUnmanagedFileLoader, a P/Invoke helper calling Win32CreateFile, which is also why they could never run anywhere but Windows.[ExcludeFromCodeCoverage(Justification = "SafeFileHandle cannot be unit tested.")].What this changes
IFilehandle members, the three realIFileStreamFactory.Newoverloads and the three mocked ones, all of which feat: add mock support forOpenHandleandIRandomAccess#1082 covers against both file systems. The twoISafeFileHandleStrategyimplementations keep theirs: nothing exercises a handle originating outside the mock, so removing those would claim coverage that does not exist.UnmanagedFileLoadercopies are removed, superseded by tests that obtain a handle from the abstraction, need no platform invocation and run on every platform.ISafeFileHandleStrategy. It now leads withIFile.OpenHandle, notes that a mock handle is not a real OS handle, and keeps the strategy where it is still needed: handles that come from somewhere else.Deleting tests rather than converting them is the one judgement call here. #1082 already covers the same three overloads against both file systems, so converting would have duplicated it — but if you would rather keep those files and port them, say so and I will.
Verification
Run in Release, which is what puts the real file system into the test matrix:
Testably.Abstractions.Tests12575 on net8.0 and 13084 on net10.0,Testing.Tests1313,MemoryMappedFiles.Tests665, parity 26, API 30. All pass, and the solution builds across all six target frameworks with no new warnings.Coverage will shift where the exclusions were lifted; that is the point, but it is worth a look at the Codacy delta before merging.
Happy to rework or reshape any of this — just say what you would prefer.