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(jest/no-identical-title): not reporting when using backticks #237

Merged
merged 4 commits into from Mar 12, 2019

Conversation

himynameisdave
Copy link
Contributor

@himynameisdave himynameisdave commented Mar 10, 2019

Currently jest/no-identical-title only checks against regular strings. As a result, this wouldn't get reported:

describe('an odd case where identical titles are not reported', () => {
    it(`this does not work as expected`, () => {
        //  ...test code here
    });
    it(`this does not work as expected`, () => {
        //  ...test code here
    });
});

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

@himynameisdave himynameisdave changed the title jest/no-identical-title not reporting when using backticks fix: jest/no-identical-title not reporting when using backticks Mar 10, 2019
@himynameisdave himynameisdave changed the title fix: jest/no-identical-title not reporting when using backticks fix(jest/no-identical-title): not reporting when using backticks Mar 10, 2019
Copy link
Collaborator

@macklinu macklinu left a comment

This is awesome - thank you for contributing a fix! Left a couple of comments around potential errors and code clarity.

rules/__tests__/no-identical-title.test.js Show resolved Hide resolved
@@ -51,10 +64,10 @@ module.exports = {
if (isDescribe(node)) {
contexts.push(newDescribeContext());
}
if (!isFirstArgLiteral(node)) {
const [firstArgument] = node.arguments;
Copy link
Collaborator

@macklinu macklinu Mar 10, 2019

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.

Copy link
Member

@SimenB SimenB Mar 11, 2019

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

Copy link
Contributor Author

@himynameisdave himynameisdave Mar 11, 2019

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);
Copy link
Collaborator

@macklinu macklinu Mar 10, 2019

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.

Copy link
Contributor Author

@himynameisdave himynameisdave Mar 11, 2019

Choose a reason for hiding this comment

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

Thanks for the tip!

Copy link
Contributor Author

@himynameisdave himynameisdave Mar 11, 2019

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 👍

@himynameisdave
Copy link
Contributor Author

@himynameisdave himynameisdave commented Mar 11, 2019

@macklinu || @SimenB : I've resolved the comments, just let me know if you need me to fix anything else up! 👍

SimenB
SimenB approved these changes Mar 12, 2019
Copy link
Member

@SimenB SimenB left a comment

Thanks! Solid work 🙂

@SimenB SimenB merged commit 4f8ef6d into jest-community:master Mar 12, 2019
1 check failed
@SimenB
Copy link
Member

@SimenB SimenB commented Mar 12, 2019

🎉 This PR is included in version 22.3.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants