fix(expectedConditions): Add tests and fix race conditions around vis…

…ibility (#4006) Add test cases to reproduce the missing element race conditions possible in expected condition methods `visibilityOf`, `textToBePresentInElement`, `textToBePresentInValue` and `elementToBeClickable`. Add error handler `falseIfMissing` to all expected conditions that depend on the presence of an element. Expected conditions check the presence of an element before other checks, but when an element is removed exactly in the moment after the `isPresent` and before `isDisplayed` in `visibilityOf` the condition used to fail. This solution does not handle missing elements in (`isEnable`, `isDisplayed`, `isSelected`) and focused only on expected conditions (see #3972) This problem was also referenced in #3578 and #3777
angular · Jan 27, 2017 · 40bbeca · 40bbeca
1 parent 3d98a16
commit 40bbeca
Show file tree

Hide file tree

Showing 4 changed files with 105 additions and 55 deletions.
diff --git a/lib/element.ts b/lib/element.ts
@@ -4,6 +4,7 @@ import {ElementHelper, ProtractorBrowser} from './browser';
 import {IError} from './exitCodes';
 import {Locator} from './locators';
 import {Logger} from './logger';
+import {falseIfMissing} from './util';
 
 let clientSideScripts = require('./clientsidescripts');
 
@@ -1071,30 +1072,14 @@ export class ElementFinder extends WebdriverWebElement {
    *     the element is present on the page.
    */
   isPresent(): wdpromise.Promise<boolean> {
-    return this.parentElementArrayFinder.getWebElements().then(
-        (arr: any[]) => {
-          if (arr.length === 0) {
-            return false;
-          }
-          return arr[0].isEnabled().then(
-              () => {
-                return true;  // is present, whether it is enabled or not
-              },
-              (err: any) => {
-                if (err instanceof wderror.StaleElementReferenceError) {
-                  return false;
-                } else {
-                  throw err;
-                }
-              });
-        },
-        (err: Error) => {
-          if (err instanceof wderror.NoSuchElementError) {
-            return false;
-          } else {
-            throw err;
-          }
-        });
+    return this.parentElementArrayFinder.getWebElements().then((arr: any[]) => {
+      if (arr.length === 0) {
+        return false;
+      }
+      return arr[0].isEnabled().then(() => {
+        return true;  // is present, whether it is enabled or not
+      }, falseIfMissing);
+    }, falseIfMissing);
   }
 
   /**

diff --git a/lib/expectedConditions.ts b/lib/expectedConditions.ts
@@ -1,6 +1,7 @@
 import {error as wderror} from 'selenium-webdriver';
 import {ProtractorBrowser} from './browser';
 import {ElementFinder} from './element';
+import {falseIfMissing, passBoolean} from './util';
 
 /**
  * Represents a library of canned expected conditions that are useful for
@@ -185,7 +186,9 @@ export class ProtractorExpectedConditions {
    *     representing whether the element is clickable.
    */
   elementToBeClickable(elementFinder: ElementFinder): Function {
-    return this.and(this.visibilityOf(elementFinder), elementFinder.isEnabled.bind(elementFinder));
+    return this.and(this.visibilityOf(elementFinder), () => {
+      return elementFinder.isEnabled().then(passBoolean, falseIfMissing);
+    });
   }
 
   /**
@@ -210,7 +213,7 @@ export class ProtractorExpectedConditions {
         // MSEdge does not properly remove newlines, which causes false
         // negatives
         return actualText.replace(/\r?\n|\r/g, '').indexOf(text) > -1;
-      });
+      }, falseIfMissing);
     };
     return this.and(this.presenceOf(elementFinder), hasText);
   }
@@ -235,7 +238,7 @@ export class ProtractorExpectedConditions {
     let hasText = () => {
       return elementFinder.getAttribute('value').then((actualText: string): boolean => {
         return actualText.indexOf(text) > -1;
-      });
+      }, falseIfMissing);
     };
     return this.and(this.presenceOf(elementFinder), hasText);
   }
@@ -389,13 +392,7 @@ export class ProtractorExpectedConditions {
    */
   visibilityOf(elementFinder: ElementFinder): Function {
     return this.and(this.presenceOf(elementFinder), () => {
-      return elementFinder.isDisplayed().then((displayed: boolean) => displayed, (err: any) => {
-        if (err instanceof wderror.NoSuchElementError) {
-          return false;
-        } else {
-          throw err;
-        }
-      });
+      return elementFinder.isDisplayed().then(passBoolean, falseIfMissing);
     });
   }
 
@@ -433,6 +430,8 @@ export class ProtractorExpectedConditions {
  *     representing whether the element is selected.
  */
   elementToBeSelected(elementFinder: ElementFinder): Function {
-    return this.and(this.presenceOf(elementFinder), elementFinder.isSelected.bind(elementFinder));
+    return this.and(this.presenceOf(elementFinder), () => {
+      return elementFinder.isSelected().then(passBoolean, falseIfMissing);
+    });
   }
 }
diff --git a/lib/util.ts b/lib/util.ts
@@ -1,5 +1,6 @@
 import {resolve} from 'path';
 import {Promise, when} from 'q';
+import {error as wderror} from 'selenium-webdriver';
 
 let STACK_SUBSTRINGS_TO_FILTER = [
   'node_modules/jasmine/', 'node_modules/selenium-webdriver', 'at Module.', 'at Object.Module.',
@@ -75,3 +76,31 @@ export function joinTestLogs(log1: any, log2: any): any {
     specResults: (log1.specResults || []).concat(log2.specResults || [])
   };
 }
+
+/**
+ * Returns false if an error indicates a missing or stale element, re-throws
+ * the error otherwise
+ *
+ * @param {*} The error to check
+ * @throws {*} The error it was passed if it doesn't indicate a missing or stale
+ *   element
+ * @return {boolean} false, if it doesn't re-throw the error
+ */
+export function falseIfMissing(error: any) {
+  if ((error instanceof wderror.NoSuchElementError) ||
+      (error instanceof wderror.StaleElementReferenceError)) {
+    return false;
+  } else {
+    throw error;
+  }
+}
+
+/**
+ * Return a boolean given boolean value.
+ *
+ * @param {boolean} value
+ * @returns {boolean} given value
+ */
+export function passBoolean(value: boolean) {
+  return value;
+}
diff --git a/spec/basic/expected_conditions_spec.js b/spec/basic/expected_conditions_spec.js
@@ -45,26 +45,6 @@ describe('expected conditions', function() {
     expect(visibilityOfHideable.call()).toBe(false);
   });
 
-  it('should have visibilityOf (handling race conditions)', function() {
-    var disabledButton = $('#disabledButton[disabled="disabled"]');
-
-    // toggle presence (of .ng-hide) between visibility evaluation to simulate race condition
-    var originalIsDisplayedFn = disabledButton.isDisplayed;
-    disabledButton.isDisplayed = function () {
-      element(by.model('disabled')).click();
-      return originalIsDisplayedFn.call(this);
-    };
-
-    var visibilityOfDisabledButtonWithInterceptor = EC.visibilityOf(disabledButton);
-
-    element(by.model('disabled')).click();
-
-    expect(originalIsDisplayedFn.call(disabledButton)).toBe(true);
-    expect(disabledButton.isPresent()).toBe(true);
-
-    expect(visibilityOfDisabledButtonWithInterceptor.call()).toBe(false);
-  });
-
   it('should have invisibilityOf', function() {
     var invisibilityOfInvalid = EC.invisibilityOf($('#INVALID'));
     var invisibilityOfHideable = EC.invisibilityOf($('#shower'));
@@ -215,4 +195,61 @@ describe('expected conditions', function() {
       browser2.switchTo().alert().accept();
     });
   });
+
+  describe('race condition handling', function () {
+
+    var disabledButton;
+
+    beforeEach(function () {
+      disabledButton = $('#disabledButton[disabled="disabled"]');
+    });
+
+    function enableButtonBeforeCallToUnmatchSelector(testElement, fnName) {
+      var originalFn = testElement[fnName];
+
+      testElement[fnName] = function () {
+        element(by.model('disabled')).click();
+        return originalFn.apply(this, arguments);
+      };
+
+      // save original fn with _ prefix
+      testElement['_' + fnName] = originalFn;
+    }
+
+    it('can deal with missing elements in visibilityOf', function() {
+      enableButtonBeforeCallToUnmatchSelector(disabledButton, 'isDisplayed');
+
+      element(by.model('disabled')).click();
+
+      expect(disabledButton._isDisplayed()).toBe(true);
+      expect(EC.visibilityOf(disabledButton).call()).toBe(false);
+    });
+
+    it('can deal with missing elements in textToBePresentInElement', function() {
+      enableButtonBeforeCallToUnmatchSelector(disabledButton, 'getText');
+
+      element(by.model('disabled')).click();
+
+      expect(disabledButton._getText()).toBe('Dummy');
+      expect(EC.textToBePresentInElement(disabledButton, 'Dummy').call()).toBe(false);
+    });
+
+    it('can deal with missing elements in textToBePresentInValue', function() {
+      enableButtonBeforeCallToUnmatchSelector(disabledButton, 'getAttribute');
+
+      element(by.model('disabled')).click();
+
+      expect(disabledButton._getAttribute('value')).toBe('');
+      expect(EC.textToBePresentInElementValue(disabledButton, '').call()).toBe(false);
+    });
+
+    it('can deal with missing elements in elementToBeClickable', function() {
+      enableButtonBeforeCallToUnmatchSelector(disabledButton, 'isEnabled');
+
+      element(by.model('disabled')).click();
+
+      expect(disabledButton._isEnabled()).toBe(false);
+      expect(EC.elementToBeClickable(disabledButton).call()).toBe(false);
+    });
+  });
 });