test: add file system test suite#6555
Conversation
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a reusable layered filesystem conformance suite covering paths, temporary resources, streams, handles, mutations, and adapter capabilities. Node filesystem tests now consume the shared suite and retain focused fixture and truncation-cursor checks. ChangesFilesystem adapter conformance
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested labels: 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
Comment |
| assert.isTrue(Option.isNone(yield* file.readAlloc(1))) | ||
| })) | ||
|
|
||
| // TODO: Decide whether closed-handle operations normalize OS-specific EBADF as BadResource. Node reports Unknown. |
There was a problem hiding this comment.
Decide whether closed-handle operations normalize OS-specific EBADF as BadResource.
On Node, those operations fail at the OS level with EBADF (“bad file descriptor”). The Node error mapper currently has no EBADF case, so it falls through to SystemErrorTag.Unknown.
| assert.strictEqual(yield* fs.readFileString(filePath), "foobarbaz") | ||
| })) | ||
|
|
||
| // TODO: Decide whether File.seek returns the resulting cursor, then change its public return type from void to Size. |
There was a problem hiding this comment.
Decide whether File.seek returns the resulting cursor, then change its public return type from void to Size.
readonly seek: (offset: SizeInput, from: SeekMode) => Effect.Effect<void>
| // TODO: Decide whether File.seek returns the resulting cursor, then change its public return type from void to Size. | ||
| // it.effect("should return the resulting cursor when seeking", () => {}) | ||
|
|
||
| // TODO: Decide whether File.truncate clamps the calling handle's cursor. |
There was a problem hiding this comment.
Decide whether File.truncate clamps the calling handle's cursor.
Node clamps the cursor in this case, Node-specific test proves that. The reusable suite does not yet require it because the public File.truncate contract does not say what happens to the cursor.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/platform-node-shared/test/NodeFileSystem.test.ts (1)
13-19: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winWrap these Effect test bodies with
Effect.fnUntraced.Each callback only returns
Effect.gen; passEffect.fnUntraced(function*() { ... })directly toit.effectinstead. As per coding guidelines, “In Effect TypeScript code, preferEffect.fnUntracedover functions that only returnEffect.gen.”Proposed pattern
- it.effect("...", () => - Effect.gen(function*() { + it.effect("...", Effect.fnUntraced(function*() { // assertions - })) + }))Also applies to: 21-33, 35-46
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/platform-node-shared/test/NodeFileSystem.test.ts` around lines 13 - 19, Update the affected it.effect callbacks, including the test at “should read a pre-existing host file when the fixture exists” and the tests at the referenced ranges, to pass Effect.fnUntraced(function*() { ... }) directly instead of an arrow function returning Effect.gen. Preserve each generator’s existing filesystem operations and assertions.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/platform-node-shared/test/NodeFileSystem.test.ts`:
- Around line 13-19: Update the affected it.effect callbacks, including the test
at “should read a pre-existing host file when the fixture exists” and the tests
at the referenced ranges, to pass Effect.fnUntraced(function*() { ... })
directly instead of an arrow function returning Effect.gen. Preserve each
generator’s existing filesystem operations and assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 10852250-fd39-471d-a7aa-7a8663c896cc
📒 Files selected for processing (2)
packages/effect/test/FileSystemTest.tspackages/platform-node-shared/test/NodeFileSystem.test.ts
| assert.strictEqual(yield* fs.readFileString(renameDestination), "renamed") | ||
| })) | ||
|
|
||
| // TODO: Define whether overwrite: false succeeds without changing an existing destination or fails with AlreadyExists. |
There was a problem hiding this comment.
Define whether overwrite: false succeeds without changing an existing destination or fails with AlreadyExists.
What remains undecided:
- Successful no-op <- copying to an existing destination with overwrite: false succeeds but leaves it untouched. This suits idempotent setup flows.
- AlreadyExists failure <- it reports that the requested copy did not happen. This suits callers that need to distinguish "copied" from "skipped"
| assertSystemError(error, { tag: "NotFound", method: "open", pathOrDescriptor: missing }) | ||
| })) | ||
|
|
||
| // TODO: Add a deterministic synchronization point before covering stream and sink finalization after interruption. |
There was a problem hiding this comment.
Add a deterministic synchronization point before covering stream and sink finalization after interruption.
Add a test-only synchronization hook that blocks after the derived handle is acquired? Then interrupt the stream or sink and assert that its handle finalizer runs.
| }) | ||
|
|
||
| describe("adapter capabilities", () => { | ||
| // TODO: Define the public working-directory contract. Decide separately whether the memory adapter defaults to virtual `/`. |
There was a problem hiding this comment.
Define the public working-directory contract. Decide separately whether the memory adapter defaults to virtual
/
In node, an absolute path is normally relative to the host process working directory. But for Memory filesystem there no natural process directory, so it needs a virtual default or explicit configuration.
| } | ||
| })) | ||
|
|
||
| // TODO: Define what ok, readable, and writable mean without user identity or permission state. |
There was a problem hiding this comment.
Define what ok, readable, and writable mean without user identity or permission state.
In Node, those checks can use the OS’s effective user and file permission bits, but for a memory filesystem has no natural equivalent unless it explicitly models.
| assertSystemError(error, { method: "realPath", pathOrDescriptor: first }) | ||
| })) | ||
|
|
||
| // TODO: Add positive chmod and chown conformance in a POSIX-only metadata capability suite. |
There was a problem hiding this comment.
Add positive chmod and chown conformance in a POSIX-only metadata capability suite.
should an in memory adapter intentionally omit identity/permission modeling?
| } | ||
| })) | ||
|
|
||
| // TODO: Define whether preserveTimestamps guarantees only mtime or both atime and mtime. Node preserves mtime but resets atime. |
There was a problem hiding this comment.
Define whether preserveTimestamps guarantees only mtime or both atime and mtime.
Node preserves mtime but resets atime, so baseline conformance currently checks only mtime
| assert.isTrue(matches[0].endsWith("src/index.ts")) | ||
| })) | ||
|
|
||
| // TODO: Add deterministic watcher-registration and subscription-control hooks before specifying event ordering, |
There was a problem hiding this comment.
Add deterministic watcher-registration and subscription-control hooks before specifying event ordering, normalized paths, or watcher cleanup
| // normalized paths, or watcher cleanup. | ||
| // it.effect("should emit normalized events when watching registered paths", () => {}) | ||
|
|
||
| // TODO: Decide whether a missing watch target reports `watch` or the host preflight operation. Node exposes `stat`. |
There was a problem hiding this comment.
TODO: Decide whether a missing watch target reports
watchor the host preflight operation. Node exposesstat.
Node currently performs that verification with an internal stat call. If the path is missing, the exposed PlatformError therefore says method: "stat", tag: "NotFound" even though the method was watch
lloydrichards
changed the title
b140b7e to
cf04641
Compare
cf04641 to
f1758ca
Compare
2 participants
Type
Description
I wanted to explore adding a
MemoryFileSystembut before jumping too deep into this realized that theNodeFileSystem.testwas a little thin. Ideally I would have some sort of suite that could be applied to bothNodeFileSystemandMemoryFileSystemto make sure the implementation is aligned. I notice a similar pattern with theKeyValueStorewhere you create a test suite that can accept an implementation and thus wanted to apply the same thing to FileSystem here before moving on to the implementation.The interface is pretty simple:
In the first commit I just ported what was already in
NodeFileSystem.testbut in the second I expanded and refactored the test suite to be a bit more behaviour driven, looking at what is currently implemented in Node but also usingsolto help with assumptions and expectations.In the end I ended up with 66 tests across 9 areas:
Full Test Suite
✓ @effect/platform-node-shared test/NodeFileSystem.test.ts (66 tests) 78ms ✓ FileSystem (node) (63) ✓ path operations (17) ✓ should preserve binary bytes when reading and writing files 2ms ✓ should preserve UTF-8 text when reading and writing files 1ms ✓ should list directory entries and metadata when creating nested directories 1ms ✓ should include nested files when listing directories recursively 1ms ✓ should require parent directories when creation is not recursive 1ms ✓ should keep recursive directory creation idempotent when the path is a directory 1ms ✓ should report existence when a path is created and removed 1ms ✓ should access existing paths and reject missing paths when checking access 1ms ✓ should remove non-empty directories when removal is recursive 1ms ✓ should reject non-empty directories when removal is not recursive 1ms ✓ should ignore missing paths only when removal is forced 0ms ✓ should apply truncating, appending, and exclusive flags when writing files 1ms ✓ should copy independent file and directory-tree contents when copying paths 3ms ✓ should replace an existing copy destination when overwrite is requested 1ms ✓ should move files and directories when renaming paths 1ms ✓ should truncate and zero-extend files when changing their size 1ms ✓ should normalize stable filesystem errors when path operations fail 1ms ✓ temporary resources (1) ✓ should clean up scoped temporary resources when their scope closes 2ms ✓ streaming (3) ✓ should stream bounded file ranges when offsets and chunk sizes are configured 1ms ✓ should write stream chunks when using a file sink 1ms ✓ should append stream chunks when a file sink uses the append flag 1ms ✓ file handles and open modes (18) ✓ should keep read cursors independent when a file has multiple handles 1ms ✓ should advance and seek the write cursor when writing through a file handle 10ms ✓ should append primitive writes when a handle uses the append flag 1ms ✓ should truncate and extend files when using a file handle 1ms ✓ should return partial reads and EOF when reading beyond file contents 1ms ✓ should reject operations when a file-handle scope closes 1ms ✓ supports the 'r' open flag 1ms ✓ supports the 'r+' open flag 1ms ✓ supports the 'w' open flag 1ms ✓ supports the 'wx' open flag 1ms ✓ supports the 'w+' open flag 1ms ✓ supports the 'wx+' open flag 1ms ✓ supports the 'a' open flag 1ms ✓ supports the 'ax' open flag 1ms ✓ supports the 'a+' open flag 1ms ✓ supports the 'ax+' open flag 1ms ✓ should reject traversal through a file when a child path targets it 1ms ✓ should apply open flags consistently when high-level writes target existing and missing paths 4ms ✓ open file state (5) ✓ should zero-fill gaps when sparse writes extend a file 0ms ✓ should keep open handles attached when their file is renamed 1ms ✓ should preserve both open handles when renaming over an existing destination 1ms ✓ should keep unlinked contents accessible when a file handle remains open 0ms ✓ should preserve the read cursor when appending through a handle 1ms ✓ copy and rename (4) ✓ should preserve existing destinations when source-based mutations fail 1ms ✓ should replace existing file destinations when copying or renaming files 1ms ✓ should preserve existing destination links and handles when copying file contents 1ms ✓ should preserve an existing copy destination when overwrite is false 1ms ✓ temporary resource lifecycle (2) ✓ should create uniquely named temporary resources when prefixes and suffixes are requested 1ms ✓ should clean up scoped temporary resources when their scope fails 1ms ✓ stream lifecycle (3) ✓ should stream no chunks when files are empty or offsets are beyond EOF 1ms ✓ should preserve the original path when opening a file stream fails 0ms ✓ should finalize derived stream and sink handles when their operations succeed or fail 1ms ✓ adapter capabilities (10) ✓ should normalize missing-path errors when primitive path operations fail 1ms ✓ should check readable and writable access when no virtual identity is available 1ms ✓ should share contents and metadata when creating hard links 1ms ✓ should preserve contents through remaining links and handles when hard links are removed 1ms ✓ should resolve symbolic links when targets are relative or absolute 1ms ✓ should preserve error context when symbolic links form a loop 1ms ✓ should report updated access and modification timestamps when metadata is available 1ms ✓ should preserve the modification timestamp when copying with metadata preservation 1ms ✓ should exclude matching entries when globbing from a root 4ms ✓ should preserve error context when watching a missing path 0ms ✓ FileSystem (node-specific) (3) ✓ should read a pre-existing host file when the fixture exists 0ms ✓ should keep the file-handle cursor when truncation does not shorten before it 1ms ✓ should clamp the file-handle cursor when truncation shortens before it 1ms Test Files 1 passed (1) Tests 66 passed (66) Start at 21:09:44 Duration 1.07s (transform 718ms, setup 516ms, import 394ms, tests 78ms, environment 0ms)There are actually some decisions that could still be made about more tests to add as well as what is expected from the
FileSystemwhich I've added a// TODOwhich I'll comment on in this PR to see if it makes sense to address now or raise as issues separately.I wanted to get this in before working on the
MemoryFileSystemmostly cause I think this would also be useful for other features like #6412 where they could reuse the same suite while also extracting deno-specific tests as needed. And also not to bloat the other PR with test cases.Related
Summary by CodeRabbit
Summary by CodeRabbit