test: don't need to remove nonexistent directories(test-fs-mkdir.js) - #17119
test: don't need to remove nonexistent directories(test-fs-mkdir.js)#17119ah-yu wants to merge 2 commits into
Conversation
Sorry, something went wrong.
the TempDirection is empty , so tempdir/test1 and tempdir/test2 are nonexistent. we dont need to delete nonexistent directories.
|
Those statements are probably there in case this test fails an assertion and a directory gets left behind as a result. I would prefer to keep those statements there. |
Sorry, something went wrong.
|
@mscdex Hi, thanks for review. unlink(pathname) is invoked before fs.mkdir(), so these statement can't guarantee there is no directory left. process.on('exit', function() unlink(pathname);}); these code will delete directories that made in this test whether the test passes or fails |
Sorry, something went wrong.
common.refreshTmpDir() should guarantee that the tmp directory is empty before any of the test cases run. Each test case uses a unique file name so these unlink() statements should be safe to remove. It's conceivable that future changes to the test will re-use pathnames. I'm not sure it's worth guarding against though. There is certainly no need to clean up the directories after the fact. @buji1993 Can you remove the process.on('exit',...) stuff and the unlink(pathname) on line 69 in your PR (line 73 in the original)? |
Sorry, something went wrong.
|
@Trott yeah, I will remove that. Thanks for review |
Sorry, something went wrong.
Trott
left a comment
There was a problem hiding this comment.
LGTM if CI passes
Sorry, something went wrong.
Sorry, something went wrong.
the TempDirection is empty , so tempdir/test1 and tempdir/test2 are nonexistent. we dont need to delete nonexistent directories. PR-URL: #17119 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
PR-URL: #17119 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Sorry, something went wrong.
the TempDirection is empty , so tempdir/test1 and tempdir/test2 are nonexistent. we dont need to delete nonexistent directories. PR-URL: #17119 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
PR-URL: #17119 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
the TempDirection is empty , so tempdir/test1 and tempdir/test2 are nonexistent. we dont need to delete nonexistent directories. PR-URL: #17119 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
PR-URL: #17119 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
the TempDirection is empty , so tempdir/test1 and tempdir/test2 are nonexistent. we dont need to delete nonexistent directories. PR-URL: #17119 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
PR-URL: #17119 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
the TempDirection is empty , so tempdir/test1 and tempdir/test2 are nonexistent. we dont need to delete nonexistent directories. PR-URL: #17119 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
PR-URL: #17119 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
the TempDirection is empty , so tempdir/test1 and tempdir/test2 are nonexistent. we dont need to delete nonexistent directories. PR-URL: #17119 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
PR-URL: #17119 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
the tempdir is empty , so tempdir/test1 and tempdir/test2 are nonexistent. we don't need to delete nonexistent directories.
Checklist
Affected core subsystem(s)
test