test: refactor the code in test-util-debug.js #10531

sivaprs · 2016-12-30T05:35:51Z

use const and let instead of var
use assert.strictEqual instead of assert.equal

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

Affected core subsystem(s)

test

Description

use const and let instead of var
use assert.strictEqual instead of assert.equal

sivaprs · 2016-12-30T05:36:57Z

cc @thefourtheye

lpinca · 2016-12-30T10:37:31Z

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

gibfahn

LGTM pending CI

gibfahn · 2016-12-30T11:05:43Z

FYI your Github name is set to sivaprasanna, people usually tend to use their full names (e.g. @thefourtheye here). If you want to change it the commands are:

# To change it everywhere:
git config --global user.name "Siva Prasanna"

# To change it for this one commit:
git commit --amend --no-edit  --author="Siva Prasanna <[email protected]>"

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!

thefourtheye · 2017-01-02T07:35:50Z

test/sequential/test-util-debug.js

-    assert.equal(err, expectErr);
-    assert.equal(out, expectOut);
+    assert.strictEqual(err, expectErr);
+    assert.strictEqual(out, expectOut);
    console.log('ok %j %j', environ, shouldWrite);


Can you remove all the console.logs while you are here?

sivaprs · 2017-01-02T12:59:52Z

cc @thefourtheye

thefourtheye · 2017-01-02T13:17:41Z

test/sequential/test-util-debug.js

-  var util = require('util');
-  var debug = util.debuglog('tud');
+  const util = require('util');
+  const debug = util.debuglog('tud');
  debug('this', { is: 'a' }, /debugging/);
  debug('number=%d string=%s obj=%j', 1234, 'asdf', { foo: 'bar' });
  console.log('ok');


You missed one here :)

console.log('ok') is expected to be there because it has check at line no. 49

Awww, right...

cjihrig · 2017-01-03T14:19:46Z

@sivaprs, it looks like something went wrong during the rebase process. This is now 17 commits.

edsadr · 2017-01-03T16:24:17Z

test/sequential/test-util-debug.js

    env: Object.assign(process.env, { NODE_DEBUG: environ })
  });

  expectErr = expectErr.split('%PID%').join(child.pid);

-  var err = '';
+  let err = '';
  child.stderr.setEncoding('utf8');
  child.stderr.on('data', function(c) {


Please change anonymous functions to arrow functions

edsadr

Please change anonymous functions to arrow functions

edsadr · 2017-01-03T16:24:28Z

test/sequential/test-util-debug.js

  child.stderr.setEncoding('utf8');
  child.stderr.on('data', function(c) {
    err += c;
  });

-  var out = '';
+  let out = '';
  child.stdout.setEncoding('utf8');
  child.stdout.on('data', function(c) {


Please change anonymous functions to arrow functions

edsadr · 2017-01-03T16:24:47Z

test/sequential/test-util-debug.js

  child.stderr.setEncoding('utf8');
  child.stderr.on('data', function(c) {
    err += c;
  });

-  var out = '';
+  let out = '';
  child.stdout.setEncoding('utf8');
  child.stdout.on('data', function(c) {
    out += c;
  });

  child.on('close', common.mustCall(function(c) {


Please change anonymous functions to arrow functions

gibfahn · 2017-01-03T16:32:06Z

@edsadr I don't think changing anonymous functions to arrow functions is required, you can do it if you want, but it's not actually necessary.

edsadr · 2017-01-03T16:42:10Z

@gibfahn is just a small Nit now that @sivaprs is touching the file would be a nice thing to add ... I had the same file in another PR including that... closed it in favor of this one, not a big deal anyway 🙂

* use const and let instead of var * use assert.strictEqual instead of assert.equal * use arrow functions * removed unwanted console log

sivaprs · 2017-01-04T06:30:59Z

@cjihrig I rebased now. PTAL.

thefourtheye · 2017-01-04T06:34:00Z

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

* use const and let instead of var * use assert.strictEqual instead of assert.equal * use arrow functions * removed unwanted console log PR-URL: #10531 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]>

jasnell · 2017-01-06T17:52:01Z

Landed in 8839d50

* use const and let instead of var * use assert.strictEqual instead of assert.equal * use arrow functions * removed unwanted console log PR-URL: nodejs#10531 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]>

* use const and let instead of var * use assert.strictEqual instead of assert.equal * use arrow functions * removed unwanted console log PR-URL: #10531 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]>

* use const and let instead of var * use assert.strictEqual instead of assert.equal * use arrow functions * removed unwanted console log PR-URL: nodejs#10531 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]>

jasnell · 2017-03-08T20:29:32Z

This will need backport URLs for v4 and v6 to land there.

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

mscdex added the util Issues and PRs related to the built-in util module. label Dec 30, 2016

lpinca approved these changes Dec 30, 2016

View reviewed changes

gibfahn approved these changes Dec 30, 2016

View reviewed changes

cjihrig approved these changes Dec 30, 2016

View reviewed changes

jasnell approved these changes Dec 30, 2016

View reviewed changes

thefourtheye approved these changes Jan 2, 2017

View reviewed changes

nodejs-github-bot added the dont-land-on-v7.x label Jan 2, 2017

thefourtheye reviewed Jan 2, 2017

View reviewed changes

nodejs-github-bot added the dont-land-on-v7.x label Jan 3, 2017

thefourtheye mentioned this pull request Jan 3, 2017

test: improve the code in test-util-debug.js #10583

Closed

3 tasks

edsadr reviewed Jan 3, 2017

View reviewed changes

test: refactor the code in test-util-debug.js

40921a5

* use const and let instead of var * use assert.strictEqual instead of assert.equal * use arrow functions * removed unwanted console log

sivaprs force-pushed the test-2016 branch from 40eb34c to 40921a5 Compare January 4, 2017 06:29

nodejs-github-bot added the dont-land-on-v7.x label Jan 4, 2017

jasnell closed this Jan 6, 2017

mscdex removed the dont-land-on-v7.x label Jan 13, 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

jasnell added dont-land-on-v4.x labels Mar 8, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: refactor the code in test-util-debug.js #10531

test: refactor the code in test-util-debug.js #10531

sivaprs commented Dec 30, 2016 •

edited

Loading

sivaprs commented Dec 30, 2016

lpinca commented Dec 30, 2016

gibfahn left a comment

gibfahn commented Dec 30, 2016

thefourtheye Jan 2, 2017

sivaprs commented Jan 2, 2017

thefourtheye Jan 2, 2017

sivaprs Jan 3, 2017

thefourtheye Jan 3, 2017

cjihrig commented Jan 3, 2017

edsadr Jan 3, 2017

edsadr left a comment

edsadr Jan 3, 2017

edsadr Jan 3, 2017

gibfahn commented Jan 3, 2017

edsadr commented Jan 3, 2017

sivaprs commented Jan 4, 2017

thefourtheye commented Jan 4, 2017

jasnell commented Jan 6, 2017

jasnell commented Mar 8, 2017

test: refactor the code in test-util-debug.js #10531

test: refactor the code in test-util-debug.js #10531

Conversation

sivaprs commented Dec 30, 2016 • edited Loading

Checklist

Affected core subsystem(s)

sivaprs commented Dec 30, 2016

lpinca commented Dec 30, 2016

gibfahn left a comment

Choose a reason for hiding this comment

gibfahn commented Dec 30, 2016

thefourtheye Jan 2, 2017

Choose a reason for hiding this comment

sivaprs commented Jan 2, 2017

thefourtheye Jan 2, 2017

Choose a reason for hiding this comment

sivaprs Jan 3, 2017

Choose a reason for hiding this comment

thefourtheye Jan 3, 2017

Choose a reason for hiding this comment

cjihrig commented Jan 3, 2017

edsadr Jan 3, 2017

Choose a reason for hiding this comment

edsadr left a comment

Choose a reason for hiding this comment

edsadr Jan 3, 2017

Choose a reason for hiding this comment

edsadr Jan 3, 2017

Choose a reason for hiding this comment

gibfahn commented Jan 3, 2017

edsadr commented Jan 3, 2017

sivaprs commented Jan 4, 2017

thefourtheye commented Jan 4, 2017

jasnell commented Jan 6, 2017

jasnell commented Mar 8, 2017

sivaprs commented Dec 30, 2016 •

edited

Loading