-
-
Notifications
You must be signed in to change notification settings - Fork 30
New: disallow identical tests #11
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR! This generally looks good -- I just have a small suggestion.
Also, can you add docs for the rule? There are two places to put docs:
- In a file in the
docs/rules/
folder - In the table in the readme here
lib/rules/no-identical-tests.js
Outdated
docs: { | ||
description: 'disallow identical tests', | ||
category: 'Tests', | ||
recommended: true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you set this to false
for now? I agree that it would be good to set it to true
eventually, but that will be a breaking change, so I think it would be better to set it to false
at first and then change it a bit afterwards.
const cache = Object.create(null); | ||
// to avoid tests being null | ||
(tests || []).forEach(test => { | ||
const testCode = sourceCode.getText(test); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Eventually, I think it would also be useful to also catch cases like this:
{
code: 'foo',
options: ['bar']
},
{
options: ['bar'],
code: 'foo'
}
But it's probably not necessary to change that as part of this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, it is an enhancement, I will finish it in another PR.:(
lib/rules/no-identical-tests.js
Outdated
// ---------------------------------------------------------------------- | ||
// Public | ||
// ---------------------------------------------------------------------- | ||
const message = 'This test case should not be identical to someone else.'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: Can you reword this message to something like this? "This test case is identical to another case."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just have a minor nitpick with the docs, otherwise this looks good to me.
I noticed the title of the PR has "[WIP]" in it. Is the PR still a work in progress, or is it ready to merge?
docs/rules/no-identical-tests.md
Outdated
Examples of **incorrect** code for this rule: | ||
|
||
```js | ||
/* eslint eslint-plugin/consistent-output: error */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: This should say /* eslint eslint-plugin/no-identical-tests: error */
docs/rules/no-identical-tests.md
Outdated
Examples of **correct** code for this rule: | ||
|
||
```js | ||
/* eslint eslint-plugin/consistent-output: error */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: This should say /* eslint eslint-plugin/no-identical-tests: error */
I'm feeling good. @not-an-aardvark |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, thanks!
fixes #5