tools,test: enforce assert.ifError with lint rule

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>
nodejs · Jan 30, 2017 · 3adda4b · 3adda4b
1 parent 727c5e3
commit 3adda4b
Show file tree

Hide file tree

Showing 10 changed files with 63 additions and 21 deletions.
diff --git a/test/.eslintrc.yaml b/test/.eslintrc.yaml
@@ -3,4 +3,5 @@
 rules:
   ## common module is mandatory in tests
   required-modules: [2, common]
+  prefer-assert-iferror: 2
   prefer-assert-methods: 2
diff --git a/test/parallel/test-crypto-pbkdf2.js b/test/parallel/test-crypto-pbkdf2.js
@@ -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);
 }
 

diff --git a/test/parallel/test-fs-long-path.js b/test/parallel/test-fs-long-path.js
@@ -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.');
@@ -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);
   }));
 }));
 

diff --git a/test/parallel/test-fs-readfile-pipe.js b/test/parallel/test-fs-readfile-pipe.js
@@ -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;

diff --git a/test/parallel/test-fs-symlink-dir-junction-relative.js b/test/parallel/test-fs-symlink-dir-junction-relative.js
@@ -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);
 }));
 

diff --git a/test/parallel/test-fs-symlink-dir-junction.js b/test/parallel/test-fs-symlink-dir-junction.js
@@ -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));
       }));

diff --git a/test/parallel/test-preload.js b/test/parallel/test-preload.js
@@ -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');
   }
 );
@@ -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');
   }
 );
@@ -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');
   }
 );
@@ -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));
   }
 );
@@ -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));
   }
 );
diff --git a/test/pummel/test-regress-GH-814.js b/test/pummel/test-regress-GH-814.js
@@ -2,6 +2,7 @@
 // Flags: --expose_gc
 
 const common = require('../common');
+const assert = require('assert');
 
 function newBuffer(size, value) {
   var buffer = Buffer.allocUnsafe(size);
@@ -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);
 }
diff --git a/test/pummel/test-regress-GH-814_2.js b/test/pummel/test-regress-GH-814_2.js
@@ -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');
@@ -64,9 +65,7 @@ function writer() {
 
 function writerCB(err, written) {
   //console.error('cb.');
-  if (err) {
-    throw err;
-  }
+  assert.ifError(err);
 }
 
 

diff --git a/tools/eslint-rules/prefer-assert-iferror.js b/tools/eslint-rules/prefer-assert-iferror.js
@@ -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)}
+          });
+        }
+      }
+    };
+  }
+};