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
Added support for async functions in no-synchronous-tests #138
Conversation
To validate |
lib/rules/no-synchronous-tests.js
Outdated
} | ||
|
||
return callee.name; | ||
function getCalleeName(callee) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know this function was already there before but it would be nice to re-use the getNodeName
method from our AST utils.
💅
lib/rules/no-synchronous-tests.js
Outdated
// allow arrow statements calling a promise with implicit return. | ||
returnStatement = bodyStatement; | ||
} | ||
function getAllowedAsyncMethocsFromOptions(options) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo? I guess it should be getAllowedAsyncMethodsFromOptions
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall it looks good 👍, but I would change the option validation to use a JSON schema.
I have implemented the requested changes. I am using isTestCase/isHookIdentifier from ast utils and got rid of the list of identifiers for tests/hooks in no-synchronous-tests. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, LGTM.
Thanks for your contribution.
One note: the updated rule throws errors when the provided options do not match a certain format.
eslint itself usually does not test those exception cases (it ignores them for code coverage: https://github.com/eslint/eslint/blob/master/lib/rules/new-cap.js#L38) and I wasn't sure how to test an exception with the RuleTester so I ignored those cases as well.