Skip to content

Commit

Permalink
fix(no-useless-not): Add support for visible and enabled options
Browse files Browse the repository at this point in the history
  • Loading branch information
@mskelton
mskelton committed Feb 28, 2024
1 parent 95fcfd8 commit e97a62b
Show file tree
Hide file tree
Showing 4 changed files with 287 additions and 35 deletions.
152 changes: 152 additions & 0 deletions src/rules/no-useless-not.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,92 @@ runRuleTester('no-useless-not', rule, {
],
output: 'expect(locator).toBeHidden()',
},
{
code: 'expect(locator).not.toBeVisible({ visible: true })',
errors: [
{
column: 17,
data: { new: 'toBeHidden', old: 'toBeVisible' },
endColumn: 32,
line: 1,
messageId: 'noUselessNot',
},
],
output: 'expect(locator).toBeHidden()',
},
{
code: 'expect(locator).toBeVisible({ visible: true })',
errors: [
{
column: 31,
data: {
new: 'toBeVisible',
old: 'toBeVisible',
property: 'visible',
},
endColumn: 44,
line: 1,
messageId: 'noUselessTruthy',
},
],
output: 'expect(locator).toBeVisible()',
},
{
code: 'expect(locator).toBeVisible({ visible: false })',
errors: [
{
column: 31,
data: {
new: 'toBeHidden',
old: 'toBeVisible',
property: 'visible',
},
endColumn: 45,
line: 1,
messageId: 'noUselessProperty',
},
],
output: 'expect(locator).toBeHidden()',
},
{
code: 'expect(locator).not.toBeVisible({ timeout: 1000, visible: true })',
errors: [
{
column: 17,
data: { new: 'toBeHidden', old: 'toBeVisible' },
endColumn: 32,
line: 1,
messageId: 'noUselessNot',
},
],
output: 'expect(locator).toBeHidden({ timeout: 1000 })',
},
{
code: 'expect(locator).not.toBeVisible({ visible: true, timeout: 1000 })',
errors: [
{
column: 17,
data: { new: 'toBeHidden', old: 'toBeVisible' },
endColumn: 32,
line: 1,
messageId: 'noUselessNot',
},
],
output: 'expect(locator).toBeHidden({ timeout: 1000 })',
},
{
code: 'expect(locator).not.toBeVisible({ visible: false })',
errors: [
{
column: 17,
data: { new: 'toBeVisible', old: 'toBeVisible' },
endColumn: 32,
line: 1,
messageId: 'noUselessNot',
},
],
output: 'expect(locator).toBeVisible()',
},
{
code: 'expect(locator).not.toBeHidden()',
errors: [
Expand All @@ -42,6 +128,66 @@ runRuleTester('no-useless-not', rule, {
],
output: 'expect(locator).toBeDisabled()',
},
{
code: 'expect(locator).toBeEnabled({ enabled: true })',
errors: [
{
column: 31,
data: {
new: 'toBeEnabled',
old: 'toBeEnabled',
property: 'enabled',
},
endColumn: 44,
line: 1,
messageId: 'noUselessTruthy',
},
],
output: 'expect(locator).toBeEnabled()',
},
{
code: 'expect(locator).not.toBeEnabled({ enabled: true })',
errors: [
{
column: 17,
data: { new: 'toBeDisabled', old: 'toBeEnabled' },
endColumn: 32,
line: 1,
messageId: 'noUselessNot',
},
],
output: 'expect(locator).toBeDisabled()',
},
{
code: 'expect(locator).toBeEnabled({ enabled: false })',
errors: [
{
column: 31,
data: {
new: 'toBeDisabled',
old: 'toBeEnabled',
property: 'enabled',
},
endColumn: 45,
line: 1,
messageId: 'noUselessProperty',
},
],
output: 'expect(locator).toBeDisabled()',
},
{
code: 'expect(locator).not.toBeEnabled({ enabled: false })',
errors: [
{
column: 17,
data: { new: 'toBeEnabled', old: 'toBeEnabled' },
endColumn: 32,
line: 1,
messageId: 'noUselessNot',
},
],
output: 'expect(locator).toBeEnabled()',
},
{
code: 'expect(locator).not.toBeDisabled()',
errors: [
Expand Down Expand Up @@ -132,13 +278,19 @@ runRuleTester('no-useless-not', rule, {
'expect.poll(() => locator).toBeDisabled()',
'expect(locator)[`toBeEnabled`]()',
'expect(locator)["toBeDisabled"]()',
// Ignores matchers with options
'expect(locator).toBeVisible({ visible })',
'expect(locator).not.toBeVisible({ visible })',
'expect(locator).toBeEnabled({ enabled })',
'expect(locator).not.toBeEnabled({ enabled })',
// Incomplete call expression
'expect(locator).toBeVisible',
'expect(locator).toBeEnabled',
'expect(locator).not.toBeHidden',
// Doesn't impact non-complimentary matchers
"expect(locator).not.toHaveText('foo')",
'expect(locator).not.toBeChecked()',
'expect(locator).not.toBeChecked({ checked })',
'expect(locator).not.toBeChecked({ checked: false })',
'expect(locator).not.toBeFocused()',
// Global aliases
Expand Down
125 changes: 94 additions & 31 deletions src/rules/no-useless-not.ts
View file
Original file line number Diff line number Diff line change
@@ -1,50 +1,109 @@
import { Rule } from 'eslint';
import { getStringValue } from '../utils/ast';
import { getRangeOffset, replaceAccessorFixer } from '../utils/fixer';
import { parseFnCall } from '../utils/parseFnCall';
import * as ESTree from 'estree';
import { getStringValue, isBooleanLiteral } from '../utils/ast';
import {
getRangeOffset,
removePropertyFixer,
replaceAccessorFixer,
} from '../utils/fixer';
import { truthy } from '../utils/misc';
import { type ParsedExpectFnCall, parseFnCall } from '../utils/parseFnCall';

const matcherMap: Record<string, string> = {
toBeDisabled: 'toBeEnabled',
toBeEnabled: 'toBeDisabled',
toBeHidden: 'toBeVisible',
toBeVisible: 'toBeHidden',
const matcherConfig: Record<string, { argName?: string; inverse: string }> = {
toBeDisabled: { inverse: 'toBeEnabled' },
toBeEnabled: {
argName: 'enabled',
inverse: 'toBeDisabled',
},
toBeHidden: { inverse: 'toBeVisible' },
toBeVisible: {
argName: 'visible',
inverse: 'toBeHidden',
},
};

function getOptions(call: ParsedExpectFnCall, name: string) {
const [arg] = call.matcherArgs;
if (arg?.type !== 'ObjectExpression') return;

const property = arg.properties.find(
(p): p is ESTree.Property =>
p.type === 'Property' &&
getStringValue(p.key) === name &&
isBooleanLiteral(p.value),
);

return {
arg,
property,
value: (property?.value as { value: boolean })?.value,
};
}

export default {
create(context) {
return {
CallExpression(node) {
const call = parseFnCall(context, node);
if (call?.type !== 'expect') return;

// As the name implies, this rule only implies if the not modifier is
// part of the matcher chain
// This rule only applies to specific matchers that have opposites
const config = matcherConfig[call.matcherName];
if (!config) return;

// If the matcher has an options argument, we need to check if it has
// a `visible` or `enabled` property that is a boolean literal.
const options = config.argName
? getOptions(call, config.argName)
: undefined;

// If an argument is provided to the `visible` or `enabled` property, but
// we can't determine it's value, we can't safely remove the `not` modifier.
if (options?.arg && options.value === undefined) return;

const notModifier = call.modifiers.find(
(mod) => getStringValue(mod) === 'not',
);
if (!notModifier) return;

// This rule only applies to specific matchers that have opposites
const matcherName = call.matcherName;
if (matcherName in matcherMap) {
const newMatcher = matcherMap[matcherName];
// If the matcher is not negated, or the matcher has no options, we can
// safely ignore it.
if (!notModifier && !options?.property) return;

// If the matcher is inverted, we need to remove the `not` modifier and
// replace the matcher with it's inverse.
const isInverted = !!notModifier !== (options?.value === false);
const newMatcherName = isInverted ? config.inverse : call.matcherName;

context.report({
data: { new: newMatcher, old: matcherName },
fix: (fixer) => [
fixer.removeRange([
notModifier.range![0] - getRangeOffset(notModifier),
notModifier.range![1] + 1,
]),
replaceAccessorFixer(fixer, call.matcher, newMatcher),
],
loc: {
end: call.matcher.loc!.end,
start: notModifier.loc!.start,
},
messageId: 'noUselessNot',
});
}
context.report({
data: {
new: newMatcherName,
old: call.matcherName,
property: config.argName ?? '',
},
fix: (fixer) => {
return [
// Remove the `not` modifier if it exists
notModifier &&
fixer.removeRange([
notModifier.range![0] - getRangeOffset(notModifier),
notModifier.range![1] + 1,
]),
// Remove the `visible` or `enabled` property if it exists
options?.property && removePropertyFixer(fixer, options.property),
// Swap the matcher name if it's different
call.matcherName !== newMatcherName &&
replaceAccessorFixer(fixer, call.matcher, newMatcherName),
].filter(truthy);
},
loc: notModifier
? { end: call.matcher.loc!.end, start: notModifier.loc!.start }
: options!.property!.loc!,
messageId: notModifier
? 'noUselessNot'
: isInverted
? 'noUselessProperty'
: 'noUselessTruthy',
});
},
};
},
Expand All @@ -58,6 +117,10 @@ export default {
fixable: 'code',
messages: {
noUselessNot: 'Unexpected usage of not.{{old}}(). Use {{new}}() instead.',
noUselessProperty:
"Unexpected usage of '{{old}}({ {{property}}: false })'. Use '{{new}}()' instead.",
noUselessTruthy:
"Unexpected usage of '{{old}}({ {{property}}: true })'. Use '{{new}}()' instead.",
},
type: 'problem',
},
Expand Down
16 changes: 13 additions & 3 deletions src/utils/ast.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,11 @@ export function isIdentifier(node: ESTree.Node, name?: string | RegExp) {
);
}

function isLiteral<T>(node: ESTree.Node, type: string, value?: T) {
function isLiteral<T>(
node: ESTree.Node,
type: string,
value?: T,
): node is ESTree.Literal {
return (
node.type === 'Literal' &&
(value === undefined
Expand All @@ -44,11 +48,17 @@ const isTemplateLiteral = (
node.quasis.length === 1 && // bail out if not simple
(value === undefined || node.quasis[0].value.raw === value);

export function isStringLiteral(node: ESTree.Node, value?: string) {
export function isStringLiteral(
node: ESTree.Node,
value?: string,
): node is ESTree.Literal {
return isLiteral(node, 'string', value);
}

export function isBooleanLiteral(node: ESTree.Node, value?: boolean) {
export function isBooleanLiteral(
node: ESTree.Node,
value?: boolean,
): node is ESTree.Literal {
return isLiteral(node, 'boolean', value);
}

Expand Down
Loading

0 comments on commit e97a62b

Please sign in to comment.