From b174bff13465bd994f6ee9900fb9d552e5f0a552 Mon Sep 17 00:00:00 2001 From: Alexander Afanasyev Date: Mon, 26 Jun 2017 12:10:30 -0400 Subject: [PATCH] feat(rules): add 'limit-selector-depth' rule --- README.md | 3 + docs/rules/limit-selector-depth.md | 41 ++++++++++++++ index.js | 5 +- lib/get-selector-depth.js | 24 ++++++++ lib/rules/limit-selector-depth.js | 41 ++++++++++++++ test/rules/limit-selector-depth.js | 89 ++++++++++++++++++++++++++++++ 6 files changed, 202 insertions(+), 1 deletion(-) create mode 100644 docs/rules/limit-selector-depth.md create mode 100644 lib/get-selector-depth.js create mode 100644 lib/rules/limit-selector-depth.js create mode 100644 test/rules/limit-selector-depth.js diff --git a/README.md b/README.md index 77c86e5..3c835b9 100644 --- a/README.md +++ b/README.md @@ -77,6 +77,7 @@ There are various types of rules implemented in the plugin. Here is a rough cate * [no-repetitive-selectors][]: Discourage repeating parts of CSS selectors * [valid-by-id][]: Prohibit use of invalid ID value when using `by.id()` locator * [valid-by-tagname][]: Prohibit use of invalid Html Tag Name value when using `by.tagName()` locator +* [limit-selector-depth][]: Warn about potentially "deep" CSS selectors with too many nodes in the path #### Style Guide Recommendations and Best Practices @@ -132,6 +133,7 @@ Rule | Default Error Level | Auto-fixable | Options [use-count-method][] | 1 | | [valid-by-id][] | 1 | | [valid-by-tagname][] | 1 | | +[limit-selector-depth][] | 1 | | number of nodes (default 5) [use-promise-all][] | 0 (Turned off) | | [by-css-shortcut][] | 0 | | [no-browser-driver][] | 0 | | @@ -182,6 +184,7 @@ See [configuring rules][] for more information. [no-array-finder-methods]: docs/rules/no-array-finder-methods.md [valid-locator-type]: docs/rules/valid-locator-type.md [no-compound-classes]: docs/rules/no-compound-classes.md +[limit-selector-depth]: docs/rules/limit-selector-depth.md [use-count-method]: docs/rules/use-count-method.md [no-browser-driver]: docs/rules/no-browser-driver.md [valid-by-id]: docs/rules/valid-by-id.md diff --git a/docs/rules/limit-selector-depth.md b/docs/rules/limit-selector-depth.md new file mode 100644 index 0000000..08650f2 --- /dev/null +++ b/docs/rules/limit-selector-depth.md @@ -0,0 +1,41 @@ +# Warn about potentially complex selectors with too many nodes + +This rule would warn if a CSS selector is too "deep" - has a significant number of nodes in the path. + +For instance, this CSS selector would be considered fragile and complex, because of 6 nodes in the path: + +```css +.content > table > tbody > tr:nth-child(2) > td.cell > input#email +``` + +A simpler version in this case could be (2 nodes): + +```css +.content input#email +``` + +The idea behind this rule is that the more nodes you have in your CSS selector path - the more fragile, the more dependant on the HTML structure of the page it is. + +Number of nodes is configurable, default value is `5`. + +## Rule details + +This rule is based on [`css-selector-parser`](https://github.com/mdevils/node-css-selector-parser) parsing abilities. + +Any use of the following patterns are considered warnings (with the default depth value of `5`): + +```js +element(by.css(".content > table > tbody > tr:nth-child(2) > td.cell > input#email")); +element.all(by.css(".content > table > tbody > tr:nth-child(2) > td.cell > input#email")); +$(".content > table > tbody > tr:nth-child(2) > td.cell > input#email"); +$$(".content > table > tbody > tr:nth-child(2) > td.cell > input#email"); +``` + +The following patterns are not warnings: + +```js +element(by.css(".myclass")); +element.all(by.css(".content input#email")); +$("#email"); +$$("tr:nth-child(2)"); +``` diff --git a/index.js b/index.js index d01c881..0ddf42b 100644 --- a/index.js +++ b/index.js @@ -35,6 +35,7 @@ var validById = require('./lib/rules/valid-by-id') var validByTagName = require('./lib/rules/valid-by-tagname') var noGetRawId = require('./lib/rules/no-get-raw-id') var noGetLocationAbsUrl = require('./lib/rules/no-get-location-abs-url') +var limitSelectorDepth = require('./lib/rules/limit-selector-depth') module.exports = { rules: { @@ -72,7 +73,8 @@ module.exports = { 'valid-by-id': validById, 'valid-by-tagname': validByTagName, 'no-get-raw-id': noGetRawId, - 'no-get-location-abs-url': noGetLocationAbsUrl + 'no-get-location-abs-url': noGetLocationAbsUrl, + 'limit-selector-depth': limitSelectorDepth }, configs: { recommended: { @@ -108,6 +110,7 @@ module.exports = { 'protractor/use-count-method': 1, 'protractor/valid-by-id': 1, 'protractor/valid-by-tagname': 1, + 'protractor/limit-selector-depth': 1, 'protractor/no-get-location-abs-url': 1, 'protractor/use-promise-all': 0, 'protractor/by-css-shortcut': 0, diff --git a/lib/get-selector-depth.js b/lib/get-selector-depth.js new file mode 100644 index 0000000..e377cc2 --- /dev/null +++ b/lib/get-selector-depth.js @@ -0,0 +1,24 @@ +'use strict' + +/** + * @fileoverview Utility function to calculate the number of nodes in a given CSS selector + * @author Alexander Afanasyev + */ + +var parser = require('./get-css-parser') + +module.exports = function (cssSelector) { + var parsedSelector = parser.parse(cssSelector) + + if (!parsedSelector) { + return 0 + } + + var rule = parsedSelector.rule + var depth = 0 + while (rule) { + rule = rule.rule + depth += 1 + } + return depth +} diff --git a/lib/rules/limit-selector-depth.js b/lib/rules/limit-selector-depth.js new file mode 100644 index 0000000..6256e04 --- /dev/null +++ b/lib/rules/limit-selector-depth.js @@ -0,0 +1,41 @@ +'use strict' + +/** + * @fileoverview Warn about potentially "deep" CSS selectors with too many nodes in the path + * @author Alexander Afanasyev + */ +var isCSSLocator = require('../find-css-locator') +var getSelectorDepth = require('../get-selector-depth') + +module.exports = { + meta: { + schema: [ + { + type: 'integer', + additionalProperties: false + } + ] + }, + + create: function (context) { + var maxDepth = context.options.length ? context.options[0] : 5 + + return { + 'CallExpression': function (node) { + if (node.arguments && node.arguments.length && node.arguments[0].hasOwnProperty('value')) { + if (isCSSLocator(node)) { + var cssSelector = node.arguments[0].value + var depth = getSelectorDepth(cssSelector) + + if (depth > maxDepth) { + context.report({ + node: node.arguments[0], + message: 'CSS selector has too many nodes.' + }) + } + } + } + } + } + } +} diff --git a/test/rules/limit-selector-depth.js b/test/rules/limit-selector-depth.js new file mode 100644 index 0000000..45e3ff9 --- /dev/null +++ b/test/rules/limit-selector-depth.js @@ -0,0 +1,89 @@ +'use strict' + +var rule = require('../../lib/rules/limit-selector-depth') +var RuleTester = require('eslint').RuleTester + +var eslintTester = new RuleTester() + +eslintTester.run('limit-selector-depth', rule, { + valid: [ + 'element(by.css(".myclass"));', + 'element.all(by.css(".myclass"));', + 'someOtherFunction("input[name=email]");', + { + options: [ + 5 + ], + code: 'element(by.css(".content > table > tbody td.cell > input#email"));' + }, + 'element(by.css("input.email.email-input.custom-input.other.classValue"));', + { + options: [ + 1 + ], + code: '$(".content");' + }, + '$$("");', + '$();' + ], + + invalid: [ + { + code: 'element(by.css(".content > table > tbody > tr:nth-child(2) > td.cell > input#email"));', + errors: [ + { + message: 'CSS selector has too many nodes.' + } + ] + }, + { + code: 'element(by.css("div div div div div div"));', + errors: [ + { + message: 'CSS selector has too many nodes.' + } + ] + }, + { + code: 'element(by.css("div > div > div > div > div > div"));', + errors: [ + { + message: 'CSS selector has too many nodes.' + } + ] + }, + { + options: [ + 1 + ], + code: '$(".content > table");', + errors: [ + { + message: 'CSS selector has too many nodes.' + } + ] + }, + { + options: [ + 0 + ], + code: '$(".content");', + errors: [ + { + message: 'CSS selector has too many nodes.' + } + ] + }, + { + options: [ + 4 + ], + code: '$$(".content > table > tbody td.cell > input#email");', + errors: [ + { + message: 'CSS selector has too many nodes.' + } + ] + } + ] +})