-
Notifications
You must be signed in to change notification settings - Fork 29.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fs: update tests for rmdir() #29815
fs: update tests for rmdir() #29815
Conversation
Update tests to create more nested folders and files, like rimraf package tests do. Each test will create 28 nested directories and 210 files. It will also create different types of files, like symlinks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good in general. Testing with symlinks and "weird" filenames seems like an improvement. I know this code was ported from rimraf so we can probably modernize it a little bit.
CI is currently failing because of linter errors. You can check those locally by running |
@iansu Can you have a look at my last commit? I moved the first part of tests in their own function so I can test it with different folder hierarchy levels and file types. Lmk what you think so I can sth similar with the rest. |
@mpaktiti That looks pretty good to me. I'd say go ahead and do something similar with the other tests. |
@nodejs/testing @nodejs/fs @bcoe This could use some reviews. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm happy with this, I think over time we might find that we make the builder more generic, but in a pinch I think this makes it easier to setup a few more scenarios for rimraf.
function makeNonEmptyDirectory() { | ||
const dirname = `rmdir-recursive-${count}`; | ||
fs.mkdirSync(path.join(dirname, 'foo', 'bar', 'baz'), { recursive: true }); | ||
function makeNonEmptyDirectory(depth, files, folders, dirname, createSymLinks) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really like this approach of building this out into a factory, so that it's easy to create a bunch of different scenarios, thanks for doing this 🎉
Update tests to create more nested folders and files, like rimraf package tests do. Each test will create 28 nested directories and 210 files. It will also create different types of files, like symlinks. PR-URL: #29815 Reviewed-By: Ben Coe <bencoe@gmail.com>
Landed in 27507ca. Thanks for the contribution! 🎉 |
Update tests to create more nested folders and files, like rimraf package tests do. Each test will create 28 nested directories and 210 files. It will also create different types of files, like symlinks. PR-URL: #29815 Reviewed-By: Ben Coe <bencoe@gmail.com>
Update tests to create more nested folders and files, like rimraf package tests do.
Each test will create 28 nested directories and 210 files.
It will also create different types of files, like symlinks.
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes