Skip to content

Conversation

@ah-yu
Copy link
Contributor

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

the TempDirection is empty , so tempdir/test1 and tempdir/test2 are nonexistent. we dont need to delete nonexistent directories.
@nodejs-github-botnodejs-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

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.

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

@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

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
ContributorAuthor

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

Copy link
Member

@TrottTrott 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

@joyeecheungjoyeecheung 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
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
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

Landed in fdbee33 and b28a28a

@jasnelljasnell closed this Nov 22, 2017
@addaleaxaddaleax 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
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
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
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
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]>
@MylesBorinsMylesBorins mentioned this pull request Dec 12, 2017
gibfahn pushed a commit that referenced this pull request Dec 19, 2017
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
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
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
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]>
@gibfahngibfahn mentioned this pull request Dec 20, 2017
gibfahn pushed a commit that referenced this pull request Dec 20, 2017
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
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]>
@gibfahngibfahn mentioned this pull request Dec 20, 2017
@MylesBorinsMylesBorins mentioned this pull request Dec 20, 2017
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fsIssues and PRs related to the fs subsystem / file system.testIssues and PRs related to the tests.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

11 participants

@ah-yu@mscdex@Trott@jasnell@danbev@cjihrig@MylesBorins@addaleax@joyeecheung@gibfahn@nodejs-github-bot