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.

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

Detect more assertions in prefer-t-regex #319

Merged
merged 7 commits into from
Jan 17, 2021
Merged

Detect more assertions in prefer-t-regex #319

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
8857217
Continue the work from `JaniM:better-prefer-t-regex`
Jan 9, 2021
64e6dd4
Fix integration test
Jan 9, 2021
b8e6cb0
Add more test coverage
Jan 13, 2021
97c1efd
Clarify comments
Jan 17, 2021
9691605
Remove unnecessary null check.
Jan 17, 2021
33d2e28
Remove null return as per sindresorhus's preferences
Jan 17, 2021
967600f
Update prefer-t-regex.js
sindresorhus Jan 17, 2021
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
205 changes: 150 additions & 55 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,170 @@ const create = context => {
'falsy'
]);

const equalityTests = new Set([
'is',
'deepEqual'
]);

// Find the latest reference to the given identifier's name.
const findReference = name => {
const reference = context.getScope().references.find(reference => reference.identifier.name === name);
const definitions = reference.resolved.defs;
return definitions[definitions.length - 1].node;

if (reference && reference.resolved) {
const definitions = reference.resolved.defs;

if (definitions.length === 0) {
return null;
}

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

// Recursively find the "origin" node of the given node.
// Note: Due to some limitation, this function will only find the reference up to two layer.
//
// So the following code will only find the reference in this order: c → b → a
// and it will have no knowledge of the number '0'.
// (assuming we run this function on the identifier c)
// ```
// let a = 0;
// let b = a;
// let c = b;
// ```
const findRootReference = node => {
if (node.type === 'Identifier') {
const reference = findReference(node.name);

if (reference && reference.init) {
return findRootReference(reference.init);
}

return node;
}

if (node.type === 'CallExpression' || node.type === 'NewExpression') {
return findRootReference(node.callee);
}

// I'm aware that there are other type of Node as well but the scope of the lint shouldn't need those.
return node;
};

// Determine if the given node is a regex expression.
// There are two ways to create regex expressions in JavaScript, the first being Regex Literal and the second being RegExp class.
// - The first way can be easily lookup using .regex field in the node.
// - The second way can't really be lookup so I just check the name of a class constructor/function and check if it's matches.
const isRegExp = lookup => {
if (lookup.regex) {
return true;
}

// Look up references in case it's a variable or RegExp declaration.
const reference = findRootReference(lookup);

if (reference) {
return reference.regex || reference.name === 'RegExp';
}

return false;
};

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.has(node.callee.property.name) &&
const isAssertion = node.callee.type === 'MemberExpression' &&
util.getNameOfRootNodeObject(node.callee) === 't';

if (!isBooleanAssertion) {
return;
}

const firstArg = node.arguments[0];
const isBooleanAssertion = isAssertion &&
booleanTests.has(node.callee.property.name);

// First argument is a call expression
const isFunctionCall = firstArg.type === 'CallExpression';
if (!isFunctionCall || !firstArg.callee.property) {
return;
}
const isEqualityAssertion = isAssertion &&
equalityTests.has(node.callee.property.name);

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 (isBooleanAssertion) {
booleanHandler(node);
} else if (isEqualityAssertion) {
equalityHandler(node);
}

let isRegExp = lookup.regex;

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

if (!isRegExp) {
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
});
})
});
};
Expand Down
40 changes: 39 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 @@ -17,12 +17,14 @@ 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));',
header + 'const a = "not a regexp";\ntest(t => t.true(a.test("foo")));',
header + 'test("main", t => t.true(foo()));',
header + 'test(t => t.regex(foo, new RegExp("\\d+")));',
header + 'test(t => t.regex(foo, RegExp("\\d+")));',
// Shouldn't be triggered since it's not a test file
'test(t => t.true(/\\d+/.test("foo")));'
],
Expand Down Expand Up @@ -61,6 +63,42 @@ 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')
},
// The same as the above tests but with `RegExp()` object instead of a regex literal
{
code: header + 'test(t => t.true(new RegExp("\\d+").test("foo")));',
output: header + 'test(t => t.regex("foo", new RegExp("\\d+")));',
errors: errors('regex')
},
{
code: header + 'test(t => t.false(foo.search(new RegExp("\\d+"))));',
output: header + 'test(t => t.notRegex(foo, new RegExp("\\d+")));',
errors: errors('notRegex')
},
{
code: header + 'const regexp = RegExp("\\d+");\ntest(t => t.true(foo.search(regexp)));',
output: header + 'const regexp = RegExp("\\d+");\ntest(t => t.regex(foo, regexp));',
errors: errors('regex')
},
{
code: header + 'test(t => t.truthy(foo.match(new RegExp("\\d+"))));',
output: header + 'test(t => t.regex(foo, new RegExp("\\d+")));',
errors: errors('regex')
},
{
code: header + 'test(t => t.false(RegExp("\\d+").test("foo")));',
output: header + 'test(t => t.notRegex("foo", RegExp("\\d+")));',
errors: errors('notRegex')
},
{
code: header + 'test(t => t.true(new RegExp("\\d+").test(foo())));',
output: header + 'test(t => t.regex(foo(), new RegExp("\\d+")));',
errors: errors('regex')
},
{
code: header + 'const reg = RegExp("\\d+");\ntest(t => t.true(reg.test(foo.bar())));',
output: header + 'const reg = RegExp("\\d+");\ntest(t => t.regex(foo.bar(), reg));',
errors: errors('regex')
}
]
});