Skip to content

Commit

Permalink
fix(ExpectedConditions): allow ExpectedConditions to handle missing e…
Browse files Browse the repository at this point in the history
…lements

Expected conditions used `presenceOf` and `visibilityOf` to check that it's
referencing elements which actually exist on the page, but there is a race
condition with this strategy: an element could disappear after the
`presenceOf`/`visibilityOf` check but before other checks, causing an error
to be thrown.  This PR handles this race condition in two ways:

1. `ElementFinder`'s `isEnabled`, `isDisplayed`, and `isSelected` functions now
   return false if no such element exists, rahter than throwing an error
2. `ExpectedConditions`'s `textToBePresent` and `textToBePresentInElementValue`
   now check for errors and also return false in those cases

This is a general solution to the problem referenced in
angular#3777 and
angular#3958.
  • Loading branch information
sjelin committed Jan 13, 2017
1 parent 6a4dc7a commit 28f3873
Show file tree
Hide file tree
Showing 2 changed files with 22 additions and 17 deletions.
25 changes: 19 additions & 6 deletions lib/element.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,9 @@ let WEB_ELEMENT_FUNCTIONS = [
'getLocation', 'isEnabled', 'isSelected', 'submit', 'clear', 'isDisplayed', 'getId', 'serialize',
'takeScreenshot'
] as (keyof WebdriverWebElement)[];
let FALSE_IF_MISSING_WEB_ELEMENT_FUNCTIONS = [
'isEnabled', 'isSelected', 'isDisplayed'
] as (keyof WebdriverWebElement)[];

/**
* ElementArrayFinder is used for operations on an array of elements (as opposed
Expand Down Expand Up @@ -82,7 +85,8 @@ export class ElementArrayFinder extends WebdriverWebElement {
constructor(
public browser_: ProtractorBrowser,
public getWebElements: () => wdpromise.Promise<WebElement[]> = null, public locator_?: any,
public actionResults_: wdpromise.Promise<any> = null) {
public actionResults_: wdpromise.Promise<any> = null,
public falseIfMissing_: boolean) {
super();

// TODO(juliemr): might it be easier to combine this with our docs and just
Expand All @@ -92,7 +96,8 @@ export class ElementArrayFinder extends WebdriverWebElement {
let actionFn = (webElem: any) => {
return webElem[fnName].apply(webElem, args);
};
return this.applyAction_(actionFn);
return this.applyAction_(actionFn,
FALSE_IF_MISSING_WEB_ELEMENT_FUNCTIONS.indexOf(fnName) !== -1);
};
});
}
Expand Down Expand Up @@ -458,8 +463,8 @@ export class ElementArrayFinder extends WebdriverWebElement {
* @private
*/
// map<U>(callbackfn: (value: T, index: number, array: T[]) => U, thisArg?: any): U[];
private applyAction_(actionFn: (value: WebElement, index: number, array: WebElement[]) => any):
ElementArrayFinder {
private applyAction_(actionFn: (value: WebElement, index: number, array: WebElement[]) => any,
falseIfMissing: boolean): ElementArrayFinder {
let callerError = new Error();
let actionResults = this.getWebElements()
.then((arr: any) => wdpromise.all(arr.map(actionFn)))
Expand All @@ -474,7 +479,8 @@ export class ElementArrayFinder extends WebdriverWebElement {
}
throw noSuchErr;
});
return new ElementArrayFinder(this.browser_, this.getWebElements, this.locator_, actionResults);
return new ElementArrayFinder(this.browser_, this.getWebElements, this.locator_, actionResults,
falseIfMissing);
}

/**
Expand Down Expand Up @@ -801,7 +807,14 @@ export class ElementFinder extends WebdriverWebElement {
// Access the underlying actionResult of ElementFinder.
this.then =
(fn: (value: any) => any | wdpromise.IThenable<any>, errorFn?: (error: any) => any) => {
return this.elementArrayFinder_.then((actionResults: any) => {
return this.elementArrayFinder_.then(null, (error) => {
if (elementArrayFinder_.falseIfMissing_ &&
(error instanceof wderror.NoSuchElementError)) {
return false;
} else {
throw error;
}
}).then((actionResults: any) => {
if (!fn) {
return actionResults[0];
}
Expand Down
14 changes: 3 additions & 11 deletions lib/expectedConditions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -210,7 +210,7 @@ export class ProtractorExpectedConditions {
// MSEdge does not properly remove newlines, which causes false
// negatives
return actualText.replace(/\r?\n|\r/g, '').indexOf(text) > -1;
});
}, () => false);
};
return this.and(this.presenceOf(elementFinder), hasText);
}
Expand All @@ -235,7 +235,7 @@ export class ProtractorExpectedConditions {
let hasText = () => {
return elementFinder.getAttribute('value').then((actualText: string): boolean => {
return actualText.indexOf(text) > -1;
});
}, () => false);
};
return this.and(this.presenceOf(elementFinder), hasText);
}
Expand Down Expand Up @@ -388,15 +388,7 @@ export class ProtractorExpectedConditions {
* representing whether the element is visible.
*/
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 this.and(this.presenceOf(elementFinder), elementFinder.isDisplayed.bind(elementFinder));
}

/**
Expand Down

0 comments on commit 28f3873

Please sign in to comment.