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

Add eslint rule valid-expect #3067

Merged
merged 7 commits into from
Mar 6, 2017
Merged
Show file tree
Hide file tree
Changes from 6 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 packages/eslint-config-fb-strict/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ module.exports = Object.assign({}, fbjsConfig, {
'flowtype/object-type-delimiter': [2, 'comma'],
'jest/no-focused-tests': [2],
'jest/no-identical-title': [2],
'jest/valid-expect': [2],
'max-len': [2, {
'code': 80,
'ignorePattern': maxLenIgnorePattern,
Expand Down
3 changes: 3 additions & 0 deletions packages/eslint-plugin-jest/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ Then configure the rules you want to use under the rules section.
"jest/no-disabled-tests": "warn",
"jest/no-focused-tests": "error",
"jest/no-identical-title": "error",
"jest/valid-expect": "error",
Copy link
Member Author

Choose a reason for hiding this comment

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

This line is a breaking change btw. If the next release is not a major, this should be dropped

}
}
```
Expand All @@ -50,6 +51,7 @@ You can also whitelist the environment variables provided by Jest by doing:
- [no-disabled-tests](/packages/eslint-plugin-jest/docs/rules/no-disabled-tests.md) - disallow disabled tests.
- [no-focused-tests](/packages/eslint-plugin-jest/docs/rules/no-focused-tests.md) - disallow focused tests.
- [no-identical-title](/packages/eslint-plugin-jest/docs/rules/no-identical-title.md) - disallow identical titles.
- [valid-expect](/packages/eslint-plugin-jest/docs/rules/valid-expect.md) - ensure expect is called correctly.

## Shareable configurations

Expand All @@ -72,6 +74,7 @@ The rules enabled in this configuration are:
- [jest/no-disabled-tests](/packages/eslint-plugin-jest/docs/rules/no-disabled-tests.md)
- [jest/no-focused-tests](/packages/eslint-plugin-jest/docs/rules/no-focused-tests.md)
- [jest/no-identical-title](/packages/eslint-plugin-jest/docs/rules/no-identical-title.md)
- [jest/valid-expect](/packages/eslint-plugin-jest/docs/rules/valid-expect.md)

## Credit

Expand Down
41 changes: 41 additions & 0 deletions packages/eslint-plugin-jest/docs/rules/valid-expect.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
# Enforce valid `expect()` usage (valid-expect)

Ensure `expect()` is called with a single argument and there is an actual expectation made.

## Rule details

This rule triggers a warning if `expect()` is called with more than one argument or without arguments.
It would also issue a warning if there is nothing called on `expect()`, e.g.:

```js
expect();
expect('something');
```

or when a matcher function was not called, e.g.:

```js
expect(true).toBeDefined
```

This rule is enabled by default.

### Default configuration

The following patterns are considered warnings:

```js
expect();
expect().toEqual('something');
expect('something', 'else');
expect('something');
expect(true).toBeDefined;
```

The following patterns are not warnings:

```js
expect('something').toEqual('something');
expect([1, 2, 3]).toEqual([1, 2, 3]);
expect(true).toBeDefined();
```
2 changes: 2 additions & 0 deletions packages/eslint-plugin-jest/src/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ module.exports = {
'jest/no-disabled-tests': 'warn',
'jest/no-focused-tests': 'error',
'jest/no-identical-title': 'error',
'jest/valid-expect': 'error',
},
},
},
Expand Down Expand Up @@ -46,5 +47,6 @@ module.exports = {
'no-disabled-tests': require('./rules/no-disabled-tests'),
'no-focused-tests': require('./rules/no-focused-tests'),
'no-identical-title': require('./rules/no-identical-title'),
'valid-expect': require('./rules/valid-expect'),
},
};
Original file line number Diff line number Diff line change
@@ -0,0 +1,81 @@
/**
* Copyright (c) 2014-present, Facebook, Inc. All rights reserved.
*
* This source code is licensed under the BSD-style license found in the
* LICENSE file in the root directory of this source tree. An additional grant
* of patent rights can be found in the PATENTS file in the same directory.
*
* @flow
*/

/* eslint-disable sort-keys */

'use strict';

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

const ruleTester = new RuleTester();

ruleTester.run('valid-expect', rules['valid-expect'], {
valid: [
'expect("something").toEqual("else");',
'expect(true).toBeDefined();',
'expect([1, 2, 3]).toEqual([1, 2, 3]);',
'expect(undefined).not.toBeDefined();',
],

invalid: [
{
code: 'expect().toBe(true);',
errors: [
{
message: 'No arguments were passed to expect().',
},
],
},
{
code: 'expect().toEqual("something");',
errors: [
{
message: 'No arguments were passed to expect().',
},
],
},
{
code: 'expect("something", "else").toEqual("something");',
errors: [
{
message: 'More than one argument was passed to expect().',
},
],
},
{
code: 'expect("something");',
errors: [
{
message: 'No assertion was called on expect().',
},
],
},
{
code: 'expect();',
errors: [
{
message: 'No arguments were passed to expect().',
},
{
message: 'No assertion was called on expect().',
},
],
},
{
code: 'expect(true).toBeDefined;',
errors: [
{
message: '"toBeDefined" was not called.',
},
],
},
],
});
22 changes: 14 additions & 8 deletions packages/eslint-plugin-jest/src/rules/types.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,31 +8,37 @@
* @flow
*/

export type Identifier = {|
type Node = MemberExpression | CallExpression;
Copy link
Member Author

Choose a reason for hiding this comment

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

I started doing this, but my flow-fu failed me. Didn't know it'd be as little as this!


export type Identifier = {
type: 'Identifier',
name: string,
value: string,
|};
parent: Node,
};

export type MemberExpression = {|
export type MemberExpression = {
type: 'MemberExpression',
name: string,
expression: CallExpression,
property: Identifier,
object: Identifier,
|};
parent: Node,
};

export type Literal = {|
export type Literal = {
type: 'Literal',
value?: string,
rawValue?: string,
|};
parent: Node,
};

export type CallExpression = {|
export type CallExpression = {
type: 'CallExpression',
arguments: [Literal],
callee: Identifier | MemberExpression,
|};
parent: Node,
};

export type EslintContext = {|
report: ({message: string, node: any}) => void
Expand Down
61 changes: 61 additions & 0 deletions packages/eslint-plugin-jest/src/rules/valid-expect.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
/**
* Copyright (c) 2014-present, Facebook, Inc. All rights reserved.
*
* This source code is licensed under the BSD-style license found in the
* LICENSE file in the root directory of this source tree. An additional grant
* of patent rights can be found in the PATENTS file in the same directory.
*
* @flow
*/
'use strict';

/*
* This implementation is ported from from eslint-plugin-jasmine.
* MIT license, Tom Vincent.
*/

import type {EslintContext, CallExpression} from './types';

module.exports = (context: EslintContext) => {
return {
CallExpression(node: CallExpression) {
if (node.callee.name === 'expect') {
// checking "expect()" arguments
if (node.arguments.length > 1) {
context.report({
message: 'More than one argument was passed to expect().',
node,
});
} else if (node.arguments.length === 0) {
context.report({
message: 'No arguments were passed to expect().',
node,
});
}

// matcher was not called
if (
node.parent &&
node.parent.type === 'MemberExpression' &&
node.parent.parent &&
node.parent.parent.type === 'ExpressionStatement'
) {
context.report({
message: `"${node.parent.property.name}" was not called.`,
node,
});
}
}
},

// nothing called on "expect()"
'CallExpression:exit'(node: CallExpression) {
if (
node.callee.name === 'expect' &&
node.parent.type === 'ExpressionStatement'
) {
context.report({message: 'No assertion was called on expect().', node});
}
},
};
};
4 changes: 2 additions & 2 deletions packages/pretty-format/src/__tests__/pretty-format-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -234,8 +234,8 @@ describe('prettyFormat()', () => {
});

it('prints a string with escapes', () => {
expect(prettyFormat('\"-\"'), '"\\"-\\""');
expect(prettyFormat('\\ \\\\'), '"\\\\ \\\\\\\\"');
expect(prettyFormat('\"-\"')).toEqual('"\\"-\\""');
expect(prettyFormat('\\ \\\\')).toEqual('"\\\\ \\\\\\\\"');
Copy link
Member

Choose a reason for hiding this comment

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

And this is the point where you proved the value of this rule! Thanks.

});

it('prints a symbol', () => {
Expand Down