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

feat(rule): add valid-describe rule #57

Merged
merged 2 commits into from
Feb 9, 2018
Merged
Show file tree
Hide file tree
Changes from all 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
1 change: 1 addition & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,7 @@ for more information about extending configuration files.
| [prefer-to-be-null](docs/rules/prefer-to-be-null.md) | Suggest using `toBeNull()` | | ![fixable](https://img.shields.io/badge/-fixable-green.svg) |
| [prefer-to-be-undefined](docs/rules/prefer-to-be-undefined.md) | Suggest using `toBeUndefined()` | | ![fixable](https://img.shields.io/badge/-fixable-green.svg) |
| [prefer-expect-assertions](docs/rules/prefer-expect-assertions.md) | Suggest using `expect.assertions()` OR `expect.hasAssertions()` | | |
| [valid-describe](docs/rules/valid-describe.md) | Enforce valid `describe()` callback | | |
| [valid-expect](docs/rules/valid-expect.md) | Enforce valid `expect()` usage | ![recommended](https://img.shields.io/badge/-recommended-lightgrey.svg) | |
| [valid-expect-in-promise](docs/rules/valid-expect-in-promise.md) | Enforce having return statement when testing with promises | | |

Expand Down
53 changes: 53 additions & 0 deletions docs/rules/valid-describe.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
# Enforce valid `describe()` callback (valid-describe)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I settled on calling this valid-describe since this addresses a few different cases. This follows the convention of the valid-expect rule. Open to other naming suggestions too!


Using an improper `describe()` callback function can lead to unexpected test errors.

## Rule Details

This rule validates that the second parameter of a `describe()` function is a callback function. This callback function:

* should not be [async](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/async_function)
* should not contain any parameters
* should not contain any `return` statements

The following `describe` function aliases are also validated:

* `describe`
* `describe.only`
* `describe.skip`
* `fdescribe`
* `xdescribe`

The following patterns are considered warnings:

```js
// Async callback functions are not allowed
describe('myFunction()', async () => {
// ...
});

// Callback function parameters are not allowed
describe('myFunction()', done => {
// ...
});

//
describe('myFunction', () => {
// No return statements are allowed in block of a callback function
return Promise.resolve().then(() => {
it('breaks', () => {
throw new Error('Fail');
});
});
});
```

The following patterns are not considered warnings:

```js
describe('myFunction()', () => {
it('returns a truthy value', () => {
expect(myFunction()).toBeTruthy();
});
});
```
2 changes: 2 additions & 0 deletions index.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ const noLargeSnapshots = require('./rules/no-large-snapshots');
const preferToBeNull = require('./rules/prefer-to-be-null');
const preferToBeUndefined = require('./rules/prefer-to-be-undefined');
const preferToHaveLength = require('./rules/prefer-to-have-length');
const validDescribe = require('./rules/valid-describe');
const validExpect = require('./rules/valid-expect');
const preferExpectAssertions = require('./rules/prefer-expect-assertions');
const validExpectInPromise = require('./rules/valid-expect-in-promise');
Expand Down Expand Up @@ -63,6 +64,7 @@ module.exports = {
'prefer-to-be-null': preferToBeNull,
'prefer-to-be-undefined': preferToBeUndefined,
'prefer-to-have-length': preferToHaveLength,
'valid-describe': validDescribe,
'valid-expect': validExpect,
'prefer-expect-assertions': preferExpectAssertions,
'valid-expect-in-promise': validExpectInPromise,
Expand Down
195 changes: 195 additions & 0 deletions rules/__tests__/valid-describe.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,195 @@
'use strict';

const RuleTester = require('eslint').RuleTester;
const rules = require('../..').rules;

const ruleTester = new RuleTester({
parserOptions: {
ecmaVersion: 8,
},
});

ruleTester.run('valid-describe', rules['valid-describe'], {
valid: [
'describe("foo")',
'describe("foo", function() {})',
'describe("foo", () => {})',
'xdescribe("foo", () => {})',
'fdescribe("foo", () => {})',
'describe.only("foo", () => {})',
'describe.skip("foo", () => {})',
`
describe('foo', () => {
it('bar', () => {
return Promise.resolve(42).then(value => {
expect(value).toBe(42)
})
})
})
`,
`
describe('foo', () => {
it('bar', async () => {
expect(await Promise.resolve(42)).toBe(42)
})
})
`,
],
invalid: [
{
code: 'describe("foo", async () => {})',
Copy link
Member

Choose a reason for hiding this comment

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

as a follow up, mind adding a rule that describe(() => {}) is invalid as well?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For sure, I'll open an issue to track that suggestion. 👍

errors: [{ message: 'No async describe callback', line: 1, column: 17 }],
},
{
code: 'describe("foo", async function () {})',
errors: [{ message: 'No async describe callback', line: 1, column: 17 }],
},
{
code: 'xdescribe("foo", async function () {})',
errors: [{ message: 'No async describe callback', line: 1, column: 18 }],
},
{
code: 'fdescribe("foo", async function () {})',
errors: [{ message: 'No async describe callback', line: 1, column: 18 }],
},
{
code: 'describe.only("foo", async function () {})',
errors: [{ message: 'No async describe callback', line: 1, column: 22 }],
},
{
code: 'describe.skip("foo", async function () {})',
errors: [{ message: 'No async describe callback', line: 1, column: 22 }],
},
{
code: `
describe('sample case', () => {
it('works', () => {
expect(true).toEqual(true);
});
describe('async', async () => {
await new Promise(setImmediate);
it('breaks', () => {
throw new Error('Fail');
});
});
});`,
errors: [{ message: 'No async describe callback', line: 6, column: 27 }],
},
{
code: `
describe('foo', function () {
return Promise.resolve().then(() => {
it('breaks', () => {
throw new Error('Fail')
})
})
})
`,
errors: [
{
message: 'Unexpected return statement in describe callback',
line: 3,
column: 9,
},
],
},
{
code: `
describe('foo', () => {
return Promise.resolve().then(() => {
it('breaks', () => {
throw new Error('Fail')
})
})
describe('nested', () => {
return Promise.resolve().then(() => {
it('breaks', () => {
throw new Error('Fail')
})
})
})
})
`,
errors: [
{
message: 'Unexpected return statement in describe callback',
line: 3,
column: 9,
},
{
message: 'Unexpected return statement in describe callback',
line: 9,
column: 11,
},
],
},
{
code: `
describe('foo', async () => {
await something()
it('does something')
describe('nested', () => {
return Promise.resolve().then(() => {
it('breaks', () => {
throw new Error('Fail')
})
})
})
})
`,
errors: [
{
message: 'No async describe callback',
line: 2,
column: 23,
},
{
message: 'Unexpected return statement in describe callback',
line: 6,
column: 11,
},
],
},
{
code: 'describe("foo", done => {})',
errors: [
{
message: 'Unexpected argument(s) in describe callback',
line: 1,
column: 17,
},
],
},
{
code: 'describe("foo", function (done) {})',
errors: [
{
message: 'Unexpected argument(s) in describe callback',
line: 1,
column: 27,
},
],
},
{
code: 'describe("foo", function (one, two, three) {})',
errors: [
{
message: 'Unexpected argument(s) in describe callback',
line: 1,
column: 27,
},
],
},
{
code: 'describe("foo", async function (done) {})',
errors: [
{ message: 'No async describe callback', line: 1, column: 17 },
{
message: 'Unexpected argument(s) in describe callback',
line: 1,
column: 33,
},
],
},
],
});
13 changes: 1 addition & 12 deletions rules/no-identical-title.js
Original file line number Diff line number Diff line change
@@ -1,12 +1,6 @@
'use strict';

const describeAliases = Object.assign(Object.create(null), {
describe: true,
'describe.only': true,
'describe.skip': true,
fdescribe: true,
xdescribe: true,
});
const isDescribe = require('./util').isDescribe;

const testCaseNames = Object.assign(Object.create(null), {
fit: true,
Expand All @@ -27,11 +21,6 @@ const getNodeName = node => {
return node.name;
};

const isDescribe = node =>
node &&
node.type === 'CallExpression' &&
describeAliases[getNodeName(node.callee)];

const isTestCase = node =>
node &&
node.type === 'CallExpression' &&
Expand Down
23 changes: 23 additions & 0 deletions rules/util.js
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,27 @@ const argument = node => node.parent.parent.arguments[0];

const argument2 = node => node.parent.parent.parent.arguments[0];

const describeAliases = Object.assign(Object.create(null), {
describe: true,
'describe.only': true,
'describe.skip': true,
fdescribe: true,
xdescribe: true,
});

const getNodeName = node => {
if (node.type === 'MemberExpression') {
return node.object.name + '.' + node.property.name;
}
return node.name;
};

const isDescribe = node =>
node.type === 'CallExpression' && describeAliases[getNodeName(node.callee)];

const isFunction = node =>
node.type === 'FunctionExpression' || node.type === 'ArrowFunctionExpression';

module.exports = {
method: method,
method2: method2,
Expand All @@ -95,4 +116,6 @@ module.exports = {
expectNotToEqualCase: expectNotToEqualCase,
expectToBeUndefinedCase: expectToBeUndefinedCase,
expectNotToBeUndefinedCase: expectNotToBeUndefinedCase,
isDescribe: isDescribe,
isFunction: isFunction,
};
63 changes: 63 additions & 0 deletions rules/valid-describe.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
'use strict';

const isDescribe = require('./util').isDescribe;
const isFunction = require('./util').isFunction;

const isAsync = node => node.async;

const hasParams = node => node.params.length > 0;

const paramsLocation = params => {
const first = params[0];
const last = params[params.length - 1];
return {
start: {
line: first.loc.start.line,
column: first.loc.start.column,
},
end: {
line: last.loc.end.line,
column: last.loc.end.column,
},
};
};

module.exports = {
meta: {
docs: {
url:
'https://github.com/jest-community/eslint-plugin-jest/blob/master/docs/rules/valid-describe.md',
},
},
create(context) {
return {
CallExpression(node) {
if (isDescribe(node)) {
const callbackFunction = node.arguments[1];
if (callbackFunction && isFunction(callbackFunction)) {
if (isAsync(callbackFunction)) {
context.report({
message: 'No async describe callback',
node: callbackFunction,
});
}
if (hasParams(callbackFunction)) {
context.report({
message: 'Unexpected argument(s) in describe callback',
loc: paramsLocation(callbackFunction.params),
});
}
callbackFunction.body.body.forEach(node => {
if (node.type === 'ReturnStatement') {
context.report({
message: 'Unexpected return statement in describe callback',
node: node,
});
}
});
}
}
},
};
},
};
Loading