-
Notifications
You must be signed in to change notification settings - Fork 147
Move rules settings to ESLint shared config: part 3 - check reported file name #244
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 all commits
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,11 +1,7 @@ | ||
import { createRuleTester } from '../test-utils'; | ||
import rule, { RULE_NAME } from '../../../lib/rules/consistent-data-testid'; | ||
|
||
const ruleTester = createRuleTester({ | ||
ecmaFeatures: { | ||
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. We don't need this anymore, set by default on |
||
jsx: true, | ||
}, | ||
}); | ||
const ruleTester = createRuleTester(); | ||
|
||
ruleTester.run(RULE_NAME, rule, { | ||
valid: [ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,11 +1,7 @@ | ||
import { createRuleTester } from '../test-utils'; | ||
import rule, { RULE_NAME } from '../../../lib/rules/no-node-access'; | ||
|
||
const ruleTester = createRuleTester({ | ||
ecmaFeatures: { | ||
jsx: true, | ||
}, | ||
}); | ||
const ruleTester = createRuleTester(); | ||
|
||
ruleTester.run(RULE_NAME, rule, { | ||
valid: [ | ||
|
@@ -51,18 +47,16 @@ ruleTester.run(RULE_NAME, rule, { | |
within(signinModal).getByPlaceholderText('Username'); | ||
`, | ||
}, | ||
/*{ | ||
// 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> | ||
} | ||
`, | ||
Comment on lines
51
to
55
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 test is reenabled now, but just because the file name doesn't match so there is nothing reported here. As mentioned in my original comment there, the rule implementation is really brittle and should make sure node methods are coming from an actual node and not a var or prop. That's out of the scope of this refactor so I'm not gonna fix it for now (it shouldn't happen that often with file-name setting tho). |
||
},*/ | ||
settings: { | ||
'testing-library/file-name': 'testing-library\\.js', | ||
}, | ||
}, | ||
{ | ||
code: ` | ||
// case: importing custom module | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,14 +1,49 @@ | ||
import { resolve } from 'path'; | ||
import { TSESLint } from '@typescript-eslint/experimental-utils'; | ||
|
||
const DEFAULT_TEST_CASE_CONFIG = { | ||
filename: 'MyComponent.test.js', | ||
}; | ||
|
||
class TestingLibraryRuleTester extends TSESLint.RuleTester { | ||
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. Our custom RuleTester just injects default test case config for each valid/invalid test case so we don't need to set |
||
run<TMessageIds extends string, TOptions extends Readonly<unknown[]>>( | ||
ruleName: string, | ||
rule: TSESLint.RuleModule<TMessageIds, TOptions>, | ||
tests: TSESLint.RunTests<TMessageIds, TOptions> | ||
): void { | ||
const { valid, invalid } = tests; | ||
|
||
const finalValid = valid.map((testCase) => { | ||
if (typeof testCase === 'string') { | ||
return { | ||
...DEFAULT_TEST_CASE_CONFIG, | ||
code: testCase, | ||
}; | ||
} | ||
|
||
return { ...DEFAULT_TEST_CASE_CONFIG, ...testCase }; | ||
}); | ||
const finalInvalid = invalid.map((testCase) => ({ | ||
...DEFAULT_TEST_CASE_CONFIG, | ||
...testCase, | ||
})); | ||
|
||
super.run(ruleName, rule, { valid: finalValid, invalid: finalInvalid }); | ||
} | ||
} | ||
|
||
export const createRuleTester = ( | ||
parserOptions: Partial<TSESLint.ParserOptions> = {} | ||
): TSESLint.RuleTester => | ||
new TSESLint.RuleTester({ | ||
): TSESLint.RuleTester => { | ||
return new TestingLibraryRuleTester({ | ||
parser: resolve('./node_modules/@typescript-eslint/parser'), | ||
parserOptions: { | ||
ecmaVersion: 2018, | ||
sourceType: 'module', | ||
ecmaFeatures: { | ||
jsx: true, | ||
}, | ||
...parserOptions, | ||
}, | ||
}); | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,6 @@ | ||
{ | ||
"compilerOptions": { | ||
"target": "es5", | ||
"target": "es6", | ||
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. When creating |
||
"module": "commonjs", | ||
"moduleResolution": "node", | ||
"esModuleInterop": true, | ||
|
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.
should the setting be
'testing-library/file-name-pattern'
? Or adding the word pattern somewhere 😄 so the intention is clearerThere 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.
You are absolutely right. I'll rename it in #247 which is the next PR where I already renamed this from
file-name
tofilename
.