Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add new rule no-noop-setup-on-error-in-before #920

Merged
merged 1 commit into from Aug 27, 2020
Merged

Add new rule no-noop-setup-on-error-in-before #920

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions README.md
View file
Open in desktop
Expand Up @@ -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 |
Expand Down
50 changes: 50 additions & 0 deletions docs/rules/no-noop-setup-on-error-in-before.md
View file
Open in desktop
@@ -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.
v-korshun marked this conversation as resolved.
Show resolved Hide resolved

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');
});
});
});
```
1 change: 1 addition & 0 deletions lib/index.js
View file
Open in desktop
Expand Up @@ -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'),
Expand Down
146 changes: 146 additions & 0 deletions lib/rules/no-noop-setup-on-error-in-before.js
View file
Open in desktop
@@ -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',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a bit hesitant to make this rule autofixable by simply deleting the offending code. That changes the behavior of the code (which we normally try to avoid) and the author would likely need to make manual fixes anyway right? What do you think?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are certainly more actions that the author would have to do to fix the tests but it would vary on a case by case basis. The one thing that would have to be done anyway - removal of the offending code, then run the tests and see which test cases failed to add error handling in individual test. I would leave it for convenience.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mongoose700 thoughts?

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;
}
},
};
},
};
185 changes: 185 additions & 0 deletions tests/lib/rules/no-noop-setup-on-error-in-before.js
View file
Open in desktop
@@ -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',
v-korshun marked this conversation as resolved.
Show resolved Hide resolved
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',
},
],
},
],
});