Merge pull request #920 from v-korshun/no-noop-setup-on-error-in-before

Add new rule `no-noop-setup-on-error-in-before`
ember-cli · Aug 27, 2020 · 2a90f87 · 2a90f87
2 parents 8be94f9 + 3da13a0
commit 2a90f87
Show file tree

Hide file tree

Showing 5 changed files with 383 additions and 0 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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;
+        }
+      },
+    };
+  },
+};
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
@@ -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',
+        },
+      ],
+    },
+  ],
+});