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.

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

Fix no-setup-in-describe to allow Mocha suite config #209

Merged
merged 4 commits into from
Sep 11, 2019
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion docs/rules/no-setup-in-describe.md
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ Any setup directly in a `describe` is run before all tests execute. This is unde
1. When doing TDD in a large codebase, all setup is run for tests that don't have `only` set. This can add a substantial amount of time per iteration.
2. If global state is altered by the setup of another describe block, your test may be affected.

This rule reports all function calls and use of the dot operator (due to getters and setters) directly in describe blocks.
This rule reports all function calls and use of the dot operator (due to getters and setters) directly in describe blocks. An exception is made for Mocha's suite configuration methods, like `this.timeout();`, which do not represent setup logic.

If you're using [dynamically generated tests](https://mochajs.org/#dynamically-generating-tests), you should disable this rule.

Expand Down Expand Up @@ -68,4 +68,8 @@ describe('something', function () {
const { a, b } = setup();
});
});
describe('something', function () {
this.timeout(5000);
it('should take awhile', function () {});
});
```
4 changes: 3 additions & 1 deletion lib/rules/no-setup-in-describe.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,9 @@ module.exports = function noSetupInDescribe(context) {
const settings = context.settings;

function isPureNode(node) {
return astUtils.isHookCall(node) || astUtils.isTestCase(node);
return astUtils.isHookCall(node) ||
astUtils.isTestCase(node) ||
astUtils.isSuiteConfigCall(node);
}

function reportCallExpression(callExpression) {
Expand Down
12 changes: 12 additions & 0 deletions lib/util/ast.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ const describeAliases = [
'suite', 'xsuite', 'suite.only', 'suite.skip'
];
const hooks = [ 'before', 'after', 'beforeEach', 'afterEach', 'beforeAll', 'afterAll' ];
const suiteConfig = [ 'timeout', 'slow', 'retries' ];
const testCaseNames = [
'it', 'it.only', 'it.skip', 'xit',
'test', 'test.only', 'test.skip',
Expand Down Expand Up @@ -43,6 +44,16 @@ function isHookCall(node) {
return isCallExpression(node) && isHookIdentifier(node.callee);
}

function isSuiteConfigExpression(node) {
return node.type === 'MemberExpression' &&
node.object.type === 'ThisExpression' &&
suiteConfig.indexOf(node.property.name) !== -1;
}

function isSuiteConfigCall(node) {
return isCallExpression(node) && isSuiteConfigExpression(node.callee);
}

function isTestCase(node) {
return isCallExpression(node) && testCaseNames.indexOf(getNodeName(node.callee)) > -1;
}
Expand Down Expand Up @@ -102,6 +113,7 @@ module.exports = {
getNodeName,
isMochaFunctionCall,
isHookCall,
isSuiteConfigCall,
isStringLiteral,
hasParentMochaFunctionCall,
findReturnStatement,
Expand Down
6 changes: 6 additions & 0 deletions test/rules/no-setup-in-describe.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,16 @@ ruleTester.run('no-setup-in-describe', rule, {
'it("", function () { a[b]; })',
'it("", function () { a["b"]; })',
'describe("", function () { it(); })',
'describe("", function () { this.slow(1); it(); })',
'describe("", function () { this.timeout(1); it(); })',
'describe("", function () { this.retries(1); it(); })',
Copy link
Owner

Choose a reason for hiding this comment

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

Can you add one test case with a computed member expression? E.g. this['retries'](1);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My change to the rule doesn't allow for that form of the expression, so I added this to the invalid test cases, but lmk if you would rather I add support for that form in the rule!

Copy link
Owner

Choose a reason for hiding this comment

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

IMHO we should handle this case as well. There is already a function for that in ast utils: getPropertyName().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All set!

'describe("", function () { it("", function () { b(); }); })',
'describe("", function () { it("", function () { a.b; }); })',
'describe("", function () { it("", function () { a[b]; }); })',
'describe("", function () { it("", function () { a["b"]; }); })',
'describe("", function () { it("", function () { this.slow(1); }); })',
'describe("", function () { it("", function () { this.timeout(1); }); })',
'describe("", function () { it("", function () { this.retries(1); }); })',
'describe("", function () { function a() { b(); }; it(); })',
'describe("", function () { function a() { b.c; }; it(); })',
'describe("", function () { afterEach(function() { b(); }); it(); })',
Expand Down