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

Tests: Refactored to es6 #9700

Closed
wants to merge 6 commits into from
Closed

Tests: Refactored to es6 #9700

wants to merge 6 commits into from

Conversation

itsmed
Copy link
Contributor

@itsmed itsmed commented Nov 19, 2016

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

Tests

Description of change

Changed var to const, assert.equal to assert.strictEqual. Changed anonymous functions to use arrow functions.

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Nov 19, 2016
@mscdex mscdex added the fs Issues and PRs related to the fs subsystem / file system. label Nov 19, 2016
@mscdex
Copy link
Contributor

mscdex commented Nov 19, 2016

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

cjihrig
cjihrig requested changes Nov 20, 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.

Can you update the commit message according to https://github.com/nodejs/node/blob/master/CONTRIBUTING.md#step-3-commit.

test/parallel/test-fs-empty-readStream.js Outdated
assert.ifError(error);

var read = fs.createReadStream(emptyFile, { 'fd': fd });
const read = fs.createReadStream(emptyFile, { 'fd': fd });
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you change { 'fd': fd } to just { fd }.

test/parallel/test-fs-empty-readStream.js Outdated

read.once('data', function() {
read.once('data', () => {
throw new Error('data event should not emit');
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you replace the throws with common.fail().

@itsmed
Copy link
Contributor Author

itsmed commented Nov 20, 2016

Ok, thanks. Changes made.

cjihrig
cjihrig approved these changes Nov 23, 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.

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

jasnell
jasnell requested changes Dec 5, 2016
View reviewed changes
Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

Almost there...

test/parallel/test-fs-empty-readStream.js Outdated
});

read.once('end', common.mustCall(function endEvent1() {}));
});

fs.open(emptyFile, 'r', function(error, fd) {
fs.open(emptyFile, 'r', (error, fd) => {
Copy link
Member

Choose a reason for hiding this comment

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

The callback here should be wrapped in a common.mustCall()

test/parallel/test-fs-empty-readStream.js Outdated

fs.open(emptyFile, 'r', function(error, fd) {
fs.open(emptyFile, 'r', (error, fd) => {
Copy link
Member

Choose a reason for hiding this comment

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

The callback should be wrapped in a common.mustCall()

@itsmed
Copy link
Contributor Author

itsmed commented Dec 19, 2016

Ok I think I've got it this time!

@italoacasas
Copy link
Contributor

italoacasas commented Jan 16, 2017 •
edited
Loading

ping @jasnell

@itsmed can you rebase please

itsmed and others added 6 commits January 18, 2017 11:59
@itsmed
Refactored to es6
c701c13 
Changed var to const, assert.equal to assert.strictEqual
@itsmed
test: es6 update, replace throw with common.fail
54ccfa2 
Replaced anonymous functions with arrow functions.
Replaced throw new Error with common.fail.
@itsmed
test: es6 update, replace throw with common.fail
9464b1d 
Replaced anonymous functions with arrow functions.
Replaced throw new Error with common.fail.
@itsmed
Refactored to es6
b2048bb 
Changed var to const, assert.equal to assert.strictEqual
@itsmed
test: es6 update, replace throw with common.fail
9baa222 
Replaced anonymous functions with arrow functions.
Replaced throw new Error with common.fail.
@itsmed
wrapped callbacks in common.mustCall
6b3e7ef
@itsmed
Copy link
Contributor Author

itsmed commented Jan 18, 2017

@italoacasas rebase done.

@italoacasas
Copy link
Contributor

italoacasas commented Jan 18, 2017 •
edited
Loading

https://ci.nodejs.org/job/node-test-pull-request/5965/console

@joyeecheung
Copy link
Member

The previous CI job is gone. New CI: https://ci.nodejs.org/job/node-test-pull-request/6230/

@italoacasas italoacasas added v7.x and removed v7.x labels Feb 6, 2017
@jasnell jasnell added the stalled Issues and PRs that are stalled. label Mar 24, 2017
@fhinkel
Copy link
Member

fhinkel commented Mar 26, 2017

@itsmed thanks for putting this together. Looks like we're missing a ) in line 61. I went ahead and fixed that in the PR, also squashed the commits into one. Landed in d13bd4a

@fhinkel fhinkel closed this Mar 26, 2017
fhinkel pushed a commit that referenced this pull request Mar 26, 2017
@itsmed @fhinkel
test: replace throw with common.fail
d13bd4a 
Replace anonymous functions with arrow functions.
Replace throw new Error with common.fail.

PR-URL: #9700
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Italo A. Casas <me@italoacasas.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
MylesBorins pushed a commit that referenced this pull request Mar 28, 2017
@itsmed @MylesBorins
test: replace throw with common.fail
d112aad 
Replace anonymous functions with arrow functions.
Replace throw new Error with common.fail.

PR-URL: #9700
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Italo A. Casas <me@italoacasas.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Mar 28, 2017
Merged
@italoacasas italoacasas mentioned this pull request Apr 10, 2017
Merged
2 tasks
MylesBorins pushed a commit that referenced this pull request Apr 18, 2017
@itsmed @MylesBorins
test: replace throw with common.fail
d76d0c8 
Replace anonymous functions with arrow functions.
Replace throw new Error with common.fail.

PR-URL: #9700
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Italo A. Casas <me@italoacasas.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
MylesBorins pushed a commit that referenced this pull request Apr 19, 2017
@itsmed @MylesBorins
test: replace throw with common.fail
cddc32c 
Replace anonymous functions with arrow functions.
Replace throw new Error with common.fail.

PR-URL: #9700
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Italo A. Casas <me@italoacasas.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Apr 19, 2017
Merged
andrew749 pushed a commit to michielbaird/node that referenced this pull request Jul 19, 2017
@itsmed @andrew749
test: replace throw with common.fail
83d8dd3 
Replace anonymous functions with arrow functions.
Replace throw new Error with common.fail.

PR-URL: nodejs/node#9700
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Italo A. Casas <me@italoacasas.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fhinkel fhinkel fhinkel approved these changes

@jasnell jasnell jasnell approved these changes

@cjihrig cjihrig cjihrig approved these changes

@italoacasas italoacasas italoacasas approved these changes

Assignees
No one assigned
Labels
fs Issues and PRs related to the fs subsystem / file system. stalled Issues and PRs that are stalled. test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
9 participants
@itsmed @mscdex @italoacasas @joyeecheung @fhinkel @jasnell @cjihrig @MylesBorins @nodejs-github-bot