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

feat(rules): add no-commented-out rule #262

Merged
merged 13 commits into from May 22, 2019
Merged

feat(rules): add no-commented-out rule #262

merged 13 commits into from May 22, 2019

Conversation

@kangax
Copy link
Contributor

@kangax kangax commented May 16, 2019

Closes #261

const message = 'Some tests seem to be commented';

function hasAssertions(node) {
return /x?(test|it|describe)((\.skip|\[['"]skip['"]\]))?\(/.test(node.value);

This comment has been minimized.

@kangax

kangax May 16, 2019
Author Contributor

I didn't bother with matching quotes here. I think this will be rare enough and the rule is fuzzy as it is.

Copy link
Member

@jeysal jeysal left a comment

Awesome @kangax, I love that you added extensive documentation and many tests right away 👍 I left a few review comments :)
Also, I'm not quite sure about the rule name - a "commented test" could be misunderstood as

test('stuff', () => {
  // check that x does y
  expect(whatever).toBe(something)
})

no-commented-out-tests is clearer, but sounds a bit weird. Or maybe you have a different idea?

src/rules/no-commented-tests.js Outdated Show resolved Hide resolved
src/rules/no-commented-tests.js Outdated Show resolved Hide resolved
src/rules/__tests__/no-commented-tests.test.js Outdated Show resolved Hide resolved
@kangax
Copy link
Contributor Author

@kangax kangax commented May 16, 2019

I agree that no-commented-out-tests would probably more of a common naming

Copy link
Member

@jeysal jeysal left a comment

Two more comments on cases that came to my mind looking at the regex, other than that LGTM :)
@SimenB also want to review maybe?

const message = 'Some tests seem to be commented';

function hasTests(node) {
return /x?(test|it|describe)(\.\w+|\[['"]\w+['"]\])?\(.*?\)/.test(node.value);

This comment has been minimized.

@jeysal

jeysal May 18, 2019
Member

  • Other than xit, there's also fit
  • The closing parenthesis might cause trouble with multi-line tests in comments, we might need the //s regex modifier? Should add some multiline test cases

This comment has been minimized.

@kangax

kangax May 20, 2019
Author Contributor

Addressed both comments and added tests. The thing about newlines is that ESLint seems to create a separate comment for each line with line-level comments. We can't detect stuff like:

// test
// (
// 'x
// )

..which, I think, is an OK limitation?

This comment has been minimized.

@jeysal

jeysal May 20, 2019
Member

For block comments as well though?

This comment has been minimized.

@jeysal

jeysal May 20, 2019
Member

Actually, best thing might be to just not look for the closing parenthesis? // test( already looks suspicious enough

This comment has been minimized.

@kangax

kangax May 20, 2019
Author Contributor

Yep, I did just that in the last commit. Let me add a multiline comment test as well.

Copy link
Member

@SimenB SimenB left a comment

LGTM, thanks! Would be nice to address @jeysal's comments about the regexp and my little nit

@@ -38,6 +38,7 @@ module.exports = {
rules: {
'jest/no-alias-methods': 'warn',
'jest/no-disabled-tests': 'warn',
'jest/no-commented-out-tests': 'warn',

This comment has been minimized.

@SimenB

SimenB May 20, 2019
Member

changing the recommended config is a breaking change.

Not sure this needs to be in the recommended config, at all, but we can revisit that before the next release 🙂

This comment has been minimized.

@kangax

kangax May 20, 2019
Author Contributor

Ah, I didn't realise this will modify recommended config. On the other hand, I think it's ok to warn since warnings usually don't fail with tools like lint-staged, etc.

This comment has been minimized.

@SimenB

SimenB May 21, 2019
Member

People can run eslint with a flag failing on warnings

This comment has been minimized.

@kangax

kangax May 21, 2019
Author Contributor

removed it

@kangax
Copy link
Contributor Author

@kangax kangax commented May 21, 2019

@SimenB anything else you want me to do here?

@jeysal
jeysal approved these changes May 21, 2019
Copy link
Member

@jeysal jeysal left a comment

Thanks @kangax ❤️

@SimenB
SimenB approved these changes May 22, 2019
Copy link
Member

@SimenB SimenB left a comment

Thanks!

@SimenB SimenB merged commit 14d83ef into jest-community:master May 22, 2019
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@SimenB
Copy link
Member

@SimenB SimenB commented May 22, 2019

🎉 This PR is included in version 22.6.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@kangax kangax deleted the kangax:no_commented_tests branch May 22, 2019
@mrtnzlml
Copy link
Contributor

@mrtnzlml mrtnzlml commented May 22, 2019

Hi @kangax! Here I am again... 😊

I ran this rule on our codebase just like I did here and I found some cases where this rule reports false positives. You may want to check it so people don't have to disable this rule by default just like we did. All these cases are reporting issues incorrectly (+ none of them is a test file):

// TODO: find out better way how to do it (type should be just an internal detail - see evaluateGlobalIdField)
// TODO: turn this into regular Jest test (?)
// TODO: unify with Git implementation from Shipit (?)

...

And also this one - reports issue on line 8 incorrectly. Cheers 🍻

@kangax
Copy link
Contributor Author

@kangax kangax commented May 22, 2019

@mrtnzlml yep, I noticed some false positives in our codebase too just now. This should fix them — #271

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants