Skip to content

Move rules settings to ESLint shared config: refactor no-debug rule #293

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

Merged
merged 6 commits into from
Mar 20, 2021
Merged

Move rules settings to ESLint shared config: refactor no-debug rule #293

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
a86ddf0
refactor(no-debug): use new rule creator
Belco90 Mar 16, 2021
bf84c07
test(no-debug): improve current invalid error assertions
Belco90 Mar 16, 2021
02a8a7b
docs(no-debug): fix typo
Belco90 Mar 20, 2021
1593c38
refactor(no-debug): report debug call expressions with detection helpers
Belco90 Mar 20, 2021
9861eb4
refactor(no-debug): report debug from renders with detection helpers
Belco90 Mar 20, 2021
06e5986
refactor(no-debug): remove unnecessary checks
Belco90 Mar 20, 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
8 changes: 1 addition & 7 deletions docs/rules/no-debug.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
# Disallow the use of `debug` (no-debug)

Just like `console.log` statements pollutes the browser's output, debug statements also pollutes the tests if one of your team mates forgot to remove it. `debug` statements should be used when you actually want to debug your tests but should not be pushed to the codebase.
Just like `console.log` statements pollutes the browser's output, debug statements also pollutes the tests if one of your teammates forgot to remove it. `debug` statements should be used when you actually want to debug your tests but should not be pushed to the codebase.

## Rule Details

Expand Down Expand Up @@ -28,12 +28,6 @@ const { screen } = require('@testing-library/react');
screen.debug();
```

If you use [custom render functions](https://testing-library.com/docs/example-react-redux) then you can set a config option in your `.eslintrc` to look for these.
Copy link
Member

Choose a reason for hiding this comment

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

This LGTM, I just wonder if we should mention that it can be configured via settings?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I'm leaving all the documentation stuff for the last step after I've migrated all the rules. I'll mention the custom render settings in the README, but not sure if we should mention it on each rule depending on render (which is what I understand you are suggesting).

Copy link
Member

Choose a reason for hiding this comment

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

Yea, correct. While I was writing it I was also considering if it's worth it to mention in for every rule or not 😅.
I think having a global doc is good.


```
"testing-library/no-debug": ["error", {"renderFunctions":["renderWithRedux", "renderWithRouter"]}],
```

## Further Reading

- [debug API in React Testing Library](https://testing-library.com/docs/react-testing-library/api#debug)
Expand Down
14 changes: 14 additions & 0 deletions lib/detect-testing-library-utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@ type IsAsyncUtilFn = (
) => boolean;
type IsFireEventMethodFn = (node: TSESTree.Identifier) => boolean;
type IsRenderUtilFn = (node: TSESTree.Identifier) => boolean;
type IsDebugUtilFn = (node: TSESTree.Identifier) => boolean;
type IsPresenceAssertFn = (node: TSESTree.MemberExpression) => boolean;
type IsAbsenceAssertFn = (node: TSESTree.MemberExpression) => boolean;
type CanReportErrorsFn = () => boolean;
Expand Down Expand Up @@ -96,6 +97,7 @@ export interface DetectionHelpers {
isAsyncUtil: IsAsyncUtilFn;
isFireEventMethod: IsFireEventMethodFn;
isRenderUtil: IsRenderUtilFn;
isDebugUtil: IsDebugUtilFn;
isPresenceAssert: IsPresenceAssertFn;
isAbsenceAssert: IsAbsenceAssertFn;
canReportErrors: CanReportErrorsFn;
Expand Down Expand Up @@ -406,6 +408,17 @@ export function detectTestingLibraryUtils<
);
};

const isDebugUtil: IsDebugUtilFn = (node) => {
return isTestingLibraryUtil(
node,
(identifierNodeName, originalNodeName) => {
return [identifierNodeName, originalNodeName]
.filter(Boolean)
.includes('debug');
}
);
};

/**
* Determines whether a given MemberExpression node is a presence assert
*
Expand Down Expand Up @@ -549,6 +562,7 @@ export function detectTestingLibraryUtils<
isAsyncUtil,
isFireEventMethod,
isRenderUtil,
isDebugUtil,
isPresenceAssert,
isAbsenceAssert,
canReportErrors,
Expand Down
8 changes: 6 additions & 2 deletions lib/node-utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -392,7 +392,7 @@ export function getPropertyIdentifierNode(
}

/**
* Gets the deepest identifier node from a given node.
* Gets the deepest identifier node in the expression from a given node.
Copy link
Member Author

Choose a reason for hiding this comment

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

Clarifying the scope for getting the deepest identifier.

*
* Opposite of {@link getReferenceNode}
*
Expand All @@ -416,11 +416,15 @@ export function getDeepestIdentifierNode(
return getDeepestIdentifierNode(node.callee);
}

if (ASTUtils.isAwaitExpression(node)) {
Copy link
Member Author

Choose a reason for hiding this comment

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

I discovered this case thanks to existing tests from no-debug !

return getDeepestIdentifierNode(node.argument);
}

return null;
}

/**
* Gets the farthest node from a given node.
* Gets the farthest node in the expression from a given node.
Copy link
Member Author

Choose a reason for hiding this comment

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

Clarifying the scope for getting the farthest identifier.

*
* Opposite of {@link getDeepestIdentifierNode}

Expand Down
193 changes: 48 additions & 145 deletions lib/rules/no-debug.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,28 +1,18 @@
import {
ESLintUtils,
TSESTree,
ASTUtils,
} from '@typescript-eslint/experimental-utils';
import {
getDocsUrl,
LIBRARY_MODULES,
hasTestingLibraryImportModule,
} from '../utils';
import {
getDeepestIdentifierNode,
getPropertyIdentifierNode,
getReferenceNode,
isObjectPattern,
isProperty,
isCallExpression,
isLiteral,
isMemberExpression,
isImportSpecifier,
isRenderVariableDeclarator,
} from '../node-utils';
import { createTestingLibraryRule } from '../create-testing-library-rule';
import { ASTUtils, TSESTree } from '@typescript-eslint/experimental-utils';

export const RULE_NAME = 'no-debug';
export type MessageIds = 'noDebug';
type Options = [{ renderFunctions?: string[] }];
type Options = [];

export default ESLintUtils.RuleCreator(getDocsUrl)<Options, MessageIds>({
export default createTestingLibraryRule<Options, MessageIds>({
name: RULE_NAME,
meta: {
type: 'problem',
Expand All @@ -46,154 +36,67 @@ export default ESLintUtils.RuleCreator(getDocsUrl)<Options, MessageIds>({
},
],
},
defaultOptions: [
{
renderFunctions: [],
},
],

create(context, [options]) {
let hasDestructuredDebugStatement = false;
const renderVariableDeclarators: TSESTree.VariableDeclarator[] = [];

const { renderFunctions } = options;
defaultOptions: [],

let hasImportedScreen = false;
let wildcardImportName: string = null;
create(context, [], helpers) {
const suspiciousDebugVariableNames: string[] = [];
const suspiciousReferenceNodes: TSESTree.Identifier[] = [];

return {
VariableDeclarator(node) {
if (isRenderVariableDeclarator(node, ['render', ...renderFunctions])) {
if (
isObjectPattern(node.id) &&
node.id.properties.some(
(property) =>
isProperty(property) &&
ASTUtils.isIdentifier(property.key) &&
property.key.name === 'debug'
)
) {
hasDestructuredDebugStatement = true;
}

if (node.id.type === 'Identifier') {
renderVariableDeclarators.push(node);
}
}
},
[`VariableDeclarator > CallExpression > Identifier[name="require"]`](
node: TSESTree.Identifier
) {
const { arguments: args } = node.parent as TSESTree.CallExpression;
const initIdentifierNode = getDeepestIdentifierNode(node.init);

const literalNodeScreenModuleName = args.find(
(args) =>
isLiteral(args) &&
typeof args.value === 'string' &&
LIBRARY_MODULES.includes(args.value)
);

if (!literalNodeScreenModuleName) {
if (!helpers.isRenderUtil(initIdentifierNode)) {
return;
}

const declaratorNode = node.parent
.parent as TSESTree.VariableDeclarator;

hasImportedScreen =
isObjectPattern(declaratorNode.id) &&
declaratorNode.id.properties.some(
(property) =>
// find debug obtained from render and save their name, like:
// const { debug } = render();
if (isObjectPattern(node.id)) {
for (const property of node.id.properties) {
if (
isProperty(property) &&
ASTUtils.isIdentifier(property.key) &&
property.key.name === 'screen'
);
},
// checks if import has shape:
// import { screen } from '@testing-library/dom';
ImportDeclaration(node: TSESTree.ImportDeclaration) {
Copy link
Member Author

Choose a reason for hiding this comment

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

it's really pleasant to remove this unnecessary checks 😌

if (!hasTestingLibraryImportModule(node)) {
return;
}

hasImportedScreen = node.specifiers.some(
(s) => isImportSpecifier(s) && s.imported.name === 'screen'
);
},
// checks if import has shape:
// import * as dtl from '@testing-library/dom';
'ImportDeclaration ImportNamespaceSpecifier'(
node: TSESTree.ImportNamespaceSpecifier
) {
const importDeclarationNode = node.parent as TSESTree.ImportDeclaration;
if (!hasTestingLibraryImportModule(importDeclarationNode)) {
return;
property.key.name === 'debug'
) {
suspiciousDebugVariableNames.push(
getDeepestIdentifierNode(property.value).name
);
}
}
}

wildcardImportName = node.local && node.local.name;
},
[`CallExpression > Identifier[name="debug"]`](node: TSESTree.Identifier) {
if (hasDestructuredDebugStatement) {
context.report({
node,
messageId: 'noDebug',
});
// find utils kept from render and save their node, like:
// const utils = render();
if (ASTUtils.isIdentifier(node.id)) {
suspiciousReferenceNodes.push(node.id);
}
},
[`CallExpression > MemberExpression > Identifier[name="debug"]`](
node: TSESTree.Identifier
) {
const memberExpression = node.parent as TSESTree.MemberExpression;
const identifier = memberExpression.object as TSESTree.Identifier;
const memberExpressionName = identifier.name;
/*
check if `debug` used following the pattern:

import { screen } from '@testing-library/dom';
...
screen.debug();
*/
const isScreenDebugUsed =
hasImportedScreen && memberExpressionName === 'screen';

/*
check if `debug` used following the pattern:

import * as dtl from '@testing-library/dom';
...
dtl.debug();
*/
const isNamespaceDebugUsed =
wildcardImportName && memberExpressionName === wildcardImportName;
CallExpression(node) {
const callExpressionIdentifier = getDeepestIdentifierNode(node);
const referenceNode = getReferenceNode(node);
const referenceIdentifier = getPropertyIdentifierNode(referenceNode);

const isDebugUtil = helpers.isDebugUtil(callExpressionIdentifier);
const isDeclaredDebugVariable = suspiciousDebugVariableNames.includes(
callExpressionIdentifier.name
);
const isChainedReferenceDebug = suspiciousReferenceNodes.some(
(suspiciousReferenceIdentifier) => {
return (
callExpressionIdentifier.name === 'debug' &&
suspiciousReferenceIdentifier.name === referenceIdentifier.name
);
}
);

if (isScreenDebugUsed || isNamespaceDebugUsed) {
if (isDebugUtil || isDeclaredDebugVariable || isChainedReferenceDebug) {
context.report({
node,
node: callExpressionIdentifier,
messageId: 'noDebug',
});
}
},
'Program:exit'() {
renderVariableDeclarators.forEach((renderVar) => {
const renderVarReferences = context
.getDeclaredVariables(renderVar)[0]
.references.slice(1);
renderVarReferences.forEach((ref) => {
const parent = ref.identifier.parent;
if (
isMemberExpression(parent) &&
ASTUtils.isIdentifier(parent.property) &&
parent.property.name === 'debug' &&
isCallExpression(parent.parent)
) {
context.report({
node: parent.property,
messageId: 'noDebug',
});
}
});
});
},
};
},
});
Loading