feat(rule): add valid-describe rule

[macklinu]

Resolves #18

This rule validates that the second parameter of a describe() function is a callback function. This callback function:

• should not be async
• should not contain any parameters
• should not contain any return statements

File naming is kebab-case, per the discussion in #60.

[macklinu]

I'm not sure why lint-staged is removing this as part of the eslint --fix step in my local environment.

[SimenB]

you probably have an eslintrc file in $HOME or something. Try adding root: true in the eslintrc.js in this repo

[macklinu]

Thanks for the tip! I had an unexpected .eslintrc file one level up from this repo on my machine. Removed that and was able to see my local linting match up with what Travis CI is reporting. 👍

[SimenB]

Fitting, my first eslint rule ever was handling a callback in describe for jasmine! 🙂 See tlvince/eslint-plugin-jasmine#17, can probably copy that implementation.

I'm not sure how we could figure out nr 2, though. You can take a look at #42, there is some promise detection in it.

[macklinu]

I'll work towards including the two other use cases in this PR - thanks for the suggestions on other implementations to look at. We can figure out a proper name for this rule before merging, but I won't worry much about that right now.

[macklinu]

I'm still working on use case number 2, but is there ever a reason to return within a describe() callback?

// invalid
describe('foo', () => {
  return Promise.resolve().then(() => {
    it('breaks', () => {
      throw new Error('Fail')
    })
  })
})

Looking at this code, return Promise.resolve() is something we want this rule to detect and report. 886ce50 is simply checking if the describe() callback body contains any return statements, not necessarily that it's returning a Promise.

I can update to check that the returned value is a Promise (by looking at #42, as suggested), but I am wondering if there's a code example where returning anything within the body of a describe() callback is a good practice.

[macklinu]

I'm still unsure about the previous comment. Also, is there ever a reason to call await within a describe() block?

// we will warn on the `async` describe callback function
describe('foo', async () => {
  // should we also warn that we are calling `await` here
  // or is the `async` warning above enough?
  await something()
  it('does something', () => {
    // ...
  })
})

[SimenB]

You have to call it synchronously, so no, there's never a reason to make describe async

[macklinu]

This is now ready for review. 👍

[macklinu]

I settled on calling this valid-describe since this addresses a few different cases. This follows the convention of the valid-expect rule. Open to other naming suggestions too!

[macklinu]

This was taken from no-identical-title:

eslint-plugin-jest/rules/no_identical_title.js

Lines 3 to 9 in 5e5ceba

	const describeAliases = Object.assign(Object.create(null), {
	describe: true,
	'describe.only': true,
	'describe.skip': true,
	fdescribe: true,
	xdescribe: true,
	});

I can extract an isDescribe(node) function, move it to util.js and import it into no-identical-title and this rule, if that refactor would be helpful.

[SimenB]

yeah, that sounds good

[macklinu]

Can node ever be falsy here? I'm not sure I need to include this node &&.

[SimenB]

not sure. Seems weird if it can be?

[macklinu]

Does seem weird, will remove that check

[macklinu]

This only warns for the first param - perhaps this should warn for all passed in params?

[SimenB]

yeah, that makes sense.

[macklinu]

Will update with a test case for multiple params in a describe() callback

[SimenB]

needs a rebase

[macklinu]

Rebased and force pushed up.

[SimenB]

as a follow up, mind adding a rule that describe(() => {}) is invalid as well?

[macklinu]

For sure, I'll open an issue to track that suggestion. 👍