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: improve code in test-fs-open.js #10312

Closed
wants to merge 1 commit into from
Closed

test: improve code in test-fs-open.js #10312

wants to merge 1 commit into from

Conversation

edsadr
Copy link
Member

@edsadr edsadr commented Dec 17, 2016 •
edited
Loading

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

test

Description of change
  • use const instead of var for required modules
  • use assert.strictEqual instead of assert.equal
  • use assert.strictEqual instead of assert.ok
  • use assert.ifError

@nodejs-github-bot nodejs-github-bot added test Issues and PRs related to the tests. lts-watch-v6.x labels Dec 17, 2016
@mscdex mscdex added the fs Issues and PRs related to the fs subsystem / file system. label Dec 17, 2016
santigimeno
santigimeno reviewed Dec 17, 2016
View reviewed changes
test/parallel/test-fs-open.js
throw err;
}
fs.open(__filename, 'r', common.mustCall((err, fd) => {
assert.ifError(err);
}));
Copy link
Member

Choose a reason for hiding this comment

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

Maybe check the err.code here too? Also, remove fd?

Copy link
Member Author

Choose a reason for hiding this comment

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

@santigimeno as I see it this should work, so error should be null ... just replaced the if (error) using assert ...

Copy link
Member

Choose a reason for hiding this comment

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

@edsadr wow, I don't know what I was thinking about. Of course you're right, sorry about that.

test/parallel/test-fs-open.js
throw err;
}
fs.open(__filename, 'rs', common.mustCall((err, fd) => {
assert.ifError(err);
}));
Copy link
Member

Choose a reason for hiding this comment

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

Same thing here

Copy link
Member Author

Choose a reason for hiding this comment

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

Same as above, this shouldn't fail either

@edsadr
Copy link
Member Author

edsadr commented Dec 17, 2016

@santigimeno removed the fd, but please see my comments

lpinca
lpinca reviewed Dec 17, 2016
View reviewed changes
test/parallel/test-fs-open.js
throw err;
}
fs.open(__filename, 'rs', common.mustCall((err) => {
console.log(err);
Copy link
Member

Choose a reason for hiding this comment

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

Has this console.log been added by mistake?

cjihrig
cjihrig approved these changes Dec 17, 2016
View reviewed changes
Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

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

LGTM minus the console.log().

@edsadr
test: improve code in test-fs-open.js
6605270 
* use const and let instead of var
* use assert.strictEqual instead of assert.equal
* use assert.strictEqual instead of assert.ok
* use assert.ifError
@edsadr
Copy link
Member Author

edsadr commented Dec 17, 2016

@lpinca @cjihrig ... sorry about that... left that by mistake... just fixed it

@italoacasas
Copy link
Contributor

Landed 15c71f6

italoacasas pushed a commit that referenced this pull request Dec 20, 2016
@edsadr @italoacasas
test: improve code in test-fs-open.js
15c71f6 
* use const and let instead of var
* use assert.strictEqual instead of assert.equal
* use assert.strictEqual instead of assert.ok
* use assert.ifError

PR-URL: #10312
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Italo A. Casas <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
cjihrig pushed a commit to cjihrig/node that referenced this pull request Dec 20, 2016
@edsadr @cjihrig
test: improve code in test-fs-open.js
0075836 
* use const and let instead of var
* use assert.strictEqual instead of assert.equal
* use assert.strictEqual instead of assert.ok
* use assert.ifError

PR-URL: nodejs#10312
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Italo A. Casas <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
@italoacasas italoacasas mentioned this pull request Dec 20, 2016
Merged
cjihrig pushed a commit that referenced this pull request Dec 20, 2016
@edsadr @cjihrig
test: improve code in test-fs-open.js
4510c72 
* use const and let instead of var
* use assert.strictEqual instead of assert.equal
* use assert.strictEqual instead of assert.ok
* use assert.ifError

PR-URL: #10312
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Italo A. Casas <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jan 22, 2017
@edsadr @MylesBorins
test: improve code in test-fs-open.js
1d467f4 
* use const and let instead of var
* use assert.strictEqual instead of assert.equal
* use assert.strictEqual instead of assert.ok
* use assert.ifError

PR-URL: #10312
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Italo A. Casas <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jan 24, 2017
@edsadr @MylesBorins
test: improve code in test-fs-open.js
a836d3b 
* use const and let instead of var
* use assert.strictEqual instead of assert.equal
* use assert.strictEqual instead of assert.ok
* use assert.ifError

PR-URL: #10312
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Italo A. Casas <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Jan 24, 2017
Merged
MylesBorins pushed a commit that referenced this pull request Jan 31, 2017
@edsadr @MylesBorins
test: improve code in test-fs-open.js
8792fb1 
* use const and let instead of var
* use assert.strictEqual instead of assert.equal
* use assert.strictEqual instead of assert.ok
* use assert.ifError

PR-URL: #10312
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Italo A. Casas <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@italoacasas italoacasas italoacasas approved these changes

@cjihrig cjihrig cjihrig approved these changes

@lpinca lpinca lpinca approved these changes

@santigimeno santigimeno santigimeno 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.
8 participants
@edsadr @italoacasas @santigimeno @lpinca @cjihrig @mscdex @MylesBorins @nodejs-github-bot