FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

test: refactor test-stream-transform-object by Trott · Pull Request #10588 · nodejs/node · GitHub

/ node Public

test: refactor test-stream-transform-object - #10588

Closed
Trott wants to merge 1 commit into
nodejs:masterfrom
Trott:transform
Closed

test: refactor test-stream-transform-object#10588
Trott wants to merge 1 commit into
nodejs:masterfrom
Trott:transform

Conversation

Trott commented Jan 3, 2017

Copy link
Copy Markdown
Member
  • use common.mustCall() as appropriate
  • eliminate exit handler
  • var -> const/let
  • provide duration for setInterval()
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

test stream

Trott added stream Issues and PRs related to the stream subsystem. test Issues and PRs related to the tests. labels Jan 3, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Can this hardcoded number also be replaced with expect.length?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Would have to be something odd like expect.length - 2 or expect[expect.length -1] or something like that. I'm not opposed but not exactly in favor either.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

It might look like overkill, but how about mustCalling this function as well?

* use common.mustCall() as appropriate
* eliminate exit handler
* var -> const/let
* provide duration for setInterval()

Trott commented Jan 6, 2017

Copy link
Copy Markdown
Member Author

@thefourtheye Nits addressed, I think. PTAL.

Trott commented Jan 6, 2017

Copy link
Copy Markdown
Member Author

Trott added a commit to Trott/io.js that referenced this pull request Jan 6, 2017
* use common.mustCall() as appropriate
* eliminate exit handler
* var -> const/let
* provide duration for setInterval()

PR-URL: nodejs#10588
Reviewed-By: Italo A. Casas <me@italoacasas.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>

Trott commented Jan 6, 2017

Copy link
Copy Markdown
Member Author

Landed in f43ea2a

Trott closed this Jan 6, 2017
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 18, 2017
* use common.mustCall() as appropriate
* eliminate exit handler
* var -> const/let
* provide duration for setInterval()

PR-URL: nodejs#10588
Reviewed-By: Italo A. Casas <me@italoacasas.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 19, 2017
* use common.mustCall() as appropriate
* eliminate exit handler
* var -> const/let
* provide duration for setInterval()

PR-URL: nodejs#10588
Reviewed-By: Italo A. Casas <me@italoacasas.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 26, 2017
* use common.mustCall() as appropriate
* eliminate exit handler
* var -> const/let
* provide duration for setInterval()

PR-URL: nodejs#10588
Reviewed-By: Italo A. Casas <me@italoacasas.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 27, 2017
* use common.mustCall() as appropriate
* eliminate exit handler
* var -> const/let
* provide duration for setInterval()

PR-URL: nodejs#10588
Reviewed-By: Italo A. Casas <me@italoacasas.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
italoacasas mentioned this pull request Jan 29, 2017
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 30, 2017
* use common.mustCall() as appropriate
* eliminate exit handler
* var -> const/let
* provide duration for setInterval()

PR-URL: nodejs#10588
Reviewed-By: Italo A. Casas <me@italoacasas.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 30, 2017
* use common.mustCall() as appropriate
* eliminate exit handler
* var -> const/let
* provide duration for setInterval()

PR-URL: nodejs#10588
Reviewed-By: Italo A. Casas <me@italoacasas.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>

Copy link
Copy Markdown
Contributor

This does not land cleanly in LTS. Added dont-land label. Please feel free to manually backport

Trott deleted the transform branch January 13, 2022 22:45
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

stream Issues and PRs related to the stream subsystem. test Issues and PRs related to the tests.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants


Back | FazBrowse Home | New Git URL