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.

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

test: create files and directories in the tmp directory instead of fixture directory #24077

Closed
wants to merge 1 commit into from

Conversation

saurabhSiddhu
Copy link
Contributor

@saurabhSiddhu saurabhSiddhu commented Nov 4, 2018

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Nov 4, 2018
@saurabhSiddhu saurabhSiddhu changed the title test: replacing fixture directory with temp test: create files and directories in the tmp directory instead of fixture directory Nov 4, 2018
const existingFile = fixtures.path('exit.js');
const existingFile2 = fixtures.path('a.js');
const existingDir = tmpdir.path;

Copy link
Member

Choose a reason for hiding this comment

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

Added empty line?

const nonexistentDir = path.join(tmpdir.path, 'non-existent', 'foo', 'bar');
const existingFile = path.join(tmpdir.path, 'existingFile.js');
const existingFile2 = path.join(tmpdir.path, 'existingFile2.js');
const existingDir = path.join(tmpdir.path, 'dir');
const existingDir2 = fixtures.path('keys');
Copy link
Member

Choose a reason for hiding this comment

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

Any reason you didn't move existingDir2 to something under tmpdir as well?

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.

Looks good to me, although some small comments that can optionally be addressed.

@Trott
Copy link
Member

Trott commented Nov 9, 2018

@Trott
Copy link
Member

Trott commented Nov 9, 2018

@Trott
Copy link
Member

Trott commented Nov 11, 2018

@Trott
Copy link
Member

Trott commented Nov 11, 2018

Resume Build doesn't rebase against latest master I don't think, so this is getting bitten by a flaky that's been fixed. I think. New full Ci...

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

@Trott
Copy link
Member

Trott commented Nov 11, 2018

Landed in 1e0005e.

Thanks for the contribution! 🎉

(If you're interested in other possible contributions to Node.js but don't have a good idea of where to start looking, some ideas are posted at https://www.nodetodo.org/next-steps/.)

@Trott Trott closed this Nov 11, 2018
Trott pushed a commit to Trott/io.js that referenced this pull request Nov 11, 2018
PR-URL: nodejs#24077
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
BridgeAR pushed a commit that referenced this pull request Nov 14, 2018
PR-URL: #24077
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
kiyomizumia pushed a commit to kiyomizumia/node that referenced this pull request Nov 15, 2018
PR-URL: nodejs#24077
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
codebytere pushed a commit that referenced this pull request Dec 14, 2018
PR-URL: #24077
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Dec 26, 2018
PR-URL: #24077
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@codebytere codebytere mentioned this pull request Jan 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants