Test revisions #10551

DavidGoussev · 2016-12-31T04:16:00Z

test: implement callback wrapper common.mustCall

In test/parallel/test-stream2-readable-wrap.js:

replace process.on('exit', ...) block by wrapping all callbacks with common.mustCall
replace assert.equal() with assert.strictEqual()
add a duration of 1 second to setTimeout() on line 61
replace var with const or let

mscdex · 2016-12-31T05:37:15Z

First line of commit messages must be <= 50 characters.

mscdex · 2016-12-31T05:37:56Z

test/parallel/test-stream2-readable-wrap.js

-  var old = new EE();
-  var r = new Readable({ highWaterMark: highWaterMark,
+  const old = new EE();
+  const r = new Readable({ highWaterMark: highWaterMark,
                         objectMode: objectMode });


The indentation needs to be adjusted here to re-align with the first 'h'.

mscdex · 2016-12-31T05:39:26Z

test/parallel/test-stream2-readable-wrap.js


-  var ended = false;
-  r.on('end', function() {
+  let ended = false;


This can be removed completely, along with any references since we're already ensuring 'end' is seen via common.mustCall() right below.

mscdex · 2016-12-31T05:41:12Z

test/parallel/test-stream2-readable-wrap.js

  function flow() {
    flowing = true;
    while (flowing && chunks-- > 0) {
-      var item = produce();
+      const item = produce();
      expected.push(item);
      console.log('old.emit', chunks, flowing);


Please remove all of these console.* statements.

mscdex · 2016-12-31T05:41:40Z

test/parallel/test-stream2-readable-wrap.js

@@ -52,19 +49,18 @@ function runTest(highWaterMark, objectMode, produce) {
    }
  }

-  var w = new Writable({ highWaterMark: highWaterMark * 2,
+ const w = new Writable({ highWaterMark: highWaterMark * 2,
                         objectMode: objectMode });


Indentation here needs to be adjusted as well.

mscdex · 2016-12-31T05:42:12Z

test/parallel/test-stream2-readable-wrap.js

  w._write = function(chunk, encoding, cb) {
    console.log('_write', chunk);
    written.push(chunk);
-    setTimeout(cb);
+    setTimeout(cb, 1000);


This should be 1 instead of 1000.

Trott · 2016-12-31T05:38:50Z

test/parallel/test-stream2-readable-wrap.js

  w._write = function(chunk, encoding, cb) {
    console.log('_write', chunk);
    written.push(chunk);
-    setTimeout(cb);
+    setTimeout(cb, 1000);


Can you change this to 1 instead of 1000? No duration specified defaults to 1ms, so using 1 keeps the behavior identical but makes things more explicit.

Trott · 2016-12-31T05:47:13Z

test/parallel/test-stream2-readable-wrap.js

-runTest(100, false, function() { return Buffer.allocUnsafe(100); });
-runTest(10, false, function() { return Buffer.from('xxxxxxxxxx'); });
-runTest(1, true, function() { return { foo: 'bar' }; });
+runTest(100, false, common.mustCall(function() { return Buffer.allocUnsafe(100); });


There's a missing ) at the end of this line. This is a syntax error. This file won't run. (It looks the pull request template was deleted. It asks that make -j4 test before submitting the pull request.)

Trott · 2016-12-31T05:48:00Z

test/parallel/test-stream2-readable-wrap.js

@@ -1,24 +1,21 @@
 'use strict';
 require('../common');


common is undefined because this line has not been changed to const common = require('../common'); So again, there's no way this test can actually run as is.

mscdex · 2016-12-31T09:03:52Z

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

Trott · 2016-12-31T18:18:01Z

Single host failing on CI is an unrelated build infrastructure issue. CI looks good.

Trott · 2016-12-31T18:22:36Z

LGTM. Thanks for doing this!

gibfahn · 2016-12-31T20:16:25Z

FYI, your name for this commit is set as dpg5000, people normally use their full name. If you want to change it the commands are:

# To change it everywhere:
git config --global user.name "David Goussev"

# To change it for a single commit:
git commit --amend --no-edit  --author="David Goussev <dgoussev@gmail.com>"

Whoever lands this Pull Request can do it for you as part of the landing process instead if you'd like. If you don't want to change it that's totally fine too!

Use common.mustCall() where appropriate, var to const/let, assert.equal() -> assert.strictEqual(), explicit time provided to setTimeout() PR-URL: nodejs#10551 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>

Trott · 2017-01-03T06:04:46Z

Landed in e1fedfb.
Thanks for the contribution! 🎉

evanlucas · 2017-01-04T16:50:24Z

This is not landing cleanly on v7.x-staging. If you would like to see it there, a backport pull request would need to be opened.

Trott · 2017-01-04T17:14:14Z

@evanlucas curl -L https://github.com/nodejs/node/pull/10551.patch | git am -3 is landing cleanly for me on v7.x-staging.

Trott · 2017-01-04T17:16:35Z

@evanlucas (Is the key that a cherry-pick has to land cleanly?)

Trott · 2017-01-06T00:06:42Z

@evanlucas You can use this, then? Or would you prefer @dpg5000 create a second PR with these changes against the v7.x-staging branch?

I suppose another possibility is don't worry about it at all and just let these changes stick around in master and only hit a release branch come v8.0.0 in April. These changes don't affect users.

Use common.mustCall() where appropriate, var to const/let, assert.equal() -> assert.strictEqual(), explicit time provided to setTimeout() PR-URL: nodejs#10551 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>

MylesBorins · 2017-03-08T09:53:37Z

would someone be open to backporting this?

DavidGoussev · 2017-03-08T15:17:42Z

Would love to help, this was my PR. What's involved with backporting? David

…

On Mar 8, 2017 1:54 AM, "Myles Borins" ***@***.***> wrote: would someone be open to backporting this? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#10551 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AMpemJJ7NJ-t6w_iOA3nkflrK-_ACwSQks5rjnrDgaJpZM4LYfMh> .

gibfahn · 2017-03-08T15:51:56Z

@dpg5000 There's a howto in #11099. It's still under review, but the process for submitting the backport PR should be okay to follow.

DavidGoussev · 2017-03-09T07:11:36Z

@gibfahn Hi Gibson, thanks! Running into a slight issue with git checkout v7.x-staging

I receive error: pathspec 'v7.x-staging' did not match any file(s) known to git.

To confirm, my origin remote points to my node fork, and my upstream remote points to https://github.com/nodejs/node.git

bnoordhuis · 2017-03-09T07:22:55Z

@dpg5000 Does it work when you run git branch v7.x-staging origin/v7.x-staging first?

gibfahn · 2017-03-09T23:35:36Z

@dpg5000 yep, see discussion in that PR from #11099 (comment) onwards.

You want to do what @bnoordhuis suggests, although I'm pretty sure it should be git branch v7.x-staging upstream/v7.x-staging

DavidGoussev · 2017-03-10T22:49:44Z

@gibfahn Hi Gibson, apparently this PR has been backported to v7.x already (successfully). Is there a previous version that it still needs to be backported to (as @MylesBorins mentioned earlier above)?

gibfahn · 2017-03-11T00:07:42Z

@dpg5000 the other release lines are v6.x and v4.x. Node v4 is about to go into maintenance, so I don't think it's worth trying to backport to it. Backporting to v6 would be good though.

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>

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>

DavidGoussev added 3 commits December 30, 2016 19:50

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

a26d602

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

fd8fa5b

test: implement callback wrapper common.mustCall

15a48bc

nodejs-github-bot added test Issues and PRs related to the tests. lts-watch-v6.x labels Dec 31, 2016

mscdex added the stream Issues and PRs related to the stream subsystem. label Dec 31, 2016

mscdex reviewed Dec 31, 2016

View reviewed changes

Trott requested changes Dec 31, 2016

View reviewed changes

Trott reviewed Dec 31, 2016

View reviewed changes

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

b550a8c

nodejs-github-bot added the lts-watch-v6.x label Dec 31, 2016

Trott approved these changes Dec 31, 2016

View reviewed changes

gibfahn approved these changes Dec 31, 2016

View reviewed changes

Trott closed this Jan 3, 2017

evanlucas added the dont-land-on-v7.x label Jan 4, 2017

evanlucas removed the dont-land-on-v7.x label Jan 5, 2017

italoacasas mentioned this pull request Jan 25, 2017

Proposal to update v7.x-staging #10989

Closed

italoacasas mentioned this pull request Jan 29, 2017

v7.5.0 proposal #11062

Merged

DavidGoussev mentioned this pull request Mar 10, 2017

(v7.x backport) test: refactor test-stream2-readable-wrap.js #11784

Closed

DavidGoussev mentioned this pull request Mar 11, 2017

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

Closed

MylesBorins added backport-to-v6.x and removed lts-watch-v6.x labels Apr 13, 2017

addaleax added backported-to-v6.x and removed backport-requested-v6.x labels Apr 13, 2017

MylesBorins mentioned this pull request Apr 19, 2017

v6.10.3 proposal #12498

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Test revisions #10551

Test revisions #10551

DavidGoussev commented Dec 31, 2016

mscdex commented Dec 31, 2016

mscdex Dec 31, 2016

mscdex Dec 31, 2016 •

edited

Loading

mscdex Dec 31, 2016

mscdex Dec 31, 2016

mscdex Dec 31, 2016

Trott Dec 31, 2016

Trott Dec 31, 2016

Trott Dec 31, 2016

mscdex commented Dec 31, 2016

Trott commented Dec 31, 2016

Trott commented Dec 31, 2016

gibfahn commented Dec 31, 2016

Trott commented Jan 3, 2017

evanlucas commented Jan 4, 2017

Trott commented Jan 4, 2017 •

edited

Loading

Trott commented Jan 4, 2017

Trott commented Jan 6, 2017

MylesBorins commented Mar 8, 2017

DavidGoussev commented Mar 8, 2017 via email

gibfahn commented Mar 8, 2017

DavidGoussev commented Mar 9, 2017

bnoordhuis commented Mar 9, 2017

gibfahn commented Mar 9, 2017

DavidGoussev commented Mar 10, 2017

gibfahn commented Mar 11, 2017

Test revisions #10551

Test revisions #10551

Conversation

DavidGoussev commented Dec 31, 2016

mscdex commented Dec 31, 2016

mscdex Dec 31, 2016

Choose a reason for hiding this comment

mscdex Dec 31, 2016 • edited Loading

Choose a reason for hiding this comment

mscdex Dec 31, 2016

Choose a reason for hiding this comment

mscdex Dec 31, 2016

Choose a reason for hiding this comment

mscdex Dec 31, 2016

Choose a reason for hiding this comment

Trott Dec 31, 2016

Choose a reason for hiding this comment

Trott Dec 31, 2016

Choose a reason for hiding this comment

Trott Dec 31, 2016

Choose a reason for hiding this comment

mscdex commented Dec 31, 2016

Trott commented Dec 31, 2016

Trott commented Dec 31, 2016

gibfahn commented Dec 31, 2016

Trott commented Jan 3, 2017

evanlucas commented Jan 4, 2017

Trott commented Jan 4, 2017 • edited Loading

Trott commented Jan 4, 2017

Trott commented Jan 6, 2017

MylesBorins commented Mar 8, 2017

DavidGoussev commented Mar 8, 2017 via email

gibfahn commented Mar 8, 2017

DavidGoussev commented Mar 9, 2017

bnoordhuis commented Mar 9, 2017

gibfahn commented Mar 9, 2017

DavidGoussev commented Mar 10, 2017

gibfahn commented Mar 11, 2017

mscdex Dec 31, 2016 •

edited

Loading

Trott commented Jan 4, 2017 •

edited

Loading