lib,test: minor refactoring

This refactors a couple tests to have upper case first characters
in comments and to use `input` instead of `i`.
It also adds a few TODOs and rewrites a few lines to use default
arguments and to prevent function recreation when unnecessary.

PR-URL: https://github.com/nodejs/node/pull/19445
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>

lib/fs.js

@@ -751,6 +751,10 @@ fs.ftruncate = function(fd, len = 0, callback) {
     len = 0;
   }
   validateUint32(fd, 'fd');
+  // TODO(BridgeAR): This does not seem right.
+  // There does not seem to be any validation before and if there is any, it
+  // should work similar to validateUint32 or not have a upper cap at all.
+  // This applies to all usage of `validateLen`.
   validateLen(len);
   len = Math.max(0, len);
   const req = new FSReqWrap();
@@ -1012,7 +1016,7 @@ fs.fchmod = function(fd, mode, callback) {
   mode = modeNum(mode);
   validateUint32(fd, 'fd');
   validateUint32(mode, 'mode');
-  // values for mode < 0 are already checked via the validateUint32 function
+  // Values for mode < 0 are already checked via the validateUint32 function
   if (mode > 0o777)
     throw new ERR_OUT_OF_RANGE('mode');
 
@@ -1025,7 +1029,8 @@ fs.fchmodSync = function(fd, mode) {
   mode = modeNum(mode);
   validateUint32(fd, 'fd');
   validateUint32(mode, 'mode');
-  if (mode < 0 || mode > 0o777)
+  // Values for mode < 0 are already checked via the validateUint32 function
+  if (mode > 0o777)
     throw new ERR_OUT_OF_RANGE('mode');
   const ctx = {};
   binding.fchmod(fd, mode, undefined, ctx);

lib/fs/promises.js

@@ -369,7 +369,7 @@ async function fchmod(handle, mode) {
   mode = modeNum(mode);
   validateFileHandle(handle);
   validateUint32(mode, 'mode');
-  if (mode < 0 || mode > 0o777)
+  if (mode > 0o777)
     throw new ERR_OUT_OF_RANGE('mode');
   return binding.fchmod(handle.fd, mode, kUsePromises);
 }

lib/internal/crypto/diffiehellman.js

@@ -39,13 +39,11 @@ function DiffieHellman(sizeOrKey, keyEncoding, generator, genEncoding) {
     );
   }
 
-  if (keyEncoding) {
-    if (typeof keyEncoding !== 'string' ||
-        (!Buffer.isEncoding(keyEncoding) && keyEncoding !== 'buffer')) {
-      genEncoding = generator;
-      generator = keyEncoding;
-      keyEncoding = false;
-    }
+  if (keyEncoding && !Buffer.isEncoding(keyEncoding) &&
+      keyEncoding !== 'buffer') {
+    genEncoding = generator;
+    generator = keyEncoding;
+    keyEncoding = false;
   }
 
   const encoding = getDefaultEncoding();

lib/internal/errors.js

@@ -752,7 +752,7 @@ E('ERR_INVALID_ARG_VALUE', (name, value, reason = 'is invalid') => {
     inspected = inspected.slice(0, 128) + '...';
   }
   return `The argument '${name}' ${reason}. Received ${inspected}`;
-}, TypeError, RangeError); // Some are currently falsy implemented as "Error"
+}, TypeError, RangeError);
 E('ERR_INVALID_ARRAY_LENGTH',
   (name, len, actual) => {
     internalAssert(typeof actual === 'number', 'actual must be a number');
@@ -762,7 +762,7 @@ E('ERR_INVALID_ASYNC_ID', 'Invalid %s value: %s', RangeError);
 E('ERR_INVALID_BUFFER_SIZE',
   'Buffer size must be a multiple of %s', RangeError);
 E('ERR_INVALID_CALLBACK', 'Callback must be a function', TypeError);
-E('ERR_INVALID_CHAR', invalidChar, TypeError); //Check falsy "Error" entries.
+E('ERR_INVALID_CHAR', invalidChar, TypeError);
 
 // This should probably be a `TypeError`.
 E('ERR_INVALID_CURSOR_POS',

lib/internal/fs.js

@@ -372,13 +372,9 @@ function validateOffsetLengthWrite(offset, length, byteLength) {
   }
 }
 
-function validatePath(path, propName) {
+function validatePath(path, propName = 'path') {
   let err;
 
-  if (propName === undefined) {
-    propName = 'path';
-  }
-
   if (typeof path !== 'string' && !isUint8Array(path)) {
     err = new ERR_INVALID_ARG_TYPE(propName, ['string', 'Buffer', 'URL'], path);
   } else {

lib/internal/process.js

@@ -68,15 +68,15 @@ function setup_cpuUsage() {
       user: cpuValues[0],
       system: cpuValues[1]
     };
-
-    // Ensure that a previously passed in value is valid. Currently, the native
-    // implementation always returns numbers <= Number.MAX_SAFE_INTEGER.
-    function previousValueIsValid(num) {
-      return Number.isFinite(num) &&
-          num <= Number.MAX_SAFE_INTEGER &&
-          num >= 0;
-    }
   };
+
+  // Ensure that a previously passed in value is valid. Currently, the native
+  // implementation always returns numbers <= Number.MAX_SAFE_INTEGER.
+  function previousValueIsValid(num) {
+    return Number.isFinite(num) &&
+        num <= Number.MAX_SAFE_INTEGER &&
+        num >= 0;
+  }
 }
 
 // The 3 entries filled in by the original process.hrtime contains

lib/zlib.js

@@ -169,12 +169,16 @@ function flushCallback(level, strategy, callback) {
 // 4. Throws ERR_OUT_OF_RANGE for infinite numbers
 function checkFiniteNumber(number, name) {
   // Common case
-  if (number === undefined || Number.isNaN(number)) {
+  if (number === undefined) {
     return false;
   }
 
   if (Number.isFinite(number)) {
-    return true;  // is a valid number
+    return true; // Is a valid number
+  }
+
+  if (Number.isNaN(number)) {
+    return false;
   }
 
   // Other non-numbers

test/parallel/test-crypto-random.js

@@ -80,43 +80,18 @@ process.setMaxListeners(256);
 }
 
 {
-  const buf = new Uint16Array(10);
-  const before = Buffer.from(buf.buffer).toString('hex');
-  crypto.randomFillSync(buf);
-  const after = Buffer.from(buf.buffer).toString('hex');
-  assert.notStrictEqual(before, after);
-}
-
-{
-  const buf = new Uint32Array(10);
-  const before = Buffer.from(buf.buffer).toString('hex');
-  crypto.randomFillSync(buf);
-  const after = Buffer.from(buf.buffer).toString('hex');
-  assert.notStrictEqual(before, after);
-}
-
-{
-  const buf = new Float32Array(10);
-  const before = Buffer.from(buf.buffer).toString('hex');
-  crypto.randomFillSync(buf);
-  const after = Buffer.from(buf.buffer).toString('hex');
-  assert.notStrictEqual(before, after);
-}
-
-{
-  const buf = new Float64Array(10);
-  const before = Buffer.from(buf.buffer).toString('hex');
-  crypto.randomFillSync(buf);
-  const after = Buffer.from(buf.buffer).toString('hex');
-  assert.notStrictEqual(before, after);
-}
-
-{
-  const buf = new DataView(new ArrayBuffer(10));
-  const before = Buffer.from(buf.buffer).toString('hex');
-  crypto.randomFillSync(buf);
-  const after = Buffer.from(buf.buffer).toString('hex');
-  assert.notStrictEqual(before, after);
+  [
+    new Uint16Array(10),
+    new Uint32Array(10),
+    new Float32Array(10),
+    new Float64Array(10),
+    new DataView(new ArrayBuffer(10))
+  ].forEach((buf) => {
+    const before = Buffer.from(buf.buffer).toString('hex');
+    crypto.randomFillSync(buf);
+    const after = Buffer.from(buf.buffer).toString('hex');
+    assert.notStrictEqual(before, after);
+  });
 }
 
 {
@@ -140,53 +115,20 @@ process.setMaxListeners(256);
 }
 
 {
-  const buf = new Uint16Array(10);
-  const before = Buffer.from(buf.buffer).toString('hex');
-  crypto.randomFill(buf, common.mustCall((err, buf) => {
-    assert.ifError(err);
-    const after = Buffer.from(buf.buffer).toString('hex');
-    assert.notStrictEqual(before, after);
-  }));
-}
-
-{
-  const buf = new Uint32Array(10);
-  const before = Buffer.from(buf.buffer).toString('hex');
-  crypto.randomFill(buf, common.mustCall((err, buf) => {
-    assert.ifError(err);
-    const after = Buffer.from(buf.buffer).toString('hex');
-    assert.notStrictEqual(before, after);
-  }));
-}
-
-{
-  const buf = new Float32Array(10);
-  const before = Buffer.from(buf.buffer).toString('hex');
-  crypto.randomFill(buf, common.mustCall((err, buf) => {
-    assert.ifError(err);
-    const after = Buffer.from(buf.buffer).toString('hex');
-    assert.notStrictEqual(before, after);
-  }));
-}
-
-{
-  const buf = new Float64Array(10);
-  const before = Buffer.from(buf.buffer).toString('hex');
-  crypto.randomFill(buf, common.mustCall((err, buf) => {
-    assert.ifError(err);
-    const after = Buffer.from(buf.buffer).toString('hex');
-    assert.notStrictEqual(before, after);
-  }));
-}
-
-{
-  const buf = new DataView(new ArrayBuffer(10));
-  const before = Buffer.from(buf.buffer).toString('hex');
-  crypto.randomFill(buf, common.mustCall((err, buf) => {
-    assert.ifError(err);
-    const after = Buffer.from(buf.buffer).toString('hex');
-    assert.notStrictEqual(before, after);
-  }));
+  [
+    new Uint16Array(10),
+    new Uint32Array(10),
+    new Float32Array(10),
+    new Float64Array(10),
+    new DataView(new ArrayBuffer(10))
+  ].forEach((buf) => {
+    const before = Buffer.from(buf.buffer).toString('hex');
+    crypto.randomFill(buf, common.mustCall((err, buf) => {
+      assert.ifError(err);
+      const after = Buffer.from(buf.buffer).toString('hex');
+      assert.notStrictEqual(before, after);
+    }));
+  });
 }
 
 {

test/parallel/test-fs-access.js

@@ -62,13 +62,9 @@ assert.strictEqual(typeof fs.R_OK, 'number');
 assert.strictEqual(typeof fs.W_OK, 'number');
 assert.strictEqual(typeof fs.X_OK, 'number');
 
-fs.access(__filename, common.mustCall((err) => {
-  assert.ifError(err);
-}));
-
-fs.access(__filename, fs.R_OK, common.mustCall((err) => {
-  assert.ifError(err);
-}));
+fs.access(__filename, common.mustCall(assert.ifError));
+fs.access(__filename, fs.R_OK, common.mustCall(assert.ifError));
+fs.access(readOnlyFile, fs.F_OK | fs.R_OK, common.mustCall(assert.ifError));
 
 fs.access(doesNotExist, common.mustCall((err) => {
   assert.notStrictEqual(err, null, 'error should exist');
@@ -76,10 +72,6 @@ fs.access(doesNotExist, common.mustCall((err) => {
   assert.strictEqual(err.path, doesNotExist);
 }));
 
-fs.access(readOnlyFile, fs.F_OK | fs.R_OK, common.mustCall((err) => {
-  assert.ifError(err);
-}));
-
 fs.access(readOnlyFile, fs.W_OK, common.mustCall(function(err) {
   assert.strictEqual(this, undefined);
   if (hasWriteAccessForReadonlyFile) {

test/parallel/test-fs-truncate.js

@@ -36,7 +36,7 @@ let stat;
 const msg = 'Using fs.truncate with a file descriptor is deprecated.' +
             ' Please use fs.ftruncate with a file descriptor instead.';
 
-// truncateSync
+// Check truncateSync
 fs.writeFileSync(filename, data);
 stat = fs.statSync(filename);
 assert.strictEqual(stat.size, 1024 * 16);
@@ -49,7 +49,7 @@ fs.truncateSync(filename);
 stat = fs.statSync(filename);
 assert.strictEqual(stat.size, 0);
 
-// ftruncateSync
+// Check ftruncateSync
 fs.writeFileSync(filename, data);
 const fd = fs.openSync(filename, 'r+');
 
@@ -64,18 +64,16 @@ fs.ftruncateSync(fd);
 stat = fs.statSync(filename);
 assert.strictEqual(stat.size, 0);
 
-// truncateSync
+// Check truncateSync
 common.expectWarning('DeprecationWarning', msg);
 fs.truncateSync(fd);
 
 fs.closeSync(fd);
 
-// async tests
+// Async tests
 testTruncate(common.mustCall(function(er) {
   assert.ifError(er);
-  testFtruncate(common.mustCall(function(er) {
-    assert.ifError(er);
-  }));
+  testFtruncate(common.mustCall(assert.ifError));
 }));
 
 function testTruncate(cb) {
@@ -105,7 +103,6 @@ function testTruncate(cb) {
   });
 }
 
-
 function testFtruncate(cb) {
   fs.writeFile(filename, data, function(er) {
     if (er) return cb(er);
@@ -136,7 +133,6 @@ function testFtruncate(cb) {
   });
 }
 
-
 // Make sure if the size of the file is smaller than the length then it is
 // filled with zeroes.
 

test/parallel/test-fs-utimes.js

@@ -111,10 +111,10 @@ function testIt(atime, mtime, callback) {
   //
   // test async code paths
   //
-  fs.utimes(tmpdir.path, atime, mtime, common.mustCall(function(err) {
+  fs.utimes(tmpdir.path, atime, mtime, common.mustCall((err) => {
     expect_ok('utimes', tmpdir.path, err, atime, mtime);
 
-    fs.utimes('foobarbaz', atime, mtime, common.mustCall(function(err) {
+    fs.utimes('foobarbaz', atime, mtime, common.mustCall((err) => {
       expect_errno('utimes', 'foobarbaz', err, 'ENOENT');
 
       // don't close this fd
@@ -124,7 +124,7 @@ function testIt(atime, mtime, callback) {
         fd = fs.openSync(tmpdir.path, 'r');
       }
 
-      fs.futimes(fd, atime, mtime, common.mustCall(function(err) {
+      fs.futimes(fd, atime, mtime, common.mustCall((err) => {
         expect_ok('futimes', fd, err, atime, mtime);
 
         common.expectsError(
@@ -148,19 +148,19 @@ function testIt(atime, mtime, callback) {
 
 const stats = fs.statSync(tmpdir.path);
 
-// run tests
+// Run tests
 const runTest = common.mustCall(testIt, 1);
 
-runTest(new Date('1982-09-10 13:37'), new Date('1982-09-10 13:37'), function() {
-  runTest(new Date(), new Date(), function() {
-    runTest(123456.789, 123456.789, function() {
-      runTest(stats.mtime, stats.mtime, function() {
-        runTest('123456', -1, function() {
+runTest(new Date('1982-09-10 13:37'), new Date('1982-09-10 13:37'), () => {
+  runTest(new Date(), new Date(), () => {
+    runTest(123456.789, 123456.789, () => {
+      runTest(stats.mtime, stats.mtime, () => {
+        runTest('123456', -1, () => {
           runTest(
             new Date('2017-04-08T17:59:38.008Z'),
             new Date('2017-04-08T17:59:38.008Z'),
-            common.mustCall(function() {
-              // done
+            common.mustCall(() => {
+              // Done
             })
           );
         });
@@ -169,7 +169,7 @@ runTest(new Date('1982-09-10 13:37'), new Date('1982-09-10 13:37'), function() {
   });
 });
 
-process.on('exit', function() {
+process.on('exit', () => {
   assert.strictEqual(tests_ok, tests_run - 2);
 });
 

test/parallel/test-fs-watch.js

@@ -1,7 +1,7 @@
 'use strict';
 const common = require('../common');
 
-// tests if `filename` is provided to watcher on supported platforms
+// Tests if `filename` is provided to watcher on supported platforms
 
 const fs = require('fs');
 const assert = require('assert');
@@ -41,7 +41,7 @@ tmpdir.refresh();
 for (const testCase of cases) {
   if (testCase.shouldSkip) continue;
   fs.mkdirSync(testCase.dirPath);
-  // long content so it's actually flushed.
+  // Long content so it's actually flushed.
   const content1 = Date.now() + testCase.fileName.toLowerCase().repeat(1e4);
   fs.writeFileSync(testCase.filePath, content1);
 
@@ -65,13 +65,13 @@ for (const testCase of cases) {
       assert.strictEqual(eventType, 'change');
     assert.strictEqual(argFilename, testCase.fileName);
 
-    watcher.start();  // starting a started watcher should be a noop
-    // end of test case
+    watcher.start(); // Starting a started watcher should be a noop
+    // End of test case
     watcher.close();
-    watcher.close(); // closing a closed watcher should be a noop
+    watcher.close(); // Closing a closed watcher should be a noop
   }));
 
-  // long content so it's actually flushed. toUpperCase so there's real change.
+  // Long content so it's actually flushed. toUpperCase so there's real change.
   const content2 = Date.now() + testCase.fileName.toUpperCase().repeat(1e4);
   interval = setInterval(() => {
     fs.writeFileSync(testCase.filePath, '');

test/parallel/test-http2-compat-serverrequest-headers.js

@@ -41,7 +41,7 @@ server.listen(0, common.mustCall(function() {
     request.url = '/one';
     assert.strictEqual(request.url, '/one');
 
-    // third-party plugins for packages like express use query params to
+    // Third-party plugins for packages like express use query params to
     // change the request method
     request.method = 'POST';
     assert.strictEqual(request.method, 'POST');

test/parallel/test-http2-ping.js

@@ -79,7 +79,7 @@ server.listen(0, common.mustCall(() => {
       message: 'HTTP2 ping cancelled'
     })));
 
-    // should throw if payload is not of type ArrayBufferView
+    // Should throw if payload is not of type ArrayBufferView
     {
       [1, true, {}, []].forEach((payload) =>
         common.expectsError(
@@ -94,7 +94,7 @@ server.listen(0, common.mustCall(() => {
       );
     }
 
-    // should throw if payload length is not 8
+    // Should throw if payload length is not 8
     {
       const shortPayload = Buffer.from('abcdefg');
       const longPayload = Buffer.from('abcdefghi');
@@ -110,7 +110,7 @@ server.listen(0, common.mustCall(() => {
       );
     }
 
-    // should throw error is callback is not of type function
+    // Should throw error is callback is not of type function
     {
       const payload = Buffer.from('abcdefgh');
       [1, true, {}, []].forEach((invalidCallback) =>

test/parallel/test-http2-util-asserts.js

@@ -13,8 +13,8 @@ const {
   Object.create(null),
   new Date(),
   new (class Foo {})()
-].forEach((i) => {
-  assertIsObject(i, 'foo', 'Object');
+].forEach((input) => {
+  assertIsObject(input, 'foo', 'Object');
 });
 
 [
@@ -39,5 +39,5 @@ assertWithinRange('foo', 1, 0, 2);
 common.expectsError(() => assertWithinRange('foo', 1, 2, 3),
                     {
                       code: 'ERR_HTTP2_INVALID_SETTING_VALUE',
-                      message: /^Invalid value for setting "foo": 1$/
+                      message: 'Invalid value for setting "foo": 1'
                     });

test/parallel/test-util-inherits.js

@@ -2,15 +2,15 @@
 
 const common = require('../common');
 const assert = require('assert');
-const inherits = require('util').inherits;
+const { inherits } = require('util');
 
-// super constructor
+// Super constructor
 function A() {
   this._a = 'a';
 }
 A.prototype.a = function() { return this._a; };
 
-// one level of inheritance
+// One level of inheritance
 function B(value) {
   A.call(this);
   this._b = value;
@@ -25,7 +25,7 @@ assert.strictEqual(b.a(), 'a');
 assert.strictEqual(b.b(), 'b');
 assert.strictEqual(b.constructor, B);
 
-// two levels of inheritance
+// Two levels of inheritance
 function C() {
   B.call(this, 'b');
   this._c = 'c';
@@ -40,7 +40,7 @@ const c = new C();
 assert.strictEqual(c.getValue(), 'abc');
 assert.strictEqual(c.constructor, C);
 
-// inherits can be called after setting prototype properties
+// Inherits can be called after setting prototype properties
 function D() {
   C.call(this);
   this._d = 'd';