From 3da13a0426d70534ea8f5d9bb7fa1bf7a064dd0f Mon Sep 17 00:00:00 2001 From: Viktar Korshun Date: Tue, 18 Aug 2020 17:46:19 -0700 Subject: [PATCH] Introduce no-noop-setup-on-error-in-before rule for tests --- README.md | 1 + .../rules/no-noop-setup-on-error-in-before.md | 50 +++++ lib/index.js | 1 + lib/rules/no-noop-setup-on-error-in-before.js | 146 ++++++++++++++ .../rules/no-noop-setup-on-error-in-before.js | 185 ++++++++++++++++++ 5 files changed, 383 insertions(+) create mode 100644 docs/rules/no-noop-setup-on-error-in-before.md create mode 100644 lib/rules/no-noop-setup-on-error-in-before.js create mode 100644 tests/lib/rules/no-noop-setup-on-error-in-before.js diff --git a/README.md b/README.md index f60eb13201..2ac10bfe18 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 0000000000..76d969d8d2 --- /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 95a38c8748..7e5014b131 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 0000000000..1b410fcf36 --- /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; + } + }, + }; + }, +}; 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 0000000000..01d6e9b0ff --- /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', + }, + ], + }, + ], +});