From b3166e30f7f18aa4b3a057287edb9cde55954319 Mon Sep 17 00:00:00 2001 From: Rich Trott Date: Sat, 5 Aug 2017 17:25:30 -0700 Subject: [PATCH 1/5] test: refactor test-fs-stat * add `use strict' * change checks that `this` is mapped to `global` in sloppy mode to checks that `this` is `undefined` * modify arguments to assertions to match docs (actual first, expected second) * add blank line below `common` declaration per test writing guide * use `assert.ifError()` as appropriate --- test/parallel/test-fs-stat.js | 39 +++++++++++++++++++++++------------ 1 file changed, 26 insertions(+), 13 deletions(-) diff --git a/test/parallel/test-fs-stat.js b/test/parallel/test-fs-stat.js index c12af535fa837e..8a6bb1c6245431 100644 --- a/test/parallel/test-fs-stat.js +++ b/test/parallel/test-fs-stat.js @@ -19,15 +19,19 @@ // OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE // USE OR OTHER DEALINGS IN THE SOFTWARE. -/* eslint-disable strict */ +'use strict'; const common = require('../common'); + const assert = require('assert'); const fs = require('fs'); fs.stat('.', common.mustCall(function(err, stats) { assert.ifError(err); assert.ok(stats.mtime instanceof Date); - assert.strictEqual(this, global); + // Confirm that we are not running in the context of the internal binding + // layer. + // Ref: https://github.com/nodejs/node/commit/463d6bac8b349acc462d345a6e298a76f7d06fb1 + assert.strictEqual(this, undefined); })); fs.stat('.', common.mustCall(function(err, stats) { @@ -38,22 +42,31 @@ fs.stat('.', common.mustCall(function(err, stats) { fs.lstat('.', common.mustCall(function(err, stats) { assert.ifError(err); assert.ok(stats.mtime instanceof Date); - assert.strictEqual(this, global); + // Confirm that we are not running in the context of the internal binding + // layer. + // Ref: https://github.com/nodejs/node/commit/463d6bac8b349acc462d345a6e298a76f7d06fb1 + assert.strictEqual(this, undefined); })); // fstat fs.open('.', 'r', undefined, common.mustCall(function(err, fd) { - assert.ok(!err); + assert.ifError(err); assert.ok(fd); fs.fstat(fd, common.mustCall(function(err, stats) { assert.ifError(err); assert.ok(stats.mtime instanceof Date); fs.close(fd, assert.ifError); - assert.strictEqual(this, global); + // Confirm that we are not running in the context of the internal binding + // layer. + // Ref: https://github.com/nodejs/node/commit/463d6bac8b349acc462d345a6e298a76f7d06fb1 + assert.strictEqual(this, undefined); })); - assert.strictEqual(this, global); + // Confirm that we are not running in the context of the internal binding + // layer. + // Ref: https://github.com/nodejs/node/commit/463d6bac8b349acc462d345a6e298a76f7d06fb1 + assert.strictEqual(this, null); })); // fstatSync @@ -72,13 +85,13 @@ fs.open('.', 'r', undefined, common.mustCall(function(err, fd) { fs.stat(__filename, common.mustCall(function(err, s) { assert.ifError(err); - assert.strictEqual(false, s.isDirectory()); - assert.strictEqual(true, s.isFile()); - assert.strictEqual(false, s.isSocket()); - assert.strictEqual(false, s.isBlockDevice()); - assert.strictEqual(false, s.isCharacterDevice()); - assert.strictEqual(false, s.isFIFO()); - assert.strictEqual(false, s.isSymbolicLink()); + assert.strictEqual(s.isDirectory(), false); + assert.strictEqual(s.isFile(), true); + assert.strictEqual(s.isSocket(), false); + assert.strictEqual(s.isBlockDevice(), false); + assert.strictEqual(s.isCharacterDevice(), false); + assert.strictEqual(s.isFIFO(), false); + assert.strictEqual(s.isSymbolicLink(), false); const keys = [ 'dev', 'mode', 'nlink', 'uid', 'gid', 'rdev', 'ino', 'size', From 9d6365a6e1642edb7353329e438238ddc0f8bd80 Mon Sep 17 00:00:00 2001 From: Rich Trott Date: Sat, 5 Aug 2017 18:10:28 -0700 Subject: [PATCH 2/5] fs: invoke callbacks with undefined context Many callbacks appear to be invoked with `this` set to `undefined` including `fs.stat()`, `fs.lstat()`, and `fs.fstat()`. However, some such as `fs.open()` and `fs.mkdtemp()` invoke their callbacks with `this` set to `null`. Change to `undefined`. --- lib/fs.js | 2 +- test/parallel/test-fs-mkdtemp.js | 2 +- test/parallel/test-fs-stat.js | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/fs.js b/lib/fs.js index d733a0eaa57053..7c4e68358b33c2 100644 --- a/lib/fs.js +++ b/lib/fs.js @@ -132,7 +132,7 @@ function makeCallback(cb) { } return function() { - return cb.apply(null, arguments); + return cb.apply(undefined, arguments); }; } diff --git a/test/parallel/test-fs-mkdtemp.js b/test/parallel/test-fs-mkdtemp.js index 5ce340afa5ad0f..38a306b85c7aa8 100644 --- a/test/parallel/test-fs-mkdtemp.js +++ b/test/parallel/test-fs-mkdtemp.js @@ -20,7 +20,7 @@ assert(common.fileExists(utf8)); function handler(err, folder) { assert.ifError(err); assert(common.fileExists(folder)); - assert.strictEqual(this, null); + assert.strictEqual(this, undefined); } fs.mkdtemp(path.join(common.tmpDir, 'bar.'), common.mustCall(handler)); diff --git a/test/parallel/test-fs-stat.js b/test/parallel/test-fs-stat.js index 8a6bb1c6245431..332a26e9bff2e8 100644 --- a/test/parallel/test-fs-stat.js +++ b/test/parallel/test-fs-stat.js @@ -66,7 +66,7 @@ fs.open('.', 'r', undefined, common.mustCall(function(err, fd) { // Confirm that we are not running in the context of the internal binding // layer. // Ref: https://github.com/nodejs/node/commit/463d6bac8b349acc462d345a6e298a76f7d06fb1 - assert.strictEqual(this, null); + assert.strictEqual(this, undefined); })); // fstatSync From 23918c4cd4e5a5ff0e1f9bf0a46465a33a8936d9 Mon Sep 17 00:00:00 2001 From: Rich Trott Date: Sat, 5 Aug 2017 18:41:10 -0700 Subject: [PATCH 3/5] test: check `this` value for `nextTick()` Depending on how many arguments are provided, `nextTick()` may run its callback with `this` set to `null` or not. Add assertions for both cases. --- test/parallel/test-next-tick.js | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/test/parallel/test-next-tick.js b/test/parallel/test-next-tick.js index 9c69efeb784d64..511d0559cd1e94 100644 --- a/test/parallel/test-next-tick.js +++ b/test/parallel/test-next-tick.js @@ -21,6 +21,7 @@ 'use strict'; const common = require('../common'); + const assert = require('assert'); process.nextTick(common.mustCall(function() { @@ -40,8 +41,23 @@ const obj = {}; process.nextTick(function(a, b) { assert.strictEqual(a, 42); assert.strictEqual(b, obj); + assert.strictEqual(this, undefined); }, 42, obj); +process.nextTick((a, b) => { + assert.strictEqual(a, 42); + assert.strictEqual(b, obj); + assert.deepStrictEqual(this, {}); +}, 42, obj); + +process.nextTick(function() { + assert.strictEqual(this, null); +}, 1, 2, 3, 4); + +process.nextTick(() => { + assert.deepStrictEqual(this, {}); +}, 1, 2, 3, 4); + process.on('exit', function() { process.nextTick(common.mustNotCall()); }); From aa07e873401a8251d62800eed315a9a4fc0b2071 Mon Sep 17 00:00:00 2001 From: Rich Trott Date: Sat, 5 Aug 2017 19:10:21 -0700 Subject: [PATCH 4/5] process: make `this` value consistent The value of `this` for callbacks of `nextTick()` can vary depending on the number of arguments. Make it consistent. --- lib/internal/process/next_tick.js | 2 +- test/parallel/test-next-tick.js | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/internal/process/next_tick.js b/lib/internal/process/next_tick.js index 685fa6717bd9f1..00698aee23020b 100644 --- a/lib/internal/process/next_tick.js +++ b/lib/internal/process/next_tick.js @@ -141,7 +141,7 @@ function setupNextTick() { callback(args[0], args[1], args[2]); break; default: - callback.apply(null, args); + callback(...args); } } } diff --git a/test/parallel/test-next-tick.js b/test/parallel/test-next-tick.js index 511d0559cd1e94..47823f45bcf7e2 100644 --- a/test/parallel/test-next-tick.js +++ b/test/parallel/test-next-tick.js @@ -51,7 +51,7 @@ process.nextTick((a, b) => { }, 42, obj); process.nextTick(function() { - assert.strictEqual(this, null); + assert.strictEqual(this, undefined); }, 1, 2, 3, 4); process.nextTick(() => { From 64e1f005cd9287894ce16a4c644d158c272b7714 Mon Sep 17 00:00:00 2001 From: Rich Trott Date: Sat, 5 Aug 2017 22:08:59 -0700 Subject: [PATCH 5/5] benchmark: cover more nextTick() code The benchmarks for `process.nextTick()` do not cover the `default` case in the internal code's `switch` statement where the callback receives more than 3 arguments. Modify two of the benchmarks to include this condition. --- benchmark/process/next-tick-breadth-args.js | 9 +++++++- benchmark/process/next-tick-depth-args.js | 25 ++++++++++++++++++--- 2 files changed, 30 insertions(+), 4 deletions(-) diff --git a/benchmark/process/next-tick-breadth-args.js b/benchmark/process/next-tick-breadth-args.js index e617fcfc824f7f..941408f2edba9a 100644 --- a/benchmark/process/next-tick-breadth-args.js +++ b/benchmark/process/next-tick-breadth-args.js @@ -24,10 +24,17 @@ function main(conf) { if (n === N) bench.end(n / 1e6); } + function cb4(arg1, arg2, arg3, arg4) { + n++; + if (n === N) + bench.end(n / 1e6); + } bench.start(); for (var i = 0; i < N; i++) { - if (i % 3 === 0) + if (i % 4 === 0) + process.nextTick(cb4, 3.14, 1024, true, false); + else if (i % 3 === 0) process.nextTick(cb3, 512, true, null); else if (i % 2 === 0) process.nextTick(cb2, false, 5.1); diff --git a/benchmark/process/next-tick-depth-args.js b/benchmark/process/next-tick-depth-args.js index 06bea4676b3615..7cac87b014487b 100644 --- a/benchmark/process/next-tick-depth-args.js +++ b/benchmark/process/next-tick-depth-args.js @@ -10,9 +10,24 @@ process.maxTickDepth = Infinity; function main(conf) { var n = +conf.millions * 1e6; + function cb4(arg1, arg2, arg3, arg4) { + if (--n) { + if (n % 4 === 0) + process.nextTick(cb4, 3.14, 1024, true, false); + else if (n % 3 === 0) + process.nextTick(cb3, 512, true, null); + else if (n % 2 === 0) + process.nextTick(cb2, false, 5.1); + else + process.nextTick(cb1, 0); + } else + bench.end(+conf.millions); + } function cb3(arg1, arg2, arg3) { if (--n) { - if (n % 3 === 0) + if (n % 4 === 0) + process.nextTick(cb4, 3.14, 1024, true, false); + else if (n % 3 === 0) process.nextTick(cb3, 512, true, null); else if (n % 2 === 0) process.nextTick(cb2, false, 5.1); @@ -23,7 +38,9 @@ function main(conf) { } function cb2(arg1, arg2) { if (--n) { - if (n % 3 === 0) + if (n % 4 === 0) + process.nextTick(cb4, 3.14, 1024, true, false); + else if (n % 3 === 0) process.nextTick(cb3, 512, true, null); else if (n % 2 === 0) process.nextTick(cb2, false, 5.1); @@ -34,7 +51,9 @@ function main(conf) { } function cb1(arg1) { if (--n) { - if (n % 3 === 0) + if (n % 4 === 0) + process.nextTick(cb4, 3.14, 1024, true, false); + else if (n % 3 === 0) process.nextTick(cb3, 512, true, null); else if (n % 2 === 0) process.nextTick(cb2, false, 5.1);