feat: add no-node-access rule (#190)

* refactor(utils): add properties and methods that returns another Node * test(no-node-access): add first scenarios * feat(no-node-access): add rule with few test cases * test(no-node-access): add scenarios * refactor(no-node-access): simplify conditions * refactor(no-node-access): add scenario when no variable is declared * refactor(no-node-access): remove conditional * refactor(utils): add DOM properties * refactor(no-node-access): add scenario for accessing document directly * docs(no-node-access): add readme * refactor(utils): export const containing all properties and methods that return a Node * docs(no-node-access): fix file location * docs(readme): add no-node-access * refactor(no-node-access): change highlight location * docs(no-node-access): fix typo * refactor(utils): add missing property that returns a Node * refactor(no-node-access): simplify checks triggering error for all methods with names matching the forbidden ones * test(no-node-access): add more scenarios with destructuring * docs(no-node-access): update examples * refactor(no-node-access): narrow error cases * refactor(no-node-access): check imports to validate whether is importing a testing-library package | update examples and testing scenarios * refactor(no-node-access): rename variable
testing-library · Jul 15, 2020 · 9d4c1e4 · 9d4c1e4
1 parent b2ef721
commit 9d4c1e4
Show file tree

Hide file tree

Showing 8 changed files with 423 additions and 5 deletions.
diff --git a/README.md b/README.md
@@ -137,6 +137,7 @@ To enable this configuration use the `extends` property in your
 | [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        |                                                                   |                    |

diff --git a/docs/rules/no-multiple-assertions-wait-for.md b/docs/rules/no-multiple-assertions-wait-for.md
@@ -18,9 +18,9 @@ const foo = async () => {
 
   // or
   await waitFor(function() {
-    expect(a).toEqual('a')
+    expect(a).toEqual('a');
     expect(b).toEqual('b');
-  })
+  });
 };
 ```
 
@@ -30,11 +30,11 @@ Examples of **correct** code for this rule:
 const foo = async () => {
   await waitFor(() => expect(a).toEqual('a'));
   expect(b).toEqual('b');
-  
+
   // or
   await waitFor(function() {
-    expect(a).toEqual('a')
-  })
+    expect(a).toEqual('a');
+  });
   expect(b).toEqual('b');
 
   // it only detects expect

diff --git a/docs/rules/no-node-access.md b/docs/rules/no-node-access.md
@@ -0,0 +1,60 @@
+# Disallow direct Node access (no-node-access)
+
+The Testing Library already provides methods for querying DOM elements.
+
+## Rule Details
+
+This rule aims to disallow DOM traversal using native HTML methods and properties, such as `closest`, `lastChild` and all that returns another Node element from an HTML tree.
+
+Examples of **incorrect** code for this rule:
+
+```js
+import { screen } from '@testing-library/react';
+
+screen.getByText('Submit').closest('button'); // chaining with Testing Library methods
+```
+
+```js
+import { screen } from '@testing-library/react';
+
+const buttons = screen.getAllByRole('button');
+expect(buttons[1].lastChild).toBeInTheDocument();
+```
+
+```js
+import { screen } from '@testing-library/react';
+
+const buttonText = screen.getByText('Submit');
+const button = buttonText.closest('button');
+```
+
+Examples of **correct** code for this rule:
+
+```js
+import { screen } from '@testing-library/react';
+
+const button = screen.getByRole('button');
+expect(button).toHaveTextContent('submit');
+```
+
+```js
+import { render, within } from '@testing-library/react';
+
+const { getByLabelText } = render(<MyComponent />);
+const signinModal = getByLabelText('Sign In');
+within(signinModal).getByPlaceholderText('Username');
+```
+
+```js
+// If is not importing a testing-library package
+
+document.getElementById('submit-btn').closest('button');
+```
+
+## Further Reading
+
+### Properties / methods that return another Node
+
+- [`Document`](https://developer.mozilla.org/en-US/docs/Web/API/Document)
+- [`Element`](https://developer.mozilla.org/en-US/docs/Web/API/Element)
+- [`Node`](https://developer.mozilla.org/en-US/docs/Web/API/Node)
diff --git a/lib/index.ts b/lib/index.ts
@@ -7,6 +7,7 @@ import noContainer from './rules/no-container';
 import noDebug from './rules/no-debug';
 import noDomImport from './rules/no-dom-import';
 import noManualCleanup from './rules/no-manual-cleanup';
+import noNodeAccess from './rules/no-node-access';
 import noWaitForEmptyCallback from './rules/no-wait-for-empty-callback';
 import noPromiseInFireEvent from './rules/no-promise-in-fire-event';
 import preferExplicitAssert from './rules/prefer-explicit-assert';
@@ -27,6 +28,7 @@ const rules = {
   'no-dom-import': noDomImport,
   'no-manual-cleanup': noManualCleanup,
   'no-multiple-assertions-wait-for': noMultipleAssertionsWaitFor,
+  'no-node-access': noNodeAccess,
   'no-promise-in-fire-event': noPromiseInFireEvent,
   'no-wait-for-empty-callback': noWaitForEmptyCallback,
   'prefer-explicit-assert': preferExplicitAssert,
@@ -51,13 +53,15 @@ const angularRules = {
   'testing-library/no-container': 'error',
   'testing-library/no-debug': 'warn',
   'testing-library/no-dom-import': ['error', 'angular'],
+  'testing-library/no-node-access': 'error',
 };
 
 const reactRules = {
   ...domRules,
   'testing-library/no-container': 'error',
   'testing-library/no-debug': 'warn',
   'testing-library/no-dom-import': ['error', 'react'],
+  'testing-library/no-node-access': 'error',
 };
 
 const vueRules = {
@@ -66,6 +70,7 @@ const vueRules = {
   'testing-library/no-container': 'error',
   'testing-library/no-debug': 'warn',
   'testing-library/no-dom-import': ['error', 'vue'],
+  'testing-library/no-node-access': 'error',
 };
 
 export = {

diff --git a/lib/rules/no-node-access.ts b/lib/rules/no-node-access.ts
@@ -0,0 +1,49 @@
+import { ESLintUtils, TSESTree } from '@typescript-eslint/experimental-utils';
+import { getDocsUrl, ALL_RETURNING_NODES } from '../utils';
+import { isIdentifier } from '../node-utils';
+
+export const RULE_NAME = 'no-node-access';
+
+export default ESLintUtils.RuleCreator(getDocsUrl)({
+  name: RULE_NAME,
+  meta: {
+    type: 'problem',
+    docs: {
+      description: 'Disallow direct Node access',
+      category: 'Best Practices',
+      recommended: 'error',
+    },
+    messages: {
+      noNodeAccess:
+        'Avoid direct Node access. Prefer using the methods from Testing Library.',
+    },
+    fixable: null,
+    schema: [],
+  },
+  defaultOptions: [],
+
+  create(context) {
+    let isImportingTestingLibrary = false;
+
+    function checkTestingEnvironment(node: TSESTree.ImportDeclaration) {
+      isImportingTestingLibrary = /testing-library/g.test(node.source.value as string);
+    }
+
+    function showErrorForNodeAccess(node: TSESTree.MemberExpression) {
+      isIdentifier(node.property) &&
+        ALL_RETURNING_NODES.includes(node.property.name) &&
+        isImportingTestingLibrary &&
+        context.report({
+          node: node,
+          loc: node.property.loc.start,
+          messageId: 'noNodeAccess',
+        });
+    }
+
+    return {
+      ['ImportDeclaration']: checkTestingEnvironment,
+      ['ExpressionStatement MemberExpression']: showErrorForNodeAccess,
+      ['VariableDeclarator MemberExpression']: showErrorForNodeAccess,
+    };
+  },
+});
diff --git a/lib/utils.ts b/lib/utils.ts
@@ -63,6 +63,41 @@ const ASYNC_UTILS = [
   'waitForDomChange',
 ];
 
+const PROPERTIES_RETURNING_NODES = [
+  'activeElement',
+  'children',
+  'firstChild',
+  'firstElementChild',
+  'fullscreenElement',
+  'lastChild',
+  'lastElementChild',
+  'nextElementSibling',
+  'nextSibling',
+  'parentElement',
+  'parentNode',
+  'pointerLockElement',
+  'previousElementSibling',
+  'previousSibling',
+  'rootNode',
+  'scripts',
+];
+
+const METHODS_RETURNING_NODES = [
+  'closest',
+  'getElementById',
+  'getElementsByClassName',
+  'getElementsByName',
+  'getElementsByTagName',
+  'getElementsByTagNameNS',
+  'querySelector',
+  'querySelectorAll',
+];
+
+const ALL_RETURNING_NODES = [
+  ...PROPERTIES_RETURNING_NODES,
+  ...METHODS_RETURNING_NODES,
+];
+
 export {
   getDocsUrl,
   SYNC_QUERIES_VARIANTS,
@@ -74,4 +109,7 @@ export {
   ALL_QUERIES_COMBINATIONS,
   ASYNC_UTILS,
   LIBRARY_MODULES,
+  PROPERTIES_RETURNING_NODES,
+  METHODS_RETURNING_NODES,
+  ALL_RETURNING_NODES,
 };
diff --git a/tests/__snapshots__/index.test.ts.snap b/tests/__snapshots__/index.test.ts.snap
@@ -15,6 +15,7 @@ Object {
       "error",
       "angular",
     ],
+    "testing-library/no-node-access": "error",
     "testing-library/no-promise-in-fire-event": "error",
     "testing-library/no-wait-for-empty-callback": "error",
     "testing-library/prefer-find-by": "error",
@@ -55,6 +56,7 @@ Object {
       "error",
       "react",
     ],
+    "testing-library/no-node-access": "error",
     "testing-library/no-promise-in-fire-event": "error",
     "testing-library/no-wait-for-empty-callback": "error",
     "testing-library/prefer-find-by": "error",
@@ -79,6 +81,7 @@ Object {
       "error",
       "vue",
     ],
+    "testing-library/no-node-access": "error",
     "testing-library/no-promise-in-fire-event": "error",
     "testing-library/no-wait-for-empty-callback": "error",
     "testing-library/prefer-find-by": "error",