Skip to content

Tests: Refactored to es6#9700

Closed
itsmed wants to merge 6 commits intonodejs:masterfrom
itsmed:strengthen-tests
Closed

Tests: Refactored to es6#9700
itsmed wants to merge 6 commits intonodejs:masterfrom
itsmed:strengthen-tests

Conversation

@itsmed
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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.

Comment thread test/parallel/test-fs-empty-readStream.js Outdated
Copy link
Copy Markdown
Contributor

@cjihrig cjihrig Nov 20, 2016

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 }.

Comment thread test/parallel/test-fs-empty-readStream.js Outdated
Copy link
Copy Markdown
Contributor

@cjihrig cjihrig Nov 20, 2016

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
Copy Markdown
Contributor Author

itsmed commented Nov 20, 2016

Ok, thanks. Changes made.

cjihrig
cjihrig approved these changes Nov 23, 2016
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
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...

Comment thread test/parallel/test-fs-empty-readStream.js Outdated
Copy link
Copy Markdown
Member

@jasnell jasnell Dec 5, 2016

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()

Comment thread test/parallel/test-fs-empty-readStream.js Outdated
Copy link
Copy Markdown
Member

@jasnell jasnell Dec 5, 2016

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
Copy Markdown
Contributor Author

itsmed commented Dec 19, 2016

Ok I think I've got it this time!

@italoacasas
Copy link
Copy Markdown

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
Copy Markdown
Contributor Author

itsmed commented Jan 18, 2017

@italoacasas rebase done.

@italoacasas
Copy link
Copy Markdown

italoacasas commented Jan 18, 2017 •
edited
Loading

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

@joyeecheung
Copy link
Copy Markdown
Member

joyeecheung commented Feb 6, 2017

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
Copy Markdown
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Italo A. Casas <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Italo A. Casas <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
@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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Italo A. Casas <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Italo A. Casas <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
@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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Italo A. Casas <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
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

+1 more reviewer

@italoacasas italoacasas italoacasas approved these changes

Reviewers whose approvals may not affect merge requirements

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.

9 participants

@itsmed @mscdex @italoacasas @joyeecheung @fhinkel @jasnell @cjihrig @MylesBorins @nodejs-github-bot