Skip to content

Commit

Permalink
test: add eslint rule to verify assertion input
Browse files Browse the repository at this point in the history
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: nodejs#20718
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
  • Loading branch information
BridgeAR committed May 18, 2018
1 parent eeb1d51 commit dd9d32f
Show file tree
Hide file tree
Showing 4 changed files with 37 additions and 10 deletions.
27 changes: 27 additions & 0 deletions test/.eslintrc.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
8 changes: 4 additions & 4 deletions test/addons-napi/test_conversions/test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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(''));
Expand Down
8 changes: 4 additions & 4 deletions test/parallel/test-child-process-exec-env.js
Original file line number Diff line number Diff line change
Expand Up @@ -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, '');
}
}

Expand All @@ -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'));
});
4 changes: 2 additions & 2 deletions test/parallel/test-crypto.js
Original file line number Diff line number Diff line change
Expand Up @@ -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'));
Expand All @@ -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());
Expand Down

0 comments on commit dd9d32f

Please sign in to comment.