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

Prevent commented out tests #84

Open
sindresorhus opened this issue Apr 15, 2016 · 10 comments
Open

Prevent commented out tests #84

sindresorhus opened this issue Apr 15, 2016 · 10 comments

Comments

@sindresorhus
Copy link
Member

We should recommend using test.skip() instead.

This:

// test('foo', => {
//  t.pass();
// });

Should instead be:

test.skip('foo', => {
    t.pass();
});
@jamestalmage
Copy link
Contributor

👍

Interesting proposal.

How should we accomplish it? Try parsing the contents of the comments, or just use regexp?

Trying to parse will have more false negatives, regexp more false positives.

@jfmengels
Copy link
Contributor

jfmengels commented Apr 15, 2016

I found https://github.com/bahmutov/eslint-rules#no-commented-out-code that looks like this. I have no idea if it works well, but it might be a good lead.

@jamestalmage
Copy link
Contributor

It looks like it parses line by line, and warns if any single line is valid JS.

This is a little more difficult to do with parsing because tests will (very likely) cover multiple lines:

test(t => {
  t.pass();
});

The first line above isn't valid JS on it's own, you need the whole thing. But if they do this when they comment it out:

// I am skipping this because: reasons.
// 
// test(t => {
//   t.pass();
// });

Then parsing the entire contents of the comments creates a syntax error.

I guess we could create a regexp to try and find the potential first line of a test, and then add lines incrementally until we found something that parsed.

Or maybe we could try the parse_dammit method from acorn.

@sindresorhus
Copy link
Member Author

I was thinking regex.

@jamestalmage
Copy link
Contributor

I was thinking regex.

Uh oh: #85

My concern with regexp is the likelihood of false positives.

@jamestalmage
Copy link
Contributor

Acorns loose parser is surprisingly good (at making bad code into a usable AST), it might allow just as many false positives through as a regex would.

@sindresorhus
Copy link
Member Author

Yeah, parsing looks like a more solid solution.

@jamestalmage
Copy link
Contributor

I think this would be difficult to handle without parsing.

@jfmengels
Copy link
Contributor

I found @platinumazure's eslint-plugin-qunit, which has this rule alreadby built-in (https://github.com/platinumazure/eslint-plugin-qunit/blob/master/lib/rules/no-commented-tests.js), and uses a regex to find it. In QUnit's case, there are bounds to be less false positives.

@revelt
Copy link

revelt commented Dec 9, 2017

I came here looking to check does it exist and if necessary, to propose it :)

Imho, this rule would be one of the most useful-ones from what I've seen so far in the proposals. It's very difficult to pinpoint commented-out rule when the tests file is long.

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

No branches or pull requests

4 participants