feat(rule): add 'no-promise-in-if' rule

Author: alecxe

README.md

@@ -52,6 +52,7 @@ Rule                         | Default       | Options
 [array-callback-return][]    | 1             |
 [no-absolute-url][]          | 1             |
 [no-expect-in-po][]          | 1             | requires plugin "settings"
+[no-promise-in-if][]         | 1             | 
 [by-css-shortcut][]          | 0             |
 
 For example, the `missing-perform` rule is enabled by default and will cause
@@ -86,6 +87,7 @@ See [configuring rules][] for more information.
 [no-absolute-url]: docs/rules/no-absolute-url.md
 [by-css-shortcut]: docs/rules/by-css-shortcut.md
 [no-expect-in-po]: docs/rules/no-expect-in-po.md
+[no-promise-in-if]: docs/rules/no-promise-in-if.md
 [configuring rules]: http://eslint.org/docs/user-guide/configuring#configuring-rules
 
 ## Recommended configuration

docs/rules/no-promise-in-if.md

@@ -0,0 +1,40 @@
+# Warn if promise is checked for truthiness inside an `if` condition
+
+It is a common error in Protractor to check if a promise is "truthy" forgetting to actually resolve promise to get a real value.
+The problem is - promise itself is "truthy" which might make things difficult to spot and debug, or give a false sense of what tests are actually testing.
+
+## Rule details
+
+This is an example violation:
+
+```js
+var elm = $("#myid");
+if (elm.isDisplayed()) {
+    // do smth
+} else {
+    // do smth else
+}
+```
+
+The code execution would never reach "do smth else" because `elm.isDisplayed()` would always be "truthy" no matter if the element is displayed or not.
+
+Instead, one has to explicitly resolve the promise to have a boolean value:
+
+```js
+var elm = $("#myid");
+elm.isDisplayed().then(function (isDisplayed) {
+  if (isDisplayed) {
+      // do smth
+  } else {
+      // do smth else
+  }
+});
+```
+
+Here is a list of methods currently searched inside if conditions:
+
+ * `isDisplayed()`
+ * `isPresent()`
+ * `isElementPresent()`
+ * `isSelected()`
+ * `isEnabled()`

index.js

@@ -17,6 +17,7 @@ var noGetInIt = require('./lib/rules/no-get-in-it')
 var arrayCallbackReturn = require('./lib/rules/array-callback-return')
 var noAbsoluteURL = require('./lib/rules/no-absolute-url')
 var noExpectInPO = require('./lib/rules/no-expect-in-po')
+var noPromiseInIf = require('./lib/rules/no-promise-in-if')
 
 module.exports = {
   rules: {
@@ -36,7 +37,8 @@ module.exports = {
     'no-get-in-it': noGetInIt,
     'array-callback-return': arrayCallbackReturn,
     'no-absolute-url': noAbsoluteURL,
-    'no-expect-in-po': noExpectInPO
+    'no-expect-in-po': noExpectInPO,
+    'no-promise-in-if': noPromiseInIf
   },
   configs: {
     recommended: {
@@ -57,6 +59,7 @@ module.exports = {
         'protractor/array-callback-return': 1,
         'protractor/no-absolute-url': 1,
         'protractor/no-expect-in-po': 1,
+        'protractor/no-promise-in-if': 1,
         'protractor/by-css-shortcut': 0
       },
       globals: {

lib/rules/no-promise-in-if.js

@@ -0,0 +1,89 @@
+'use strict'
+
+/**
+ * @fileoverview Warn if promise is checked for truthiness inside an `if` condition
+ * @author Alexander Afanasyev
+ */
+
+var methodNames = [
+  'isDisplayed',
+  'isPresent',
+  'isElementPresent',
+  'isSelected',
+  'isEnabled'
+]
+
+module.exports = {
+  meta: {
+    schema: []
+  },
+
+  create: function (context) {
+    // --------------------------------------------------------------------------
+    // Helpers
+    // --------------------------------------------------------------------------
+
+    /**
+     * Checks if a node has a promise method called.
+     * @param {ASTNode} node The AST node to check, expecting CallExpression.
+     * @returns {Bool} method name, false if no promise truthiness check found.
+     * @private
+     */
+    function hasPromiseMethod (node) {
+      if (node.type === 'CallExpression' && node.callee) {
+        var property = node.callee.property
+        if (property && methodNames.indexOf(property.name) >= 0) {
+          return property.name
+        }
+      }
+      return false
+    }
+    /**
+     * Checks if a node has a promise checked for truthiness.
+     * @param {ASTNode} node The AST node to check.
+     * @param {boolean} inBooleanPosition `false` if checking branch of a condition.
+     *  `true` in all other cases
+     * @returns {Bool} method name, false if no promise truthiness check found.
+     * @private
+     */
+    function hasPromise (node, inBooleanPosition) {
+      switch (node.type) {
+        case 'CallExpression':
+          return hasPromiseMethod(node)
+
+        case 'UnaryExpression':
+          return hasPromise(node.argument, true)
+
+        case 'BinaryExpression':
+          return hasPromise(node.left, false) || hasPromise(node.right, false)
+
+        case 'LogicalExpression':
+          var leftHasPromise = hasPromise(node.left, inBooleanPosition)
+          var rightHasPromise = hasPromise(node.right, inBooleanPosition)
+
+          return leftHasPromise || rightHasPromise
+
+        case 'AssignmentExpression':
+          return (node.operator === '=') && hasPromise(node.right, inBooleanPosition)
+
+        case 'SequenceExpression':
+          return hasPromise(node.expressions[node.expressions.length - 1], inBooleanPosition)
+      }
+      return false
+    }
+
+    return {
+      IfStatement: function (node) {
+        if (node.test) {
+          var result = hasPromise(node.test, true)
+          if (result) {
+            context.report({
+              node: node,
+              message: 'Unexpected "' + result + '()" inside if condition'
+            })
+          }
+        }
+      }
+    }
+  }
+}

test/rules/no-promise-in-if.js

@@ -0,0 +1,87 @@
+'use strict'
+
+var rule = require('../../lib/rules/no-promise-in-if')
+var RuleTester = require('eslint').RuleTester
+var eslintTester = new RuleTester()
+
+eslintTester.run('no-promise-in-if', rule, {
+  valid: [
+    'elm.isDisplayed().then(function (isDisplayed) { if (isDisplayed) { console.log("here"); } });',
+    'if (something) { console.log("here"); }',
+    'if (something) { elm.isDisplayed().then(function (isDisplayed) { if (isDisplayed) { console.log("here"); } }); }',
+    'if(a = isEnabled());',
+    'for(;;);',
+    'if(isSelected === "str" && typeof b){}',
+    'if(1, isElementPresent);',
+    'if(xyz === "str1" || abc==="str2" && isPresent){}'
+  ],
+
+  invalid: [
+    {
+      code: 'if (elm.isDisplayed()) { console.log("here"); }',
+      errors: [{
+        message: 'Unexpected "isDisplayed()" inside if condition'
+      }]
+    },
+    {
+      code: 'if (!elm.isEnabled()) { console.log("here"); }',
+      errors: [{
+        message: 'Unexpected "isEnabled()" inside if condition'
+      }]
+    },
+    {
+      code: 'if (something) {} else if (elm.isPresent()) { console.log("here"); }',
+      errors: [{
+        message: 'Unexpected "isPresent()" inside if condition'
+      }]
+    },
+    {
+      code: 'if (something) {} else if (!browser.isElementPresent(elm)) { console.log("here"); }',
+      errors: [{
+        message: 'Unexpected "isElementPresent()" inside if condition'
+      }]
+    },
+    {
+      code: 'if (something && elm.isSelected()) { console.log("here"); }',
+      errors: [{
+        message: 'Unexpected "isSelected()" inside if condition'
+      }]
+    },
+    {
+      code: 'if (something && (elm.isSelected() || somethingelse)) { console.log("here"); }',
+      errors: [{
+        message: 'Unexpected "isSelected()" inside if condition'
+      }]
+    },
+    {
+      code: 'if (something) {} else if (something && (elm.isDisplayed() || somethingelse)) { console.log("here"); }',
+      errors: [{
+        message: 'Unexpected "isDisplayed()" inside if condition'
+      }]
+    },
+    {
+      code: 'if (something) {} else if ((!browser.isElementPresent(elm) || somethingelse) && something) { console.log("here"); }',
+      errors: [{
+        message: 'Unexpected "isElementPresent()" inside if condition'
+      }]
+    },
+    {
+      code: 'if (a === elm.isDisplayed()) { console.log("here"); }',
+      errors: [{
+        message: 'Unexpected "isDisplayed()" inside if condition'
+      }]
+    },
+    {
+      code: 'if (!elm.isDisplayed() === b) { console.log("here"); }',
+      errors: [{
+        message: 'Unexpected "isDisplayed()" inside if condition'
+      }]
+    },
+    {
+      code: 'if (b = elm.isPresent()) { console.log("here"); }',
+      errors: [{
+        message: 'Unexpected "isPresent()" inside if condition'
+      }]
+    }
+  ]
+})