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

no-commented-tests #85

Closed
wants to merge 2 commits into from

Conversation

jamestalmage
Copy link
Contributor

Prevents commented out tests.

Fixes #84.

Still needs docs.

What should the rule name be? Some ideas:

  • no-commented-tests
  • no-commented-out-tests
  • no-tests-in-comments
  • no-comment-tests

@jamestalmage
Copy link
Contributor Author

Looks like I am misusing parse_dammit.

@jamestalmage
Copy link
Contributor Author

Some more ideas for this:

Speed

If I had to guess, parsing every comment is going to incur some significant performance penalties. Especially since I will have to parse most comments twice (see parse_dammit link above). It will likely be faster to first scan the comment block with a regular expression:

/test(?:\(|\.)/.test(src)

Accuracy

Should we maybe require valid JS between the opening ( and ending ) of test(...)?
I am thinking someone might do this:

// can't use test(...) here, because
test.cb(t => { });

Or they might be cutting / pasting code out of the comment to a refactored test (leaving it in a non-parseable state).

Disabling

Sometimes your comments contain example code. Especially if you are using JSDoc to generate documentation.

Ideally, you would want to disable this rule near the the offending line in the comment, like this:

test.cb('foo', t => {
  // My comments might describe test code, like:
  //
  // We have to use the `test.cb()` form for this test because... // eslint-disable-line
  //
  doSomething(t.end);
});

Not sure how disable comments are attached / handled, but I would imaging they attach to actual Nodes (not comments). How does it apply to leading / trailing comments? Need to play with it some more.


// TODO: Probably should not throw here. Maybe return `null` and modify the message warning the exact location could not be found?
// This should be unreachable code, so would like to encourage bug reports if that ever proves untrue.
throw new Error('unable to translate location');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think throwing is fine if this is meant to be never hit. The error message should improved though. It should say that this should really never happen and tell the user to open a bug report with the link https://github.com/sindresorhus/eslint-plugin-ava/issues/new.

@sindresorhus
Copy link
Member

Looks promising.

As for naming, we should probably follow our convention (e.g. no-only-test) of singular test and name it no-commented-test?

It will likely be faster to first scan the comment block with a regular expression

👍

Should we maybe require valid JS between the opening

👍

Especially if you are using JSDoc to generate documentation.

Can't we just ignore JSDoc comments? Comments starting with /**.

@jamestalmage
Copy link
Contributor Author

Ah yes - ignoring jsdoc. That should work.

@revelt
Copy link

revelt commented Jan 29, 2018

hi guys! Sorry to pester but I'm really keen to use this feature. Where are we stuck?

@sindresorhus
Copy link
Member

@revelt It's all in the feedback comments.

@sindresorhus
Copy link
Member

Closing for lack of activity.

@revelt
Copy link

revelt commented Jan 11, 2019

It's pity, this would have been a nice rule.

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

Successfully merging this pull request may close these issues.

3 participants