GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Tests: Refactored to es6 by itsmed · Pull Request #9700 · nodejs/node

Conversation

@itsmed

@itsmed itsmed commented Nov 19, 2016

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
lin7sh reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to Node.js core tests and test infrastructure. label Nov 19, 2016
@mscdex mscdex added the fs Issues and PRs related to file-system APIs and the fs module. label Nov 19, 2016
@mscdex

mscdex commented Nov 19, 2016

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
cjihrig
cjihrig requested changes Nov 20, 2016
View reviewed changes

@cjihrig cjihrig left a comment

Copy link
Copy Markdown
Contributor

There was a problem hiding this 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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread test/parallel/test-fs-empty-readStream.js Outdated

@cjihrig cjihrig Nov 20, 2016

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
lpinca reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
Comment thread test/parallel/test-fs-empty-readStream.js Outdated

@cjihrig cjihrig Nov 20, 2016

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@itsmed

itsmed commented Nov 20, 2016

Copy link
Copy Markdown
Contributor Author

Ok, thanks. Changes made.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
cjihrig
cjihrig approved these changes Nov 23, 2016
View reviewed changes

@cjihrig cjihrig left a comment

Copy link
Copy Markdown
Contributor

There was a problem hiding this 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/

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
jasnell
jasnell requested changes Dec 5, 2016
View reviewed changes

@jasnell jasnell left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Almost there...

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread test/parallel/test-fs-empty-readStream.js Outdated

@jasnell jasnell Dec 5, 2016

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread test/parallel/test-fs-empty-readStream.js Outdated

@jasnell jasnell Dec 5, 2016

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@nodejs-github-bot nodejs-github-bot added lts-watch-v6.x labels Dec 16, 2016
@itsmed

itsmed commented Dec 19, 2016

Copy link
Copy Markdown
Contributor Author

Ok I think I've got it this time!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@italoacasas

italoacasas commented Jan 16, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown

ping @jasnell

@itsmed can you rebase please
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
italoacasas
italoacasas approved these changes Jan 16, 2017
View reviewed changes
jasnell
jasnell approved these changes Jan 16, 2017
View reviewed changes
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
@nodejs-github-bot nodejs-github-bot removed lts-watch-v6.x labels Jan 18, 2017
@itsmed

itsmed commented Jan 18, 2017

Copy link
Copy Markdown
Contributor Author

@italoacasas rebase done.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@italoacasas

italoacasas commented Jan 18, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@joyeecheung

joyeecheung commented Feb 6, 2017

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@italoacasas italoacasas added v7.x and removed v7.x labels Feb 6, 2017
@jasnell jasnell added the stalled Issues and PRs manually marked as stalled and scheduled for automatic closure. label Mar 24, 2017
fhinkel
fhinkel approved these changes Mar 26, 2017
View reviewed changes
@fhinkel

fhinkel commented Mar 26, 2017

Copy link
Copy Markdown
Contributor

@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
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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
V7.8.0 proposal #12104
Merged
@italoacasas italoacasas mentioned this pull request Apr 10, 2017
v7.9.0 Release Proposal #12319
Merged
2 tasks done
@MylesBorins MylesBorins added the land-on-v6.x label Apr 18, 2017
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
v6.10.3 proposal #12498
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

@jasnell jasnell jasnell approved these changes
@cjihrig cjihrig cjihrig approved these changes
+2 more reviewers
@fhinkel fhinkel fhinkel approved these changes
@italoacasas italoacasas italoacasas approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

fs Issues and PRs related to file-system APIs and the fs module. stalled Issues and PRs manually marked as stalled and scheduled for automatic closure. test Issues and PRs related to Node.js core tests and test infrastructure.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

9 participants

@itsmed @mscdex @italoacasas @joyeecheung @fhinkel @jasnell @cjihrig @MylesBorins @nodejs-github-bot
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.