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: improve code in test-fs-write-file-sync #10624

Closed
wants to merge 1 commit into from

Conversation

edsadr
Copy link
Member

@edsadr edsadr commented Jan 5, 2017

  • use let and const instead of var
  • use assert.strictEqual instead of assert.equal
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

test

@nodejs-github-bot nodejs-github-bot added dont-land-on-v7.x test Issues and PRs related to the tests. labels Jan 5, 2017
@mscdex mscdex added the fs Issues and PRs related to the fs subsystem / file system. label Jan 5, 2017

fs.writeFileSync(file1, '123', {mode: mode});

content = fs.readFileSync(file1, {encoding: 'utf8'});
assert.equal('123', content);
assert.strictEqual('123', content);
Copy link
Member

Choose a reason for hiding this comment

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

Nit: can you please swap the arguments here and throughout the rest of the file?

Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM subject to fixing nit mentioned by last reviewer

@edsadr
Copy link
Member Author

edsadr commented Jan 5, 2017

@lpinca , @mhdawson ... did the nit


assert.equal(mode, fs.statSync(file1).mode & 0o777);
assert.strictEqual(mode, fs.statSync(file1).mode & 0o777);
Copy link
Member

Choose a reason for hiding this comment

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

Nit: can you also please swap the arguments here?

* use let and const instead of var
* use assert.strictEqual instead of assert.equal
* swap assertions arguments to match the standard
@edsadr
Copy link
Member Author

edsadr commented Jan 7, 2017

@lpinca, changed those

@lpinca
Copy link
Member

lpinca commented Jan 7, 2017

@lpinca
Copy link
Member

lpinca commented Jan 7, 2017

Landed in 8a12368.

@lpinca lpinca closed this Jan 7, 2017
lpinca pushed a commit that referenced this pull request Jan 7, 2017
* use let and const instead of var
* use assert.strictEqual instead of assert.equal
* swap assertions arguments to match the standard

PR-URL: #10624
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 18, 2017
* use let and const instead of var
* use assert.strictEqual instead of assert.equal
* swap assertions arguments to match the standard

PR-URL: nodejs#10624
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 19, 2017
* use let and const instead of var
* use assert.strictEqual instead of assert.equal
* swap assertions arguments to match the standard

PR-URL: nodejs#10624
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
targos pushed a commit that referenced this pull request Jan 28, 2017
* use let and const instead of var
* use assert.strictEqual instead of assert.equal
* swap assertions arguments to match the standard

PR-URL: #10624
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@italoacasas italoacasas mentioned this pull request Jan 29, 2017
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 30, 2017
* use let and const instead of var
* use assert.strictEqual instead of assert.equal
* swap assertions arguments to match the standard

PR-URL: nodejs#10624
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 30, 2017
* use let and const instead of var
* use assert.strictEqual instead of assert.equal
* swap assertions arguments to match the standard

PR-URL: nodejs#10624
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@jasnell
Copy link
Member

jasnell commented Mar 8, 2017

This will need a backport PR to land on v4

jasnell pushed a commit that referenced this pull request Mar 8, 2017
* use let and const instead of var
* use assert.strictEqual instead of assert.equal
* swap assertions arguments to match the standard

PR-URL: #10624
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
MylesBorins pushed a commit that referenced this pull request Mar 9, 2017
* use let and const instead of var
* use assert.strictEqual instead of assert.equal
* swap assertions arguments to match the standard

PR-URL: #10624
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@MylesBorins MylesBorins mentioned this pull request Mar 9, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fs Issues and PRs related to the fs subsystem / file system. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants