Implement no-async-describe rule #188
Conversation
| ); | ||
| } | ||
|
|
||
| function containsDirectAwait(node) { |
There was a problem hiding this comment.
I don't know if this could be implemented with a funky ramda function that looks for a nested object with a type of AwaitExpression not contained within one with a type of FunctionExpression etc. -- but I have to admit that I'm not really familiar with that.
There was a problem hiding this comment.
Don’t worry, it’s not a requirement to write funky ramda expressions 😉.
docs/rules/no-async-describe.md
Outdated
| Example: | ||
|
|
||
| ```js | ||
| describe(async function () { |
There was a problem hiding this comment.
Is this a valid example? Doesn’t describe require a string as the first argument?
There was a problem hiding this comment.
No, it isn't 🙄
Fixed in 7c9b6dc
docs/rules/no-async-describe.md
Outdated
| The following patterns are considered problems, whether or not the function uses `await`: | ||
|
|
||
| ```js | ||
| describe'something', async function () { |
docs/rules/no-async-describe.md
Outdated
| it('should work', function () {}); | ||
| }); | ||
|
|
||
| describe'something', async () => { |
docs/rules/no-async-describe.md
Outdated
|
|
||
| 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.
Why does it look for it.only and test.only etc? Maybe the whole paragraph can be removed as the first paragraph in this section already explains that describe, context and suite is supported.
There was a problem hiding this comment.
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 😆
docs/rules/no-async-describe.md
Outdated
|
|
||
| ```js | ||
| var describeOnly = describe.only; | ||
| describeOnly.apply(describe); |
There was a problem hiding this comment.
I’m not sure if this example makes sense. It is completely unrelated to async.
There was a problem hiding this comment.
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.
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 describe and it don't take a string argument.
Fixed in 7c9b6dc
lib/rules/no-async-describe.js
Outdated
| if (isAsyncFunction(fnArg)) { | ||
| context.report({ | ||
| node: fnArg, | ||
| message: `Do not pass an async function to ${name}()`, |
There was a problem hiding this comment.
I would suggest something like Unexpected async function in ${name}() to be more in line with messages of other rules.
There was a problem hiding this comment.
Fixed in a5fa46a. Looks like I copy/pasted from the no-mocha-global-arrows rule.
lib/rules/no-async-describe.js
Outdated
| 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.
When a function is async, isn’t the first token always the async identifier? If so it might be easier to just get the first token of the function node and use fixer.remove.
There was a problem hiding this comment.
That's a good point, fixed in 0c113b5
We also need go get rid of the whitespace after async, so we can remove the range from the start of the async token to the start of the next function or ( token.
| ruleTester.run('no-async-describe', rule, { | ||
| valid: [ | ||
| 'describe()', | ||
| 'describe(function () {})', |
There was a problem hiding this comment.
The function should be the second argument of describe.
There was a problem hiding this comment.
Of course! 👍
Fixed in 7c9b6dc
There was a problem hiding this comment.
@papandreou Thanks for the great work, this looks already quite good. I’ve just left a few comments, but only minor things.
|
Thanks for the feedback, I think I've addressed all of it now. By the way, the linting setup for this project was a pleasure to work with. I could really feel the presence of the maintainer's opinions in my editor, and that helped a great deal! The only slightly annoying thing is that I can't even run the test suite if everything doesn't lint, because of eslint-plugin-mocha/package.json Line 16 in 27fa81c // :)
Could it be changed to |
That’s true, I usually use only |
I've seen
describe('my test', async function () { ... })cause subtle problems, so I think it makes sense to add a rule for it.