-
Notifications
You must be signed in to change notification settings - Fork 61
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.
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
Implement no-async-describe rule #188
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,47 @@ | ||
# Disallow async functions passed to describe (no-async-describe) | ||
|
||
This rule disallows the use of an async function with `describe`. It usually indicates a copy/paste error or that you're trying to use `describe` for setup code, which should happen in `before` or `beforeEach`. Also, it can lead to [the contained `it` blocks not being picked up](https://github.com/mochajs/mocha/issues/2975). | ||
|
||
Example: | ||
|
||
```js | ||
describe(async function () { | ||
// This work should happen in a beforeEach: | ||
const theThing = await getTheThing(); | ||
|
||
it('should foo', function () { | ||
// ... | ||
}); | ||
}); | ||
``` | ||
|
||
## Rule Details | ||
|
||
The rule supports "describe", "context" and "suite" suite function names and different valid suite name prefixes like "skip" or "only". | ||
|
||
The following patterns are considered problems, whether or not the function uses `await`: | ||
|
||
```js | ||
describe'something', async function () { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Missing |
||
it('should work', function () {}); | ||
}); | ||
|
||
describe'something', async () => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same here. |
||
it('should work', function () {}); | ||
}); | ||
``` | ||
|
||
If the `describe` function does not contain `await`, a fix of removing `async` will be suggested. | ||
|
||
This rule looks for every `describe.only`, `it.only`, `suite.only`, `test.only`, `context.only` and `specify.only` occurrences within the source code. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why does it look for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Whoops, that part was copy/pasted from the docs for another rule where it did make sense. Maybe I was getting anxious because the other rules are so thoroughly documented 😆 |
||
Of course there are some edge-cases which can’t be detected by this rule, eg.: | ||
|
||
```js | ||
var describeOnly = describe.only; | ||
describeOnly.apply(describe); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I’m not sure if this example makes sense. It is completely unrelated to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess one edge case that we can’t detect might be something like this: async function mySuite() {
it('my test', () => {});
}
describe('my suite', mySuite); There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. D'oh, brain fart. I think I copy pasted from https://github.com/lo1tuma/eslint-plugin-mocha/blob/master/test/rules/no-mocha-arrows.js#L15 and then my brain reset itself into a state where Fixed in 7c9b6dc |
||
``` | ||
|
||
## When Not To Use It | ||
|
||
- If you use another library which exposes a similar API as mocha (e.g. `describe.only`), you should turn this rule off because it would raise warnings. | ||
- In environments that have not yet adopted ES6 language features (ES3/5). |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,71 @@ | ||
'use strict'; | ||
|
||
/* eslint "complexity": [ "error", 5 ] */ | ||
|
||
/** | ||
* @fileoverview Disallow async functions as arguments to describe | ||
*/ | ||
|
||
const astUtils = require('../util/ast'); | ||
const { additionalSuiteNames } = require('../util/settings'); | ||
|
||
module.exports = function (context) { | ||
const sourceCode = context.getSourceCode(); | ||
|
||
function isFunction(node) { | ||
return ( | ||
node.type === 'FunctionExpression' || | ||
node.type === 'FunctionDeclaration' || | ||
node.type === 'ArrowFunctionExpression' | ||
); | ||
} | ||
|
||
function containsDirectAwait(node) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't know if this could be implemented with a funky ramda function that looks for a nested object with a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Don’t worry, it’s not a requirement to write funky ramda expressions 😉. |
||
if (node.type === 'AwaitExpression') { | ||
return true; | ||
} else if (node.type && !isFunction(node)) { | ||
return Object.keys(node).some(function (key) { | ||
if (Array.isArray(node[key])) { | ||
return node[key].some(containsDirectAwait); | ||
} else if (key !== 'parent' && node[key] && typeof node[key] === 'object') { | ||
return containsDirectAwait(node[key]); | ||
} | ||
return false; | ||
}); | ||
} | ||
return false; | ||
} | ||
|
||
function fixAsyncFunction(fixer, fn) { | ||
if (!containsDirectAwait(fn.body)) { | ||
return fixer.replaceTextRange( | ||
[ fn.start, fn.end ], | ||
sourceCode.text.slice(fn.range[0], fn.range[1]).replace(/^async /, '') | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When a function is async, isn’t the first token always the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's a good point, fixed in 0c113b5 We also need go get rid of the whitespace after |
||
); | ||
} | ||
return undefined; | ||
} | ||
|
||
function isAsyncFunction(node) { | ||
return node && (node.type === 'FunctionExpression' || node.type === 'ArrowFunctionExpression') && node.async; | ||
} | ||
|
||
return { | ||
CallExpression(node) { | ||
const name = astUtils.getNodeName(node.callee); | ||
|
||
if (astUtils.isDescribe(node, additionalSuiteNames(context.settings))) { | ||
const fnArg = node.arguments.slice(-1)[0]; | ||
if (isAsyncFunction(fnArg)) { | ||
context.report({ | ||
node: fnArg, | ||
message: `Do not pass an async function to ${name}()`, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would suggest something like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in a5fa46a. Looks like I copy/pasted from the |
||
fix(fixer) { | ||
return fixAsyncFunction(fixer, fnArg); | ||
} | ||
}); | ||
} | ||
} | ||
} | ||
}; | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,74 @@ | ||
'use strict'; | ||
|
||
const RuleTester = require('eslint').RuleTester; | ||
const rule = require('../../lib/rules/no-async-describe'); | ||
const ruleTester = new RuleTester(); | ||
|
||
ruleTester.run('no-async-describe', rule, { | ||
valid: [ | ||
'describe()', | ||
'describe(function () {})', | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The function should be the second argument of describe. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Of course! 👍 Fixed in 7c9b6dc |
||
{ code: '() => { a.b }', parserOptions: { ecmaVersion: 6 } }, | ||
'it()', | ||
{ code: 'it(async function () {})', parserOptions: { ecmaVersion: 8 } }, | ||
{ code: 'it(async () => {})', parserOptions: { ecmaVersion: 8 } } | ||
], | ||
|
||
invalid: [ | ||
{ | ||
code: 'describe(async function () {})', | ||
output: 'describe(function () {})', | ||
parserOptions: { ecmaVersion: 8 }, errors: [ { | ||
message: 'Do not pass an async function to describe()', | ||
line: 1, | ||
column: 10 | ||
} ] | ||
}, | ||
{ | ||
code: 'foo(async function () {})', | ||
output: 'foo(function () {})', | ||
settings: { | ||
mocha: { | ||
additionalSuiteNames: [ 'foo' ] | ||
} | ||
}, | ||
parserOptions: { ecmaVersion: 8 }, errors: [ { | ||
message: 'Do not pass an async function to foo()', | ||
line: 1, | ||
column: 5 | ||
} ] | ||
}, | ||
{ | ||
code: 'describe(async () => {})', | ||
output: 'describe(() => {})', | ||
parserOptions: { ecmaVersion: 8 }, | ||
errors: [ { | ||
message: 'Do not pass an async function to describe()', | ||
line: 1, | ||
column: 10 | ||
} ] | ||
}, | ||
{ | ||
code: 'describe(async () => {await foo;})', | ||
// Do not offer a fix for an async function that contains await | ||
output: null, | ||
parserOptions: { ecmaVersion: 8 }, | ||
errors: [ { | ||
message: 'Do not pass an async function to describe()', | ||
line: 1, | ||
column: 10 | ||
} ] | ||
}, | ||
{ | ||
code: 'describe(async () => {async function bar() {await foo;}})', | ||
// Do offer a fix despite a nested async function containing await | ||
output: 'describe(() => {async function bar() {await foo;}})', | ||
parserOptions: { ecmaVersion: 8 }, | ||
errors: [ { | ||
message: 'Do not pass an async function to describe()', | ||
line: 1, | ||
column: 10 | ||
} ] | ||
} | ||
] | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a valid example? Doesn’t
describe
require a string as the first argument?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, it isn't 🙄
Fixed in 7c9b6dc