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(no-identical-title): always consider .each titles unique #910

Merged
merged 1 commit into from
Sep 29, 2021

Conversation

G-Rath
Copy link
Collaborator

@G-Rath G-Rath commented Sep 25, 2021

@SimenB I think we should try and get this sorted before the next major too, as
it has been similarly disruptive to a small subset of users.

I'd be interested in your opinion on this - my understanding of the rule is that
it's trying to enforce this:

"you should be able to differentiate between two tests by their title alone"

As the title should always be provided to reporters whereas other information
(like what line in code the test is on) isn't guaranteed, and so having titles
that are identical on the same nesting level makes it very hard to tell which is
what test in code.

This is easy to handle with static titles, but .each supports injecting values
into their static titles, making it harder to decide if a title is unique or not
since we cannot predict the final title(s).

I think this boils down to deciding which of these cases we should consider
unique:

.each()('go') // (function, no injection)
.each``('go') // (template, no injection)
.each()('%s') // (function, %-based injection)
.each()('$s') // (function, $-based injection)
.each``('$s') // (template, $-based injection)

Ideally we should be treating all "no injection"s as identical, but the general
issue is how reliably can we determine if there is an injection or not (which I
think we can, we just need the time to implement it).

I think for now we should say for this rule that any .each title is unique
(which is what this change does), and then follow-up with implementing logic to
handle injections - either as a new rule (e.g. require-injection-in-each) or
as an option on this rule (e.g. ignoreEachWithInjections), or maybe both
depending on how it plays out.

Fixes #836

@G-Rath G-Rath requested a review from SimenB September 25, 2021 20:33
@G-Rath G-Rath mentioned this pull request Sep 25, 2021
2 tasks
@G-Rath G-Rath added each support Relates to supporting the `each` method rule: no-identical-title bug labels Sep 25, 2021
@SimenB
Copy link
Member

SimenB commented Sep 29, 2021

I'd be interested in your opinion on this - my understanding of the rule is that it's trying to enforce this:

"you should be able to differentiate between two tests by their title alone"

As the title should always be provided to reporters whereas other information (like what line in code the test is on) isn't guaranteed, and so having titles that are identical on the same nesting level makes it very hard to tell which is what test in code.

Correct 🙂

I think for now we should say for this rule that any .each title is unique
(which is what this change does)

Yeah, seems like a reasonable way to go about it 👍

@SimenB SimenB merged commit a41a40e into main Sep 29, 2021
@SimenB SimenB deleted the adjust-no-identical-title branch September 29, 2021 07:36
github-actions bot pushed a commit that referenced this pull request Sep 29, 2021
# [24.5.0](v24.4.3...v24.5.0) (2021-09-29)

### Bug Fixes

* **no-deprecated-functions:** remove `process.cwd` from resolve paths ([#889](#889)) ([6940488](6940488))
* **no-identical-title:** always consider `.each` titles unique ([#910](#910)) ([a41a40e](a41a40e))

### Features

* create `prefer-expect-resolves` rule ([#822](#822)) ([2556020](2556020))
* create `prefer-to-be` rule ([#864](#864)) ([3a64aea](3a64aea))
* **require-top-level-describe:** support enforcing max num of describes ([#912](#912)) ([14a2d13](14a2d13))
* **valid-title:** allow custom matcher messages ([#913](#913)) ([ffc9392](ffc9392))
@github-actions
Copy link

🎉 This PR is included in version 24.5.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

github-actions bot pushed a commit that referenced this pull request Sep 29, 2021
# [25.0.0-next.5](v25.0.0-next.4...v25.0.0-next.5) (2021-09-29)

### Bug Fixes

* **no-deprecated-functions:** remove `process.cwd` from resolve paths ([#889](#889)) ([6940488](6940488))
* **no-identical-title:** always consider `.each` titles unique ([#910](#910)) ([a41a40e](a41a40e))
* **valid-expect-in-promise:** support `finally` ([#914](#914)) ([9c89855](9c89855))
* **valid-expect-in-promise:** support additional test functions ([#915](#915)) ([4798005](4798005))

### Features

* create `prefer-expect-resolves` rule ([#822](#822)) ([2556020](2556020))
* create `prefer-to-be` rule ([#864](#864)) ([3a64aea](3a64aea))
* **require-top-level-describe:** support enforcing max num of describes ([#912](#912)) ([14a2d13](14a2d13))
* **valid-title:** allow custom matcher messages ([#913](#913)) ([ffc9392](ffc9392))
@github-actions
Copy link

🎉 This PR is included in version 25.0.0-next.5 🎉

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 this pull request may close these issues.

New handling of .each calls breaks no-identical-title
2 participants