Skip to content

Commit

Permalink
tools,test: enforce assert.ifError with lint rule
Browse files Browse the repository at this point in the history
This adds an ESLint rule to enforce the use of `assert.ifError(err)`
instead of `if (err) throw err;` in tests.

PR-URL: #10671
Ref: #10543
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Michal Zasso <targos@protonmail.com>
  • Loading branch information
not-an-aardvark authored and italoacasas committed Jan 30, 2017
1 parent 727c5e3 commit 3adda4b
Show file tree
Hide file tree
Showing 10 changed files with 63 additions and 21 deletions.
1 change: 1 addition & 0 deletions test/.eslintrc.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -3,4 +3,5 @@
rules:
## common module is mandatory in tests
required-modules: [2, common]
prefer-assert-iferror: 2
prefer-assert-methods: 2
2 changes: 1 addition & 1 deletion test/parallel/test-crypto-pbkdf2.js
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ assert.strictEqual(key.toString('hex'), expected);

crypto.pbkdf2('password', 'salt', 32, 32, 'sha256', common.mustCall(ondone));
function ondone(err, key) {
if (err) throw err;
assert.ifError(err);
assert.strictEqual(key.toString('hex'), expected);
}

Expand Down
5 changes: 3 additions & 2 deletions test/parallel/test-fs-long-path.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
const common = require('../common');
const fs = require('fs');
const path = require('path');
const assert = require('assert');

if (!common.isWindows) {
common.skip('this test is Windows-specific.');
Expand All @@ -21,10 +22,10 @@ console.log({
});

fs.writeFile(fullPath, 'ok', common.mustCall(function(err) {
if (err) throw err;
assert.ifError(err);

fs.stat(fullPath, common.mustCall(function(err, stats) {
if (err) throw err;
assert.ifError(err);
}));
}));

Expand Down
2 changes: 1 addition & 1 deletion test/parallel/test-fs-readfile-pipe.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ const dataExpected = fs.readFileSync(__filename, 'utf8');

if (process.argv[2] === 'child') {
fs.readFile('/dev/stdin', function(er, data) {
if (er) throw er;
assert.ifError(er);
process.stdout.write(data);
});
return;
Expand Down
2 changes: 1 addition & 1 deletion test/parallel/test-fs-symlink-dir-junction-relative.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ common.refreshTmpDir();

// Test fs.symlink()
fs.symlink(linkData, linkPath1, 'junction', common.mustCall(function(err) {
if (err) throw err;
assert.ifError(err);
verifyLink(linkPath1);
}));

Expand Down
8 changes: 4 additions & 4 deletions test/parallel/test-fs-symlink-dir-junction.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,18 +14,18 @@ console.log('linkData: ' + linkData);
console.log('linkPath: ' + linkPath);

fs.symlink(linkData, linkPath, 'junction', common.mustCall(function(err) {
if (err) throw err;
assert.ifError(err);

fs.lstat(linkPath, common.mustCall(function(err, stats) {
if (err) throw err;
assert.ifError(err);
assert.ok(stats.isSymbolicLink());

fs.readlink(linkPath, common.mustCall(function(err, destination) {
if (err) throw err;
assert.ifError(err);
assert.strictEqual(destination, linkData);

fs.unlink(linkPath, common.mustCall(function(err) {
if (err) throw err;
assert.ifError(err);
assert(!common.fileExists(linkPath));
assert(common.fileExists(linkData));
}));
Expand Down
12 changes: 6 additions & 6 deletions test/parallel/test-preload.js
Original file line number Diff line number Diff line change
Expand Up @@ -34,15 +34,15 @@ const fixtureThrows = fixture('throws_error4.js');
// test preloading a single module works
childProcess.exec(nodeBinary + ' ' + preloadOption([fixtureA]) + ' ' + fixtureB,
function(err, stdout, stderr) {
if (err) throw err;
assert.ifError(err);
assert.strictEqual(stdout, 'A\nB\n');
});

// test preloading multiple modules works
childProcess.exec(
nodeBinary + ' ' + preloadOption([fixtureA, fixtureB]) + ' ' + fixtureC,
function(err, stdout, stderr) {
if (err) throw err;
assert.ifError(err);
assert.strictEqual(stdout, 'A\nB\nC\n');
}
);
Expand All @@ -63,7 +63,7 @@ childProcess.exec(
childProcess.exec(
nodeBinary + ' ' + preloadOption([fixtureA]) + '-e "console.log(\'hello\');"',
function(err, stdout, stderr) {
if (err) throw err;
assert.ifError(err);
assert.strictEqual(stdout, 'A\nhello\n');
}
);
Expand Down Expand Up @@ -110,7 +110,7 @@ childProcess.exec(
nodeBinary + ' ' + preloadOption([fixtureA]) +
'-e "console.log(\'hello\');" ' + preloadOption([fixtureA, fixtureB]),
function(err, stdout, stderr) {
if (err) throw err;
assert.ifError(err);
assert.strictEqual(stdout, 'A\nB\nhello\n');
}
);
Expand All @@ -131,7 +131,7 @@ childProcess.exec(
nodeBinary + ' ' + '--require ' + fixture('cluster-preload.js') + ' ' +
fixture('cluster-preload-test.js'),
function(err, stdout, stderr) {
if (err) throw err;
assert.ifError(err);
assert.ok(/worker terminated with code 43/.test(stdout));
}
);
Expand All @@ -142,7 +142,7 @@ childProcess.exec(
nodeBinary + ' ' + '--expose_debug_as=v8debug ' + '--require ' +
fixture('cluster-preload.js') + ' ' + 'cluster-preload-test.js',
function(err, stdout, stderr) {
if (err) throw err;
assert.ifError(err);
assert.ok(/worker terminated with code 43/.test(stdout));
}
);
5 changes: 2 additions & 3 deletions test/pummel/test-regress-GH-814.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// Flags: --expose_gc

const common = require('../common');
const assert = require('assert');

function newBuffer(size, value) {
var buffer = Buffer.allocUnsafe(size);
Expand Down Expand Up @@ -59,7 +60,5 @@ var timeToQuit = Date.now() + 8e3; //Test during no more than this seconds.


function cb(err, written) {
if (err) {
throw err;
}
assert.ifError(err);
}
5 changes: 2 additions & 3 deletions test/pummel/test-regress-GH-814_2.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// Flags: --expose_gc

const common = require('../common');
const assert = require('assert');

const fs = require('fs');
const testFileName = require('path').join(common.tmpDir, 'GH-814_test.txt');
Expand Down Expand Up @@ -64,9 +65,7 @@ function writer() {

function writerCB(err, written) {
//console.error('cb.');
if (err) {
throw err;
}
assert.ifError(err);
}


Expand Down
42 changes: 42 additions & 0 deletions tools/eslint-rules/prefer-assert-iferror.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
/**
* @fileoverview Prohibit the `if (err) throw err;` pattern
* @author Teddy Katz
*/

'use strict';

module.exports = {
create(context) {
const sourceCode = context.getSourceCode();

function hasSameTokens(nodeA, nodeB) {
const aTokens = sourceCode.getTokens(nodeA);
const bTokens = sourceCode.getTokens(nodeB);

return aTokens.length === bTokens.length &&
aTokens.every((token, index) => {
return token.type === bTokens[index].type &&
token.value === bTokens[index].value;
});
}

return {
IfStatement(node) {
const firstStatement = node.consequent.type === 'BlockStatement' ?
node.consequent.body[0] :
node.consequent;
if (
firstStatement &&
firstStatement.type === 'ThrowStatement' &&
hasSameTokens(node.test, firstStatement.argument)
) {
context.report({
node: firstStatement,
message: 'Use assert.ifError({{argument}}) instead.',
data: {argument: sourceCode.getText(node.test)}
});
}
}
};
}
};

0 comments on commit 3adda4b

Please sign in to comment.