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(jest/no-identical-title): not reporting when using backticks #237
Conversation
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.
This is awesome - thank you for contributing a fix! Left a couple of comments around potential errors and code clarity.
@@ -51,10 +64,10 @@ module.exports = { | |||
if (isDescribe(node)) { | |||
contexts.push(newDescribeContext()); | |||
} | |||
if (!isFirstArgLiteral(node)) { | |||
const [firstArgument] = node.arguments; |
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 think that firstArgument
can technically be undefined. I'm not 100% sure we are protecting against that case. For example, the following code:
describe()
produces the following AST
{
"type": "Program",
"start": 0,
"end": 10,
"range": [
0,
10
],
"body": [
{
"type": "ExpressionStatement",
"start": 0,
"end": 10,
"range": [
0,
10
],
"expression": {
"type": "CallExpression",
"start": 0,
"end": 10,
"range": [
0,
10
],
"callee": {
"type": "Identifier",
"start": 0,
"end": 8,
"range": [
0,
8
],
"name": "describe"
},
"arguments": []
}
}
],
"sourceType": "module"
}
So node.arguments[0] === undefined
.
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.
Good catch, should be a test case
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.
Yep totally agree, will add 👍
rules/util.js
Outdated
@@ -134,6 +134,8 @@ const isString = node => | |||
(node.type === 'Literal' && typeof node.value === 'string') || | |||
node.type === 'TemplateLiteral'; | |||
|
|||
const hasExpressions = node => !!(node.expressions && node.expressions.length); |
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.
The double !!
was a little unclear reading at first. May I suggest?
const hasExpressions = node => node.expressions && node.expressions.length > 0;
That way, the function is always returning a boolean without the extra type cast.
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.
Thanks for the tip!
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 think that my thinking was for if node.expressions
was not defined, the entire statement would evaluate to undefined
(instead of an actual Boolean). Doesn't matter though because undefined
is falsey, so I'll make that change to the test against node.expressions.length
👍
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.
Thanks! Solid work 🙂
🎉 This PR is included in version 22.3.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Currently
jest/no-identical-title
only checks against regular strings. As a result, this wouldn't get reported:This ticket fixes that by testing against
TemplateLiteral
nodes, but will never report if that node is using string interpolation (ie: has expressions,${}
inside it).Resolves #232