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.

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

(v6.x backport) test: refactor test-stream2-readable-wrap.js #11797

Closed

Conversation

DavidGoussev
Copy link
Contributor

@DavidGoussev DavidGoussev commented Mar 11, 2017

test: refactor test-stream2-readable-wrap.js

implement callback wrapper common.mustCall

remove process.on('exit') with callback wrapper common.mustCall

PR-URL: #11797
Backport-of: #10551

MylesBorins and others added 30 commits March 6, 2017 17:50
new year new alias

PR-URL: nodejs#10586
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Remove the numbers from the comments to make it clear that
assert does not follow the
[CJS spec](http://wiki.commonjs.org/wiki/Unit_Testing/1.0).
Additionally, clean up the existing comments for consistent
formatting/language and ease of reading.

PR-URL: nodejs#10579
Fixes: nodejs#9063
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
PR-URL: nodejs#10446
Reviewed-By: James M Snell <jasnell@gmail.com>
* use const instead of var
* use common.mustCall to control functions execution
* use assert.strictEqual instead of assert.equal
* use arrow functions
* remove console.error

PR-URL: nodejs#10521
Reviewed-By: Prince John Wesley <princejohnwesley@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
We have had nodejs#9728
open for a while but the frequency of the failures
seems to be such that we should mark it as flaky
while we continue to investigate.

PR-URL: nodejs#10618
Reviewed-by: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Fixes: nodejs/CTC#41

PR-URL: nodejs#10604
Fixes: https://github.com/nodejs/CTC#41
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Michal Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
* Change === to == in one place
* Add explanation about another non-strict if-statement

PR-URL: nodejs#11513
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Modify the `[Writable]` and `[Readable]` links so they point
directly to the right sections in the stream.html doc

PR-URL: nodejs#11517
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Add documentation for http clientRequest.aborted.

PR-URL: nodejs#11544
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Communicate about leaked globals via `AssertionError` rather than
`console.log()`.

PR-URL: nodejs#11547
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Refs: nodejs#9399

PR-URL: nodejs#11548
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Add a `test-addons-clean` to the Makefile
to clean up files generated during testing addons.

PR-URL: nodejs#11519
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Fixing a typo in comments, the word 'remaining' had a typo.

PR-URL: nodejs#11503
Fixes: nodejs#11491
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
* Remove comment referring to the CommonJS Unit Testing 1.0 spec. This
  module is no longer intended to comply with that spec.
* Remove puzzling "THIS IS NOT TESTED NOR LIKELY TO WORK OUTSIDE V8!"
  comment. No doubt, it made sense at one time.
* Favor `===` over `==` in two places.

PR-URL: nodejs#11511
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
The intention of test case is to make sure that `timeout` property is honored
and the code in context terminates and throws correct exception. However in
test case, the code inside context would complete before `timeout` for windows
and would sometimes fail. Updated the code so it guarantee to not complete
execution until timeout is triggered.

Fixes: nodejs#11261
PR-URL: nodejs#11530
Reviewed-By: James M Snell <jasnell.gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Josh Gavant <josh.gavant@outlook.com>
The IPC channel is referenced with the message event too.

PR-URL: nodejs#11494
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
These two functions in the querystring are used as a fallback.
To test them, two test cases were added which make errors that
will be caught.

PR-URL: nodejs#11326
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Currently the maximum number of tick is duplicated in two places. This
commit introduces a constant that both can use.

PR-URL: nodejs#11199
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
PR-URL: nodejs#11473
Ref: nodejs#11199 (comment)
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
PR-URL: nodejs#11482
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
This was semi-overlooked in nodejs#10236
because it always worked and didn’t need additional changes.

Ref: nodejs#10236
PR-URL: nodejs#11486
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
PR-URL: nodejs#11377
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
If any tests leave processes running after testing results are complete,
fail the test run.

PR-URL: nodejs#11269
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
I noticed that the link to http-parser is pointing to the joyent
organization. There is a redirect to the nodejs organization but
perhaps this should be updated anyway.

PR-URL: nodejs#11477
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Interpretation of escape sequences with echo is not as consistent
across platforms as printf, so use the latter instead.

PR-URL: nodejs#11466
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
PR-URL: nodejs#11454
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
`TLSSocket` should not have a hard dependency on `tls.Server`, since it
may be running without it in cases like `STARTTLS`.

Fix: nodejs#10704
PR-URL: nodejs#10706
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Refs: nodejs#11242
PR-URL: nodejs#11313
Reviewed-By: Brian White <mscdex@mscdex.net>
Also removes extraneous argument.

PR-URL: nodejs#11413
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Fixes: nodejs#9710
PR-URL: nodejs#11136
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
mscdex and others added 9 commits March 8, 2017 16:18
Creating an object in JS and using a typed array to transfer values
from C++ to JS is faster than creating an object and setting
properties in C++.

The included benchmark shows ~34% increase in performance with this
change.

PR-URL: nodejs#11497
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
PR-URL: nodejs#11516
Reviewed-By: Jackson Tian <shyvo1987@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
PR-URL: nodejs#11522
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Formatting changes for upcoming linter update.

PR-URL: nodejs#10561
Reviewed-By: Teddy Katz <teddy.katz@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
We have been stalled on ESLint 3.8.0 for some time. Current ESLint is
3.13.0. We have been unable to upgrade because of more aggressive
reporting on some rules, including indentation.

ESLint configuration options and bugfixes are now such that we can
reasonably upgrade.

PR-URL: nodejs#10561
Reviewed-By: Teddy Katz <teddy.katz@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
ESLint `indent` rule now has options that duplicate functionality in our
custom `align-function-arguments` rule. Remove
`align-function-arguments` custom rule.

PR-URL: nodejs#10561
Reviewed-By: Teddy Katz <teddy.katz@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
This adds an anchor for v6.10.0 in the LTS column.

PR-URL: nodejs#11534
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
PR-URL: nodejs#11452
Refs: nodejs#11290
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@nodejs-github-bot nodejs-github-bot added test Issues and PRs related to the tests. v6.x labels Mar 11, 2017
@mscdex mscdex added the stream Issues and PRs related to the stream subsystem. label Mar 11, 2017
console.log('ok');
});
const objectChunks = [ 5, 'a', false, 0, '', 'xyz', { x: 4 }, 7, [], 555 ];
runTest(1, true, function() { return objectChunks.shift(); });
Copy link
Member

@gibfahn gibfahn Mar 12, 2017

Choose a reason for hiding this comment

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

Could you add a newline at the end of the file? This errors if you run make lint.

image

/Users/gib/wrk/com/node/test/parallel/test-stream2-readable-wrap.js
  71:63  error  Newline required at end of file but not found  eol-last

✖ 1 problem (1 error, 0 warnings)

@gibfahn gibfahn mentioned this pull request Mar 12, 2017
2 tasks
@DavidGoussev
Copy link
Contributor Author

test: add last line hard return

test: refactor test-stream2-readable-wrap.js

implement callback wrapper common.mustCall

remove process.on('exit') with callback wrapper common.mustCall

PR-URL: #11797
Backport-of: #10551

@jasnell
Copy link
Member

jasnell commented Mar 22, 2017

Needs a rebase on the updated v6.x-staging branch

@MylesBorins
Copy link
Contributor

@dpg5000 reminder that this needs a rebase

@DavidGoussev
Copy link
Contributor Author

sorry for the delay, everyone. I believe i've got the requested rebase down (if not, please let me know)...

@gibfahn
Copy link
Member

gibfahn commented Apr 15, 2017

@dpg5000 I'm afraid it doesn't look rebased.

image

You want the commits to be 1.

Try this:

git fetch --all
git checkout v6.x-staging
git reset --hard upstream/v6.x-staging
git checkout backport-to-10551-v6.x
git status # Make sure you're up to date
git rebase v6.x-staging

When I do this on your branch I get:

➜  node git:(acd9bc315b) ✗ ❯ g s                                                      ~/wrk/com/node
rebase in progress; onto acd9bc315b
You are currently rebasing branch 'pr-11797' on 'acd9bc315b'.
  (fix conflicts and then run "git rebase --continue")
  (use "git rebase --skip" to skip this patch)
  (use "git rebase --abort" to check out the original branch)

Unmerged paths:
  (use "git reset HEAD <file>..." to unstage)
  (use "git add <file>..." to mark resolution)

	both modified:   test/parallel/test-stream2-readable-wrap.js

no changes added to commit (use "git add" and/or "git commit -a")

Then fix the conflicts by removing the >>>>/====/<<<< markers and the old code.

vim test/parallel/test-stream2-readable-wrap.js # Or whatever editor

Then add the file and continue the rebase:

git add test/parallel/test-stream2-readable-wrap.js
git rebase --continue

You can check that it worked with:

git log --graph --decorate --oneline # Graph log.

image

You should have one commit on top of v6.x-staging. Have a look at https://github.com/gibfahn/node/tree/pr-11797, it should be about right.

MylesBorins pushed a commit that referenced this pull request Apr 15, 2017
Use common.mustCall() where appropriate, var to const/let,
assert.equal() -> assert.strictEqual(), explicit time provided to
setTimeout()

Backport-PR-URL: #11797
PR-URL: #10551
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
@MylesBorins
Copy link
Contributor

I went ahead and landed the backport commit on the head of v6.x-staging as 42d1fb1

Thanks for the work @dpg5000

@DavidGoussev
Copy link
Contributor Author

DavidGoussev commented Apr 15, 2017 via email

MylesBorins pushed a commit that referenced this pull request Apr 19, 2017
Use common.mustCall() where appropriate, var to const/let,
assert.equal() -> assert.strictEqual(), explicit time provided to
setTimeout()

Backport-PR-URL: #11797
PR-URL: #10551
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
andrew749 pushed a commit to michielbaird/node that referenced this pull request Jul 19, 2017
Use common.mustCall() where appropriate, var to const/let,
assert.equal() -> assert.strictEqual(), explicit time provided to
setTimeout()

Backport-PR-URL: nodejs/node#11797
PR-URL: nodejs/node#10551
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
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.