From 79885d118cc954ef746d02e4666190e98703ec43 Mon Sep 17 00:00:00 2001 From: Tilman Potthof Date: Tue, 17 Jan 2017 02:39:40 +0100 Subject: [PATCH 1/2] fix(expectedConditions): add test cases for missing element race conditions Add test cases to reproduce the missing element race conditions possible in expected condition methods `visibilityOf`, `textToBePresentInElement`, `textToBePresentInValue` and `elementToBeClickable`. Issue is referenced in https://github.com/angular/protractor/issues/3578 https://github.com/angular/protractor/issues/3777 --- spec/basic/expected_conditions_spec.js | 77 +++++++++++++++++++------- 1 file changed, 57 insertions(+), 20 deletions(-) diff --git a/spec/basic/expected_conditions_spec.js b/spec/basic/expected_conditions_spec.js index d110ded1d..e91155070 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); + }); + }); }); From 3f29921769c8b2eb6ca7dca6ecd8c87ca9594f43 Mon Sep 17 00:00:00 2001 From: Tilman Potthof Date: Tue, 24 Jan 2017 23:52:56 +0100 Subject: [PATCH 2/2] fix(ExpectedConditions): handle missing element race conditions 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. --- lib/element.ts | 33 +++++++++------------------------ lib/expectedConditions.ts | 21 ++++++++++----------- lib/util.ts | 29 +++++++++++++++++++++++++++++ 3 files changed, 48 insertions(+), 35 deletions(-) diff --git a/lib/element.ts b/lib/element.ts index b60b004da..b2de0ebfe 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 c92eaa3c6..205435f4b 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 ec22c527c..346051702 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; +}