Skip to content
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.

Sign up for GitHub

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

Jump to bottom

test: don't need to remove nonexistent directories(test-fs-mkdir.js) #17119

Closed
wants to merge 2 commits into from
Closed

test: don't need to remove nonexistent directories(test-fs-mkdir.js) #17119

wants to merge 2 commits into from

Conversation

ah-yu
Copy link
Contributor

@ah-yu ah-yu commented Nov 18, 2017

the tempdir is empty , so tempdir/test1 and tempdir/test2 are nonexistent. we don't need to delete nonexistent directories.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

test

@ah-yu
test: dont need to remove nonexistent directory(test-fs-mkdir.js)
fc2d748 
the TempDirection is empty , so tempdir/test1 and tempdir/test2 are nonexistent. we dont need to delete nonexistent directories.
@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Nov 18, 2017
@mscdex
Copy link
Contributor

mscdex commented Nov 18, 2017 •
edited
Loading

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.

@mscdex mscdex added the fs Issues and PRs related to the fs subsystem / file system. label Nov 18, 2017
@ah-yu
Copy link
Contributor Author

ah-yu commented Nov 19, 2017

@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

@Trott
Copy link
Member

Trott commented Nov 19, 2017

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.

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)?

@ah-yu
Copy link
Contributor Author

ah-yu commented Nov 20, 2017

@Trott yeah, I will remove that. Thanks for review

Trott
Trott approved these changes Nov 21, 2017
View reviewed changes
Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM if CI passes

@Trott
Copy link
Member

Trott commented Nov 21, 2017

CI: https://ci.nodejs.org/job/node-test-pull-request/11580/

@joyeecheung joyeecheung added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 21, 2017
jasnell pushed a commit that referenced this pull request Nov 22, 2017
@ah-yu @jasnell
test: dont need to remove nonexistent directory
fdbee33 
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
jasnell pushed a commit that referenced this pull request Nov 22, 2017
@ah-yu @jasnell
test: remove unlink function which is needless
b28a28a 
PR-URL: #17119
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
@jasnell
Copy link
Member

jasnell commented Nov 22, 2017

Landed in fdbee33 and b28a28a

@jasnell jasnell closed this Nov 22, 2017
@addaleax addaleax removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 28, 2017
MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
@ah-yu @MylesBorins
test: dont need to remove nonexistent directory
b6729df 
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
@ah-yu @MylesBorins
test: remove unlink function which is needless
256eaa9 
PR-URL: #17119
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
@ah-yu @MylesBorins
test: dont need to remove nonexistent directory
5c70cef 
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
@ah-yu @MylesBorins
test: remove unlink function which is needless
42a8f03 
PR-URL: #17119
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Dec 12, 2017
Merged
gibfahn pushed a commit that referenced this pull request Dec 19, 2017
@ah-yu @gibfahn
test: dont need to remove nonexistent directory
3028fa7 
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
gibfahn pushed a commit that referenced this pull request Dec 19, 2017
@ah-yu @gibfahn
test: remove unlink function which is needless
edfa62a 
PR-URL: #17119
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
MylesBorins pushed a commit that referenced this pull request Dec 19, 2017
@ah-yu @MylesBorins
test: dont need to remove nonexistent directory
7c57ab7 
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
MylesBorins pushed a commit that referenced this pull request Dec 19, 2017
@ah-yu @MylesBorins
test: remove unlink function which is needless
7cbdeef 
PR-URL: #17119
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
@gibfahn gibfahn mentioned this pull request Dec 20, 2017
Closed
gibfahn pushed a commit that referenced this pull request Dec 20, 2017
@ah-yu @gibfahn
test: dont need to remove nonexistent directory
179a4f4 
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
gibfahn pushed a commit that referenced this pull request Dec 20, 2017
@ah-yu @gibfahn
test: remove unlink function which is needless
7900f8a 
PR-URL: #17119
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
@gibfahn gibfahn mentioned this pull request Dec 20, 2017
Merged
@MylesBorins MylesBorins mentioned this pull request Dec 20, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cjihrig cjihrig cjihrig approved these changes

@Trott Trott Trott approved these changes

@jasnell jasnell jasnell approved these changes

@danbev danbev danbev approved these changes

Assignees
No one assigned
Labels
fs Issues and PRs related to the fs subsystem / file system. test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.