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

Catch & fix out-of-order assertion arguments #307

Merged
merged 27 commits into from
Aug 11, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
95b9941
Test detecting out-of-ordering assertion-arguments
ninevra Jul 14, 2020
6542f06
Allow specifying autofix output in testCase()
ninevra Jul 15, 2020
cebdcbd
Test autofix output
ninevra Jul 15, 2020
5783fe0
Allow multiple errors, arbitrary error objects in testCase()
ninevra Jul 14, 2020
5369b12
Offset error loc in testCase() to account for wrapping
ninevra Jul 15, 2020
6bb9270
Test reported error location
ninevra Jul 15, 2020
24f639d
Test in combination with other error types
ninevra Jul 14, 2020
e1fab59
Test with t.is() and t.not()
ninevra Jul 14, 2020
e2b7990
Test autofixing with parentheses
ninevra Jul 15, 2020
6af165a
Document detecting out-of-order assertion-arguments
ninevra Jul 14, 2020
a565059
Detect and fix out-of-order assertion-arguments
ninevra Jul 15, 2020
4031607
Test with single-argument assertions
ninevra Jul 15, 2020
09af125
Support 1-arg assertions with comparison expressions
ninevra Jul 15, 2020
197b0fd
Document support for 1-arg assertions
ninevra Jul 15, 2020
38a1b5a
Support t.throwsAsync()
ninevra Jul 14, 2020
44e68f7
Support t.notThrowsAsync()
ninevra Jul 14, 2020
88a5a91
Support t.assert()
ninevra Jul 15, 2020
9724e19
Test not autofixing nodes with comments
ninevra Jul 15, 2020
61f328d
Don't autofix nodes with comments
ninevra Jul 15, 2020
ee539aa
Document not autofixing nodes with comments
ninevra Jul 15, 2020
b9f29f6
Refactor to reduce cyclomatic complexity
ninevra Jul 16, 2020
bd959e1
Reword documentation
ninevra Jul 16, 2020
c08dea6
Test t.like() support
ninevra Jul 15, 2020
729d5bd
Update assertion-arguments.js
sindresorhus Aug 10, 2020
93425a3
Replace uses of token#start, token#end
ninevra Aug 10, 2020
997096e
Use for...of
ninevra Aug 10, 2020
fbc95b5
Refactor for...of
ninevra Aug 10, 2020
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
2 changes: 2 additions & 0 deletions docs/rules/assertion-arguments.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@ Enforces passing the right number of arguments to assertion methods like `t.is()

Assertion messages are optional arguments that can be given to any assertion call to improve the error message, should the assertion fail.

This rule also attempts to enforce passing actual values before expected values. If exactly one of the first two arguments to a two-argument assertion is a static expression such as `{a: 1}`, then the static expression must come second. (`t.regex()` and `t.notRegex()` are excluded from this check, because either their `contents` argument or their `regex` argument could plausibly be the actual or expected value.) If the argument to a one-argument assertion is a binary relation such as `'static' === dynamic`, a similar check is performed on its left- and right-hand sides. Errors of these kinds are usually fixable.

## Fail

```js
Expand Down
1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
"dependencies": {
"deep-strict-equal": "^0.2.0",
"enhance-visitors": "^1.0.0",
"eslint-utils": "^2.1.0",
"espree": "^7.1.0",
"espurify": "^2.0.1",
"import-modules": "^2.0.0",
Expand Down
214 changes: 208 additions & 6 deletions rules/assertion-arguments.js
Original file line number Diff line number Diff line change
@@ -1,9 +1,14 @@
'use strict';
const {visitIf} = require('enhance-visitors');
const {getStaticValue, isOpeningParenToken, isCommaToken} = require('eslint-utils');
const util = require('../util');
const createAvaRule = require('../create-ava-rule');

const expectedNbArguments = {
assert: {
min: 1,
max: 2
},
deepEqual: {
min: 2,
max: 3
Expand Down Expand Up @@ -44,6 +49,10 @@ const expectedNbArguments = {
min: 1,
max: 2
},
notThrowsAsync: {
min: 1,
max: 2
},
pass: {
min: 0,
max: 1
Expand Down Expand Up @@ -72,6 +81,10 @@ const expectedNbArguments = {
min: 1,
max: 3
},
throwsAsync: {
min: 1,
max: 3
},
true: {
min: 1,
max: 2
Expand All @@ -86,6 +99,111 @@ const expectedNbArguments = {
}
};

const actualExpectedAssertions = new Set([
'deepEqual',
'is',
'like',
'not',
'notDeepEqual',
'throws',
'throwsAsync'
]);

const relationalActualExpectedAssertions = new Set([
'assert',
'truthy',
'falsy',
'true',
'false'
]);

const comparisonOperators = new Map([
['>', '<'],
['>=', '<='],
['==', '=='],
['===', '==='],
['!=', '!='],
['!==', '!=='],
['<=', '>='],
['<', '>']
]);

const flipOperator = operator => comparisonOperators.get(operator);

function isStatic(node) {
const staticValue = getStaticValue(node);
return staticValue !== null && typeof staticValue.value !== 'function';
}

function * sourceRangesOfArguments(sourceCode, callExpression) {
const openingParen = sourceCode.getTokenAfter(
callExpression.callee,
{filter: token => isOpeningParenToken(token)}
);

const closingParen = sourceCode.getLastToken(callExpression);

for (const [index, argument] of callExpression.arguments.entries()) {
const previousToken = index === 0 ?
openingParen :
sourceCode.getTokenBefore(
argument,
{filter: token => isCommaToken(token)}
);

const nextToken = index === callExpression.arguments.length - 1 ?
closingParen :
sourceCode.getTokenAfter(
argument,
{filter: token => isCommaToken(token)}
);

const firstToken = sourceCode.getTokenAfter(
previousToken,
{includeComments: true}
);

const lastToken = sourceCode.getTokenBefore(
nextToken,
{includeComments: true}
);

yield [firstToken.range[0], lastToken.range[1]];
}
}

function sourceOfBinaryExpressionComponents(sourceCode, node) {
const {operator, left, right} = node;

const operatorToken = sourceCode.getFirstTokenBetween(
left,
right,
{filter: token => token.value === operator}
);

const previousToken = sourceCode.getTokenBefore(node);
const nextToken = sourceCode.getTokenAfter(node);

const leftRange = [
sourceCode.getTokenAfter(previousToken, {includeComments: true}).range[0],
sourceCode.getTokenBefore(operatorToken, {includeComments: true}).range[1]
];

const rightRange = [
sourceCode.getTokenAfter(operatorToken, {includeComments: true}).range[0],
sourceCode.getTokenBefore(nextToken, {includeComments: true}).range[1]
];

return [leftRange, operatorToken, rightRange];
}

function noComments(sourceCode, ...nodes) {
return nodes.every(node => {
const {leading, trailing} = sourceCode.getComments(node);
return leading.length === 0 && trailing.length === 0;
});
}

const create = context => {
const ava = createAvaRule();
const options = context.options[0] || {};
Expand Down Expand Up @@ -141,19 +259,102 @@ const create = context => {
report(node, `Not enough arguments. Expected at least ${nArgs.min}.`);
} else if (node.arguments.length > nArgs.max) {
report(node, `Too many arguments. Expected at most ${nArgs.max}.`);
} else if (enforcesMessage && nArgs.min !== nArgs.max) {
const hasMessage = gottenArgs === nArgs.max;
} else {
if (enforcesMessage && nArgs.min !== nArgs.max) {
const hasMessage = gottenArgs === nArgs.max;

if (!hasMessage && shouldHaveMessage) {
report(node, 'Expected an assertion message, but found none.');
} else if (hasMessage && !shouldHaveMessage) {
report(node, 'Expected no assertion message, but found one.');
if (!hasMessage && shouldHaveMessage) {
report(node, 'Expected an assertion message, but found none.');
} else if (hasMessage && !shouldHaveMessage) {
report(node, 'Expected no assertion message, but found one.');
}
}

checkArgumentOrder({node, assertion: members[0], context});
}
})
});
};

function checkArgumentOrder({node, assertion, context}) {
const [first, second] = node.arguments;
if (actualExpectedAssertions.has(assertion) && second) {
const [leftNode, rightNode] = [first, second];
if (isStatic(leftNode) && !isStatic(rightNode)) {
context.report(
makeOutOfOrder2ArgumentReport({node, leftNode, rightNode, context})
);
}
} else if (
relationalActualExpectedAssertions.has(assertion) &&
first &&
first.type === 'BinaryExpression' &&
comparisonOperators.has(first.operator)
) {
const [leftNode, rightNode] = [first.left, first.right];
if (isStatic(leftNode) && !isStatic(rightNode)) {
context.report(
makeOutOfOrder1ArgumentReport({node: first, leftNode, rightNode, context})
);
}
}
}

function makeOutOfOrder2ArgumentReport({node, leftNode, rightNode, context}) {
const sourceCode = context.getSourceCode();
const [leftRange, rightRange] = sourceRangesOfArguments(sourceCode, node);
const report = {
message: 'Expected values should come after actual values.',
loc: {
start: sourceCode.getLocFromIndex(leftRange[0]),
end: sourceCode.getLocFromIndex(rightRange[1])
}
};

if (noComments(sourceCode, leftNode, rightNode)) {
report.fix = fixer => {
const leftText = sourceCode.getText().slice(...leftRange);
const rightText = sourceCode.getText().slice(...rightRange);
return [
fixer.replaceTextRange(leftRange, rightText),
fixer.replaceTextRange(rightRange, leftText)
];
};
}

return report;
}

function makeOutOfOrder1ArgumentReport({node, leftNode, rightNode, context}) {
const sourceCode = context.getSourceCode();
const [
leftRange,
operatorToken,
rightRange
] = sourceOfBinaryExpressionComponents(sourceCode, node);
const report = {
message: 'Expected values should come after actual values.',
loc: {
start: sourceCode.getLocFromIndex(leftRange[0]),
end: sourceCode.getLocFromIndex(rightRange[1])
}
};

if (noComments(sourceCode, leftNode, rightNode, node)) {
report.fix = fixer => {
const leftText = sourceCode.getText().slice(...leftRange);
const rightText = sourceCode.getText().slice(...rightRange);
return [
fixer.replaceTextRange(leftRange, rightText),
fixer.replaceText(operatorToken, flipOperator(node.operator)),
fixer.replaceTextRange(rightRange, leftText)
];
};
}

return report;
}

const schema = [{
type: 'object',
properties: {
Expand All @@ -170,6 +371,7 @@ const schema = [{
module.exports = {
create,
meta: {
fixable: 'code',
docs: {
url: util.getDocsUrl(__filename)
},
Expand Down
Loading