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(valid-expect): validate async expect calls #78

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
32 changes: 32 additions & 0 deletions docs/rules/valid-expect.md
Expand Up @@ -33,6 +33,19 @@ expect('something', 'else');
expect('something');
expect(true).toBeDefined;
expect(Promise.resolve('hello')).resolves;

// 👎 expect(Promise).resolves is not returned
test('foo', () => {
expect(Promise.resolve('hello')).resolves.toBeDefined();
});
// 👎 expect(Promise).resolves is not awaited
test('foo', async () => {
expect(Promise.resolve('hello')).resolves.toBeDefined();
});
// 👎 expect(awaited Promise) should not use .resolves or .rejects property
test('foo', async () => {
expect(await Promise.resolve('hello')).resolves.toBeDefined();
});
```

The following patterns are not warnings:
Expand All @@ -42,4 +55,23 @@ expect('something').toEqual('something');
expect([1, 2, 3]).toEqual([1, 2, 3]);
expect(true).toBeDefined();
expect(Promise.resolve('hello')).resolves.toEqual('hello');

// 👍 expect(Promise).resolves is returned
test('foo', () => {
return expect(Promise.resolve('hello')).resolves.toBeDefined();
});
// 👍 expect(Promise).resolves is awaited
test('foo', async () => {
await expect(Promise.resolve('hello')).resolves.toBeDefined();
});
// 👍 expect(Promise).rejects is implicitly returned
test('foo', () => expect(Promise.reject('hello')).rejects.toBeDefined());
// 👍 expect(awaited Promise) is not used with .resolves
test('foo', async () => {
expect(await Promise.resolve('hello')).toBeDefined();
});
// 👍 expect(awaited Promise) is not used with .rejects
test('foo', async () => {
expect(await Promise.reject('hello')).toBeDefined();
});
```
3 changes: 3 additions & 0 deletions package.json
Expand Up @@ -73,5 +73,8 @@
},
"commitlint": {
"extends": ["@commitlint/config-conventional"]
},
"dependencies": {
"lodash.get": "^4.4.2"
}
}
62 changes: 59 additions & 3 deletions rules/__tests__/valid-expect.test.js
Expand Up @@ -3,16 +3,24 @@
const RuleTester = require('eslint').RuleTester;
const rule = require('../valid-expect');

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

ruleTester.run('valid-expect', rule, {
valid: [
'expect("something").toEqual("else");',
'expect(true).toBeDefined();',
'expect([1, 2, 3]).toEqual([1, 2, 3]);',
'expect(undefined).not.toBeDefined();',
'expect(Promise.resolve(2)).resolves.toBeDefined();',
'expect(Promise.reject(2)).rejects.toBeDefined();',
'test("foo", () => { return expect(Promise.resolve(2)).resolves.toBeDefined(); });',
'test("foo", async () => { await expect(Promise.reject(2)).rejects.toBeDefined(); });',
'test("foo", () => expect(Promise.resolve(2)).resolves.toBeDefined());',
'test("foo", async () => await expect(Promise.reject(2)).rejects.toBeDefined());',
'test("foo", async () => { expect(await Promise.resolve(2)).toBeDefined(); });',
'test("foo", async () => { expect(await Promise.reject(2)).toBeDefined(); });',
],

invalid: [
Expand Down Expand Up @@ -131,5 +139,53 @@ ruleTester.run('valid-expect', rule, {
},
],
},
{
code:
'test("foo", () => { expect(Promise.resolve(2)).resolves.toBeDefined(); });',
errors: [{ message: "Must return or await 'expect.resolves' statement" }],
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 assert on the locations 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.

Yep, will make those changes 👍

},
{
code:
'test("foo", async () => { expect(Promise.resolve(2)).resolves.toBeDefined(); });',
errors: [{ message: "Must await 'expect.resolves' statement" }],
},
{
code:
'test("foo", () => { expect(Promise.reject(2)).rejects.toBeDefined(); });',
errors: [{ message: "Must return or await 'expect.rejects' statement" }],
},
{
code:
'test("foo", async () => { expect(Promise.reject(2)).rejects.toBeDefined(); });',
errors: [{ message: "Must await 'expect.rejects' statement" }],
},
{
code:
'test("foo", async () => { expect(await Promise.resolve(2)).resolves.toBeDefined(); });',
errors: [
{ message: "Cannot use 'resolves' with an awaited expect expression" },
],
},
{
code:
'test("foo", async () => { expect(await Promise.reject(2)).rejects.toBeDefined(); });',
errors: [
{ message: "Cannot use 'rejects' with an awaited expect expression" },
],
},
{
code:
'test("foo", async () => { expect(await Promise.resolve(undefined)).resolves.not.toBeDefined(); });',
errors: [
{ message: "Cannot use 'resolves' with an awaited expect expression" },
],
},
{
code:
'test("foo", async () => { expect(await Promise.reject(undefined)).rejects.not.toBeDefined(); });',
errors: [
{ message: "Cannot use 'rejects' with an awaited expect expression" },
],
},
],
});
57 changes: 55 additions & 2 deletions rules/valid-expect.js
Expand Up @@ -5,19 +5,72 @@
* MIT license, Tom Vincent.
*/

const getDocsUrl = require('./util').getDocsUrl;
const get = require('lodash.get');
const util = require('./util');

const expectProperties = ['not', 'resolves', 'rejects'];

module.exports = {
meta: {
docs: {
url: getDocsUrl(__filename),
url: util.getDocsUrl(__filename),
},
},
create(context) {
function validateAsyncExpects(node) {
const callback = node.arguments[1];
if (
callback &&
util.isFunction(callback) &&
callback.body.type === 'BlockStatement'
) {
callback.body.body
.filter(node => node.type === 'ExpressionStatement')
.filter(node => {
const objectName = get(
node,
'expression.callee.object.object.callee.name'
);
const propertyName = get(
node,
'expression.callee.object.property.name'
);

return (
node.expression.type === 'CallExpression' &&
objectName === 'expect' &&
(propertyName === 'resolves' || propertyName === 'rejects')
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we can reuse the logic somehow with the other resolves/rejects checks

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good idea. I'll look into that while resolving the failing test case you reported below.

);
})
.forEach(node => {
const propertyName = get(
node,
'expression.callee.object.property.name'
);
const isAwaitInsideExpect =
get(node, 'expression.callee.object.object.arguments[0].type') ===
'AwaitExpression';

context.report({
node,
message: isAwaitInsideExpect
? "Cannot use '{{ propertyName }}' with an awaited expect expression"
: callback.async
? "Must await 'expect.{{ propertyName }}' statement"
: "Must return or await 'expect.{{ propertyName }}' statement",
data: { propertyName },
});
});
}
}

return {
CallExpression(node) {
if (util.isTestCase(node)) {
validateAsyncExpects(node);
return;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If we are in a test case function, there is no need to proceed, since the node.callee.name will never be expect.

}

const calleeName = node.callee.name;

if (calleeName === 'expect') {
Expand Down
4 changes: 4 additions & 0 deletions yarn.lock
Expand Up @@ -3660,6 +3660,10 @@ lodash.camelcase@4.3.0:
version "4.3.0"
resolved "https://registry.yarnpkg.com/lodash.camelcase/-/lodash.camelcase-4.3.0.tgz#b28aa6288a2b9fc651035c7711f65ab6190331a6"

lodash.get@^4.4.2:
version "4.4.2"
resolved "https://registry.yarnpkg.com/lodash.get/-/lodash.get-4.4.2.tgz#2d177f652fa31e939b4438d5341499dfa3825e99"

lodash.kebabcase@4.1.1:
version "4.1.1"
resolved "https://registry.yarnpkg.com/lodash.kebabcase/-/lodash.kebabcase-4.1.1.tgz#8489b1cb0d29ff88195cceca448ff6d6cc295c36"
Expand Down