feat: add mock support for OpenHandle and IRandomAccess - #1082
Conversation
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
One or more issues must be addressed before approval.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
Open (3)
What changed in this PR
Adds mock support for IFile.OpenHandle and IRandomAccess, including synthetic handle tracking, sharing, offsets, disposal, and parity coverage.
Changes:
- Adds feature-gated interfaces and real/mock implementations.
- Adds handle lifecycle, statistics, and
DeleteOnClosesupport. - Adds tests, parity checks, and API baselines.
| File | Description |
|---|---|
| Tests/Testably.Abstractions.Tests/Testably.Abstractions.Tests.csproj | Updated as part of this pull request. |
| Tests/Testably.Abstractions.Tests/FileSystem/RandomAccess/WriteTests.cs | Updated as part of this pull request. |
| Tests/Testably.Abstractions.Tests/FileSystem/RandomAccess/Tests.cs | Updated as part of this pull request. |
| Tests/Testably.Abstractions.Tests/FileSystem/RandomAccess/ReadTests.cs | Updated as part of this pull request. |
| Tests/Testably.Abstractions.Tests/FileSystem/RandomAccess/LengthTests.cs | Updated as part of this pull request. |
| Tests/Testably.Abstractions.Tests/FileSystem/RandomAccess/HandleIdentityTests.cs | Updated as part of this pull request. |
| Tests/Testably.Abstractions.Tests/FileSystem/RandomAccess/AppendTests.cs | Updated as part of this pull request. |
| Tests/Testably.Abstractions.Tests/FileSystem/FileStreamFactory/OpenHandleStreamTests.cs | Updated as part of this pull request. |
| Tests/Testably.Abstractions.Tests/FileSystem/File/SafeFileHandleTests.cs | Updated as part of this pull request. |
| Tests/Testably.Abstractions.Tests/FileSystem/File/OpenHandleTests.cs | Updated as part of this pull request. |
| Tests/Testably.Abstractions.Testing.Tests/Testably.Abstractions.Testing.Tests.csproj | Updated as part of this pull request. |
| Tests/Testably.Abstractions.Testing.Tests/Statistics/FileSystem/FileStatisticsTests.cs | Updated as part of this pull request. |
| Tests/Testably.Abstractions.Parity.Tests/TestHelpers/Parity.cs | Updated as part of this pull request. |
| Tests/Testably.Abstractions.Parity.Tests/ParityTests.cs | Updated as part of this pull request. |
| Tests/Testably.Abstractions.Parity.Tests/Net9ParityTests.cs | Updated as part of this pull request. |
| Tests/Testably.Abstractions.Parity.Tests/Net8ParityTests.cs | Updated as part of this pull request. |
| Tests/Testably.Abstractions.Parity.Tests/Net10ParityTests.cs | Updated as part of this pull request. |
| Tests/Helpers/Testably.Abstractions.TestHelpers/Testably.Abstractions.TestHelpers.csproj | Updated as part of this pull request. |
| Tests/Api/Testably.Abstractions.Core.Api.Tests/Expected/Testably.Abstractions.FileSystem.Interface_net9.0.txt | Updated as part of this pull request. |
| Tests/Api/Testably.Abstractions.Core.Api.Tests/Expected/Testably.Abstractions.FileSystem.Interface_net8.0.txt | Updated as part of this pull request. |
| Tests/Api/Testably.Abstractions.Core.Api.Tests/Expected/Testably.Abstractions.FileSystem.Interface_net6.0.txt | Updated as part of this pull request. |
| Tests/Api/Testably.Abstractions.Core.Api.Tests/Expected/Testably.Abstractions.FileSystem.Interface_net10.0.txt | Updated as part of this pull request. |
| Tests/Api/Testably.Abstractions.Api.Tests/Expected/Testably.Abstractions.Testing_net9.0.txt | Updated as part of this pull request. |
| Tests/Api/Testably.Abstractions.Api.Tests/Expected/Testably.Abstractions.Testing_net8.0.txt | Updated as part of this pull request. |
| Tests/Api/Testably.Abstractions.Api.Tests/Expected/Testably.Abstractions.Testing_net6.0.txt | Updated as part of this pull request. |
| Tests/Api/Testably.Abstractions.Api.Tests/Expected/Testably.Abstractions.Testing_net10.0.txt | Updated as part of this pull request. |
| Tests/Api/Testably.Abstractions.Api.Tests/Expected/Testably.Abstractions_net9.0.txt | Updated as part of this pull request. |
| Tests/Api/Testably.Abstractions.Api.Tests/Expected/Testably.Abstractions_net8.0.txt | Updated as part of this pull request. |
| Tests/Api/Testably.Abstractions.Api.Tests/Expected/Testably.Abstractions_net6.0.txt | Updated as part of this pull request. |
| Tests/Api/Testably.Abstractions.Api.Tests/Expected/Testably.Abstractions_net10.0.txt | Updated as part of this pull request. |
| Source/Testably.Abstractions/RealFileSystem.cs | Updated as part of this pull request. |
| Source/Testably.Abstractions/FileSystem/RandomAccessWrapper.cs | Updated as part of this pull request. |
| Source/Testably.Abstractions/FileSystem/FileWrapper.cs | Updated as part of this pull request. |
| Source/Testably.Abstractions.Testing/Testably.Abstractions.Testing.csproj | Updated as part of this pull request. |
| Source/Testably.Abstractions.Testing/Statistics/IFileSystemStatistics.cs | Updated as part of this pull request. |
| Source/Testably.Abstractions.Testing/Statistics/FileSystemStatistics.cs | Updated as part of this pull request. |
| Source/Testably.Abstractions.Testing/MockFileSystem.cs | Updated as part of this pull request. |
| Source/Testably.Abstractions.Testing/Helpers/FileModeHelper.cs | Updated as part of this pull request. |
| Source/Testably.Abstractions.Testing/Helpers/ExceptionFactory.cs | Updated as part of this pull request. |
| Source/Testably.Abstractions.Testing/FileSystem/RandomAccessMock.cs | Updated as part of this pull request. |
| Source/Testably.Abstractions.Testing/FileSystem/MockSafeFileHandleRegistry.cs | Updated as part of this pull request. |
| Source/Testably.Abstractions.Testing/FileSystem/FileStreamMock.cs | Updated as part of this pull request. |
| Source/Testably.Abstractions.Testing/FileSystem/FileStreamFactoryMock.cs | Updated as part of this pull request. |
| Source/Testably.Abstractions.Testing/FileSystem/FileMock.cs | Updated as part of this pull request. |
| Source/Testably.Abstractions.FileSystem.Interface/IRandomAccess.cs | Updated as part of this pull request. |
| Source/Testably.Abstractions.FileSystem.Interface/IFileSystem.cs | Updated as part of this pull request. |
| Source/Testably.Abstractions.FileSystem.Interface/IFile.cs | Updated as part of this pull request. |
| Feature.Flags.props | Updated as part of this pull request. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
Pushed fixes for all three findings. Concurrent writes could lose each other — real, and my own notes had called for a per-file gate that I then never implemented. Read-modify-write sequences are now serialised per file,
I also fixed the two findings Copilot raised on #1083 that are really about code introduced here: a disposed or null handle from outside the mock now throws before reaching While fixing the last of these I found a bug in my own first attempt: deferring the deletion to the last handle silently dropped the intent when another handle was open, so the file was never deleted at all. It is now held as a pending deletion and applied when the last handle closes. Two things raised separately, per your note on #1081: #1084 / #1085 for 10502 tests pass; |
9b4bf6d to
6556d04
Compare
|
@Mpdreamz : The first PR is merged and released as 10.4.0-pre.1. You should now be able to rebase this branch onto main, update the package versions and reset the build scope in Build.cs back to "Default". |
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>
6556d04 to
40d6e6f
Compare
|
Thanks for the release. All three steps done:
Locally: One follow-on. I said on this PR that once #1085 landed the three tests skipping the Windows-simulating variant should stop skipping. I tried it, and it was only half right — deletion is fixed, but two other things still block them, so I have kept the skips with precise reasons instead of a guess:
Once #1091 is in, the #1083 is rebased on top of this one and is green too. |
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>
08e00be to
81eb0c6
Compare
|
Swept the code comments here after your note on #1091, so this is a pure comment change — no behaviour touched, and the suites are unchanged. Roughly 80 lines gone:
Sixteen lines remain, all explaining something the code cannot: why synthetic handle values start at
|
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>
|
Your CI caught two real divergences, and finding out why my local runs missed them is the more useful half of this. Why I missed them. #if DEBUG
if (Settings.RealFileSystemTests == Settings.TestSettingStatus.AlwaysEnabled)
#else
if (Settings.RealFileSystemTests != Settings.TestSettingStatus.AlwaysDisabled)
#endifI had been running Debug throughout, so every "passes against both the real and the mocked file system" I have written on these PRs was, locally, the mock only. Running
That is worth flagging because it reverses a Copilot finding on this PR, which asked me to make the deletion follow the container rather than the path. That is right on Windows and wrong on Unix, and I implemented it without checking the real file system — the mock's original path-based behaviour was closer to correct than my "fix". The three tests now assert the Unix semantics directly.
Release, on this machine, all green: #1083 is rebased on top and verified the same way. |
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>
|
Fixed — one test was behind both failures, and it was mine.
That is the second time on this PR that a platform difference I took from documentation rather than measurement turned out not to exist, after the The Windows job on #1083 was collateral: it was cancelled by fail-fast when ubuntu failed, not a failure of its own — the run's job list shows only ubuntu failing. Release, on macOS: |
vbreuss
left a comment
There was a problem hiding this comment.
Thanks @Mpdreamz for this PR. I had a look myself and also let Claude review it locally.
With this change, the documentation is no longer up to date:
1. safe-file-handles.mdx now contradicts the feature
"Because the mock has no kernel handle, you have to tell it how to translate handles into mock paths."
"install a custom strategy as soon as your code under test reaches forSafeFileHandle"
A handle obtained from File.OpenHandle needs no strategy at all now - that is the headline improvement in this branch, and the page says the opposite.
2. the statistics table is missing RandomAccess
The table is introduced as "one property per IFileSystem sub-property" and now has a hole.
3. there is no RandomAccess documentation page
Would you add/fix documentation as part of this PR or do you prefer a follow-up?
- `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>
|
All three, in this PR — the documentation belongs with the feature, and you are right that it currently contradicts it. 1. It also records two things this branch settled against the real file system, both of which would otherwise be surprises: a handle resolves to the file it was opened on, so reads and writes survive a rename and do not follow the name; and 2. Statistics table. 3. A I moved the Two notes on what I did not invent: the Release on macOS: |
@Mpdreamz : You only reacted to the review summary comment, but not on the review comments themselves. Did you overlook them? |
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>
|
@vbreuss You're right, I had overlooked them, sorry. Every inline comment has an answer now, and the fixes are in five new commits on top of what you already reviewed (no rebase, so the earlier history is unchanged):
Verified in Release on net10.0 (macOS): |
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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>
|
This pull request does not currently match the merge queue conditions, so it cannot be queued from here. The box comes back if it matches again. |
|
Thanks, @Mpdreamz, this is a great addition to the library 🥳 |
|
This is addressed in release v7.1.0. |


Summary
Step 3 of the split: mock support for the
IFile.OpenHandleandIRandomAccessabstractions added in #1081.Stacked on #1081 — the diff shows that branch's commit too, and shrinks to this one commit once #1081 merges. It will need rebasing onto the released
Testably.Abstractions.Interfaceonce you have cut the pre-release; theProjectReferenceswitch in here is the placeholder for that and should be reverted to aPackageReferenceat the version bump.How the mock handles a sealed
SafeFileHandleSafeFileHandleis sealed and wraps an OS handle, so the mock cannot produce one a real syscall would accept. It hands out a handle whose value is synthetic and remembers which file that value stands for — the same indirectionISafeFileHandleStrategyalready provides for handles created elsewhere, in the other direction.MockSafeFileHandleRegistryresolves handles it created and falls back to the configuredISafeFileHandleStrategyfor everything else, so existing strategy-based code is unaffected. Handle values start at0x4000_0000, far above any plausible file descriptor, so a mock handle mistakenly passed to a real syscall fails with an invalid-handle error rather than addressing an unrelated file.Behaviour, each covered by a test that runs against both the real and the mocked file system:
FileSystemStreamon the same path observe the same contentFileSharebookkeeping as streamsSetLengthObjectDisposedException; its share lock is released;FileOptions.DeleteOnClosedeletes once the last handle to the file closesUnauthorizedAccessException;GetLengthworks on either, being metadataFlushToDiskStatisticsBecause a sealed
SafeFileHandlegives no disposal callback, closure is noticed on the next access toMockFileSystem.Storage, which every file system operation goes through. When no handle is open that is a singlevolatile boolread.Addressing the review on #1081
Handles resolved by path rather than by opened identity — correct, and fixed. I measured it against the BCL first: open a handle,
File.Movethe file, read → the original content still comes back. The registry now stores the container the handle was opened on and resolves through that; only foreign handles, which carry nothing but a path, are looked up by name.HandleIdentityTestscovers rename and the case where another file later takes the old path.FileMode.Appendhandles overwrite instead of appending — the premise does not hold everywhere, so this is fixed only where it is real. Measured on macOS:File.OpenHandle(path, FileMode.Append, FileAccess.Write)thenRandomAccess.Write(handle, [9], 0)over[1,2,3,4]yields[9,2,3,4]— the offset is honoured, which is what the mock already did. Linux is the exception: itspwrite(2)appends when the descriptor carriesO_APPEND, whatever offset is passed, contrary to POSIX. The mock now reproduces that underExecute.IsLinux, andAppendTestsasserts both behaviours, so your Linux CI is the arbiter if I have this the wrong way round.Every closed handle value retained indefinitely — fixed, and the storage is gone rather than capped. Values are issued sequentially and never reused, so a value inside the issued range that is no longer registered is a closed handle; the set it needed has been deleted.
DeleteOnCloseremoves shared storage before the last handle closes — fixed. A deletion whose file still has open handles stays pending and is applied when the last one closes.DeleteOnCloseomitted from delete-access bookkeeping — not applied, and I think the suggestion is wrong here.FileHandle.GrantAccesstreatsdeleteAccessas "this is a delete operation", requiring every other handle to have been opened with exactlyFileShare.Deleteon Windows. Passing it for an ordinary open regressed sharing;DeleteOnCloseis a property of the close, not of the open. Happy to be corrected if it was meant differently.Two things I did not fix, both pre-existing
InMemoryStorage._fileHandles, so they do not move with a renamed file and a handle permitting delete sharing does not let the simulated Windows file system rename or delete the file underneath it. The three affected tests skip the Windows-simulating variant and say why. Real Windows allows this, so it is a genuine (small) divergence — worth its own issue rather than a drive-by change in here.FileStreamFactoryMockre-opens the path behind a handle, whereas a realFileStreamadopts the handle it is given. A handle therefore has to permitFileShare.ReadWritebefore a mocked stream can be layered on it, whichOpenHandleStreamTestsnotes.Extensions reaching the registration
You asked whether extension packages could get at the handle registration, with
MemoryMappedFilein mind. Nothing public is exposed yet —MockSafeFileHandleRegistryisinternal, and I would rather agree the shape with you than guess. The natural options are a public resolution method onMockFileSystem, or promoting the registry itself. Say which you prefer and I will add it here or in a follow-up.Verification
Run in Release, which is what puts the real file system into the test matrix, on macOS with the .NET 8, 9 and 10 SDKs:
Testably.Abstractions.Tests12575 on net8.0, 13054 on net9.0 and 13084 on net10.0;Testing.Tests1313;MemoryMappedFiles.Tests665; parity 26; API 30. All pass.Earlier revisions of this description reported Debug numbers, which silently leave the real file system out of the matrix — that is how the
DeleteOnCloseandSetLengthdivergences your CI found got past me.Happy to rework or reshape any of this — just say what you would prefer.