Skip to content

Commit

Permalink
feat(rules): add 'bare-element-finders' rule
Browse files Browse the repository at this point in the history
  • Loading branch information
alecxe committed Jul 9, 2017
1 parent 9d129ce commit 6205a05
Show file tree
Hide file tree
Showing 8 changed files with 260 additions and 18 deletions.
3 changes: 3 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ There are various types of rules implemented in the plugin. Here is a rough cate
* [no-get-raw-id][]: Warn about using removed `getRawId()` method
* [no-get-location-abs-url][]: Warn about using deprecated `getLocationAbsUrl()` method
* [no-promise-in-if][]: Warn if promise is checked for truthiness inside an `if` condition
* [bare-element-finders][]: Warn if a bare `ElementFinder` or `ElementArrayFinder` is declared with no applied action
#### Locating Elements
Expand Down Expand Up @@ -135,6 +136,7 @@ Rule | Default Error Level | Auto-fixable | Options
[valid-by-id][] | 1 | |
[valid-by-tagname][] | 1 | |
[limit-selector-depth][] | 1 | | number of nodes (default 5)
[bare-element-finders][] | 1 | |
[use-promise-all][] | 0 (Turned off) | |
[by-css-shortcut][] | 0 | |
[no-browser-driver][] | 0 | |
Expand Down Expand Up @@ -192,6 +194,7 @@ See [configuring rules][] for more information.
[valid-by-tagname]: docs/rules/valid-by-tagname.md
[no-get-raw-id]: docs/rules/no-get-raw-id.md
[no-get-location-abs-url]: docs/rules/no-get-location-abs-url.md
[bare-element-finders]: docs/rules/bare-element-finders.md
[configuring rules]: http://eslint.org/docs/user-guide/configuring#configuring-rules
## Recommended configuration
Expand Down
43 changes: 43 additions & 0 deletions docs/rules/bare-element-finders.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
# Warn if a bare ElementFinder or ElementArrayFinder is declared with no applied action

This rule is inspired by [this problem](https://stackoverflow.com/questions/44841276/e2e-testing-in-protractor-signin) when there was an `ElementFinder` declared but no action was applied.

In this case, Protractor would not actually even try to locate the element, because [according to documentation](http://www.protractortest.org/#/locators#actions):

> The ElementFinder knows how to locate the DOM element using the locator you passed in as a parameter, but it has not actually done so yet.
> It will not contact the browser until an action method has been called.
It is easy to forget to actually call an ElementFinder method and the rule would be handy in these kind of cases.

## Rule details


Any use of the following patterns are considered warnings:

```js
element(by.id('signin_submit_btn'));
element.all(by.className('myClass'));
element.all(by.css(".class")).get(0);
$(".class");
$$(".class").first();
element.all(by.css(".class")).last();
element(by.css(".class1")).element(by.css(".class2"));
element(by.css(".class1")).$(".class2");
$$(".class1").first().element(by.css(".class2"));
$(".class1").$(".class2");
```

The following patterns are not warnings:

```js
element(by.id('signin_submit_btn')).click();
element.all(by.className('myClass')).first().click();
element.all(by.css(".class")).get(0).sendKeys("test");
$(".class").sendKeys("test");
$$(".class").first().sendKeys("test");
element.all(by.css(".class")).last().click();
element(by.css(".class1")).element(by.css(".class2")).click();
element(by.css(".class1")).$(".class2").sendKeys("test");
$$(".class1").first().element(by.css(".class2")).sendKeys("test");
$(".class1").$(".class2").sendKeys("test");
```
5 changes: 4 additions & 1 deletion index.js
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ 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')
var bareElementFinders = require('./lib/rules/bare-element-finders')

module.exports = {
rules: {
Expand Down Expand Up @@ -74,7 +75,8 @@ module.exports = {
'valid-by-tagname': validByTagName,
'no-get-raw-id': noGetRawId,
'no-get-location-abs-url': noGetLocationAbsUrl,
'limit-selector-depth': limitSelectorDepth
'limit-selector-depth': limitSelectorDepth,
'bare-element-finders': bareElementFinders
},
configs: {
recommended: {
Expand Down Expand Up @@ -112,6 +114,7 @@ module.exports = {
'protractor/valid-by-tagname': 1,
'protractor/limit-selector-depth': 1,
'protractor/no-get-location-abs-url': 1,
'protractor/bare-element-finders': 1,
'protractor/use-promise-all': 0,
'protractor/by-css-shortcut': 0,
'protractor/no-browser-driver': 0
Expand Down
28 changes: 12 additions & 16 deletions lib/is-element-array-finder.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,8 @@
* @fileoverview Utility function to determine if a node is an ElementArrayFinder
* @author Alexander Afanasyev
*/
module.exports = function (node) {

module.exports = function isElementArrayFinder (node) {
var callee = node.callee

if (callee) {
Expand All @@ -15,25 +16,20 @@ module.exports = function (node) {
return true
}

// handling element.all()
if (callee.object && callee.object.name === 'element' && callee.property && callee.property.name === 'all') {
return true
}

// handling nested expressions
if (callee.object && callee.property) {
// handling chained $$
if (callee.property.name === '$$') {
return true
}
// handling element.all() and nested .all()
if (callee.property && (callee.property.name === 'all' || callee.property.name === '$$')) {
if (callee.object) {
if (callee.object.type === 'Identifier') {
return callee.object.name === 'element'
}

// handling element.all() and chained element() and all()
if (callee.property.name === 'all') {
// TODO: inspect all the callees and search for element or $?
return true
if (callee.object.type === 'CallExpression') {
return isElementFinder(callee.object) || isElementArrayFinder(callee.object)
}
}
}
}

return false
}
var isElementFinder = require('./is-element-finder')
2 changes: 1 addition & 1 deletion lib/is-element-finder.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@
* @author Alexander Afanasyev
*/

var isElementArrayFinder = require('./is-element-array-finder')
var elementArrayFinderGetMethods = [
'get',
'last',
Expand Down Expand Up @@ -44,3 +43,4 @@ module.exports = function (node) {

return false
}
var isElementArrayFinder = require('./is-element-array-finder')
39 changes: 39 additions & 0 deletions lib/rules/bare-element-finders.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
'use strict'

/**
* @fileoverview Warn if a bare ElementFinder or ElementArrayFinder is declared with no applied action
* @author Alexander Afanasyev
*/

var isElementFinder = require('../is-element-finder')
var isElementArrayFinder = require('../is-element-array-finder')

module.exports = {
meta: {
schema: []
},

create: function (context) {
function isBareExpression (node) {
return node.parent.type === 'ExpressionStatement'
}

return {
CallExpression: function (node) {
var isNodeElementFinder = isElementFinder(node)
var isNodeElementArrayFinder = isElementArrayFinder(node)

if (isNodeElementFinder || isNodeElementArrayFinder) {
if (isBareExpression(node)) {
var target = isNodeElementFinder ? 'ElementFinder' : 'ElementArrayFinder'

context.report({
node: node,
message: 'Bare ' + target + ' with no applied action detected.'
})
}
}
}
}
}
}
1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
"scripts": {
"pretest": "standard && eslint .",
"test": "mocha",
"coverage": "istanbul cover ./node_modules/mocha/bin/_mocha --report html -- -R spec",
"watch": "mocha --watch",
"semantic-release": "semantic-release pre && npm publish && semantic-release post",
"coveralls": "istanbul cover ./node_modules/mocha/bin/_mocha --report lcovonly -- -R spec && cat ./coverage/lcov.info | ./node_modules/coveralls/bin/coveralls.js && rm -rf ./coverage"
Expand Down
157 changes: 157 additions & 0 deletions test/rules/bare-element-finders.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,157 @@
'use strict'

var rule = require('../../lib/rules/bare-element-finders')
var RuleTester = require('eslint').RuleTester

var eslintTester = new RuleTester()

eslintTester.run('bare-element-finders', rule, {
valid: [
'',
'all()',
'test.all()',
'$$',
'var myElement = element(by.id("signin_submit_btn"));',
'var myElements = element.all(by.className("myClass"));',
'var myElement = $("#signin_submit_btn");',
'var myElements = $$(".myClass");',
'element(by.id("signin_submit_btn")).click();',
'element.all(by.className("myClass")).first().click();',
'element.all(by.css(".class")).get(0).sendKeys("test");',
'$(".class").sendKeys("test");',
'$$(".class").first().sendKeys("test");',
'element.all(by.css(".class")).last().click();',
'element(by.css(".class1")).element(by.css(".class2")).click();',
'element(by.css(".class1")).$(".class2").sendKeys("test");',
'$$(".class1").first().element(by.css(".class2")).sendKeys("test");',
'$(".class1").$(".class2").sendKeys("test");',
'function test() { return q.all([promise1, promise2]); }',
'function test() { return element(by.id("signin_submit_btn")); }',
'function test() { return element.all(by.css("class1")); }',
'function test() { return $$("class1"); }',
'function test() { return $("class1"); }'
],

invalid: [
{
code: 'element(by.id("signin_submit_btn"));',
errors: [
{
message: 'Bare ElementFinder with no applied action detected.'
}
]
},
{
code: 'function test() { element(by.id("signin_submit_btn")); }',
errors: [
{
message: 'Bare ElementFinder with no applied action detected.'
}
]
},
{
code: 'element.all(by.className("myClass"));',
errors: [
{
message: 'Bare ElementArrayFinder with no applied action detected.'
}
]
},
{
code: 'element.all(by.css(".class")).get(0);',
errors: [
{
message: 'Bare ElementFinder with no applied action detected.'
}
]
},
{
code: '$(".class");',
errors: [
{
message: 'Bare ElementFinder with no applied action detected.'
}
]
},
{
code: '$$(".class").first();',
errors: [
{
message: 'Bare ElementFinder with no applied action detected.'
}
]
},
{
code: 'element.all(by.css(".class")).last();',
errors: [
{
message: 'Bare ElementFinder with no applied action detected.'
}
]
},
{
code: 'element(by.css(".class1")).element(by.css(".class2"));',
errors: [
{
message: 'Bare ElementFinder with no applied action detected.'
}
]
},
{
code: 'element(by.css(".class1")).$(".class2");',
errors: [
{
message: 'Bare ElementFinder with no applied action detected.'
}
]
},
{
code: '$$(".class1").first().element(by.css(".class2"));',
errors: [
{
message: 'Bare ElementFinder with no applied action detected.'
}
]
},
{
code: '$(".class1").$(".class2");',
errors: [
{
message: 'Bare ElementFinder with no applied action detected.'
}
]
},
{
code: '$(".class1").$$(".class2");',
errors: [
{
message: 'Bare ElementArrayFinder with no applied action detected.'
}
]
},
{
code: '$$(".class1").$$(".class2").all(by.className("class3"));',
errors: [
{
message: 'Bare ElementArrayFinder with no applied action detected.'
}
]
},
{
code: 'element.all(by.className("class3")).$$(".class4");',
errors: [
{
message: 'Bare ElementArrayFinder with no applied action detected.'
}
]
},
{
code: 'element(by.id("id1")).element(by.id("id2")).$$(".class4");',
errors: [
{
message: 'Bare ElementArrayFinder with no applied action detected.'
}
]
}
]
})

0 comments on commit 6205a05

Please sign in to comment.