-
Notifications
You must be signed in to change notification settings - Fork 147
Move rules settings to ESLint shared config: part 2 - check imports #239
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.
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
Changes from 3 commits
d2844a1
824150d
4b16c7b
4f8e43f
e9d7bc3
476b7f6
400d343
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,7 +1,12 @@ | ||
import { TSESLint, TSESTree } from '@typescript-eslint/experimental-utils'; | ||
|
||
export type DetectionHelpers = { | ||
getIsImportingTestingLibrary: () => boolean; | ||
getIsTestingLibraryImported: () => boolean; | ||
canReportErrors: () => boolean; | ||
}; | ||
|
||
export type TestingLibrarySettings = { | ||
'testing-library/module'?: string; | ||
}; | ||
|
||
/** | ||
|
@@ -13,48 +18,84 @@ export function detectTestingLibraryUtils< | |
TRuleListener extends TSESLint.RuleListener = TSESLint.RuleListener | ||
>( | ||
ruleCreate: ( | ||
context: Readonly<TSESLint.RuleContext<TMessageIds, TOptions>>, | ||
context: Readonly< | ||
TSESLint.RuleContext<TMessageIds, TOptions> & { | ||
settings: TestingLibrarySettings; | ||
} | ||
>, | ||
optionsWithDefault: Readonly<TOptions>, | ||
detectionHelpers: Readonly<DetectionHelpers> | ||
) => TRuleListener | ||
) { | ||
return ( | ||
context: Readonly<TSESLint.RuleContext<TMessageIds, TOptions>>, | ||
context: Readonly< | ||
TSESLint.RuleContext<TMessageIds, TOptions> & { | ||
settings: TestingLibrarySettings; | ||
} | ||
>, | ||
optionsWithDefault: Readonly<TOptions> | ||
): TRuleListener => { | ||
let isImportingTestingLibrary = false; | ||
): TSESLint.RuleListener => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've replaced returned type from |
||
let isImportingTestingLibraryModule = false; | ||
let isImportingCustomModule = false; | ||
|
||
// TODO: init here options based on shared ESLint config | ||
// Init options based on shared ESLint settings | ||
const customModule = context.settings['testing-library/module']; | ||
|
||
// helpers for Testing Library detection | ||
// Helpers for Testing Library detection. | ||
const helpers: DetectionHelpers = { | ||
getIsImportingTestingLibrary() { | ||
return isImportingTestingLibrary; | ||
getIsTestingLibraryImported() { | ||
Belco90 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
if (!customModule) { | ||
return true; | ||
} | ||
|
||
return isImportingTestingLibraryModule || isImportingCustomModule; | ||
}, | ||
canReportErrors() { | ||
return this.getIsTestingLibraryImported(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it's been a long time since I've seen a |
||
}, | ||
}; | ||
|
||
// instructions for Testing Library detection | ||
// Instructions for Testing Library detection. | ||
// `ImportDeclaration` must be first in order to know if Testing Library | ||
// is imported ASAP. | ||
Belco90 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
const detectionInstructions: TSESLint.RuleListener = { | ||
ImportDeclaration(node: TSESTree.ImportDeclaration) { | ||
isImportingTestingLibrary = /testing-library/g.test( | ||
node.source.value as string | ||
); | ||
if (!isImportingTestingLibraryModule) { | ||
// check only if testing library import not found yet so we avoid | ||
// to override isImportingTestingLibraryModule after it's found | ||
isImportingTestingLibraryModule = /testing-library/g.test( | ||
node.source.value as string | ||
); | ||
} | ||
|
||
if (!isImportingCustomModule) { | ||
// check only if custom module import not found yet so we avoid | ||
// to override isImportingCustomModule after it's found | ||
const importName = String(node.source.value); | ||
isImportingCustomModule = importName.endsWith(customModule); | ||
} | ||
}, | ||
}; | ||
|
||
// update given rule to inject Testing Library detection | ||
const ruleInstructions = ruleCreate(context, optionsWithDefault, helpers); | ||
const enhancedRuleInstructions = Object.assign({}, ruleInstructions); | ||
const enhancedRuleInstructions: TSESLint.RuleListener = {}; | ||
|
||
// The order here is important too: detection instructions must come before | ||
// than rule instructions to: | ||
// - detect Testing Library things before the rule is applied | ||
// - be able to prevent the rule about to be applied if necessary | ||
Belco90 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
const allKeys = new Set( | ||
Object.keys(detectionInstructions).concat(Object.keys(ruleInstructions)) | ||
); | ||
|
||
Object.keys(detectionInstructions).forEach((instruction) => { | ||
(enhancedRuleInstructions as TSESLint.RuleListener)[instruction] = ( | ||
node | ||
) => { | ||
allKeys.forEach((instruction) => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Now I iterate through all keys to be able to prevent original rule instructions. |
||
enhancedRuleInstructions[instruction] = (node) => { | ||
if (instruction in detectionInstructions) { | ||
detectionInstructions[instruction](node); | ||
} | ||
|
||
if (ruleInstructions[instruction]) { | ||
if (helpers.canReportErrors() && ruleInstructions[instruction]) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here is where the plugin prevents original rule instructions to be executed if conditions are not met! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm a bit lost in understanding what's the difference between the method There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. in addition to that, if I understand correctly, we would just execute the rules if TL is imported, otherwise, nothing gets executed - is that correct? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm anticipating next PR with
Yes, but by default TL is always considered as imported due to the "aggressive reporting" mode. |
||
return ruleInstructions[instruction](node); | ||
} | ||
}; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -25,11 +25,10 @@ export default createTestingLibraryRule<Options, MessageIds>({ | |
}, | ||
defaultOptions: [], | ||
|
||
create: (context, _, helpers) => { | ||
create(context) { | ||
function showErrorForNodeAccess(node: TSESTree.MemberExpression) { | ||
isIdentifier(node.property) && | ||
ALL_RETURNING_NODES.includes(node.property.name) && | ||
helpers.getIsImportingTestingLibrary() && | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is not necessary anymore! ✨ |
||
context.report({ | ||
node: node, | ||
loc: node.property.loc.start, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,105 @@ | ||
import { createRuleTester } from './lib/test-utils'; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I added this test file to check both |
||
import rule, { RULE_NAME } from './fake-rule'; | ||
|
||
const ruleTester = createRuleTester({ | ||
ecmaFeatures: { | ||
jsx: true, | ||
}, | ||
}); | ||
|
||
ruleTester.run(RULE_NAME, rule, { | ||
valid: [ | ||
{ | ||
code: ` | ||
// case: nothing related to Testing Library at all | ||
import { shallow } from 'enzyme'; | ||
|
||
const wrapper = shallow(<MyComponent />); | ||
`, | ||
}, | ||
{ | ||
code: ` | ||
// case: render imported for different custom module | ||
import { render } from '@somewhere/else' | ||
|
||
const utils = render(); | ||
`, | ||
settings: { | ||
'testing-library/module': 'test-utils', | ||
}, | ||
}, | ||
], | ||
invalid: [ | ||
{ | ||
code: ` | ||
// case: render imported from any module by default (aggressive reporting) | ||
import { render } from '@somewhere/else' | ||
import { somethingElse } from 'another-module' | ||
|
||
const utils = render(); | ||
`, | ||
errors: [ | ||
{ | ||
line: 6, | ||
column: 21, | ||
messageId: 'fakeError', | ||
}, | ||
], | ||
}, | ||
{ | ||
code: ` | ||
// case: render imported from Testing Library module | ||
import { render } from '@testing-library/react' | ||
import { somethingElse } from 'another-module' | ||
|
||
const utils = render(); | ||
`, | ||
errors: [ | ||
{ | ||
line: 6, | ||
column: 21, | ||
messageId: 'fakeError', | ||
}, | ||
], | ||
}, | ||
{ | ||
code: ` | ||
// case: render imported from config custom module | ||
import { render } from 'test-utils' | ||
import { somethingElse } from 'another-module' | ||
|
||
const utils = render(); | ||
`, | ||
settings: { | ||
'testing-library/module': 'test-utils', | ||
}, | ||
errors: [ | ||
{ | ||
line: 6, | ||
column: 21, | ||
messageId: 'fakeError', | ||
}, | ||
], | ||
}, | ||
{ | ||
code: ` | ||
// case: render imported from Testing Library module if | ||
// custom module setup | ||
import { render } from '@testing-library/react' | ||
import { somethingElse } from 'another-module' | ||
|
||
const utils = render(); | ||
`, | ||
settings: { | ||
'testing-library/module': 'test-utils', | ||
}, | ||
errors: [ | ||
{ | ||
line: 7, | ||
column: 21, | ||
messageId: 'fakeError', | ||
}, | ||
], | ||
}, | ||
], | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,42 @@ | ||
/** | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fake rule trying to report stuff asap to be able to run the previous test file. |
||
* @file Fake rule to be able to test createTestingLibraryRule and | ||
* detectTestingLibraryUtils properly | ||
*/ | ||
import { TSESTree } from '@typescript-eslint/experimental-utils'; | ||
import { createTestingLibraryRule } from '../lib/create-testing-library-rule'; | ||
|
||
export const RULE_NAME = 'fake-rule'; | ||
type Options = []; | ||
type MessageIds = 'fakeError'; | ||
|
||
export default createTestingLibraryRule<Options, MessageIds>({ | ||
name: RULE_NAME, | ||
meta: { | ||
type: 'problem', | ||
docs: { | ||
description: 'Fake rule to test rule maker and detection helpers', | ||
category: 'Possible Errors', | ||
recommended: false, | ||
}, | ||
messages: { | ||
fakeError: 'fake error reported', | ||
}, | ||
fixable: null, | ||
schema: [], | ||
}, | ||
defaultOptions: [], | ||
create(context) { | ||
const reportRenderIdentifier = (node: TSESTree.Identifier) => { | ||
if (node.name === 'render') { | ||
context.report({ | ||
node, | ||
messageId: 'fakeError', | ||
}); | ||
} | ||
}; | ||
|
||
return { | ||
'CallExpression Identifier': reportRenderIdentifier, | ||
}; | ||
}, | ||
Belco90 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -51,22 +51,38 @@ ruleTester.run(RULE_NAME, rule, { | |
within(signinModal).getByPlaceholderText('Username'); | ||
`, | ||
}, | ||
{ | ||
/*{ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll reenable this test in the PR part 3 since there is where I'm gonna include the test file name pattern to report or not rules, so this will be solved then. First of many bugs I'll find during this refactor 😅 |
||
// TODO: this one should be valid indeed. Rule implementation must be improved | ||
// to track where the nodes are coming from. This one wasn't reported before | ||
// just because this code is not importing TL module, but that's a really | ||
// brittle check. Instead, this one shouldn't be reported since `children` | ||
// it's just a property not related to a node | ||
code: ` | ||
const Component = props => { | ||
return <div>{props.children}</div> | ||
} | ||
`, | ||
}, | ||
},*/ | ||
{ | ||
// Not importing a testing-library package | ||
code: ` | ||
const closestButton = document.getElementById('submit-btn').closest('button'); | ||
expect(closestButton).toBeInTheDocument(); | ||
// case: importing custom module | ||
const closestButton = document.getElementById('submit-btn').closest('button'); | ||
expect(closestButton).toBeInTheDocument(); | ||
`, | ||
settings: { | ||
'testing-library/module': 'test-utils', | ||
}, | ||
}, | ||
], | ||
invalid: [ | ||
{ | ||
code: ` | ||
// case: without importing TL (aggressive reporting) | ||
const closestButton = document.getElementById('submit-btn') | ||
expect(closestButton).toBeInTheDocument(); | ||
`, | ||
Comment on lines
+79
to
+83
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This was a valid case before but it isn't anymore. I'll find more like this in other rules. |
||
errors: [{ messageId: 'noNodeAccess', line: 3 }], | ||
}, | ||
{ | ||
code: ` | ||
import { screen } from '@testing-library/react'; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't think of a better way of typing the settings!