Repository: lo1tuma/eslint-plugin-mocha (ts)

Issue:
Rule proposal: forbid top level before / after hooks 
Those should be only used in a describe block 

Pull request:
Add `no-top-level-hooks` rule (fixes #37)
Add `no-top-level-hooks` rule (fixes #37)

Since this rule has a lot of common code with `no-hooks` and `no-sibling-hooks`, I refactored them all to use common methods. It's all much cleaner now, but it might be a lot to look at for one PR. Let me know if you wish for me to split this PR up into two (one for the work on this rule and one for the refactoring)

Patch:
diff --git a/docs/rules/README.md b/docs/rules/README.md
index 9cee1af..6fa5e63 100644
--- a/docs/rules/README.md
+++ b/docs/rules/README.md
@@ -11,3 +11,4 @@
 * [no-sibling-hooks](no-sibling-hooks.md) - disallow duplicate uses of a hook at the same level inside a describe
 * [no-mocha-arrows](no-mocha-arrows.md) - disallow arrow functions as arguments to mocha globals
 * [no-hooks](no-hooks.md) - disallow hooks
+* [no-top-level-hooks](no-top-level-hooks.md) - disallow top-level hooks
diff --git a/docs/rules/no-sibling-hooks.md b/docs/rules/no-sibling-hooks.md
index 282a85b..7d33d47 100644
--- a/docs/rules/no-sibling-hooks.md
+++ b/docs/rules/no-sibling-hooks.md
@@ -57,3 +57,4 @@ describe('foo', function () {
 ## When Not To Use It
 
 * If you use another library which exposes a similar API as mocha (e.g. `before`, `after`), you should turn this rule off, because it would raise warnings.
+* If you turned `no-hooks` on, you should turn this rule off, because it would raise several warnings for the same root cause.
diff --git a/docs/rules/no-top-level-hooks.md b/docs/rules/no-top-level-hooks.md
new file mode 100644
index 0000000..1b0183d
--- /dev/null
+++ b/docs/rules/no-top-level-hooks.md
@@ -0,0 +1,33 @@
+# Disallow top-level hooks (no-top-level-hooks)
+
+Mocha proposes hooks that allow code to be run before or after every or all tests. This helps define a common setup or teardown process for every test.
+These hooks should only be declared inside test suites, as they would otherwise be run before or after every test or test suite of the project, even if the test suite of the file they were declared in was skipped. This can lead to very confusing and unwanted effects.
+
+## Rule Details
+
+This rule looks for every call to `before`, `after`, `beforeEach` and `afterEach` that are not in a test suite.
+
+The following patterns are considered warnings:
+
+```js
+before(function () { /* ... */ }); // Not allowed
+after(function () { /* ... */ }); // Not allowed
+beforeEach(function () { /* ... */ }); // Not allowed
+afterEach(function () { /* ... */ }); // Not allowed
+```
+
+These patterns would not be considered warnings:
+
+```js
+describe('foo', function () {
+    before(function () { /* ... */ });
+    after(function () { /* ... */ });
+    beforeEach(function () { /* ... */ });
+    afterEach(function () { /* ... */ });
+});
+```
+
+## When Not To Use It
+
+* If you use another library which exposes a similar API as mocha (e.g. `before`, `after`), you should turn this rule off, because it would raise warnings.
+* If you turned `no-hooks` on, you should turn this rule off, because it would raise several warnings for the same root cause.
diff --git a/index.js b/index.js
index 30fbabe..ef433da 100644
--- a/index.js
+++ b/index.js
@@ -12,7 +12,8 @@ module.exports = {
         'valid-suite-description': require('./lib/rules/valid-suite-description'),
         'no-mocha-arrows': require('./lib/rules/no-mocha-arrows'),
         'no-hooks': require('./lib/rules/no-hooks'),
-        'no-sibling-hooks': require('./lib/rules/no-sibling-hooks')
+        'no-sibling-hooks': require('./lib/rules/no-sibling-hooks'),
+        'no-top-level-hooks': require('./lib/rules/no-top-level-hooks')
     },
     configs: {
         recommended: {
diff --git a/lib/rules/no-hooks.js b/lib/rules/no-hooks.js
index 6696de2..792f4de 100644
--- a/lib/rules/no-hooks.js
+++ b/lib/rules/no-hooks.js
@@ -1,11 +1,11 @@
 'use strict';
 
-module.exports = function (context) {
-    var hooks = [ 'before', 'after', 'beforeEach', 'afterEach' ];
+var astUtil = require('../util/ast');
 
+module.exports = function (context) {
     return {
         CallExpression: function (node) {
-            if (node.callee.type === 'Identifier' && hooks.indexOf(node.callee.name) !== -1) {
+            if (astUtil.isHookIdentifier(node.callee)) {
                 context.report({
                     node: node.callee,
                     message: 'Unexpected use of Mocha `' + node.callee.name + '` hook'
diff --git a/lib/rules/no-sibling-hooks.js b/lib/rules/no-sibling-hooks.js
index 8cb89d4..4270a63 100644
--- a/lib/rules/no-sibling-hooks.js
+++ b/lib/rules/no-sibling-hooks.js
@@ -1,6 +1,6 @@
 'use strict';
 
-var describeAliases = [ 'describe', 'xdescribe', 'context', 'xcontext' ];
+var astUtil = require('../util/ast');
 
 function newDescribeLayer(describeNode) {
     return {
@@ -12,36 +12,22 @@ function newDescribeLayer(describeNode) {
     };
 }
 
-function isDescribeIdentifier(node) {
-    return node.type === 'Identifier' && describeAliases.indexOf(node.name) !== -1;
-}
-
-function isDescribe(node) {
-  return node
-      && node.type === 'CallExpression'
-      && (isDescribeIdentifier(node.callee)
-          // eslint-disable-next-line no-extra-parens
-          || (node.callee.type === 'MemberExpression' && isDescribeIdentifier(node.callee.object))
-      );
-}
-
 module.exports = function (context) {
-    var hooks = [ 'before', 'after', 'beforeEach', 'afterEach' ],
-        isUsed = [];
+    var isUsed = [];
 
     return {
         Program: function (node) {
             isUsed.push(newDescribeLayer(node));
         },
 
-        CallExpression: function (node) { // eslint-disable-line complexity
+        CallExpression: function (node) {
             var name = node.callee && node.callee.name;
-            if (isDescribe(node)) {
+            if (astUtil.isDescribe(node)) {
               isUsed.push(newDescribeLayer(node));
               return;
             }
 
-            if (node.callee.type !== 'Identifier' || hooks.indexOf(name) === -1) {
+            if (!astUtil.isHookIdentifier(node.callee)) {
               return;
             }
 
diff --git a/lib/rules/no-top-level-hooks.js b/lib/rules/no-top-level-hooks.js
new file mode 100644
index 0000000..83f36f4
--- /dev/null
+++ b/lib/rules/no-top-level-hooks.js
@@ -0,0 +1,33 @@
+'use strict';
+
+var astUtil = require('../util/ast');
+
+module.exports = function (context) {
+    var testSuiteStack = [];
+
+    return {
+        CallExpression: function (node) {
+            if (astUtil.isDescribe(node)) {
+                testSuiteStack.push(node);
+                return;
+            }
+
+            if (!astUtil.isHookIdentifier(node.callee)) {
+              return;
+            }
+
+            if (testSuiteStack.length === 0) {
+                context.report({
+                    node: node.callee,
+                    message: 'Unexpected use of Mocha `' + node.callee.name + '` hook outside of a test suite'
+                });
+            }
+        },
+
+        'CallExpression:exit': function (node) {
+            if (testSuiteStack[testSuiteStack.length - 1] === node) {
+                testSuiteStack.pop();
+            }
+        }
+    };
+};
diff --git a/lib/util/ast.js b/lib/util/ast.js
new file mode 100644
index 0000000..951a56b
--- /dev/null
+++ b/lib/util/ast.js
@@ -0,0 +1,31 @@
+/* eslint-env node*/
+
+'use strict';
+
+var describeAliases = [ 'describe', 'xdescribe', 'context', 'xcontext' ],
+    hooks = [ 'before', 'after', 'beforeEach', 'afterEach' ];
+
+function isDescribeIdentifier(node) {
+    return node.type === 'Identifier' && describeAliases.indexOf(node.name) !== -1;
+}
+
+function isDescribe(node) {
+  return node
+      && node.type === 'CallExpression'
+      && (isDescribeIdentifier(node.callee)
+          // eslint-disable-next-line no-extra-parens
+          || (node.callee.type === 'MemberExpression' && isDescribeIdentifier(node.callee.object))
+      );
+}
+
+function isHookIdentifier(node) {
+  return node
+      && node.type === 'Identifier'
+      && hooks.indexOf(node.name) !== -1;
+}
+
+module.exports = {
+  isDescribe: isDescribe,
+  isDescribeIdentifier: isDescribeIdentifier,
+  isHookIdentifier: isHookIdentifier
+};


Test patch:
diff --git a/test/rules/no-top-level-hooks.js b/test/rules/no-top-level-hooks.js
new file mode 100644
index 0000000..59710b1
--- /dev/null
+++ b/test/rules/no-top-level-hooks.js
@@ -0,0 +1,78 @@
+'use strict';
+
+var RuleTester = require('eslint').RuleTester,
+    rules = require('../../').rules,
+    ruleTester = new RuleTester();
+
+ruleTester.run('no-top-level-hooks', rules['no-top-level-hooks'], {
+
+    valid: [
+        'describe(function() { before(function() {}); });',
+        'describe(function() { after(function() {}); });',
+        'describe(function() { beforeEach(function() {}); });',
+        'describe(function() { afterEach(function() {}); });',
+        'describe(function() { it(function() {}); });',
+        'describe(function() { describe(function() { before(function() {}); }); });',
+        'foo.before()',
+        'foo.after()',
+        'foo.beforeEach()',
+        'foo.afterEach()',
+        'before.foo()',
+        'after.foo()',
+        'beforeEach.foo()',
+        'afterEach.foo()',
+        'var before = 2; before + 3;'
+    ],
+
+    invalid: [
+        {
+            code: 'before(function() {});',
+            errors: [ {
+                message: 'Unexpected use of Mocha `before` hook outside of a test suite',
+                column: 1,
+                line: 1
+            } ]
+        },
+        {
+            code: 'after(function() {});',
+            errors: [ {
+                message: 'Unexpected use of Mocha `after` hook outside of a test suite',
+                column: 1,
+                line: 1
+            } ]
+        },
+        {
+            code: 'beforeEach(function() {});',
+            errors: [ {
+                message: 'Unexpected use of Mocha `beforeEach` hook outside of a test suite',
+                column: 1,
+                line: 1
+            } ]
+        },
+        {
+            code: 'afterEach(function() {});',
+            errors: [ {
+                message: 'Unexpected use of Mocha `afterEach` hook outside of a test suite',
+                column: 1,
+                line: 1
+            } ]
+        },
+        {
+            code: 'describe(function() {}); before(function() {});',
+            errors: [ {
+                message: 'Unexpected use of Mocha `before` hook outside of a test suite',
+                column: 26,
+                line: 1
+            } ]
+        },
+        {
+            code: 'before(function() {}); describe(function() {});',
+            errors: [ {
+                message: 'Unexpected use of Mocha `before` hook outside of a test suite',
+                column: 1,
+                line: 1
+            } ]
+        }
+    ]
+
+});