Repository: ember-cli/eslint-plugin-ember (js)

Issue:
Disallow noop `setupOnerror` in before/beforeEach
Right now nothing prevents engineers to setup noop error handler in before/beforeEach which will swallow all rejection and errors in the module. We should not allow to use noop setupOnerror in before/beforeEach since it could mask errors/rejections in tests unintentionally.



Pull request description:
Add new rule `no-noop-setup-on-error-in-before`
Fixes #918

Patch:
diff --git a/README.md b/README.md
index f60eb132..2ac10bfe 100644
--- a/README.md
+++ b/README.md
@@ -183,6 +183,7 @@ Rules are grouped by category to help you understand their purpose. Each rule ha
 | :white_check_mark: | [no-ember-testing-in-module-scope](./docs/rules/no-ember-testing-in-module-scope.md) | disallow use of `Ember.testing` in module scope |
 |  | [no-invalid-test-waiters](./docs/rules/no-invalid-test-waiters.md) | disallow incorrect usage of test waiter APIs |
 | :white_check_mark: | [no-legacy-test-waiters](./docs/rules/no-legacy-test-waiters.md) | disallow the use of the legacy test waiter APIs |
+| :wrench: | [no-noop-setup-on-error-in-before](./docs/rules/no-noop-setup-on-error-in-before.md) | disallows using no-op setupOnerror in `before` or `beforeEach` |
 | :white_check_mark: | [no-pause-test](./docs/rules/no-pause-test.md) | disallow usage of the `pauseTest` helper in tests |
 |  | [no-replace-test-comments](./docs/rules/no-replace-test-comments.md) | disallow 'Replace this with your real tests' comments in test files |
 | :white_check_mark: | [no-restricted-resolver-tests](./docs/rules/no-restricted-resolver-tests.md) | disallow the use of patterns that use the restricted resolver in tests |
diff --git a/docs/rules/no-noop-setup-on-error-in-before.md b/docs/rules/no-noop-setup-on-error-in-before.md
new file mode 100644
index 00000000..76d969d8
--- /dev/null
+++ b/docs/rules/no-noop-setup-on-error-in-before.md
@@ -0,0 +1,50 @@
+# no-noop-setup-on-error-in-before
+
+:wrench: The `--fix` option on the [command line](https://eslint.org/docs/user-guide/command-line-interface#fixing-problems) can automatically fix some of the problems reported by this rule.
+
+Disallows use of no-op `setupOnerror` in `before`/`beforeEach` since it could mask errors or rejections in tests unintentionally
+
+## Rule Details
+
+This rule aims to avoid single no-op `setupOnerror` for all tests in the module. In certain situations(maybe the majority of the test cases throw an error), the author of the test might resort to the definition of single no-op `setupOnerror` in `before`/`beforeEach`. This might make sense at the time of writing the tests, but modules tend to grow and no-op error handler would swallow any promise rejection or error that otherwise would be caught by test.
+
+## Examples
+
+Examples of **incorrect** code for this rule:
+
+```js
+import { setupOnerror } from '@ember/test-helpers';
+import { module } from 'qunit';
+
+module('foo', function (hooks) {
+  hooks.beforeEach(function () {
+    setupOnerror(() => {});
+  });
+});
+```
+
+```js
+import { setupOnerror } from '@ember/test-helpers';
+import { module } from 'qunit';
+
+module('foo', function (hooks) {
+  hooks.before(function () {
+    setupOnerror(() => {});
+  });
+});
+```
+
+Examples of **correct** code for this rule:
+
+```js
+import { setupOnerror } from '@ember/test-helpers';
+import { module, test } from 'qunit';
+
+module('foo', function (hooks) {
+  test('something', function () {
+    setupOnerror((error) => {
+      assert.equal(error.message, 'test', 'Should have message');
+    });
+  });
+});
+```
diff --git a/lib/index.js b/lib/index.js
index 95a38c87..7e5014b1 100644
--- a/lib/index.js
+++ b/lib/index.js
@@ -42,6 +42,7 @@ module.exports = {
     'no-legacy-test-waiters': require('./rules/no-legacy-test-waiters'),
     'no-mixins': require('./rules/no-mixins'),
     'no-new-mixins': require('./rules/no-new-mixins'),
+    'no-noop-setup-on-error-in-before': require('./rules/no-noop-setup-on-error-in-before'),
     'no-observers': require('./rules/no-observers'),
     'no-old-shims': require('./rules/no-old-shims'),
     'no-on-calls-in-components': require('./rules/no-on-calls-in-components'),
diff --git a/lib/rules/no-noop-setup-on-error-in-before.js b/lib/rules/no-noop-setup-on-error-in-before.js
new file mode 100644
index 00000000..1b410fcf
--- /dev/null
+++ b/lib/rules/no-noop-setup-on-error-in-before.js
@@ -0,0 +1,146 @@
+'use strict';
+
+const { getImportIdentifier } = require('../utils/import');
+const types = require('../utils/types');
+
+//------------------------------------------------------------------------------
+// General rule - Disallows no-op `setupOnError` in `before` or `beforeEach`.
+//------------------------------------------------------------------------------
+
+module.exports = {
+  meta: {
+    type: 'problem',
+    docs: {
+      description: 'disallows using no-op setupOnerror in `before` or `beforeEach`',
+      category: 'Testing',
+      recommended: false,
+      url:
+        'https://github.com/ember-cli/eslint-plugin-ember/tree/master/docs/rules/no-noop-setup-on-error-in-before.md',
+    },
+    fixable: 'code',
+    schema: [],
+    messages: {
+      main: 'Using no-op setupOnerror in `before` or `beforeEach` is not allowed',
+    },
+  },
+
+  create(context) {
+    let importedSetupOnerrorName, importedModuleName;
+    let isInModule = false;
+    let isInBeforeEachHook = false;
+    let isInBeforeHook = false;
+    let hooksName;
+    const sourceCode = context.getSourceCode();
+
+    function reportErrorForNodeIfInBefore(node) {
+      const isInBeforeOrBeforeEach =
+        (isInBeforeEachHook || isInBeforeHook) &&
+        types.isIdentifier(node.callee) &&
+        node.callee.name === importedSetupOnerrorName;
+
+      const callback = node.arguments[0];
+      const isFunction =
+        types.isArrowFunctionExpression(callback) ||
+        types.isFunctionDeclaration(callback) ||
+        types.isFunctionExpression(callback);
+
+      const isNoop =
+        callback &&
+        isFunction &&
+        callback.body &&
+        callback.body.type === 'BlockStatement' &&
+        callback.body.body.length === 0;
+
+      if (isInBeforeOrBeforeEach && isNoop) {
+        context.report({
+          node,
+          messageId: 'main',
+          *fix(fixer) {
+            yield fixer.remove(node);
+            const semicolon = sourceCode.getTokenAfter(node);
+            if (semicolon && semicolon.value === ';') {
+              yield fixer.remove(semicolon);
+            }
+          },
+        });
+      }
+    }
+
+    return {
+      ImportDeclaration(node) {
+        if (node.source.value === '@ember/test-helpers') {
+          importedSetupOnerrorName =
+            importedSetupOnerrorName ||
+            getImportIdentifier(node, '@ember/test-helpers', 'setupOnerror');
+        }
+        if (node.source.value === 'qunit') {
+          importedModuleName = importedModuleName || getImportIdentifier(node, 'qunit', 'module');
+        }
+      },
+
+      CallExpression(node) {
+        if (types.isIdentifier(node.callee) && node.callee.name === importedModuleName) {
+          isInModule = true;
+          if (node.arguments.length > 1 && node.arguments[1]) {
+            const moduleCallback = node.arguments[1];
+            hooksName =
+              moduleCallback.params &&
+              moduleCallback.params.length > 0 &&
+              types.isIdentifier(moduleCallback.params[0]) &&
+              moduleCallback.params[0].name;
+          }
+        }
+        if (types.isIdentifier(node.callee) && node.callee.name === importedSetupOnerrorName) {
+          reportErrorForNodeIfInBefore(node);
+        }
+      },
+
+      'CallExpression:exit'(node) {
+        if (types.isIdentifier(node.callee) && node.callee.name === importedModuleName) {
+          isInModule = false;
+          hooksName = undefined;
+        }
+      },
+
+      // potentially entering a `beforeEach` hook
+      'CallExpression[callee.property.name="beforeEach"]'(node) {
+        if (
+          isInModule &&
+          hooksName &&
+          types.isIdentifier(node.callee.object) &&
+          node.callee.object.name === hooksName
+        ) {
+          isInBeforeEachHook = true;
+        }
+      },
+
+      // potentially exiting a `beforeEach` hook
+      'CallExpression[callee.property.name="beforeEach"]:exit'() {
+        if (isInBeforeEachHook) {
+          isInBeforeEachHook = false;
+          hooksName = undefined;
+        }
+      },
+
+      // potentially entering a `before` hook
+      'CallExpression[callee.property.name="before"]'(node) {
+        if (
+          isInModule &&
+          hooksName &&
+          types.isIdentifier(node.callee.object) &&
+          node.callee.object.name === hooksName
+        ) {
+          isInBeforeHook = true;
+        }
+      },
+
+      // potentially exiting a `before` hook
+      'CallExpression[callee.property.name="before"]:exit'() {
+        if (isInBeforeHook) {
+          isInBeforeHook = false;
+          hooksName = undefined;
+        }
+      },
+    };
+  },
+};


Test patch:
diff --git a/tests/lib/rules/no-noop-setup-on-error-in-before.js b/tests/lib/rules/no-noop-setup-on-error-in-before.js
new file mode 100644
index 00000000..01d6e9b0
--- /dev/null
+++ b/tests/lib/rules/no-noop-setup-on-error-in-before.js
@@ -0,0 +1,185 @@
+//------------------------------------------------------------------------------
+// Requirements
+//------------------------------------------------------------------------------
+
+const rule = require('../../../lib/rules/no-noop-setup-on-error-in-before');
+const RuleTester = require('eslint').RuleTester;
+
+//------------------------------------------------------------------------------
+// Tests
+//------------------------------------------------------------------------------
+
+const ruleTester = new RuleTester({
+  parserOptions: {
+    ecmaVersion: 6,
+    sourceType: 'module',
+  },
+});
+
+ruleTester.run('no-noop-setup-on-error-in-before', rule, {
+  valid: [
+    `
+        import { setupOnerror } from '@ember/test-helpers';
+        import { module, test } from 'qunit';
+
+        module('foo', function(hooks) {
+          test('something', function() {
+            setupOnerror((error) => {
+              assert.equal(error.message, 'test', 'Should have message');
+            });
+          })
+        });
+        `,
+    `
+        import { setupOnerror } from '@ember/test-helpers';
+        import { module } from 'qunit';
+
+        module('foo', function(hooks) {
+          hooks.beforeEach(function() {
+            setupOnerror((error) => {
+              assert.equal(error.message, 'test', 'Should have message');
+            });
+          });
+        });
+      `,
+    `
+        import { setupOnerror } from '@ember/test-helpers';
+        import { module, test } from 'qunit';
+
+        module('foo', function(hooks) {
+          test('something', function() {
+            setupOnerror(() => {
+            });
+          })
+        });
+      `,
+    `
+      import { setupOnerror } from '@ember/test-helpers';
+      import { module, test } from 'qunit';
+      import moduleBody from 'somewhere';
+
+      module('foo', moduleBody());
+    `,
+    `
+        import { setupOnerror } from '@ember/test-helpers';
+        import { module } from 'qunit';
+
+        module('foo', function() {
+        });
+      `,
+  ],
+  invalid: [
+    {
+      code: `
+        import { setupOnerror } from '@ember/test-helpers';
+        import { module as moduleVariable } from 'qunit';
+
+        moduleVariable('foo', function(hooks) {
+          hooks.beforeEach(function() {
+setupOnerror(() => {});
+          });
+        });
+      `,
+      output: `
+        import { setupOnerror } from '@ember/test-helpers';
+        import { module as moduleVariable } from 'qunit';
+
+        moduleVariable('foo', function(hooks) {
+          hooks.beforeEach(function() {
+
+          });
+        });
+      `,
+      errors: [
+        {
+          messageId: 'main',
+          type: 'CallExpression',
+        },
+      ],
+    },
+    {
+      code: `
+        import { setupOnerror } from '@ember/test-helpers';
+        import { module } from 'qunit';
+
+        module('foo', function(hooks) {
+          hooks.beforeEach(function() {
+setupOnerror(function(){});
+          });
+        });
+      `,
+      output: `
+        import { setupOnerror } from '@ember/test-helpers';
+        import { module } from 'qunit';
+
+        module('foo', function(hooks) {
+          hooks.beforeEach(function() {
+
+          });
+        });
+      `,
+      errors: [
+        {
+          messageId: 'main',
+          type: 'CallExpression',
+        },
+      ],
+    },
+    {
+      code: `
+        import { setupOnerror } from '@ember/test-helpers';
+        import { module } from 'qunit';
+
+        module('foo', function(hooks) {
+          hooks.beforeEach(function() {
+setupOnerror(function noop(){});
+          });
+        });
+      `,
+      output: `
+        import { setupOnerror } from '@ember/test-helpers';
+        import { module } from 'qunit';
+
+        module('foo', function(hooks) {
+          hooks.beforeEach(function() {
+
+          });
+        });
+      `,
+      errors: [
+        {
+          messageId: 'main',
+          type: 'CallExpression',
+        },
+      ],
+    },
+    {
+      code: `
+        import { setupOnerror } from '@ember/test-helpers';
+        import { module } from 'qunit';
+
+        module('foo', function(hooks) {
+          hooks.before(function() {
+setupOnerror(() => {});
+          });
+        });
+      `,
+      output: `
+        import { setupOnerror } from '@ember/test-helpers';
+        import { module } from 'qunit';
+
+        module('foo', function(hooks) {
+          hooks.before(function() {
+
+          });
+        });
+      `,
+      errors: [
+        {
+          messageId: 'main',
+          type: 'CallExpression',
+        },
+      ],
+    },
+  ],
+});