From 5341c894152d3df0054ba511538cb5cdb7df38a2 Mon Sep 17 00:00:00 2001 From: Tilman Potthof Date: Fri, 27 Jan 2017 05:50:17 +0100 Subject: [PATCH] fix(expectedConditions): Add tests and fix race conditions around visibility (#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 https://github.com/angular/protractor/pull/3972) This problem was also referenced in https://github.com/angular/protractor/issues/3578 and https://github.com/angular/protractor/issues/3777 --- lib/element.ts | 33 +++-------- lib/expectedConditions.ts | 21 ++++--- lib/util.ts | 29 ++++++++++ spec/basic/expected_conditions_spec.js | 77 +++++++++++++++++++------- 4 files changed, 105 insertions(+), 55 deletions(-) diff --git a/lib/element.ts b/lib/element.ts index 0d71070..15e7908 100644 --- 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 { - 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 index c92eaa3..205435f 100644 --- 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 index ec22c52..3460517 100644 --- 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 index d110ded..e911550 100644 --- 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); + }); + }); });