diff --git a/README.md b/README.md index 67f525f9..7995b942 100644 --- a/README.md +++ b/README.md @@ -125,27 +125,28 @@ To enable this configuration use the `extends` property in your ## Supported Rules -| Rule | Description | Configurations | Fixable | -| -------------------------------------------------------------------------------- | -------------------------------------------------------------------------- | ----------------------------------------------------------------- | ------------------ | -| [await-async-query](docs/rules/await-async-query.md) | Enforce async queries to have proper `await` | ![dom-badge][] ![angular-badge][] ![react-badge][] ![vue-badge][] | | -| [await-async-utils](docs/rules/await-async-utils.md) | Enforce async utils to be awaited properly | ![dom-badge][] ![angular-badge][] ![react-badge][] ![vue-badge][] | | -| [await-fire-event](docs/rules/await-fire-event.md) | Enforce async fire event methods to be awaited | ![vue-badge][] | | -| [consistent-data-testid](docs/rules/consistent-data-testid.md) | Ensure `data-testid` values match a provided regex. | | | -| [no-await-sync-query](docs/rules/no-await-sync-query.md) | Disallow unnecessary `await` for sync queries | ![dom-badge][] ![angular-badge][] ![react-badge][] ![vue-badge][] | | -| [no-container](docs/rules/no-container.md) | Disallow the use of `container` methods | ![angular-badge][] ![react-badge][] ![vue-badge][] | | -| [no-debug](docs/rules/no-debug.md) | Disallow the use of `debug` | ![angular-badge][] ![react-badge][] ![vue-badge][] | | -| [no-dom-import](docs/rules/no-dom-import.md) | Disallow importing from DOM Testing Library | ![angular-badge][] ![react-badge][] ![vue-badge][] | ![fixable-badge][] | -| [no-manual-cleanup](docs/rules/no-manual-cleanup.md) | Disallow the use of `cleanup` | | | -| [no-multiple-assertions-wait-for](docs/rules/no-multiple-assertions-wait-for.md) | Disallow the use of multiple expect inside `waitFor` | | | -| [no-node-access](docs/rules/no-node-access.md) | Disallow direct Node access | ![angular-badge][] ![react-badge][] ![vue-badge][] | | | -| [no-promise-in-fire-event](docs/rules/no-promise-in-fire-event.md) | Disallow the use of promises passed to a `fireEvent` method | | | -| [no-wait-for-empty-callback](docs/rules/no-wait-for-empty-callback.md) | Disallow empty callbacks for `waitFor` and `waitForElementToBeRemoved` | ![dom-badge][] ![angular-badge][] ![react-badge][] ![vue-badge][] | | -| [prefer-explicit-assert](docs/rules/prefer-explicit-assert.md) | Suggest using explicit assertions rather than just `getBy*` queries | | | -| [prefer-find-by](docs/rules/prefer-find-by.md) | Suggest using `findBy*` methods instead of the `waitFor` + `getBy` queries | ![dom-badge][] ![angular-badge][] ![react-badge][] ![vue-badge][] | ![fixable-badge][] | -| [prefer-presence-queries](docs/rules/prefer-presence-queries.md) | Enforce specific queries when checking element is present or not | | | +| Rule | Description | Configurations | Fixable | +| -------------------------------------------------------------------------------- | ---------------------------------------------------------------------------------------- | ----------------------------------------------------------------- | ------------------ | +| [await-async-query](docs/rules/await-async-query.md) | Enforce async queries to have proper `await` | ![dom-badge][] ![angular-badge][] ![react-badge][] ![vue-badge][] | | +| [await-async-utils](docs/rules/await-async-utils.md) | Enforce async utils to be awaited properly | ![dom-badge][] ![angular-badge][] ![react-badge][] ![vue-badge][] | | +| [await-fire-event](docs/rules/await-fire-event.md) | Enforce async fire event methods to be awaited | ![vue-badge][] | | +| [consistent-data-testid](docs/rules/consistent-data-testid.md) | Ensure `data-testid` values match a provided regex. | | | +| [no-await-sync-query](docs/rules/no-await-sync-query.md) | Disallow unnecessary `await` for sync queries | ![dom-badge][] ![angular-badge][] ![react-badge][] ![vue-badge][] | | +| [no-container](docs/rules/no-container.md) | Disallow the use of `container` methods | ![angular-badge][] ![react-badge][] ![vue-badge][] | | +| [no-debug](docs/rules/no-debug.md) | Disallow the use of `debug` | ![angular-badge][] ![react-badge][] ![vue-badge][] | | +| [no-dom-import](docs/rules/no-dom-import.md) | Disallow importing from DOM Testing Library | ![angular-badge][] ![react-badge][] ![vue-badge][] | ![fixable-badge][] | +| [no-manual-cleanup](docs/rules/no-manual-cleanup.md) | Disallow the use of `cleanup` | | | +| [no-multiple-assertions-wait-for](docs/rules/no-multiple-assertions-wait-for.md) | Disallow the use of multiple expect inside `waitFor` | | | +| [no-node-access](docs/rules/no-node-access.md) | Disallow direct Node access | ![angular-badge][] ![react-badge][] ![vue-badge][] | | +| [no-promise-in-fire-event](docs/rules/no-promise-in-fire-event.md) | Disallow the use of promises passed to a `fireEvent` method | | | +| [no-wait-for-empty-callback](docs/rules/no-wait-for-empty-callback.md) | Disallow empty callbacks for `waitFor` and `waitForElementToBeRemoved` | ![dom-badge][] ![angular-badge][] ![react-badge][] ![vue-badge][] | | +| [prefer-explicit-assert](docs/rules/prefer-explicit-assert.md) | Suggest using explicit assertions rather than just `getBy*` queries | | | +| [prefer-find-by](docs/rules/prefer-find-by.md) | Suggest using `findBy*` methods instead of the `waitFor` + `getBy` queries | ![dom-badge][] ![angular-badge][] ![react-badge][] ![vue-badge][] | ![fixable-badge][] | +| [prefer-presence-queries](docs/rules/prefer-presence-queries.md) | Enforce specific queries when checking element is present or not | | | | [prefer-user-event](docs/rules/prefer-user-event.md) | Suggest using `userEvent` library instead of `fireEvent` for simulating user interaction | ![dom-badge][] ![angular-badge][] ![react-badge][] ![vue-badge][] | | -| [prefer-screen-queries](docs/rules/prefer-screen-queries.md) | Suggest using screen while using queries | ![dom-badge][] ![angular-badge][] ![react-badge][] ![vue-badge][] | | -| [prefer-wait-for](docs/rules/prefer-wait-for.md) | Use `waitFor` instead of deprecated wait methods | | ![fixable-badge][] | +| [prefer-screen-queries](docs/rules/prefer-screen-queries.md) | Suggest using screen while using queries | ![dom-badge][] ![angular-badge][] ![react-badge][] ![vue-badge][] | | +| [prefer-wait-for](docs/rules/prefer-wait-for.md) | Use `waitFor` instead of deprecated wait methods | | ![fixable-badge][] | +| [render-result-naming-convention](docs/rules/render-result-naming-convention.md) | Enforce a valid naming for return value from `render` | ![angular-badge][] ![react-badge][] ![vue-badge][] | | [build-badge]: https://img.shields.io/travis/testing-library/eslint-plugin-testing-library?style=flat-square [build-url]: https://travis-ci.org/testing-library/eslint-plugin-testing-library diff --git a/docs/rules/render-result-naming-convention.md b/docs/rules/render-result-naming-convention.md new file mode 100644 index 00000000..ffd6ec8f --- /dev/null +++ b/docs/rules/render-result-naming-convention.md @@ -0,0 +1,78 @@ +# Enforce a valid naming for return value from `render` (render-result-naming-convention) + +> The name `wrapper` is old cruft from `enzyme` and we don't need that here. The return value from `render` is not "wrapping" anything. It's simply a collection of utilities that you should actually not often need anyway. + +## Rule Details + +This rule aims to ensure the return value from `render` is named properly. + +Ideally, you should destructure the minimum utils that you need from `render`, combined with using queries from [`screen` object](https://github.com/testing-library/eslint-plugin-testing-library/blob/master/docs/rules/prefer-screen-queries.md). In case you need to save the collection of utils returned in a variable, its name should be either `view` or `utils`, as `render` is not wrapping anything: it's just returning a collection of utilities. Every other name for that variable will be considered invalid. + +To sum up these rules, the allowed naming convention for return value from `render` is: + +- destructuring +- `view` +- `utils` + +Examples of **incorrect** code for this rule: + +```javascript +import { render } from '@testing-library/framework'; + +// ... + +// return value from `render` shouldn't be kept in a var called "wrapper" +const wrapper = render(); +``` + +```javascript +import { render } from '@testing-library/framework'; + +// ... + +// return value from `render` shouldn't be kept in a var called "component" +const component = render(); +``` + +```javascript +import { render } from '@testing-library/framework'; + +// ... + +// to sum up: return value from `render` shouldn't be kept in a var called other than "view" or "utils" +const somethingElse = render(); +``` + +Examples of **correct** code for this rule: + +```javascript +import { render } from '@testing-library/framework'; + +// ... + +// destructuring return value from `render` is correct +const { unmount, rerender } = render(); +``` + +```javascript +import { render } from '@testing-library/framework'; + +// ... + +// keeping return value from `render` in a var called "view" is correct +const view = render(); +``` + +```javascript +import { render } from '@testing-library/framework'; + +// ... + +// keeping return value from `render` in a var called "utils" is correct +const utils = render(); +``` + +## Further Reading + +- [Common Mistakes with React Testing Library](https://kentcdodds.com/blog/common-mistakes-with-react-testing-library#using-wrapper-as-the-variable-name-for-the-return-value-from-render) +- [`render` Result](https://testing-library.com/docs/react-testing-library/api#render-result) diff --git a/lib/index.ts b/lib/index.ts index 81390a91..d8a64006 100644 --- a/lib/index.ts +++ b/lib/index.ts @@ -15,8 +15,9 @@ import preferPresenceQueries from './rules/prefer-presence-queries'; import preferScreenQueries from './rules/prefer-screen-queries'; import preferUserEvent from './rules/prefer-user-event'; import preferWaitFor from './rules/prefer-wait-for'; -import noMultipleAssertionsWaitFor from './rules/no-multiple-assertions-wait-for' +import noMultipleAssertionsWaitFor from './rules/no-multiple-assertions-wait-for'; import preferFindBy from './rules/prefer-find-by'; +import renderResultNamingConvention from './rules/render-result-naming-convention'; const rules = { 'await-async-query': awaitAsyncQuery, @@ -38,6 +39,7 @@ const rules = { 'prefer-screen-queries': preferScreenQueries, 'prefer-user-event': preferUserEvent, 'prefer-wait-for': preferWaitFor, + 'render-result-naming-convention': renderResultNamingConvention, }; const domRules = { @@ -57,6 +59,7 @@ const angularRules = { 'testing-library/no-debug': 'warn', 'testing-library/no-dom-import': ['error', 'angular'], 'testing-library/no-node-access': 'error', + 'testing-library/render-result-naming-convention': 'error', }; const reactRules = { @@ -65,6 +68,7 @@ const reactRules = { 'testing-library/no-debug': 'warn', 'testing-library/no-dom-import': ['error', 'react'], 'testing-library/no-node-access': 'error', + 'testing-library/render-result-naming-convention': 'error', }; const vueRules = { @@ -74,6 +78,7 @@ const vueRules = { 'testing-library/no-debug': 'warn', 'testing-library/no-dom-import': ['error', 'vue'], 'testing-library/no-node-access': 'error', + 'testing-library/render-result-naming-convention': 'error', }; export = { diff --git a/lib/rules/render-result-naming-convention.ts b/lib/rules/render-result-naming-convention.ts new file mode 100644 index 00000000..5d63011f --- /dev/null +++ b/lib/rules/render-result-naming-convention.ts @@ -0,0 +1,145 @@ +import { ESLintUtils, TSESTree } from '@typescript-eslint/experimental-utils'; +import { getDocsUrl, hasTestingLibraryImportModule } from '../utils'; +import { + isCallExpression, + isIdentifier, + isImportSpecifier, + isMemberExpression, + isObjectPattern, + isRenderVariableDeclarator, +} from '../node-utils'; + +export const RULE_NAME = 'render-result-naming-convention'; + +const ALLOWED_VAR_NAMES = ['view', 'utils']; +const ALLOWED_VAR_NAMES_TEXT = ALLOWED_VAR_NAMES.map( + name => '`' + name + '`' +).join(', '); + +export default ESLintUtils.RuleCreator(getDocsUrl)({ + name: RULE_NAME, + meta: { + type: 'suggestion', + docs: { + description: 'Enforce a valid naming for return value from `render`', + category: 'Best Practices', + recommended: false, + }, + messages: { + invalidRenderResultName: `\`{{ varName }}\` is not a recommended name for \`render\` returned value. Instead, you should destructure it, or call it using one of the valid choices: ${ALLOWED_VAR_NAMES_TEXT}`, + }, + fixable: null, + schema: [ + { + type: 'object', + properties: { + renderFunctions: { + type: 'array', + }, + }, + }, + ], + }, + defaultOptions: [ + { + renderFunctions: [], + }, + ], + + create(context, [options]) { + const { renderFunctions } = options; + let renderAlias: string | undefined; + let wildcardImportName: string | undefined; + + return { + // check named imports + ImportDeclaration(node: TSESTree.ImportDeclaration) { + if (!hasTestingLibraryImportModule(node)) { + return; + } + const renderImport = node.specifiers.find( + node => isImportSpecifier(node) && node.imported.name === 'render' + ); + + if (!renderImport) { + return; + } + + renderAlias = renderImport.local.name; + }, + // check wildcard imports + 'ImportDeclaration ImportNamespaceSpecifier'( + node: TSESTree.ImportNamespaceSpecifier + ) { + if ( + !hasTestingLibraryImportModule( + node.parent as TSESTree.ImportDeclaration + ) + ) { + return; + } + + wildcardImportName = node.local.name; + }, + VariableDeclarator(node: TSESTree.VariableDeclarator) { + // check if destructuring return value from render + if (isObjectPattern(node.id)) { + return; + } + + const isValidRenderDeclarator = isRenderVariableDeclarator(node, [ + ...renderFunctions, + renderAlias, + ]); + const isValidWildcardImport = !!wildcardImportName; + + // check if is a Testing Library related import + if (!isValidRenderDeclarator && !isValidWildcardImport) { + return; + } + + const renderFunctionName = + isCallExpression(node.init) && + isIdentifier(node.init.callee) && + node.init.callee.name; + + const renderFunctionObjectName = + isCallExpression(node.init) && + isMemberExpression(node.init.callee) && + isIdentifier(node.init.callee.property) && + isIdentifier(node.init.callee.object) && + node.init.callee.property.name === 'render' && + node.init.callee.object.name; + + const isRenderAlias = !!renderAlias; + const isCustomRender = renderFunctions.includes(renderFunctionName); + const isWildCardRender = + renderFunctionObjectName && + renderFunctionObjectName === wildcardImportName; + + // check if is a qualified render function + if (!isRenderAlias && !isCustomRender && !isWildCardRender) { + return; + } + + const renderResultName = isIdentifier(node.id) && node.id.name; + const isAllowedRenderResultName = ALLOWED_VAR_NAMES.includes( + renderResultName + ); + + // check if return value var name is allowed + if (isAllowedRenderResultName) { + return; + } + + context.report({ + node, + messageId: 'invalidRenderResultName', + data: { + varName: renderResultName, + }, + }); + }, + }; + }, +}); diff --git a/lib/utils.ts b/lib/utils.ts index be015f6b..7232f306 100644 --- a/lib/utils.ts +++ b/lib/utils.ts @@ -25,8 +25,8 @@ const LIBRARY_MODULES = [ ]; const hasTestingLibraryImportModule = (node: TSESTree.ImportDeclaration) => { - return LIBRARY_MODULES.includes(node.source.value.toString()) -} + return LIBRARY_MODULES.includes(node.source.value.toString()); +}; const SYNC_QUERIES_VARIANTS = ['getBy', 'getAllBy', 'queryBy', 'queryAllBy']; const ASYNC_QUERIES_VARIANTS = ['findBy', 'findAllBy']; diff --git a/tests/__snapshots__/index.test.ts.snap b/tests/__snapshots__/index.test.ts.snap index da608e52..1f425687 100644 --- a/tests/__snapshots__/index.test.ts.snap +++ b/tests/__snapshots__/index.test.ts.snap @@ -21,6 +21,7 @@ Object { "testing-library/prefer-find-by": "error", "testing-library/prefer-screen-queries": "error", "testing-library/prefer-user-event": "warn", + "testing-library/render-result-naming-convention": "error", }, } `; @@ -64,6 +65,7 @@ Object { "testing-library/prefer-find-by": "error", "testing-library/prefer-screen-queries": "error", "testing-library/prefer-user-event": "warn", + "testing-library/render-result-naming-convention": "error", }, } `; @@ -90,6 +92,7 @@ Object { "testing-library/prefer-find-by": "error", "testing-library/prefer-screen-queries": "error", "testing-library/prefer-user-event": "warn", + "testing-library/render-result-naming-convention": "error", }, } `; diff --git a/tests/lib/rules/render-result-naming-convention.test.ts b/tests/lib/rules/render-result-naming-convention.test.ts new file mode 100644 index 00000000..f21959ff --- /dev/null +++ b/tests/lib/rules/render-result-naming-convention.test.ts @@ -0,0 +1,347 @@ +import { createRuleTester } from '../test-utils'; +import rule, { + RULE_NAME, +} from '../../../lib/rules/render-result-naming-convention'; + +const ruleTester = createRuleTester({ + ecmaFeatures: { + jsx: true, + }, +}); + +ruleTester.run(RULE_NAME, rule, { + valid: [ + { + code: ` + import { render } from '@testing-library/react'; + + test('should not report straight destructured render result', () => { + const { rerender, getByText } = render(); + const button = getByText('some button'); + }); + `, + }, + { + code: ` + import * as RTL from '@testing-library/react'; + + test('should not report straight destructured render result from wildcard import', () => { + const { rerender, getByText } = RTL.render(); + const button = getByText('some button'); + }); + `, + }, + { + code: ` + import { render } from '@testing-library/react'; + + test('should not report straight render result called "utils"', async () => { + const utils = render(); + await utils.findByRole('button'); + }); + `, + }, + { + code: ` + import { render } from '@testing-library/react'; + + test('should not report straight render result called "view"', async () => { + const view = render(); + await view.findByRole('button'); + }); + `, + }, + { + code: ` + import { render } from '@testing-library/react'; + + const setup = () => render(); + + test('should not report destructured render result from wrapping function', () => { + const { rerender, getByText } = setup(); + const button = getByText('some button'); + }); + `, + }, + { + code: ` + import { render } from '@testing-library/react'; + + const setup = () => render(); + + test('should not report render result called "utils" from wrapping function', async () => { + const utils = setup(); + await utils.findByRole('button'); + }); + `, + }, + { + code: ` + import { render } from '@testing-library/react'; + + const setup = () => render(); + + test('should not report render result called "view" from wrapping function', async () => { + const view = setup(); + await view.findByRole('button'); + }); + `, + }, + { + code: ` + import { screen } from '@testing-library/react'; + import { customRender } from 'test-utils'; + + test('should not report straight destructured render result from custom render', () => { + const { unmount } = customRender(); + const button = screen.getByText('some button'); + }); + `, + options: [ + { + renderFunctions: ['customRender'], + }, + ], + }, + { + code: ` + import { customRender } from 'test-utils'; + + test('should not report render result called "view" from custom render', async () => { + const view = customRender(); + await view.findByRole('button'); + }); + `, + options: [ + { + renderFunctions: ['customRender'], + }, + ], + }, + { + code: ` + import { customRender } from 'test-utils'; + + test('should not report render result called "utils" from custom render', async () => { + const utils = customRender(); + await utils.findByRole('button'); + }); + `, + options: [ + { + renderFunctions: ['customRender'], + }, + ], + }, + { + code: ` + import { render } from '@foo/bar'; + + test('should not report from render not related to testing library', () => { + const wrapper = render(); + const button = wrapper.getByText('some button'); + }); + `, + }, + { + code: ` + import { render } from '@foo/bar'; + + test('should not report from render not imported from testing library', () => { + const wrapper = render(); + const button = wrapper.getByText('some button'); + }); + `, + }, + { + code: ` + import * as RTL from '@foo/bar'; + + test('should not report from wildcard render not imported from testing library', () => { + const wrapper = RTL.render(); + const button = wrapper.getByText('some button'); + }); + `, + }, + { + code: ` + function render() { + return 'whatever'; + } + + test('should not report from custom render not related to testing library', () => { + const wrapper = render(); + const button = wrapper.getByText('some button'); + }); + `, + }, + { + code: ` + import { render } from '@testing-library/react'; + + const setup = () => { + // this one must have a valid name + const view = render(); + return view; + }; + + test('should not report render result called "view" from wrapping function', async () => { + // this isn't a render technically so it can be called "wrapper" + const wrapper = setup(); + await wrapper.findByRole('button'); + }); + `, + }, + ], + invalid: [ + { + code: ` + import { render } from '@testing-library/react'; + + test('should report straight render result called "wrapper"', async () => { + const wrapper = render(); + await wrapper.findByRole('button'); + }); + `, + errors: [ + { + messageId: 'invalidRenderResultName', + data: { + varName: 'wrapper', + }, + line: 5, + column: 17, + }, + ], + }, + { + code: ` + import * as RTL from '@testing-library/react'; + + test('should report straight render result called "wrapper" from wildcard import', () => { + const wrapper = RTL.render(); + const button = wrapper.getByText('some button'); + }); + `, + errors: [ + { + messageId: 'invalidRenderResultName', + data: { + varName: 'wrapper', + }, + line: 5, + column: 17, + }, + ], + }, + { + code: ` + import { render } from '@testing-library/react'; + + test('should report straight render result called "component"', async () => { + const component = render(); + await component.findByRole('button'); + }); + `, + errors: [ + { + messageId: 'invalidRenderResultName', + data: { + varName: 'component', + }, + line: 5, + column: 17, + }, + ], + }, + { + code: ` + import { render } from '@testing-library/react'; + + test('should report straight render result called "notValidName"', async () => { + const notValidName = render(); + await notValidName.findByRole('button'); + }); + `, + errors: [ + { + messageId: 'invalidRenderResultName', + line: 5, + column: 17, + }, + ], + }, + { + code: ` + import { render as testingLibraryRender } from '@testing-library/react'; + + test('should report renamed render result called "wrapper"', async () => { + const wrapper = testingLibraryRender(); + await wrapper.findByRole('button'); + }); + `, + errors: [ + { + messageId: 'invalidRenderResultName', + data: { + varName: 'wrapper', + }, + line: 5, + column: 17, + }, + ], + }, + { + code: ` + import { render } from '@testing-library/react'; + + const setup = () => { + // this one must have a valid name + const wrapper = render(); + return wrapper; + }; + + test('should report render result called "wrapper" from wrapping function', async () => { + // this isn't a render technically so it can be called "wrapper" + const wrapper = setup(); + await wrapper.findByRole('button'); + }); + `, + errors: [ + { + messageId: 'invalidRenderResultName', + data: { + varName: 'wrapper', + }, + line: 6, + column: 17, + }, + ], + }, + { + code: ` + import { customRender } from 'test-utils'; + + test('should report from custom render function ', () => { + const wrapper = customRender(); + const button = wrapper.getByText('some button'); + }); + `, + options: [ + { + renderFunctions: ['customRender'], + }, + ], + errors: [ + { + messageId: 'invalidRenderResultName', + data: { + varName: 'wrapper', + }, + line: 5, + column: 17, + }, + ], + }, + ], +});