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

False positives for no-identical-title rule #340

Closed
ranyitz opened this issue Jul 22, 2019 · 9 comments · Fixed by #341 or #343
Closed

False positives for no-identical-title rule #340

ranyitz opened this issue Jul 22, 2019 · 9 comments · Fixed by #341 or #343

Comments

@ranyitz
Copy link
Contributor

ranyitz commented Jul 22, 2019

The Problem

Given the following code (simplified version of the use-case):

it('data.foo should contain bar and baz', () => {
  const data = {
    foo: 'bar_baz',
  };

  const test = {
    content: key => data[key],
  };

  expect(test.content('foo')).to.contain('bar');
  expect(test.content('foo')).to.contain('baz');
});

We get the following error:

  11:10  error  Test title is used multiple times in the same describe block  jest/no-identical-title

Why it Happened?

As part of #316 & #317 we now treat test.<anything>() as a test case. While only (test|it).skip, (test|it).only & (test|it).todo are Jest valid options.

In a project that works with its and describes it's possible that a user will choose the name test for a utility object.

(Sadly our case 🙈 )

While in our project we can change those object names, it will create a better developer experience if eslint-plugin-jest will avoid raising false-positive errors in this case.

Proposed Solution

In the following context https://github.com/jest-community/eslint-plugin-jest/pull/317/files#diff-fecc1b14fa96780438948ea994aa4b29R123

Verify that the node.callee.property.name is one of the following: skip|only|todo.

@SimenB
Copy link
Member

SimenB commented Jul 22, 2019

Should check that expect, test, it and describe are globals and not defined locally. We have a util for this already:

export const scopeHasLocalReference = (
scope: TSESLint.Scope.Scope,
referenceName: string,
) => {
const references = collectReferences(scope);
return (
// referenceName was found as a local variable or function declaration.
references.locals.has(referenceName) ||
// referenceName was not found as an unresolved reference,
// meaning it is likely not an implicit global reference.
!references.unresolved.has(referenceName)
);
};

Wanna send a PR?

@ranyitz
Copy link
Contributor Author

ranyitz commented Jul 22, 2019

@SimenB That's a much better solution 👍

Sure, I'd love to!

@SimenB
Copy link
Member

SimenB commented Jul 22, 2019

Awesome! Should be enough to add the check to the is{ExpectIdentifier,TestCase,Describe,Hook} utils and we shouldn't be caught out by something like this again

@SimenB
Copy link
Member

SimenB commented Jul 22, 2019

Thinking about it, it's probably a somewhat expensive call, I'm not sure if we're able to cache it on a single file somehow? If not, might be best to just export a util we can call in rules that's e.g. isGlobalExpect or something, which can be used to bail early in rules. Not sure what makes the most sense 🙂 @G-Rath maybe has ideas?

@ranyitz
Copy link
Contributor Author

ranyitz commented Jul 22, 2019

@SimenB yes, also when thinking about it, verifying skip|only|todo after test/it/describe would be good in any case. We're using the isTestCase util in many places and can get some obvious false positives like test.fooBarBaz().

@SimenB
Copy link
Member

SimenB commented Jul 22, 2019

We also have test.each though, and I think some future rule might be checking that you use only existent members makes sense. Not sure.

A first go can of course limit this to the known members, which was the case previously.

@ranyitz
Copy link
Contributor Author

ranyitz commented Jul 22, 2019

@SimenB Yeah you're right, we should add each to the list of valid properties.

@SimenB
Copy link
Member

SimenB commented Jul 22, 2019

🎉 This issue has been resolved in version 22.13.5 🎉

The release is available on:

Your semantic-release bot 📦🚀

@SimenB
Copy link
Member

SimenB commented Jul 23, 2019

🎉 This issue has been resolved in version 22.13.6 🎉

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
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants