Skip to content

Add equality checks to prefer-t-regex rule #297

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.

Sign up for GitHub

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

Closed
wants to merge 3 commits into from
Closed

Add equality checks to prefer-t-regex rule #297

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
34d7f08
implement is % deepEqual checks to prefer-t-regex
JaniM Feb 28, 2020
c18bc37
removed an unnecessary check
JaniM Feb 28, 2020
b6a93c5
use better variable names
JaniM Mar 13, 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
10 changes: 9 additions & 1 deletion docs/rules/prefer-t-regex.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ Translations: [Français](https://github.com/avajs/ava-docs/blob/master/fr_FR/re

The AVA [`t.regex()` assertion](https://github.com/avajs/ava/blob/master/docs/03-assertions.md#regexcontents-regex-message) can test a string against a regular expression.

This rule will enforce the use of `t.regex()` instead of manually using `RegExp#test()`, which will make your code look clearer and produce better failure output.
This rule will enforce the use of `t.regex()` instead of manually using `RegExp#test()`, which will make your code look clearer and produce better failure output. The rule will also prevent equality assertions with a regex and a non-regex, which is almost always an error.

This rule is fixable. It will replace the use of `RegExp#test()`, `String#match()`, or `String#search()` with `t.regex()`.

Expand All @@ -27,6 +27,14 @@ test('main', t => {
});
```

```js
const test = require('ava');

test('main', t => {
t.is('foo', /\w+/);
});
```


## Pass

Expand Down
164 changes: 111 additions & 53 deletions rules/prefer-t-regex.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,75 +13,133 @@ const create = context => {
'falsy'
];

const equalityTests = ['is', 'deepEqual'];

const findReference = name => {
const reference = context.getScope().references.find(reference => reference.identifier.name === name);
const definitions = reference.resolved.defs;

// Many integration tests have identifiers that match zero definitions
if (definitions.length === 0) {
return undefined;
}

return definitions[definitions.length - 1].node;
};

const isRegExp = lookup => {
let isRegex = lookup.regex;

// It's not a regexp but an identifier
if (!isRegex && lookup.type === 'Identifier') {
const reference = findReference(lookup.name);

// Not all possible references have an init field
if (reference && reference.init) {
isRegex = reference.init.regex;
}
}

return isRegex;
};

const booleanHandler = node => {
const firstArg = node.arguments[0];

// First argument is a call expression
const isFunctionCall = firstArg.type === 'CallExpression';
if (!isFunctionCall || !firstArg.callee.property) {
return;
}

const {name} = firstArg.callee.property;
let lookup = {};
let variable = {};

if (name === 'test') {
// `lookup.test(variable)`
lookup = firstArg.callee.object;
variable = firstArg.arguments[0];
} else if (['search', 'match'].includes(name)) {
// `variable.match(lookup)`
lookup = firstArg.arguments[0];
variable = firstArg.callee.object;
}

if (!isRegExp(lookup)) {
return;
}

const assertion = ['true', 'truthy'].includes(node.callee.property.name) ? 'regex' : 'notRegex';

const fix = fixer => {
const source = context.getSourceCode();
return [
fixer.replaceText(node.callee.property, assertion),
fixer.replaceText(firstArg, `${source.getText(variable)}, ${source.getText(lookup)}`)
];
};

context.report({
node,
message: `Prefer using the \`t.${assertion}()\` assertion.`,
fix
});
};

const equalityHandler = node => {
const [firstArg, secondArg] = node.arguments;

const firstArgumentIsRegex = isRegExp(firstArg);
const secondArgumentIsRegex = isRegExp(secondArg);

// If both are regex, or neither are, the expression is ok
if (firstArgumentIsRegex === secondArgumentIsRegex) {
return;
}

const matchee = secondArgumentIsRegex ? firstArg : secondArg;
const regex = secondArgumentIsRegex ? secondArg : firstArg;

const assertion = 'regex';

const fix = fixer => {
const source = context.getSourceCode();
return [
fixer.replaceText(node.callee.property, assertion),
fixer.replaceText(firstArg, `${source.getText(matchee)}`),
fixer.replaceText(secondArg, `${source.getText(regex)}`)
];
};

context.report({
node,
message: `Prefer using the \`t.${assertion}()\` assertion.`,
fix
});
};

return ava.merge({
CallExpression: visitIf([
ava.isInTestFile,
ava.isInTestNode
])(node => {
// Call a boolean assertion, for example, `t.true`, `t.false`, …
const isBooleanAssertion = node.callee.type === 'MemberExpression' &&
booleanTests.includes(node.callee.property.name) &&
const isAssertion = node.callee.type === 'MemberExpression' &&
util.getNameOfRootNodeObject(node.callee) === 't';

if (!isBooleanAssertion) {
return;
}

const firstArg = node.arguments[0];

// First argument is a call expression
const isFunctionCall = firstArg.type === 'CallExpression';
if (!isFunctionCall || !firstArg.callee.property) {
return;
}

const {name} = firstArg.callee.property;
let lookup = {};
let variable = {};

if (name === 'test') {
// `lookup.test(variable)`
lookup = firstArg.callee.object;
variable = firstArg.arguments[0];
} else if (['search', 'match'].includes(name)) {
// `variable.match(lookup)`
lookup = firstArg.arguments[0];
variable = firstArg.callee.object;
}

let isRegExp = lookup.regex;
// Call a boolean assertion, for example, `t.true`, `t.false`, …
const isBooleanAssertion = isAssertion &&
booleanTests.includes(node.callee.property.name);

// It's not a regexp but an identifier
if (!isRegExp && lookup.type === 'Identifier') {
const reference = findReference(lookup.name);
isRegExp = reference.init.regex;
}
// Call an equality assertion, ie. 't.is', 't.deepEqual'
const isEqualityAssertion = isAssertion &&
equalityTests.includes(node.callee.property.name);

if (!isRegExp) {
return;
if (isBooleanAssertion) {
booleanHandler(node);
} else if (isEqualityAssertion) {
equalityHandler(node);
}

const assertion = ['true', 'truthy'].includes(node.callee.property.name) ? 'regex' : 'notRegex';

const fix = fixer => {
const source = context.getSourceCode();
return [
fixer.replaceText(node.callee.property, assertion),
fixer.replaceText(firstArg, `${source.getText(variable)}, ${source.getText(lookup)}`)
];
};

context.report({
node,
message: `Prefer using the \`t.${assertion}()\` assertion.`,
fix
});
})
});
};
Expand Down
22 changes: 21 additions & 1 deletion test/prefer-t-regex.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ ruleTester.run('prefer-t-regex', rule, {
valid: [
header + 'test(t => t.regex("foo", /\\d+/));',
header + 'test(t => t.regex(foo(), /\\d+/));',
header + 'test(t => t.is(/\\d+/.test("foo")), true);',
header + 'test(t => t.is(/\\d+/.test("foo"), true));',
header + 'test(t => t.true(1 === 1));',
header + 'test(t => t.true(foo.bar()));',
header + 'const a = /\\d+/;\ntest(t => t.truthy(a));',
Expand Down Expand Up @@ -62,6 +62,26 @@ ruleTester.run('prefer-t-regex', rule, {
code: header + 'const reg = /\\d+/;\ntest(t => t.true(reg.test(foo.bar())));',
output: header + 'const reg = /\\d+/;\ntest(t => t.regex(foo.bar(), reg));',
errors: errors('regex')
},
{
code: header + 'test(t => t.is(foo(), /\\d+/));',
Copy link
Member

Choose a reason for hiding this comment

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

Can you also add version of these tests using new RegExp()?

Copy link
Author

Choose a reason for hiding this comment

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

Do we want to expand the functionality for this? The original regex detection logic only matches regex literals, not expressions resolving to a regex. Resolving whether or not any given expression resolves to a regex pretty much requires eval, so we'd have to special-case new RegExp().

Copy link
Member

Choose a reason for hiding this comment

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

We don't need to resolve expression, but we should support an inline new RegExp() call.

output: header + 'test(t => t.regex(foo(), /\\d+/));',
errors: errors('regex')
},
{
code: header + 'test(t => t.is(/\\d+/, foo()));',
output: header + 'test(t => t.regex(foo(), /\\d+/));',
errors: errors('regex')
},
{
code: header + 'test(t => t.deepEqual(foo(), /\\d+/));',
output: header + 'test(t => t.regex(foo(), /\\d+/));',
errors: errors('regex')
},
{
code: header + 'test(t => t.deepEqual(/\\d+/, foo()));',
output: header + 'test(t => t.regex(foo(), /\\d+/));',
errors: errors('regex')
}
]
});