| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
|
Review requested:
|
Sorry, something went wrong.
| * @enum {('readFile'|'writeFile'|'appendFile'|'stat'|'lstat'|'access'| | ||
| * 'exists'|'unlink'|'mkdir'|'rmdir'|'readdir')[]} Supported fs APIs | ||
| */ | ||
| const SUPPORTED_APIS = [ |
There was a problem hiding this comment.
what are the unsupported ones? is the plan to support them gradually?
Sorry, something went wrong.
There was a problem hiding this comment.
rename, copyFile, rm, symlinks, etc. There is a lot of other operations from fs that arent included in here. And yes, I plan to support all of them gradually, although I think the operations in this iteration looks fine for the initial scope
Sorry, something went wrong.
Codecov Report❌ Patch coverage is 86.75388% with 162 lines in your changes missing coverage. Please review.
@@ Coverage Diff @@
## main #61468 +/- ##
==========================================
+ Coverage 88.54% 89.83% +1.28%
==========================================
Files 704 672 -32
Lines 208753 204401 -4352
Branches 40280 39272 -1008
==========================================
- Hits 184847 183621 -1226
+ Misses 15907 13125 -2782
+ Partials 7999 7655 -344
... and 136 files with indirect coverage changes 🚀 New features to boost your workflow:
|
Sorry, something went wrong.
|
Thanks for the contribution! I will take a look ASAP, though I think it would be great to have a review from @nodejs/fs as well. I'm also wondering if it might make sense to break down this PR into even more granular iterations, as it's already a non-trivial change! @nodejs/test_runner any thoughts? |
Sorry, something went wrong.
|
Thanks @ljharb for the comment, really appreciate it! There was a lot of interesting stuff I learned while reviewing your questions. Some of them should already be resolved, while I'll resolve/comment on the others ASAP. @pmarchini thanks and I think that makes total sense. In issue #55902, we already exchanged some ideas about this. I think it makes sense to centralize that discussion there, what do you think? Either way, I'm happy to close this PR and work it based on the chosen scope and granularity, if necessary! |
Sorry, something went wrong.
|
The notable-change PRs with changes that should be highlighted in changelogs. label has been added by @avivkeller. Please suggest a text for the release notes if you'd like to include a more detailed summary, then proceed to update the PR description with the text or a link to the notable change suggested text comment. Otherwise, the commit will be placed in the Other Notable Changes section. |
Sorry, something went wrong.
| | Property | Type | Default | | ||
| | ------------------------------------------- | -------- | -------------------------------- | | ||
| | `dev` | `number` | `0` | | ||
| | `ino` | `number` | `0` | | ||
| | `mode` | `number` | File: `0o100644`, Dir: `0o40644` | | ||
| | `nlink` | `number` | `1` | | ||
| | `uid` | `number` | `0` | | ||
| | `gid` | `number` | `0` | | ||
| | `rdev` | `number` | `0` | | ||
| | `size` | `number` | Content length | | ||
| | `blksize` | `number` | `4096` | | ||
| | `blocks` | `number` | `ceil(size / 512)` | | ||
| | `atime`/`mtime`/`ctime`/`birthtime` | `Date` | Creation time | | ||
| | `atimeMs`/`mtimeMs`/`ctimeMs`/`birthtimeMs` | `number` | Creation time (ms) | |
There was a problem hiding this comment.
I wonder, and perhaps this is too much for a single PR, if these should be mock-able.
{
files: {
myFilePath: 'content',
myOtherFile: { atime: 123, content: 'some data' },
}
}
Sorry, something went wrong.
| const { call: FunctionCall, bind: FunctionBind } = Function.prototype; | ||
| const BufferPrototypeToString = FunctionCall.call(FunctionBind, FunctionCall, Buffer.prototype.toString); | ||
| const BufferFrom = Buffer.from; | ||
| const BufferConcat = Buffer.concat; | ||
| const BufferAlloc = Buffer.alloc; |
There was a problem hiding this comment.
Don't we have these in primordials?
Sorry, something went wrong.
| /** | ||
| * @param {string} syscall | ||
| * @param {string} filepath | ||
| * @returns {Error} | ||
| */ | ||
| function createENOENT(syscall, filepath) { | ||
| return new UVException({ | ||
| __proto__: null, | ||
| errno: UV_ENOENT, | ||
| syscall, | ||
| path: filepath, | ||
| message: 'no such file or directory', | ||
| }); | ||
| } | ||
|
|
||
| /** | ||
| * @param {string} syscall | ||
| * @param {string} filepath | ||
| * @returns {Error} | ||
| */ | ||
| function createENOTDIR(syscall, filepath) { | ||
| return new UVException({ | ||
| __proto__: null, | ||
| errno: UV_ENOTDIR, | ||
| syscall, | ||
| path: filepath, | ||
| message: 'not a directory', | ||
| }); | ||
| } | ||
|
|
||
| /** | ||
| * @param {string} syscall | ||
| * @param {string} filepath | ||
| * @returns {Error} | ||
| */ | ||
| function createENOTEMPTY(syscall, filepath) { | ||
| return new UVException({ | ||
| __proto__: null, | ||
| errno: UV_ENOTEMPTY, | ||
| syscall, | ||
| path: filepath, | ||
| message: 'directory not empty', | ||
| }); | ||
| } | ||
|
|
||
| /** | ||
| * @param {string} syscall | ||
| * @param {string} filepath | ||
| * @returns {Error} | ||
| */ | ||
| function createEEXIST(syscall, filepath) { | ||
| return new UVException({ | ||
| __proto__: null, | ||
| errno: UV_EEXIST, | ||
| syscall, | ||
| path: filepath, | ||
| message: 'file already exists', | ||
| }); | ||
| } | ||
|
|
||
| /** | ||
| * @param {string} syscall | ||
| * @param {string} filepath | ||
| * @returns {Error} | ||
| */ | ||
| function createEISDIR(syscall, filepath) { | ||
| return new UVException({ | ||
| __proto__: null, | ||
| errno: UV_EISDIR, | ||
| syscall, | ||
| path: filepath, | ||
| message: 'illegal operation on a directory', | ||
| }); | ||
| } |
There was a problem hiding this comment.
A few notes on these functions:
Perhaps I missed some usages, but these functions are only called once, do they need to be functions?
We need to ensure that these error messages line up with fs's. Can we add a test to validate that?
Sorry, something went wrong.
|
Should perhaps this PR wait on #61478, and use it as the mock implementation? |
Sorry, something went wrong.
|
I'll be closing this PR to avoid polluting the PR list, since #61478 includes the mock in its scope and, IMO, uses a more elegant approach. I'll also be watching the comments on this PR and on the original issue, in case this work can be useful in the future or help the project in any way. Thank you @ljharb and @avivkeller for taking the time to review the changes. |
Sorry, something went wrong.
|
Thank you @sozua. This happens to even collaborators :( |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
Add experimental mock.fs API to the test runner for mocking file system operations in tests. This allows tests to simulate file operations without actually reading from or writing to disk.
Note: There is an existing draft PR #59194 by @joaoGabriel55 working on this feature, though it appears to have stalled (last updated August 2025). This implementation takes a different approach based on maintainer feedback in the issue discussion:
Features
Examples
Basic usage
Isolation mode
Write operations
Async/promises
Directory operations
Next Steps
This PR provides an initial set of commonly-used fs operations. Future iterations could add file operations like rename, copyFile, rm, and truncate, as well as symlink support (symlink, readlink, link).
Refs: #55902