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: refactor test-fs-readfile-unlink #15173

Closed
wants to merge 1 commit into from
Closed

test: refactor test-fs-readfile-unlink #15173

wants to merge 1 commit into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Sep 4, 2017

  • Use tmp directory instead of mutating the fixtures directory.
  • Add comment explaining that the unlinkSync() at the end of the test is
    part of the test. Otherwise it may be tempting to remove it as
    unnecessary tmp directory cleanup.
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

test fs

@Trott Trott added fs Issues and PRs related to the fs subsystem / file system. test Issues and PRs related to the tests. labels Sep 4, 2017
@vsemozhetbyt
Copy link
Contributor

vsemozhetbyt commented Sep 4, 2017 •
edited
Loading

A nit: test-fs-readline-unlink -> test-fs-readfile-unlink in the commit title.

@vsemozhetbyt vsemozhetbyt changed the title test: refactor test-fs-readline-unlink test: refactor test-fs-readfile-unlink Sep 4, 2017
@Trott
test: refactor test-fs-readfile-unlink
5ebdbc1 
* Use tmp directory instead of mutating the fixtures directory.
* Add comment explaining that the unlinkSync() at the end of the test is
  part of the test. Otherwise it may be tempting to remove it as
  unnecessary tmp directory cleanup.
@targos
Copy link
Member

targos commented Sep 6, 2017

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

jasnell pushed a commit that referenced this pull request Sep 7, 2017
@Trott @jasnell
test: refactor test-fs-readfile-unlink
7487d61 
* Use tmp directory instead of mutating the fixtures directory.
* Add comment explaining that the unlinkSync() at the end of the test is
  part of the test. Otherwise it may be tempting to remove it as
  unnecessary tmp directory cleanup.

PR-URL: #15173
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@jasnell
Copy link
Member

jasnell commented Sep 7, 2017

Landed in 7487d61

@jasnell jasnell closed this Sep 7, 2017
MylesBorins pushed a commit that referenced this pull request Sep 10, 2017
@Trott @MylesBorins
test: refactor test-fs-readfile-unlink
1b457a9 
* Use tmp directory instead of mutating the fixtures directory.
* Add comment explaining that the unlinkSync() at the end of the test is
  part of the test. Otherwise it may be tempting to remove it as
  unnecessary tmp directory cleanup.

PR-URL: #15173
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Sep 10, 2017
Merged
MylesBorins pushed a commit that referenced this pull request Sep 11, 2017
@Trott @MylesBorins
test: refactor test-fs-readfile-unlink
e8bbcef 
* Use tmp directory instead of mutating the fixtures directory.
* Add comment explaining that the unlinkSync() at the end of the test is
  part of the test. Otherwise it may be tempting to remove it as
  unnecessary tmp directory cleanup.

PR-URL: #15173
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Sep 12, 2017
@Trott @MylesBorins
test: refactor test-fs-readfile-unlink
0b9fde4 
* Use tmp directory instead of mutating the fixtures directory.
* Add comment explaining that the unlinkSync() at the end of the test is
  part of the test. Otherwise it may be tempting to remove it as
  unnecessary tmp directory cleanup.

PR-URL: #15173
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
addaleax pushed a commit to addaleax/node that referenced this pull request Sep 13, 2017
@Trott @addaleax
test: refactor test-fs-readfile-unlink
1006ac8 
* Use tmp directory instead of mutating the fixtures directory.
* Add comment explaining that the unlinkSync() at the end of the test is
  part of the test. Otherwise it may be tempting to remove it as
  unnecessary tmp directory cleanup.

PR-URL: nodejs#15173
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Sep 20, 2017
@Trott @MylesBorins
test: refactor test-fs-readfile-unlink
2ecc5ea 
* Use tmp directory instead of mutating the fixtures directory.
* Add comment explaining that the unlinkSync() at the end of the test is
  part of the test. Otherwise it may be tempting to remove it as
  unnecessary tmp directory cleanup.

PR-URL: #15173
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Sep 20, 2017
Merged
MylesBorins pushed a commit that referenced this pull request Sep 21, 2017
@Trott @MylesBorins
test: refactor test-fs-readfile-unlink
ec84335 
* Use tmp directory instead of mutating the fixtures directory.
* Add comment explaining that the unlinkSync() at the end of the test is
  part of the test. Otherwise it may be tempting to remove it as
  unnecessary tmp directory cleanup.

PR-URL: #15173
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Sep 26, 2017
@Trott @MylesBorins
test: refactor test-fs-readfile-unlink
2babae4 
* Use tmp directory instead of mutating the fixtures directory.
* Add comment explaining that the unlinkSync() at the end of the test is
  part of the test. Otherwise it may be tempting to remove it as
  unnecessary tmp directory cleanup.

PR-URL: #15173
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@Trott Trott deleted the vacay-8 branch January 13, 2022 22:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hiroppy hiroppy hiroppy approved these changes

@cjihrig cjihrig cjihrig approved these changes

@targos targos targos approved these changes

@lpinca lpinca lpinca approved these changes

@benjamingr benjamingr benjamingr 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.
10 participants
@Trott @vsemozhetbyt @targos @jasnell @danbev @benjamingr @lpinca @cjihrig @hiroppy @MylesBorins