Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

suggestion: prefer test or it #12

Closed
dylang opened this issue Nov 11, 2017 · 10 comments · Fixed by severest/retrobot#25
Closed

suggestion: prefer test or it #12

dylang opened this issue Nov 11, 2017 · 10 comments · Fixed by severest/retrobot#25

Comments

@dylang
Copy link

dylang commented Nov 11, 2017

Suggest using test or it (always-test-or-it)

For consistency in code, always use test or always use it.

Rule details

When test is preferred, this rule triggers a warning for any instances of it.
When it is preferred, this rule triggers a warning for any instances of test.

This rule is not set by default.

Default configuration

The following pattern is considered warning:

it('example 1', () => {
   expect(files).toHaveLength(1);
});

test('example 2', () => {
   expect(files).toHaveLength(1);
});

The following pattern is not warning when test is set:

test('example 1', () => {
   expect(files).toHaveLength(1);
});

test('example 2', () => {
   expect(files).toHaveLength(1);
});

The following pattern is not warning when it is set:

it('example 1', () => {
   expect(files).toHaveLength(1);
});

it('example 2', () => {
   expect(files).toHaveLength(1);
});
@SimenB
Copy link
Member

SimenB commented Nov 11, 2017

Good idea! Should also be autofixable. Would you send a PR for it? 🙂

Couple of q's:

  • What should the default be?
  • Should we allow config to differentiate between an it within a describe and by itself at the top level?

@macklinu
Copy link
Collaborator

macklinu commented Feb 3, 2018

Should we allow config to differentiate between an it within a describe and by itself at the top level?

I think that could be nice.

When using describe(), I prefer using only it().

describe('myFunction()', () => {
  it('does this thing')
  it('does that thing')
  // etc.
})

When not using describe(), I prefer using only test().

test('does this thing')
test('does that thing')

I've worked in codebases that have done one or the other, so being able to configure this could help ensure consistency in those codebases.

@macklinu
Copy link
Collaborator

macklinu commented Feb 9, 2018

What should the default be?

I'm not sure what the default should be here. 🤔 Does a rule need a default, or could we force an option to be provided? (Perhaps that's not a best practice of ESLint rules?)

The enum option I imagine with an example .eslintrc usage:

  • test: all tests must use the test() function
  • it: all tests must use the it() function
  • describe/it: all tests must use the it() function
{
  "rules": {
    "jest/always-test-or-it": ["error", "test"]
  }
}

I would like to contribute a PR for this unless anyone else would like to.

@SimenB
Copy link
Member

SimenB commented Feb 10, 2018

I would like to force top level to be test and for all nested within describe to be it.

Not sure how to best formulate that.

Feel free to send a PR, might be easier to decide once we have some code to look at 🙂

@ranyitz
Copy link
Contributor

ranyitz commented Feb 10, 2018

@dylang I really like the idea 😀
@macklinu In case you're busy or anything I can also take this.

macklinu added a commit to macklinu/eslint-plugin-jest that referenced this issue Feb 10, 2018
@macklinu
Copy link
Collaborator

@ranyitz if you're interested in picking this up, that's cool with me! I hacked on it a little bit yesterday, mostly out of curiosity for how to implement a fixer for a rule.

I pushed up macklinu@76acf1f, which handles renaming it() to test() and vice versa based on which option you provide. It doesn't ensure test() functions are at the top level, nor does it ensure that all it() functions are nested within describe().

I'm happy to hand off this branch if you would like to take this issue. 🙂

@ranyitz
Copy link
Contributor

ranyitz commented Feb 10, 2018

Hey @macklinu That's really nice of you! 😊
I would be happy to take this issue.

I'll try to summarize the tasks, so it'll be easier to discuss what's needs to be done:

  • create the it option: enforce it calls and fix test to be it.
  • create the test option: enfore test calls and fix it to be test.
  • create the describe/it option: enforce test on top level tests and enforce it when nested inside of describe, fix it to test when using in top level, fix test to it when using inside of describe.
  • add documentation.

Also, I have a suggestion regarding the name, what do you say about consistent-test-it?
IMHO It's a bit more eslint'ish...

@SimenB
Copy link
Member

SimenB commented Feb 10, 2018

consistent-test-it is a good name

@macklinu
Copy link
Collaborator

Yeah either that or consistent-test-name sounds good to me. 👍

I would like to force top level to be test and for all nested within describe to be it.

@SimenB are you thinking there should only be two options?

  • test - enforce test() calls at the top level and fix it() to be test()
  • it - enforce it() calls and fix test() to be it(); also enforce it() calls to be included within a describe() block

Or should we split the it option into it and describe/it? I guess I never really use it() without describe(), so I could see us collapsing it into two options, which is a little simpler from a user configuration perspective. 🤔

@SimenB
Copy link
Member

SimenB commented Feb 10, 2018

I think 3 options makes sense. Can be an object instead of a single string as well. { fn: 'test', withinDescribe: 'it'} or something

# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
4 participants