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.

Sign up for GitHub

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

Jump to bottom

fix(jest/no-identical-title): not reporting when using backticks #237

Merged
merged 4 commits into from
Mar 12, 2019
Merged

fix(jest/no-identical-title): not reporting when using backticks #237

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
6ad217e
fix(no-identical-titles): report if identical TemplateLiterals
himynameisdave Mar 10, 2019
4b12161
refactor: move some methods to utils
himynameisdave Mar 10, 2019
626ff33
fix(no-identical-title): guard if there is no first argument
himynameisdave Mar 11, 2019
94b723e
fix(no-identical-title): remove boolean casting
himynameisdave Mar 11, 2019
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
30 changes: 30 additions & 0 deletions rules/__tests__/no-identical-title.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,17 @@ ruleTester.run('no-identical-title', rule, {
' });',
'});',
].join('\n'),
{
code: [
'describe("describe", () => {',
' it(`testing ${someVar} with the same title`, () => {});',
' it(`testing ${someVar} with the same title`, () => {});',
macklinu marked this conversation as resolved.
Show resolved Hide resolved
'});',
].join('\n'),
env: {
es6: true,
},
},
],

invalid: [
Expand Down Expand Up @@ -212,5 +223,24 @@ ruleTester.run('no-identical-title', rule, {
},
],
},
{
code: [
'describe("describe", () => {',
' it(`testing backticks with the same title`, () => {});',
' it(`testing backticks with the same title`, () => {});',
'});',
].join('\n'),
env: {
es6: true,
},
errors: [
{
message:
'Test title is used multiple times in the same describe block.',
column: 5,
line: 3,
},
],
},
],
});
23 changes: 18 additions & 5 deletions rules/no-identical-title.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,12 @@
'use strict';

const { getDocsUrl, isDescribe, isTestCase } = require('./util');
const {
getDocsUrl,
isDescribe,
isTestCase,
isString,
hasExpressions,
} = require('./util');

const newDescribeContext = () => ({
describeTitles: [],
Expand Down Expand Up @@ -34,8 +40,15 @@ const handleDescribeBlockTitles = (context, titles, node, title) => {
titles.push(title);
};

const isFirstArgLiteral = node =>
node.arguments && node.arguments[0] && node.arguments[0].type === 'Literal';
const isFirstArgValid = arg => {
if (!isString(arg)) {
return false;
}
if (arg.type === 'TemplateLiteral' && hasExpressions(arg)) {
return false;
}
return true;
};

module.exports = {
meta: {
Expand All @@ -51,10 +64,10 @@ module.exports = {
if (isDescribe(node)) {
contexts.push(newDescribeContext());
}
if (!isFirstArgLiteral(node)) {
const [firstArgument] = node.arguments;
Copy link
Collaborator

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

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

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 👍

if (!isFirstArgValid(firstArgument)) {
return;
}

const title = node.arguments[0].value;
handleTestCaseTitles(context, currentLayer.testTitles, node, title);
handleDescribeBlockTitles(
Expand Down
3 changes: 3 additions & 0 deletions rules/util.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

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

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

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 👍


/**
* Generates the URL to documentation for the given rule name. It uses the
* package version to build the link to a tagged version of the
Expand Down Expand Up @@ -212,6 +214,7 @@ module.exports = {
isFunction,
isTestCase,
isString,
hasExpressions,
getDocsUrl,
scopeHasLocalReference,
composeFixers,
Expand Down