From dd9d32f94ce5ed6d136c16677ef323239e708e32 Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Mon, 14 May 2018 17:28:26 +0200 Subject: [PATCH] test: add eslint rule to verify assertion input The input for `assert.deepStrictEqual` and similar expect the actual input first and the expected input as second argument. This verifies that this is actually done correct in our tests. This is important so the possible error message actually makes sense. PR-URL: https://github.com/nodejs/node/pull/20718 Reviewed-By: Rich Trott Reviewed-By: Trivikram Kamat Reviewed-By: James M Snell --- test/.eslintrc.yaml | 27 ++++++++++++++++++++ test/addons-napi/test_conversions/test.js | 8 +++--- test/parallel/test-child-process-exec-env.js | 8 +++--- test/parallel/test-crypto.js | 4 +-- 4 files changed, 37 insertions(+), 10 deletions(-) diff --git a/test/.eslintrc.yaml b/test/.eslintrc.yaml index 7c90a5026b2..f07dd076d93 100644 --- a/test/.eslintrc.yaml +++ b/test/.eslintrc.yaml @@ -19,6 +19,33 @@ rules: ## common module is mandatory in tests node-core/required-modules: [error, common] + no-restricted-syntax: + # Config copied from .eslintrc.js + - error + - 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='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]" + message: "assert.throws() must be invoked with at least two arguments." + - selector: "CallExpression[callee.name='setTimeout'][arguments.length<2]" + message: "setTimeout() must be invoked with at least two arguments." + - selector: "CallExpression[callee.name='setInterval'][arguments.length<2]" + message: "setInterval() must be invoked with at least 2 arguments." + - selector: "ThrowStatement > CallExpression[callee.name=/Error$/]" + message: "Use new keyword when throwing an Error." + # Specific to /test + - selector: "CallExpression[callee.object.name='assert'][callee.property.name='notDeepStrictEqual'][arguments.0.type='Literal']:not([arguments.1.type='Literal']):not([arguments.1.type='ObjectExpression']):not([arguments.1.type='ArrayExpression'])" + message: "The first argument should be the `actual`, not the `expected` value." + - selector: "CallExpression[callee.object.name='assert'][callee.property.name='notStrictEqual'][arguments.0.type='Literal']:not([arguments.1.type='Literal']):not([arguments.1.type='ObjectExpression']):not([arguments.1.type='ArrayExpression'])" + message: "The first argument should be the `actual`, not the `expected` value." + - selector: "CallExpression[callee.object.name='assert'][callee.property.name='deepStrictEqual'][arguments.0.type='Literal']:not([arguments.1.type='Literal']):not([arguments.1.type='ObjectExpression']):not([arguments.1.type='ArrayExpression'])" + message: "The first argument should be the `actual`, not the `expected` value." + # TODO: Activate the `strictEqual` rule as soon as it produces less churn. + # - selector: "CallExpression[callee.object.name='assert'][callee.property.name='strictEqual'][arguments.0.type='Literal']:not([arguments.1.type='Literal']):not([arguments.1.type='ObjectExpression']):not([arguments.1.type='ArrayExpression'])" + # message: "The first argument should be the `actual`, not the `expected` value." # Global scoped methods and vars globals: WebAssembly: false diff --git a/test/addons-napi/test_conversions/test.js b/test/addons-napi/test_conversions/test.js index 73d2c3314f6..27e1fb322c4 100644 --- a/test/addons-napi/test_conversions/test.js +++ b/test/addons-napi/test_conversions/test.js @@ -118,10 +118,10 @@ assert.deepStrictEqual(new String(''), test.toObject('')); assert.deepStrictEqual(new Number(0), test.toObject(0)); assert.deepStrictEqual(new Number(Number.NaN), test.toObject(Number.NaN)); assert.deepStrictEqual(new Object(testSym), test.toObject(testSym)); -assert.notDeepStrictEqual(false, test.toObject(false)); -assert.notDeepStrictEqual(true, test.toObject(true)); -assert.notDeepStrictEqual('', test.toObject('')); -assert.notDeepStrictEqual(0, test.toObject(0)); +assert.notDeepStrictEqual(test.toObject(false), false); +assert.notDeepStrictEqual(test.toObject(true), true); +assert.notDeepStrictEqual(test.toObject(''), ''); +assert.notDeepStrictEqual(test.toObject(0), 0); assert.ok(!Number.isNaN(test.toObject(Number.NaN))); assert.strictEqual('', test.toString('')); diff --git a/test/parallel/test-child-process-exec-env.js b/test/parallel/test-child-process-exec-env.js index c34fdb7b49a..41aaf992cfc 100644 --- a/test/parallel/test-child-process-exec-env.js +++ b/test/parallel/test-child-process-exec-env.js @@ -34,10 +34,10 @@ function after(err, stdout, stderr) { console.log(`error!: ${err.code}`); console.log(`stdout: ${JSON.stringify(stdout)}`); console.log(`stderr: ${JSON.stringify(stderr)}`); - assert.strictEqual(false, err.killed); + assert.strictEqual(err.killed, false); } else { success_count++; - assert.notStrictEqual('', stdout); + assert.notStrictEqual(stdout, ''); } } @@ -56,7 +56,7 @@ child.stdout.on('data', function(chunk) { process.on('exit', function() { console.log('response: ', response); - assert.strictEqual(1, success_count); - assert.strictEqual(0, error_count); + assert.strictEqual(success_count, 1); + assert.strictEqual(error_count, 0); assert.ok(response.includes('HELLO=WORLD')); }); diff --git a/test/parallel/test-crypto.js b/test/parallel/test-crypto.js index 9a93e8422fd..88d49167100 100644 --- a/test/parallel/test-crypto.js +++ b/test/parallel/test-crypto.js @@ -129,7 +129,7 @@ assert(tlsCiphers.every((value) => noCapitals.test(value))); validateList(tlsCiphers); // Assert that we have sha1 and sha256 but not SHA1 and SHA256. -assert.notStrictEqual(0, crypto.getHashes().length); +assert.notStrictEqual(crypto.getHashes().length, 0); assert(crypto.getHashes().includes('sha1')); assert(crypto.getHashes().includes('sha256')); assert(!crypto.getHashes().includes('SHA1')); @@ -139,7 +139,7 @@ assert(!crypto.getHashes().includes('rsa-sha1')); validateList(crypto.getHashes()); // Assume that we have at least secp384r1. -assert.notStrictEqual(0, crypto.getCurves().length); +assert.notStrictEqual(crypto.getCurves().length, 0); assert(crypto.getCurves().includes('secp384r1')); assert(!crypto.getCurves().includes('SECP384R1')); validateList(crypto.getCurves());