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

New: disallow identical tests #11

Merged
merged 8 commits into from
Jul 1, 2017

Conversation

aladdin-add
Copy link
Contributor

fixes #5

Copy link
Contributor

@not-an-aardvark not-an-aardvark left a 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

docs: {
description: 'disallow identical tests',
category: 'Tests',
recommended: true,
Copy link
Contributor

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);
Copy link
Contributor

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.

Copy link
Contributor Author

@aladdin-add aladdin-add Jul 1, 2017

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.:(

// ----------------------------------------------------------------------
// Public
// ----------------------------------------------------------------------
const message = 'This test case should not be identical to someone else.';
Copy link
Contributor

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."

Copy link
Contributor

@not-an-aardvark not-an-aardvark left a 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?

Examples of **incorrect** code for this rule:

```js
/* eslint eslint-plugin/consistent-output: error */
Copy link
Contributor

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 */

Examples of **correct** code for this rule:

```js
/* eslint eslint-plugin/consistent-output: error */
Copy link
Contributor

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 */

@aladdin-add aladdin-add changed the title [WIP] New: disallow identical tests New: disallow identical tests Jul 1, 2017
@aladdin-add
Copy link
Contributor Author

I'm feeling good. @not-an-aardvark

Copy link
Contributor

@not-an-aardvark not-an-aardvark left a 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!

@not-an-aardvark not-an-aardvark merged commit 8c19ab5 into eslint-community:master Jul 1, 2017
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.

Rule: disallow identical tests
2 participants