Skip to content

fix: guard against null identifier nodes #309

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 8 commits into from
Apr 3, 2021
Merged

fix: guard against null identifier nodes #309

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
c176c2a
fix: guard against null deepest identifier node
Belco90 Apr 2, 2021
83f27d8
fix: guard against null property identifier node
Belco90 Apr 2, 2021
91abe97
ci: decrease coverage threshold
Belco90 Apr 2, 2021
abfe5e8
fix: enable TS strict mode
Belco90 Apr 3, 2021
751889b
fix: remove closing comment leftover
Belco90 Apr 3, 2021
8ceb80f
refactor: declare test cases typings as expected
Belco90 Apr 3, 2021
c20dbb9
fix: remove wrong boolean check for detection
Belco90 Apr 3, 2021
cc040a5
Merge "main" branch into "306_null-identifiers"
Belco90 Apr 3, 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
6 changes: 0 additions & 6 deletions jest.config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +6,6 @@ module.exports = {
},
coverageThreshold: {
global: {
branches: 100,
functions: 100,
lines: 100,
statements: 100,
},
'./lib/node-utils.ts': {
branches: 90,
functions: 90,
lines: 90,
Comment on lines 9 to 11
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've dropped the coverage threshold a bit since I prefer to guard against null nodes everywhere even if there is no possible code for it. We'll see what I do with this after releasing v4.

Expand Down
21 changes: 19 additions & 2 deletions lib/detect-testing-library-utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,6 +160,11 @@ export function detectTestingLibraryUtils<

const referenceNode = getReferenceNode(node);
const referenceNodeIdentifier = getPropertyIdentifierNode(referenceNode);

if (!referenceNodeIdentifier) {
return false;
}

const importedUtilSpecifier = getImportedUtilSpecifier(
referenceNodeIdentifier
);
Expand Down Expand Up @@ -477,6 +482,10 @@ export function detectTestingLibraryUtils<
const isRenderVariableDeclarator: IsRenderVariableDeclaratorFn = (node) => {
const initIdentifierNode = getDeepestIdentifierNode(node.init);

if (!initIdentifierNode) {
return false;
}

return isRenderUtil(initIdentifierNode);
};

Expand Down Expand Up @@ -605,7 +614,11 @@ export function detectTestingLibraryUtils<
node: TSESTree.MemberExpression | TSESTree.Identifier
): TSESTree.ImportClause | TSESTree.Identifier | undefined => {
const identifierName: string | undefined = getPropertyIdentifierNode(node)
.name;
?.name;

if (!identifierName) {
return undefined;
}

return findImportedUtilSpecifier(identifierName);
};
Expand Down Expand Up @@ -633,7 +646,11 @@ export function detectTestingLibraryUtils<
}

const identifierName: string | undefined = getPropertyIdentifierNode(node)
.name;
?.name;

if (!identifierName) {
return false;
}

return hasImportMatch(importNode, identifierName);
};
Expand Down
6 changes: 5 additions & 1 deletion lib/rules/no-await-sync-events.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,10 @@ export default createTestingLibraryRule<Options, MessageIds>({
'AwaitExpression > CallExpression'(node: TSESTree.CallExpression) {
const simulateEventFunctionIdentifier = getDeepestIdentifierNode(node);

if (!simulateEventFunctionIdentifier) {
return;
}

const isSimulateEventMethod =
helpers.isUserEventMethod(simulateEventFunctionIdentifier) ||
helpers.isFireEventMethod(simulateEventFunctionIdentifier);
Expand Down Expand Up @@ -77,7 +81,7 @@ export default createTestingLibraryRule<Options, MessageIds>({
messageId: 'noAwaitSyncEvents',
data: {
name: `${
getPropertyIdentifierNode(node).name
getPropertyIdentifierNode(node)?.name
}.${simulateEventFunctionName}`,
},
});
Expand Down
9 changes: 9 additions & 0 deletions lib/rules/no-container.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,11 @@ export default createTestingLibraryRule<Options, MessageIds>({
return {
CallExpression(node) {
const callExpressionIdentifier = getDeepestIdentifierNode(node);

if (!callExpressionIdentifier) {
return;
}

if (helpers.isRenderUtil(callExpressionIdentifier)) {
detectRenderWrapper(callExpressionIdentifier);
}
Expand All @@ -103,6 +108,10 @@ export default createTestingLibraryRule<Options, MessageIds>({
VariableDeclarator(node) {
const initIdentifierNode = getDeepestIdentifierNode(node.init);

if (!initIdentifierNode) {
return;
}

const isRenderWrapperVariableDeclarator = initIdentifierNode
? renderWrapperNames.includes(initIdentifierNode.name)
: false;
Expand Down
21 changes: 18 additions & 3 deletions lib/rules/no-debug.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,10 @@ export default createTestingLibraryRule<Options, MessageIds>({
VariableDeclarator(node) {
const initIdentifierNode = getDeepestIdentifierNode(node.init);

if (!initIdentifierNode) {
return;
}

const isRenderWrapperVariableDeclarator = initIdentifierNode
? renderWrapperNames.includes(initIdentifierNode.name)
: false;
Expand All @@ -68,9 +72,11 @@ export default createTestingLibraryRule<Options, MessageIds>({
ASTUtils.isIdentifier(property.key) &&
property.key.name === 'debug'
) {
suspiciousDebugVariableNames.push(
getDeepestIdentifierNode(property.value).name
);
const identifierNode = getDeepestIdentifierNode(property.value);

if (identifierNode) {
suspiciousDebugVariableNames.push(identifierNode.name);
}
}
}
}
Expand All @@ -83,13 +89,22 @@ export default createTestingLibraryRule<Options, MessageIds>({
},
CallExpression(node) {
const callExpressionIdentifier = getDeepestIdentifierNode(node);
Copy link
Member Author

@Belco90 Belco90 Apr 2, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

@timdeschryver This is really weird: getDeepestIdentifierNode (and some other utils for finding identifiers) are typed as returning TSESTree.Identifier | null. However, TS is assuming they always returning TSESTree.Identifier but never null:

CleanShot 2021-04-02 at 12 02 28

Then, if I update the function to return '' instead of return null and update the returned type to TSESTree.Identifier | string, TS starts complaining about returned value saved in all these rules might not be an Identifier. Is this a bug on TS? Am I missing something else? I don't get why it always assumes null won't be returned.

Copy link

Choose a reason for hiding this comment

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

I suspect turning on strictNullChecks in the tsconfig would fix that.

Copy link
Member

Choose a reason for hiding this comment

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

Oh wow... TIL...
@epmatsw is right, you'll have to turn the strict mode on.
I would encourage to turn on strict mode though, not just strict null checks.

Copy link
Member Author

Choose a reason for hiding this comment

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

Aah damn! I thought we had strict enabled, but anyway I didn't expect those nullable values to be silent. I'm gonna enable strict mode and fix all corresponding issues then. Thanks guys.


if (!callExpressionIdentifier) {
return;
}

if (helpers.isRenderUtil(callExpressionIdentifier)) {
detectRenderWrapper(callExpressionIdentifier);
}

const referenceNode = getReferenceNode(node);
const referenceIdentifier = getPropertyIdentifierNode(referenceNode);

if (!referenceIdentifier) {
return;
}

const isDebugUtil = helpers.isDebugUtil(callExpressionIdentifier);
const isDeclaredDebugVariable = suspiciousDebugVariableNames.includes(
callExpressionIdentifier.name
Expand Down
4 changes: 4 additions & 0 deletions lib/rules/no-promise-in-fire-event.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,10 @@ export default createTestingLibraryRule<Options, MessageIds>({
if (isCallExpression(node)) {
const domElementIdentifier = getDeepestIdentifierNode(node);

if (!domElementIdentifier) {
return;
}

if (
helpers.isAsyncQuery(domElementIdentifier) ||
isPromiseIdentifier(domElementIdentifier)
Expand Down
5 changes: 5 additions & 0 deletions lib/rules/no-render-in-setup.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,11 @@ export default createTestingLibraryRule<Options, MessageIds>({
(hook) => hook !== allowTestingFrameworkSetupHook
);
const callExpressionIdentifier = getDeepestIdentifierNode(node);

if (!callExpressionIdentifier) {
return;
}

const isRenderIdentifier = helpers.isRenderUtil(
callExpressionIdentifier
);
Expand Down
4 changes: 4 additions & 0 deletions lib/rules/no-wait-for-empty-callback.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,10 @@ export default createTestingLibraryRule<Options, MessageIds>({
const parentCallExpression = node.parent as TSESTree.CallExpression;
const parentIdentifier = getPropertyIdentifierNode(parentCallExpression);

if (!parentIdentifier) {
return false;
}

return helpers.isAsyncUtil(parentIdentifier, [
'waitFor',
'waitForElementToBeRemoved',
Expand Down
4 changes: 4 additions & 0 deletions lib/rules/no-wait-for-multiple-assertions.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,10 @@ export default createTestingLibraryRule<Options, MessageIds>({
callExpressionNode
);

if (!callExpressionIdentifier) {
return;
}

if (!helpers.isAsyncUtil(callExpressionIdentifier, ['waitFor'])) {
return;
}
Expand Down
4 changes: 4 additions & 0 deletions lib/rules/no-wait-for-side-effects.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,10 @@ export default createTestingLibraryRule<Options, MessageIds>({
callExpressionNode
);

if (!callExpressionIdentifier) {
return;
}

if (!helpers.isAsyncUtil(callExpressionIdentifier, ['waitFor'])) {
return;
}
Expand Down
5 changes: 5 additions & 0 deletions lib/rules/render-result-naming-convention.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,11 @@ export default createTestingLibraryRule<Options, MessageIds>({
return {
CallExpression(node) {
const callExpressionIdentifier = getDeepestIdentifierNode(node);

if (!callExpressionIdentifier) {
return;
}

if (helpers.isRenderUtil(callExpressionIdentifier)) {
detectRenderWrapper(callExpressionIdentifier);
}
Expand Down
4 changes: 4 additions & 0 deletions tests/create-testing-library-rule.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -307,6 +307,10 @@ ruleTester.run(RULE_NAME, rule, {
const utils = somethingElse.render()
`,
},

// Weird edge cases
`(window as any).__THING = false;`,
`thing.method.lastCall.args[0]();`,
],
invalid: [
// Test Cases for Imports
Expand Down
4 changes: 4 additions & 0 deletions tests/lib/rules/no-debug.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,10 @@ ruleTester.run(RULE_NAME, rule, {
debug()
`,
},

`// cover edge case for https://github.com/testing-library/eslint-plugin-testing-library/issues/306
thing.method.lastCall.args[0]();
Copy link
Member Author

Choose a reason for hiding this comment

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

Test for issue mentioned by @epmatsw

`,
],

invalid: [
Expand Down