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
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -62,10 +62,13 @@
"mocha"
],
"dependencies": {
"acorn": "^3.0.4",
"deep-strict-equal": "^0.1.0",
"espree": "^3.1.3",
"estraverse": "^4.2.0",
"espurify": "^1.5.0",
"multimatch": "^2.1.0",
"num-lines": "^1.0.0",
"object-assign": "^4.0.1",
"pkg-up": "^1.0.0"
},
Expand Down
112 changes: 112 additions & 0 deletions rules/no-commented-test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,112 @@
'use strict';
var acorn = require('acorn');
var acornLoose = require('acorn/dist/acorn_loose');
var estraverse = require('estraverse');
var numLines = require('num-lines');
var createAvaRule = require('../create-ava-rule');

module.exports = function (context) {
var processedUpTo = -1;

function concatComments(commentBlock) {
var endOfLastComment = commentBlock[commentBlock.length - 1].end;

// Comments are often attached as "trailing" on one node, and "leading" on the next.
// Avoid double processing the same block.
if (processedUpTo >= endOfLastComment) {
return null;
}

processedUpTo = endOfLastComment;

return commentBlock.map(function (commentNode) {
return commentNode.value;
}).join('\n');
}

function process(commentBlock) {
if (!commentBlock || !commentBlock.length) {
return;
}

var src = concatComments(commentBlock);

// Does a best effort parse of the code. AST will contain `x` Identifiers for chunks it can't parse.
var ast = acornLoose.parse_dammit(src, {locations: true});

// Traverse the contents of the comment block, looking for a test call.
estraverse.traverse(ast, {
enter: function (node) {
if (node.type === 'CallExpression' && isTestFunctionCall(node.callee)) {
try {
// Strict parse - don't report if the commented out test contains syntax errors
acorn.parse(src.substring(node.start, node.end));

var loc = translateLocation(node.loc.start, commentBlock);

context.report({
message: 'use test.skip instead of commenting out tests',
loc: loc
});
} catch (e) {}
}
}
});
}

var ava = createAvaRule();

function visitNode(node) {
if (!ava.isTestFile) {
return;
}

process(node.leadingComments);
process(node.trailingComments);
}

// build the visitor object, we want to asses the comments around any XxxStatement node.
var visitor = {};

Object.keys(estraverse.Syntax).forEach(function (name) {
if (/Statement$/.test(name)) {
visitor[name] = visitNode;
}
});

return ava.merge(visitor);
};

function translateLocation(locationInComments, commentBlock) {
var linesLeft = locationInComments.line;

for (var i = 0; i < commentBlock.length; i++) {
var commentNode = commentBlock[i];
var linesThisComment = numLines(commentNode.value);

if (linesThisComment >= linesLeft) {
return {
line: commentNode.loc.start.line + linesLeft - 1,
column: linesLeft === 1 ?
commentNode.loc.start.column + locationInComments.column + 2 :
locationInComments.column
};
}

linesLeft -= linesThisComment;
}

// 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.

}

function isTestFunctionCall(node) {
Copy link
Member

Choose a reason for hiding this comment

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

Move this to util.js?

if (node.type === 'Identifier') {
return node.name === 'test';
} else if (node.type === 'MemberExpression') {
return isTestFunctionCall(node.object);
}

return false;
}
48 changes: 48 additions & 0 deletions test/no-commented-tests.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
import test from 'ava';
import {RuleTester} from 'eslint';
import rule from '../rules/no-commented-test';

const ruleTester = new RuleTester({
env: {
es6: true
}
});

function errorAt(line, column) {
return {
ruleId: 'no-commented-tests',
line,
column
};
}
const header = `const test = require('ava');\n`;

test(() => {
ruleTester.run('no-commented-tests', rule, {
valid: [
header + 'test("my test name", t => { t.pass(); });',
header + 'something(); // Is this a test(..)',
header + 'something(); // Some sentence ending with the word test (followed by parens)',
// shouldn't be triggered since it's not a test file
'test.cb(t => {});\n// test.cb(t=> {})'
],
invalid: [
{
code: header + 'test.cb(t => { t.pass(); });\n// test.cb(t=> {})',
errors: [errorAt(3, 4)]
},
{
code: header + 'test.cb(t => { t.pass(); });\n // test.cb(t=> {})',
errors: [errorAt(3, 5)]
},
{
code: header + 'test.cb(t => { t.pass(); });\n// test.cb(t=> {})',
Copy link
Member

Choose a reason for hiding this comment

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

Space after t on all of these.

errors: [errorAt(3, 5)]
},
{
code: header + 'test.cb(t => { t.pass(); });\n// This is not javascript!\n// test.cb(t=> {})',
errors: [errorAt(4, 4)]
}
Copy link
Member

@sindresorhus sindresorhus Apr 17, 2016

Choose a reason for hiding this comment

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

I would like to see some more tests.

  • Multi-line comment
  • Unfinished: test(t =>

]
});
});