False positives for `no-identical-title` rule

[ranyitz]

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]

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

eslint-plugin-jest/src/rules/tsUtils.ts

Lines 249 to 261 in b27c80d

	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]

@SimenB That's a much better solution 👍

Sure, I'd love to!

[SimenB]

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]

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]

@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]

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]

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

[SimenB]

🎉 This issue has been resolved in version 22.13.5 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀