From 8b69c00975d4d3f6a6870e0150509aaf54033bae Mon Sep 17 00:00:00 2001 From: Rich Trott Date: Thu, 13 Sep 2018 13:59:13 -0700 Subject: [PATCH 01/12] test: remove string literal message from assertion Remove string literal message in assert.strictEqual() call in napi test testFinalizer.js. PR-URL: https://github.com/nodejs/node/pull/22849 Reviewed-By: Teddy Katz Reviewed-By: Matteo Collina Reviewed-By: Anna Henningsen Reviewed-By: Trivikram Kamat Reviewed-By: Luigi Pinca Reviewed-By: Colin Ihrig --- test/addons-napi/test_general/testFinalizer.js | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/test/addons-napi/test_general/testFinalizer.js b/test/addons-napi/test_general/testFinalizer.js index d5df4c8c724f98..b440ed5e501ad8 100644 --- a/test/addons-napi/test_general/testFinalizer.js +++ b/test/addons-napi/test_general/testFinalizer.js @@ -29,5 +29,4 @@ test_general.wrap(finalizeAndWrap); test_general.addFinalizerOnly(finalizeAndWrap, common.mustCall()); finalizeAndWrap = null; global.gc(); -assert.strictEqual(test_general.derefItemWasCalled(), true, - 'finalize callback was called'); +assert.strictEqual(test_general.derefItemWasCalled(), true); From 9579f7aa2b8dfcb0c42f38944b7571575212a0ab Mon Sep 17 00:00:00 2001 From: Rich Trott Date: Thu, 13 Sep 2018 13:56:36 -0700 Subject: [PATCH 02/12] test: remove string literal message in assertions Remove string literal message in assert.strictEqual() calls in test-async-await.js. PR-URL: https://github.com/nodejs/node/pull/22849 Reviewed-By: Teddy Katz Reviewed-By: Matteo Collina Reviewed-By: Anna Henningsen Reviewed-By: Trivikram Kamat Reviewed-By: Luigi Pinca Reviewed-By: Colin Ihrig --- test/async-hooks/test-async-await.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/test/async-hooks/test-async-await.js b/test/async-hooks/test-async-await.js index 7f88cd9b18176f..f5e886e9d50001 100644 --- a/test/async-hooks/test-async-await.js +++ b/test/async-hooks/test-async-await.js @@ -64,15 +64,15 @@ const timeout = common.platformTimeout(10); function checkPromisesInitState() { for (const initState of promisesInitState.values()) { - assert.strictEqual(initState, 'resolved', - 'promise initialized without being resolved'); + // Promise should not be initialized without being resolved. + assert.strictEqual(initState, 'resolved'); } } function checkPromisesExecutionState() { for (const executionState of promisesExecutionState.values()) { - assert.strictEqual(executionState, 'after', - 'mismatch between before and after hook calls'); + // Check for mismatch between before and after hook calls. + assert.strictEqual(executionState, 'after'); } } From efe0d1780fa5e252167404ed18193a545dc7d347 Mon Sep 17 00:00:00 2001 From: Rich Trott Date: Sat, 1 Sep 2018 22:47:37 -0700 Subject: [PATCH 03/12] test: improve assertion in test-inspector.js Remove an unecessary string literal from assert.strictEqual() call in test-inspector.js. The string literal is printed instead of the value that causes an error. Removing the string literal allows the value that caused the error to be printed. This improves the troubleshooting experience when the test fails due to that assertion. PR-URL: https://github.com/nodejs/node/pull/22849 Reviewed-By: Teddy Katz Reviewed-By: Matteo Collina Reviewed-By: Anna Henningsen Reviewed-By: Trivikram Kamat Reviewed-By: Luigi Pinca Reviewed-By: Colin Ihrig --- test/sequential/test-inspector.js | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/test/sequential/test-inspector.js b/test/sequential/test-inspector.js index ba1fce25a04fad..d641015a3c9954 100644 --- a/test/sequential/test-inspector.js +++ b/test/sequential/test-inspector.js @@ -33,8 +33,7 @@ function checkBadPath(err) { } function checkException(message) { - assert.strictEqual(message.exceptionDetails, undefined, - 'An exception occurred during execution'); + assert.strictEqual(message.exceptionDetails, undefined); } function assertScopeValues({ result }, expected) { From d1c0f51c2d3e4ab58db643519bc55d3579b81103 Mon Sep 17 00:00:00 2001 From: Rich Trott Date: Sun, 2 Sep 2018 10:55:33 -0700 Subject: [PATCH 04/12] test: simplify assertion in http2 tests In test-http2-timeout-large-write.js and test-http2-timeout-large-write-file.js: Use assert.ok() on a boolean that the test itself creates and sets, rather than assert.strictEqual(). This allows us to use a static message without running afoul of the upcoming "do not use string literals with assert.strictEqual()" lint rule. PR-URL: https://github.com/nodejs/node/pull/22849 Reviewed-By: Teddy Katz Reviewed-By: Matteo Collina Reviewed-By: Anna Henningsen Reviewed-By: Trivikram Kamat Reviewed-By: Luigi Pinca Reviewed-By: Colin Ihrig --- test/sequential/test-http2-timeout-large-write-file.js | 2 +- test/sequential/test-http2-timeout-large-write.js | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/test/sequential/test-http2-timeout-large-write-file.js b/test/sequential/test-http2-timeout-large-write-file.js index 910e7a0fc497bd..bb366cfff04091 100644 --- a/test/sequential/test-http2-timeout-large-write-file.js +++ b/test/sequential/test-http2-timeout-large-write-file.js @@ -48,7 +48,7 @@ server.on('stream', common.mustCall((stream) => { })); server.setTimeout(serverTimeout); server.on('timeout', () => { - assert.strictEqual(didReceiveData, false, 'Should not timeout'); + assert.ok(!didReceiveData, 'Should not timeout'); }); server.listen(0, common.mustCall(() => { diff --git a/test/sequential/test-http2-timeout-large-write.js b/test/sequential/test-http2-timeout-large-write.js index a15fb46af6d28a..73114776df0a0e 100644 --- a/test/sequential/test-http2-timeout-large-write.js +++ b/test/sequential/test-http2-timeout-large-write.js @@ -40,13 +40,13 @@ server.on('stream', common.mustCall((stream) => { stream.write(content); stream.setTimeout(serverTimeout); stream.on('timeout', () => { - assert.strictEqual(didReceiveData, false, 'Should not timeout'); + assert.ok(!didReceiveData, 'Should not timeout'); }); stream.end(); })); server.setTimeout(serverTimeout); server.on('timeout', () => { - assert.strictEqual(didReceiveData, false, 'Should not timeout'); + assert.ok(!didReceiveData, 'Should not timeout'); }); server.listen(0, common.mustCall(() => { From c924e1674ca5ccb3b82b05529d8512662b1224aa Mon Sep 17 00:00:00 2001 From: Rich Trott Date: Wed, 12 Sep 2018 20:16:23 -0700 Subject: [PATCH 05/12] test: refactor flag check Refactor test-vm-run-in-new-context so that check for `--expose-gc` flag will not run afoul of an upcoming lint rule that checks that string literals are not used for the `message` argument of `assert.strictEqual()`. PR-URL: https://github.com/nodejs/node/pull/22849 Reviewed-By: Teddy Katz Reviewed-By: Matteo Collina Reviewed-By: Anna Henningsen Reviewed-By: Trivikram Kamat Reviewed-By: Luigi Pinca Reviewed-By: Colin Ihrig --- test/parallel/test-vm-run-in-new-context.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/parallel/test-vm-run-in-new-context.js b/test/parallel/test-vm-run-in-new-context.js index e844cd6816bba3..51577668cfe7f8 100644 --- a/test/parallel/test-vm-run-in-new-context.js +++ b/test/parallel/test-vm-run-in-new-context.js @@ -26,8 +26,8 @@ const common = require('../common'); const assert = require('assert'); const vm = require('vm'); -assert.strictEqual(typeof global.gc, 'function', - 'Run this test with --expose-gc'); +if (typeof global.gc !== 'function') + assert.fail('Run this test with --expose-gc'); // Run a string const result = vm.runInNewContext('\'passed\';'); From bc9ae8ebc4312efee61c1deec602acf1e84f8a46 Mon Sep 17 00:00:00 2001 From: Rich Trott Date: Wed, 12 Sep 2018 20:24:28 -0700 Subject: [PATCH 06/12] test: remove string literal from assertion Remove string literal from `assert.strictEqual()` call `message` parameter and make it a comment above the assertion instead. PR-URL: https://github.com/nodejs/node/pull/22849 Reviewed-By: Teddy Katz Reviewed-By: Matteo Collina Reviewed-By: Anna Henningsen Reviewed-By: Trivikram Kamat Reviewed-By: Luigi Pinca Reviewed-By: Colin Ihrig --- test/parallel/test-next-tick-domain.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/parallel/test-next-tick-domain.js b/test/parallel/test-next-tick-domain.js index 5c526197926df7..3e55ef3225fc40 100644 --- a/test/parallel/test-next-tick-domain.js +++ b/test/parallel/test-next-tick-domain.js @@ -27,5 +27,5 @@ const origNextTick = process.nextTick; require('domain'); -assert.strictEqual(origNextTick, process.nextTick, - 'Requiring domain should not change nextTick'); +// Requiring domain should not change nextTick. +assert.strictEqual(origNextTick, process.nextTick); From 8e4fc42f8e909850a228bb24494e2e25a14d707f Mon Sep 17 00:00:00 2001 From: Rich Trott Date: Wed, 12 Sep 2018 21:08:53 -0700 Subject: [PATCH 07/12] test: remove string literal message from assertion Remove string literal from assert.strictEqual message to improve output of AssertionError. PR-URL: https://github.com/nodejs/node/pull/22849 Reviewed-By: Teddy Katz Reviewed-By: Matteo Collina Reviewed-By: Anna Henningsen Reviewed-By: Trivikram Kamat Reviewed-By: Luigi Pinca Reviewed-By: Colin Ihrig --- test/parallel/test-http-information-processing.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/parallel/test-http-information-processing.js b/test/parallel/test-http-information-processing.js index af589477f05dd1..43d1bdafdf4dd1 100644 --- a/test/parallel/test-http-information-processing.js +++ b/test/parallel/test-http-information-processing.js @@ -36,8 +36,8 @@ server.listen(0, function() { }); req.on('response', function(res) { - assert.strictEqual(countdown.remaining, 1, - 'Full response received before all 102 Processing'); + // Check that all 102 Processing received before full response received. + assert.strictEqual(countdown.remaining, 1); assert.strictEqual(200, res.statusCode, `Final status code was ${res.statusCode}, not 200.`); res.setEncoding('utf8'); From 495bb6d621c1ed146fe9f6fb2cba8d697a92d834 Mon Sep 17 00:00:00 2001 From: Rich Trott Date: Wed, 12 Sep 2018 21:10:45 -0700 Subject: [PATCH 08/12] test: remove string literal arg from assertion Remove unnecessary string literal from assert.deepStrictEqual() call. PR-URL: https://github.com/nodejs/node/pull/22849 Reviewed-By: Teddy Katz Reviewed-By: Matteo Collina Reviewed-By: Anna Henningsen Reviewed-By: Trivikram Kamat Reviewed-By: Luigi Pinca Reviewed-By: Colin Ihrig --- test/parallel/test-fs-readfile.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/parallel/test-fs-readfile.js b/test/parallel/test-fs-readfile.js index 118f2e43aab500..648bf692d1dcc8 100644 --- a/test/parallel/test-fs-readfile.js +++ b/test/parallel/test-fs-readfile.js @@ -54,6 +54,6 @@ for (const e of fileInfo) { fs.readFile(e.name, common.mustCall((err, buf) => { console.log(`Validating readFile on file ${e.name} of length ${e.len}`); assert.ifError(err); - assert.deepStrictEqual(buf, e.contents, 'Incorrect file contents'); + assert.deepStrictEqual(buf, e.contents); })); } From 1db83a7e0ba5e75865b562b1308b4a8077629de3 Mon Sep 17 00:00:00 2001 From: Rich Trott Date: Thu, 13 Sep 2018 13:04:09 -0700 Subject: [PATCH 09/12] test: remove string literal from assertion Remove string literal as assertion message in call to assert.strictEqual() in test-dns-resolveany-bad-ancount. PR-URL: https://github.com/nodejs/node/pull/22849 Reviewed-By: Teddy Katz Reviewed-By: Matteo Collina Reviewed-By: Anna Henningsen Reviewed-By: Trivikram Kamat Reviewed-By: Luigi Pinca Reviewed-By: Colin Ihrig --- test/parallel/test-dns-resolveany-bad-ancount.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/parallel/test-dns-resolveany-bad-ancount.js b/test/parallel/test-dns-resolveany-bad-ancount.js index 4b13421b316aee..71fcbe03cd58f1 100644 --- a/test/parallel/test-dns-resolveany-bad-ancount.js +++ b/test/parallel/test-dns-resolveany-bad-ancount.js @@ -40,8 +40,8 @@ server.bind(0, common.mustCall(async () => { assert.strictEqual(err.syscall, 'queryAny'); assert.strictEqual(err.hostname, 'example.org'); const descriptor = Object.getOwnPropertyDescriptor(err, 'message'); - assert.strictEqual(descriptor.enumerable, - false, 'The error message should be non-enumerable'); + // The error message should be non-enumerable. + assert.strictEqual(descriptor.enumerable, false); server.close(); })); })); From b423af60439b38e48c9105cf6eca20ad24b5fc0c Mon Sep 17 00:00:00 2001 From: Rich Trott Date: Thu, 13 Sep 2018 13:40:11 -0700 Subject: [PATCH 10/12] test: remove string literal from assertion Remove string literal as assertion message in call to assert.strictEqual() in test-dns-lookup. PR-URL: https://github.com/nodejs/node/pull/22849 Reviewed-By: Teddy Katz Reviewed-By: Matteo Collina Reviewed-By: Anna Henningsen Reviewed-By: Trivikram Kamat Reviewed-By: Luigi Pinca Reviewed-By: Colin Ihrig --- test/parallel/test-dns-lookup.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/parallel/test-dns-lookup.js b/test/parallel/test-dns-lookup.js index 3413bcffd8abe9..c9b54b9faf95ef 100644 --- a/test/parallel/test-dns-lookup.js +++ b/test/parallel/test-dns-lookup.js @@ -134,8 +134,8 @@ dns.lookup('example.com', common.mustCall((error, result, addressType) => { assert.strictEqual(tickValue, 1); assert.strictEqual(error.code, 'ENOENT'); const descriptor = Object.getOwnPropertyDescriptor(error, 'message'); - assert.strictEqual(descriptor.enumerable, - false, 'The error message should be non-enumerable'); + // The error message should be non-enumerable. + assert.strictEqual(descriptor.enumerable, false); })); // Make sure that the error callback is called From dcf4a35c7f576f9f2d2cdce5c25ca77e79cb7c4a Mon Sep 17 00:00:00 2001 From: Rich Trott Date: Thu, 13 Sep 2018 13:47:57 -0700 Subject: [PATCH 11/12] test: prepare test-assert for strictEqual linting Make minor modifications to test-assert.js to prepare it for linting rule that forbids the use of string literals for the third argument of assert.strictEqual(). PR-URL: https://github.com/nodejs/node/pull/22849 Reviewed-By: Teddy Katz Reviewed-By: Matteo Collina Reviewed-By: Anna Henningsen Reviewed-By: Trivikram Kamat Reviewed-By: Luigi Pinca Reviewed-By: Colin Ihrig --- test/parallel/test-assert.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/parallel/test-assert.js b/test/parallel/test-assert.js index 6449fe1c3f0cf1..257c7f5c06ebf7 100644 --- a/test/parallel/test-assert.js +++ b/test/parallel/test-assert.js @@ -304,11 +304,11 @@ try { } try { - assert.strictEqual(1, 2, 'oh no'); + assert.strictEqual(1, 2, 'oh no'); // eslint-disable-line no-restricted-syntax } catch (e) { assert.strictEqual(e.message, 'oh no'); - assert.strictEqual(e.generatedMessage, false, - 'Message incorrectly marked as generated'); + // Message should not be marked as generated. + assert.strictEqual(e.generatedMessage, false); } { From f6f5920b2f62f66d6601b856791bb20868ba94e9 Mon Sep 17 00:00:00 2001 From: Rich Trott Date: Mon, 30 Jul 2018 16:33:41 -0700 Subject: [PATCH 12/12] tools: prevent string literals in some assertions String literals provided as the third argument to assert.strictEqual() or assert.deepStrictEqual() will mask the values that are causing issues. Use a lint rule to prevent such usage. PR-URL: https://github.com/nodejs/node/pull/22849 Reviewed-By: Teddy Katz Reviewed-By: Matteo Collina Reviewed-By: Anna Henningsen Reviewed-By: Trivikram Kamat Reviewed-By: Luigi Pinca Reviewed-By: Colin Ihrig --- .eslintrc.js | 5 +++++ lib/.eslintrc.yaml | 4 ++++ test/.eslintrc.yaml | 4 ++++ .../test-timers-same-timeout-wrong-list-deleted.js | 8 +++----- test/parallel/test-timers-unref-reuse-no-exposed-list.js | 6 ++---- 5 files changed, 18 insertions(+), 9 deletions(-) diff --git a/.eslintrc.js b/.eslintrc.js index 9c3a78d81cd27a..e4192c3539925c 100644 --- a/.eslintrc.js +++ b/.eslintrc.js @@ -156,6 +156,7 @@ module.exports = { ], /* eslint-disable max-len */ // If this list is modified, please copy the change to lib/.eslintrc.yaml + // and test/.eslintrc.yaml. 'no-restricted-syntax': [ 'error', { @@ -166,6 +167,10 @@ module.exports = { selector: "CallExpression[callee.object.name='assert'][callee.property.name='rejects'][arguments.length<2]", message: 'assert.rejects() must be invoked with at least two arguments.', }, + { + selector: "CallExpression[callee.object.name='assert'][callee.property.name='strictEqual'][arguments.2.type='Literal']", + message: 'Do not use a literal for the third argument of assert.strictEqual()' + }, { selector: "CallExpression[callee.object.name='assert'][callee.property.name='throws'][arguments.1.type='Literal']:not([arguments.1.regex])", message: 'Use an object as second argument of assert.throws()', diff --git a/lib/.eslintrc.yaml b/lib/.eslintrc.yaml index e7cab0ad931b4d..a1a0b9490a9177 100644 --- a/lib/.eslintrc.yaml +++ b/lib/.eslintrc.yaml @@ -2,10 +2,14 @@ rules: no-restricted-syntax: # Config copied from .eslintrc.js - error + - selector: "CallExpression[callee.object.name='assert'][callee.property.name='deepStrictEqual'][arguments.2.type='Literal']" + message: "Do not use a literal for the third argument of assert.deepStrictEqual()" - selector: "CallExpression[callee.object.name='assert'][callee.property.name='doesNotThrow']" message: "Please replace `assert.doesNotThrow()` and add a comment next to the code instead." - selector: "CallExpression[callee.object.name='assert'][callee.property.name='rejects'][arguments.length<2]" message: "assert.rejects() must be invoked with at least two arguments." + - selector: "CallExpression[callee.object.name='assert'][callee.property.name='strictEqual'][arguments.2.type='Literal']" + message: "Do not use a literal for the third argument of assert.strictEqual()" - selector: "CallExpression[callee.object.name='assert'][callee.property.name='throws'][arguments.1.type='Literal']:not([arguments.1.regex])" message: "Use an object as second argument of assert.throws()" - selector: "CallExpression[callee.object.name='assert'][callee.property.name='throws'][arguments.length<2]" diff --git a/test/.eslintrc.yaml b/test/.eslintrc.yaml index 25026fec5a103a..63d2127d738597 100644 --- a/test/.eslintrc.yaml +++ b/test/.eslintrc.yaml @@ -23,10 +23,14 @@ rules: no-restricted-syntax: # Config copied from .eslintrc.js - error + - selector: "CallExpression[callee.object.name='assert'][callee.property.name='deepStrictEqual'][arguments.2.type='Literal']" + message: "Do not use a literal for the third argument of assert.deepStrictEqual()" - selector: "CallExpression[callee.object.name='assert'][callee.property.name='doesNotThrow']" message: "Please replace `assert.doesNotThrow()` and add a comment next to the code instead." - selector: "CallExpression[callee.object.name='assert'][callee.property.name='rejects'][arguments.length<2]" message: "assert.rejects() must be invoked with at least two arguments." + - selector: "CallExpression[callee.object.name='assert'][callee.property.name='strictEqual'][arguments.2.type='Literal']" + message: "Do not use a literal for the third argument of assert.strictEqual()" - selector: "CallExpression[callee.object.name='assert'][callee.property.name='throws'][arguments.1.type='Literal']:not([arguments.1.regex])" message: "Use an object as second argument of assert.throws()" - selector: "CallExpression[callee.object.name='assert'][callee.property.name='throws'][arguments.length<2]" diff --git a/test/parallel/test-timers-same-timeout-wrong-list-deleted.js b/test/parallel/test-timers-same-timeout-wrong-list-deleted.js index c66ba0a57efa11..8ad267cd16f699 100644 --- a/test/parallel/test-timers-same-timeout-wrong-list-deleted.js +++ b/test/parallel/test-timers-same-timeout-wrong-list-deleted.js @@ -37,7 +37,7 @@ const handle1 = setTimeout(common.mustCall(function() { // Make sure our clearTimeout succeeded. One timer finished and // the other was canceled, so none should be active. - assert.strictEqual(activeTimers.length, 0, 'Timers remain.'); + assert.strictEqual(activeTimers.length, 0); })); })); }), 1); @@ -53,11 +53,9 @@ const handle1 = setTimeout(common.mustCall(function() { // Make sure our clearTimeout succeeded. One timer finished and // the other was canceled, so none should be active. - assert.strictEqual(activeTimers.length, 3, - 'There should be 3 timers in the list.'); + assert.strictEqual(activeTimers.length, 3); assert(shortTimer instanceof Timer, 'The shorter timer is not in the list.'); - assert.strictEqual(longTimers.length, 2, - 'Both longer timers should be in the list.'); + assert.strictEqual(longTimers.length, 2); // When this callback completes, `listOnTimeout` should now look at the // correct list and refrain from removing the new TIMEOUT list which diff --git a/test/parallel/test-timers-unref-reuse-no-exposed-list.js b/test/parallel/test-timers-unref-reuse-no-exposed-list.js index 33b2da2f9e214f..269a2b5e85fc1d 100644 --- a/test/parallel/test-timers-unref-reuse-no-exposed-list.js +++ b/test/parallel/test-timers-unref-reuse-no-exposed-list.js @@ -4,11 +4,9 @@ require('../common'); const assert = require('assert'); const timer1 = setTimeout(() => {}, 1).unref(); -assert.strictEqual(timer1._handle._list, undefined, - 'timer1._handle._list should be undefined'); +assert.strictEqual(timer1._handle._list, undefined); // Check that everything works even if the handle was not re-used. setTimeout(() => {}, 1); const timer2 = setTimeout(() => {}, 1).unref(); -assert.strictEqual(timer2._handle._list, undefined, - 'timer2._handle._list should be undefined'); +assert.strictEqual(timer2._handle._list, undefined);