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

Introduce a lint rule to report error when testing promises. If a exp… #42

Merged
merged 19 commits into from
Jan 17, 2018
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
d70d9ed
feat: implement valid-expect-in-promise rule
tushardhole Dec 30, 2017
ab0f43c
fix: do not add the rule to recommended
tushardhole Jan 9, 2018
da393e0
fix: do not validate await expressions for valid_expect_in_promise
tushardhole Jan 9, 2018
dbba301
fix: scenario with expect in nested then/catch
tushardhole Jan 10, 2018
4651b68
docs: updating docs for valid-expect-in-promise rule
tushardhole Jan 10, 2018
5b25a69
test: add failing test case
SimenB Jan 14, 2018
8b6367d
fix: scenario where promise is returned later
tushardhole Jan 14, 2018
ede8427
fix: adding more scenarios where promise is returned later
tushardhole Jan 14, 2018
8614010
test: adding more tests
tushardhole Jan 14, 2018
9abdd01
fix: adding scenarios for non test functions
tushardhole Jan 14, 2018
3314dbf
fix: refactor to avoid multiple execution of getTestFuncBody
tushardhole Jan 14, 2018
00422dd
fix: scenario with arrow-short-hand-fn with implicit return statement
tushardhole Jan 14, 2018
1242897
fix: scenario with multiline function in then block
tushardhole Jan 15, 2018
0bd4572
fix: scenario with short hand arrow function in then block
tushardhole Jan 15, 2018
4237828
fix: do not validate tests with done async param
tushardhole Jan 15, 2018
567b85c
fix: scenario where expect in then is preceded by return
tushardhole Jan 15, 2018
5a69337
fix: duplicate warning for same promise
tushardhole Jan 16, 2018
7e44766
fix: better naming
tushardhole Jan 16, 2018
e2dc02c
test: better formatting
tushardhole Jan 17, 2018
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions index.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ const preferToBeUndefined = require('./rules/prefer_to_be_undefined');
const preferToHaveLength = require('./rules/prefer_to_have_length');
const validExpect = require('./rules/valid_expect');
const preferExpectAssertions = require('./rules/prefer_expect_assertions');
const validExpectInPromise = require('./rules/valid_expect_in_promise');

const snapshotProcessor = require('./processors/snapshot-processor');

Expand Down Expand Up @@ -64,5 +65,6 @@ module.exports = {
'prefer-to-have-length': preferToHaveLength,
'valid-expect': validExpect,
'prefer-expect-assertions': preferExpectAssertions,
'valid-expect-in-promise': validExpectInPromise,
},
};
144 changes: 144 additions & 0 deletions rules/__tests__/valid_expect_in_promise.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,144 @@
'use strict';

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

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

const expectedMsg =
'Promise should be returned to test its fulfillment or rejection';

ruleTester.run('valid-expect-in-promise', rules['valid-expect-in-promise'], {
invalid: [
{
code:
'it("it1", () => { somePromise.then(' +
'() => {expect(someThing).toEqual(true)})})',
errors: [
{
column: 19,
endColumn: 76,
message: expectedMsg,
},
],
},
{
code:
'it("it1", function() { getSomeThing().getPromise().then(' +
'function() {expect(someThing).toEqual(true)})})',
errors: [
{
column: 24,
endColumn: 102,
message: expectedMsg,
},
],
},
{
code:
'it("it1", function() { Promise.resolve().then(' +
'function() {expect(someThing).toEqual(true)})})',
errors: [
{
column: 24,
endColumn: 92,
message: expectedMsg,
},
],
},
{
code:
'it("it1", function() { somePromise.catch(' +
'function() {expect(someThing).toEqual(true)})})',
errors: [
{
column: 24,
endColumn: 87,
message: expectedMsg,
},
],
},
{
code:
'it("it1", function() { somePromise.then(' +
'function() { expect(someThing).toEqual(true)})})',
errors: [
{
column: 24,
endColumn: 87,
message: expectedMsg,
},
],
},
{
code:
'it("it1", function() { Promise.resolve().then(' +
'function() { /*fulfillment*/ expect(someThing).toEqual(true)}, ' +
'function() { /*rejection*/ expect(someThing).toEqual(true)})})',
errors: [
Copy link
Member

Choose a reason for hiding this comment

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

why is this 2 errors? It should just be one, there is only a single promise here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will fix that.

{
column: 24,
endColumn: 170,
message: expectedMsg,
},
{
column: 24,
endColumn: 170,
message: expectedMsg,
},
],
},
{
code:
'it("it1", function() { Promise.resolve().then(' +
'function() { /*fulfillment*/}, ' +
'function() { /*rejection*/ expect(someThing).toEqual(true)})})',
errors: [
{
column: 24,
endColumn: 138,
message: expectedMsg,
},
],
},
],

valid: [
'it("it1", () => { return somePromise.then(() => {expect(someThing).toEqual(true)})})',

'it("it1", function() { return somePromise.catch(' +
'function() {expect(someThing).toEqual(true)})})',

'it("it1", function() { somePromise.then(' +
'function() {doSomeThingButNotExpect()})})',

'it("it1", function() { return getSomeThing().getPromise().then(' +
'function() {expect(someThing).toEqual(true)})})',

'it("it1", function() { return Promise.resolve().then(' +
'function() {expect(someThing).toEqual(true)})})',

'it("it1", function() { return Promise.resolve().then(' +
'function() { /*fulfillment*/ expect(someThing).toEqual(true)}, ' +
'function() { /*rejection*/ expect(someThing).toEqual(true)})})',

'it("it1", function() { return Promise.resolve().then(' +
'function() { /*fulfillment*/}, ' +
'function() { /*rejection*/ expect(someThing).toEqual(true)})})',

'it("it1", function() { return somePromise.then()})',
Copy link
Member

Choose a reason for hiding this comment

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

can you add assertions that await is ok without return?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will work on that.


'it("it1", async () => { await Promise.resolve().then(' +
'function() {expect(someThing).toEqual(true)})})',

'it("it1", async () => ' +
'{ await somePromise.then(() => {expect(someThing).toEqual(true)})})',

'it("it1", async () => { await getSomeThing().getPromise().then(' +
'function() {expect(someThing).toEqual(true)})})',
],
});
85 changes: 85 additions & 0 deletions rules/valid_expect_in_promise.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,85 @@
'use strict';

const reportMsg =
'Promise should be returned to test its fulfillment or rejection';

const isThenOrCatch = node => {
return node.property.name == 'then' || node.property.name == 'catch';
};

const isFunction = type => {
return type == 'FunctionExpression' || type == 'ArrowFunctionExpression';
};

const isBodyCallExpression = argumentBody => {
try {
return argumentBody.body[0].expression.type == 'CallExpression';
} catch (e) {
return false;
}
};

const isExpectCall = calleeObject => {
try {
return calleeObject.callee.name == 'expect';
} catch (e) {
return false;
}
};

const reportReturnRequired = (context, node) => {
context.report({
loc: {
end: {
column: node.parent.parent.loc.end.column,
line: node.parent.parent.loc.end.line,
},
start: node.parent.parent.loc.start,
},
message: reportMsg,
node,
});
};

const verifyExpectWithReturn = (argument, node, context) => {
if (
argument &&
isFunction(argument.type) &&
isBodyCallExpression(argument.body)
) {
const calleeInThenOrCatch = argument.body.body[0].expression.callee.object;
if (isExpectCall(calleeInThenOrCatch)) {
if (node.parent.parent.type != 'ReturnStatement') {
reportReturnRequired(context, node);
}
}
}
};

const isAwaitExpression = node => {
return node.parent.parent && node.parent.parent.type == 'AwaitExpression';
};

module.exports = context => {
return {
MemberExpression(node) {
if (
node.type == 'MemberExpression' &&
isThenOrCatch(node) &&
node.parent.type == 'CallExpression' &&
!isAwaitExpression(node)
) {
const parent = node.parent;
const arg1 = parent.arguments[0];
const arg2 = parent.arguments[1];

// then block can have two args, fulfillment & rejection
// then block can have one args, fulfillment
// catch block can have one args, rejection
// ref: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise
verifyExpectWithReturn(arg1, node, context);
verifyExpectWithReturn(arg2, node, context);
}
},
};
};