Adds rule "no setup in describe" #147
Conversation
donabrams
changed the title
There was a problem hiding this comment.
Hi @donabrams and thank you for the contribution. Overall it looks like a good starting point, although it is quite hard to correctly detect setup code. I would say let’s start with this approach and see how it works in practice.
Fixes #38
.eslintrc
Outdated
| @@ -103,7 +103,7 @@ | |||
| "comma-dangle": [ 2, "never" ], | |||
| "comma-spacing": [ 2, { "before": false, "after": true } ], | |||
| "comma-style": [ 2, "last" ], | |||
| "complexity": [ 2, 4 ], | |||
| "complexity": [ 2, 8 ], | |||
There was a problem hiding this comment.
Please don’t change this rule globally. It is ok to make exception from this rule, but then please only for those functions where it is really necessary via eslint-disable-comment.
There was a problem hiding this comment.
This was erroring in plugins other than this one, so I set it to the highest value other plugins required. I can bring this back down and add eslint-disable-line to the other plugins if wanted
There was a problem hiding this comment.
I’ve fixed that problem in #149. So you just need to rebase from master.
| @@ -11,6 +11,7 @@ | |||
| * [no-nested-tests](no-nested-tests.md) - disallow tests to be nested within other tests | |||
| * [no-pending-tests](no-pending-tests.md) - disallow pending/unimplemented mocha tests | |||
| * [no-return-and-callback](no-return-and-callback.md) - disallow returning in a test or hook function that uses a callback | |||
| * [no-setup-in-describe](no-setup-in-describe.md) - disallow calling functions and dot operaters directly in describe blocks | |||
There was a problem hiding this comment.
Dot operators? Do you mean member expressions? If so, please also consider that member expression could be written as foo[bar].
There was a problem hiding this comment.
Ooh, yes, on it!
- handle bracket notation (
foo[bar])
| }); | ||
| ``` | ||
|
|
||
| Any setup directly in a `describe` is run before all tests execute. This is undesirable primarily for two reasons: |
There was a problem hiding this comment.
What about top-level code outside of a describe?
There was a problem hiding this comment.
I'd love to handle this, but it could result in worse performance on non-test code (currently a .length check is all the overhead that is incurred outside a describe). Thoughts on that tradeoff?
There was a problem hiding this comment.
Hmm, actually I just don't know how to flag a file as having a describe with eslint. What's the perf on a hasDescribe(file) and can this even be done?
There was a problem hiding this comment.
I’m not sure why it should be necessary to check if there is a describe in a specific file, you could just look for setup code directly in the Program node.
Anyway I’m not sure if this is wanted, because most people probably prefer to have their require calls at the top of the file. So maybe we leave it for now as it is and if some one requests that feature we can implement this behind an option.
|
|
||
| This rule reports all function calls and use of the dot operator (due to getters and setters) directly in describe blocks. | ||
|
|
||
| If you're using [dynamically generated tests](https://mochajs.org/#dynamically-generating-tests), you should disable this rule. |
There was a problem hiding this comment.
Ah, I was just going to ask about this 😀 .
I think it is ok that this rule will only work for non dynamically generated tests.
lib/rules/no-setup-in-describe.js
Outdated
| DESCRIBE = 2, | ||
| PURE = 3; | ||
|
|
||
| function isPureN(node) { |
There was a problem hiding this comment.
Node. I'll expand it.
- expand
NtoNode
test/rules/pure-describe.js
Outdated
| @@ -0,0 +1,89 @@ | |||
| 'use strict'; | |||
There was a problem hiding this comment.
Why does this has a different filename than the actual rule?
There was a problem hiding this comment.
Oops, changed the name
- rename this file
test/rules/pure-describe.js
Outdated
| } ] | ||
| }, | ||
| { | ||
| code: 'describe("", function () { a.b; });', |
There was a problem hiding this comment.
Can you also add a test for a['b']?
|
I've addressed everything I don't have questions about. |
|
Hi @donabrams, sorry for the late feedback. This looks almost good to me, only a rebase is missing and the removal of the |
… only activate in describe blocks
|
Alright, rebased and complexity "decreased" |
Setup for test cases in mocha should be done in
before,beforeEach, oritblocks. Unfortunately there is nothing stopping you from doing setup directly inside adescribeblock and it's not immediately obvious setup should be inside abeforeorbeforeEach.This rule catches many cases where setup is accidentally done directly in a describe block and promotes the use of the hooks
beforeandbeforeEach.This rule's code is a little more complex than I'd like because it does what it can to minimize processing time outside a describe block (
Array.lengthcheck only).This rule should never be activated by default because it blocks the use of dynamically generated tests.