Test revisions by DavidGoussev · Pull Request #10551 · nodejs/node · GitHub

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

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

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

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

Please remove all of these console.* statements.

Indentation here needs to be adjusted as well.

This should be 1 instead of 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.

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

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.

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

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

LGTM. Thanks for doing this!

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>

Landed in e1fedfb.
Thanks for the contribution! 🎉

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.

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

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

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

would someone be open to backporting this?

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

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

@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

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

@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

@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)?

@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

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

-                    var item = produce();
+                    const item = produce();
                     expected.push(item);
                     console.log('old.emit', chunks, flowing);

                   console.log('_write', chunk);
                   written.push(chunk);
-                  setTimeout(cb);
+                  setTimeout(cb, 1000);

                   console.log('_write', chunk);
                   written.push(chunk);
-                  setTimeout(cb);
+                  setTimeout(cb, 1000);

-              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); });

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

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

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

Test revisions by DavidGoussev · Pull Request #10551 · nodejs/node · GitHub

Uh oh!

Uh oh!

Test revisions - #10551

Test revisions#10551DavidGoussev wants to merge 4 commits intonodejs:masternodejs/node:masterfrom DavidGoussev:test_revisionsDavidGoussev/node:test_revisionsCopy head branch name to clipboard

Conversation

DavidGoussev commented Dec 31, 2016

Uh oh!

mscdex commented Dec 31, 2016

Uh oh!

mscdex Dec 31, 2016

Choose a reason for hiding this comment

Uh oh!

mscdex Dec 31, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mscdex Dec 31, 2016

Choose a reason for hiding this comment

Uh oh!

mscdex Dec 31, 2016

Choose a reason for hiding this comment

Uh oh!

mscdex Dec 31, 2016

Choose a reason for hiding this comment

Uh oh!

Trott Dec 31, 2016

Choose a reason for hiding this comment

Uh oh!

Trott Dec 31, 2016

Choose a reason for hiding this comment

Uh oh!

Trott Dec 31, 2016

Choose a reason for hiding this comment

Uh oh!

mscdex commented Dec 31, 2016

Uh oh!

Trott commented Dec 31, 2016

Uh oh!

Trott commented Dec 31, 2016

Uh oh!

gibfahn commented Dec 31, 2016

Uh oh!

Trott commented Jan 3, 2017

Uh oh!

evanlucas commented Jan 4, 2017

Uh oh!

Trott commented Jan 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Trott commented Jan 4, 2017

Uh oh!

Trott commented Jan 6, 2017

Uh oh!

MylesBorins commented Mar 8, 2017

Uh oh!

DavidGoussev commented Mar 8, 2017 via email

Uh oh!

gibfahn commented Mar 8, 2017

Uh oh!

DavidGoussev commented Mar 9, 2017

Uh oh!

bnoordhuis commented Mar 9, 2017

Uh oh!

gibfahn commented Mar 9, 2017

Uh oh!

DavidGoussev commented Mar 10, 2017

Uh oh!

gibfahn commented Mar 11, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

Test revisions#10551
DavidGoussev wants to merge 4 commits into
nodejs:masterfrom
DavidGoussev:test_revisions

mscdex Dec 31, 2016 •
edited
Loading

Trott commented Jan 4, 2017 •
edited
Loading