Skip to content
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

Move rules settings to ESLint shared config: part 3 - check reported file name #244

Merged
merged 3 commits into from
Nov 1, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
19 changes: 18 additions & 1 deletion lib/detect-testing-library-utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import { TSESLint, TSESTree } from '@typescript-eslint/experimental-utils';

export type TestingLibrarySettings = {
'testing-library/module'?: string;
'testing-library/file-name'?: string;
};

export type TestingLibraryContext<
Expand All @@ -25,9 +26,12 @@ export type EnhancedRuleCreate<

export type DetectionHelpers = {
getIsTestingLibraryImported: () => boolean;
getIsValidFileName: () => boolean;
canReportErrors: () => boolean;
};

const DEFAULT_FILE_NAME_PATTERN = '^.*\\.(test|spec)\\.[jt]sx?$';

/**
* Enhances a given rule `create` with helpers to detect Testing Library utils.
*/
Expand All @@ -45,6 +49,9 @@ export function detectTestingLibraryUtils<

// Init options based on shared ESLint settings
const customModule = context.settings['testing-library/module'];
const fileNamePattern =
context.settings['testing-library/file-name'] ??
Copy link
Collaborator

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 clearer

Copy link
Member Author

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 to filename.

DEFAULT_FILE_NAME_PATTERN;

// Helpers for Testing Library detection.
const helpers: DetectionHelpers = {
Expand All @@ -68,11 +75,21 @@ export function detectTestingLibraryUtils<
return isImportingTestingLibraryModule || isImportingCustomModule;
},

/**
* Gets if name of the file being analyzed is valid or not.
*
* This is based on "testing-library/file-name" setting.
*/
getIsValidFileName() {
const fileName = context.getFilename();
return !!fileName.match(fileNamePattern);
},

/**
* Wraps all conditions that must be met to report rules.
*/
canReportErrors() {
return this.getIsTestingLibraryImported();
return this.getIsTestingLibraryImported() && this.getIsValidFileName();
},
};

Expand Down
34 changes: 29 additions & 5 deletions tests/create-testing-library-rule.test.ts
Original file line number Diff line number Diff line change
@@ -1,11 +1,7 @@
import { createRuleTester } from './lib/test-utils';
import rule, { RULE_NAME } from './fake-rule';

const ruleTester = createRuleTester({
ecmaFeatures: {
jsx: true,
},
});
const ruleTester = createRuleTester();

ruleTester.run(RULE_NAME, rule, {
valid: [
Expand Down Expand Up @@ -38,6 +34,15 @@ ruleTester.run(RULE_NAME, rule, {
'testing-library/module': 'test-utils',
},
},
{
code: `
// case: import module forced to be reported but not matching file name
import { foo } from 'report-me'
`,
settings: {
'testing-library/file-name': 'testing-library\\.js',
},
},
],
invalid: [
{
Expand All @@ -47,6 +52,25 @@ ruleTester.run(RULE_NAME, rule, {
`,
errors: [{ line: 3, column: 7, messageId: 'fakeError' }],
},
{
filename: 'MyComponent.spec.js',
code: `
// case: import module forced to be reported but from .spec.js named file
import { foo } from 'report-me'
`,
errors: [{ line: 3, column: 7, messageId: 'fakeError' }],
},
{
filename: 'MyComponent.testing-library.js',
code: `
// case: import module forced to be reported with custom file name
import { foo } from 'report-me'
`,
settings: {
'testing-library/file-name': 'testing-library\\.js',
},
errors: [{ line: 3, column: 7, messageId: 'fakeError' }],
},
{
code: `
// case: render imported from any module by default (aggressive reporting)
Expand Down
6 changes: 1 addition & 5 deletions tests/lib/rules/consistent-data-testid.test.ts
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: {
Copy link
Member Author

Choose a reason for hiding this comment

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

We don't need this anymore, set by default on createRuleTester.

jsx: true,
},
});
const ruleTester = createRuleTester();

ruleTester.run(RULE_NAME, rule, {
valid: [
Expand Down
6 changes: 1 addition & 5 deletions tests/lib/rules/no-container.test.ts
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-container';

const ruleTester = createRuleTester({
ecmaFeatures: {
jsx: true,
},
});
const ruleTester = createRuleTester();

ruleTester.run(RULE_NAME, rule, {
valid: [
Expand Down
6 changes: 1 addition & 5 deletions tests/lib/rules/no-debug.test.ts
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-debug';

const ruleTester = createRuleTester({
ecmaFeatures: {
jsx: true,
},
});
const ruleTester = createRuleTester();

ruleTester.run(RULE_NAME, rule, {
valid: [
Expand Down
6 changes: 1 addition & 5 deletions tests/lib/rules/no-multiple-assertions-wait-for.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,7 @@ import rule, {
RULE_NAME,
} from '../../../lib/rules/no-multiple-assertions-wait-for';

const ruleTester = createRuleTester({
ecmaFeatures: {
jsx: true,
},
});
const ruleTester = createRuleTester();

ruleTester.run(RULE_NAME, rule, {
valid: [
Expand Down
18 changes: 6 additions & 12 deletions tests/lib/rules/no-node-access.test.ts
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: [
Expand Down Expand Up @@ -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
Copy link
Member Author

Choose a reason for hiding this comment

The 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
Expand Down
6 changes: 1 addition & 5 deletions tests/lib/rules/no-render-in-setup.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,7 @@ import { createRuleTester } from '../test-utils';
import { TESTING_FRAMEWORK_SETUP_HOOKS } from '../../../lib/utils';
import rule, { RULE_NAME } from '../../../lib/rules/no-render-in-setup';

const ruleTester = createRuleTester({
ecmaFeatures: {
jsx: true,
},
});
const ruleTester = createRuleTester();

ruleTester.run(RULE_NAME, rule, {
valid: [
Expand Down
6 changes: 1 addition & 5 deletions tests/lib/rules/no-side-effects-wait-for.test.ts
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-side-effects-wait-for';

const ruleTester = createRuleTester({
ecmaFeatures: {
jsx: true,
},
});
const ruleTester = createRuleTester();

ruleTester.run(RULE_NAME, rule, {
valid: [
Expand Down
6 changes: 1 addition & 5 deletions tests/lib/rules/prefer-find-by.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,11 +14,7 @@ import rule, {
MessageIds,
} from '../../../lib/rules/prefer-find-by';

const ruleTester = createRuleTester({
ecmaFeatures: {
jsx: true,
},
});
const ruleTester = createRuleTester();

function buildFindByMethod(queryMethod: string) {
return `${getFindByQueryVariant(queryMethod)}${queryMethod.split('By')[1]}`;
Expand Down
6 changes: 1 addition & 5 deletions tests/lib/rules/render-result-naming-convention.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,7 @@ import rule, {
RULE_NAME,
} from '../../../lib/rules/render-result-naming-convention';

const ruleTester = createRuleTester({
ecmaFeatures: {
jsx: true,
},
});
const ruleTester = createRuleTester();

ruleTester.run(RULE_NAME, rule, {
valid: [
Expand Down
39 changes: 37 additions & 2 deletions tests/lib/test-utils.ts
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 {
Copy link
Member Author

Choose a reason for hiding this comment

The 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 filename config to a valid value to be reported.

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,
},
});
};
2 changes: 1 addition & 1 deletion tsconfig.json
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"compilerOptions": {
"target": "es5",
"target": "es6",
Copy link
Member Author

Choose a reason for hiding this comment

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

When creating TestingLibraryRuleTester I ran into this issue so I had to update the target to "es6".

"module": "commonjs",
"moduleResolution": "node",
"esModuleInterop": true,
Expand Down