Skip to content
This repository has been archived by the owner on Jul 29, 2024. It is now read-only.

fix(ExpectedConditions): allow ExpectedConditions to handle missing e… #3972

Closed
wants to merge 1 commit into from

Conversation

sjelin
Copy link
Contributor

@sjelin sjelin commented Jan 13, 2017

…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
#3777 and
#3958.

I'm assigning @juliemr, since it's a low level change, and @mgiambalvo, since we discussed this issue earlier today

@@ -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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we be more specific about which errors we mask here?

return this.elementArrayFinder_.then((actionResults: any) => {
return this.elementArrayFinder_.then(null, (error) => {
if (elementArrayFinder_.falseIfMissing_ &&
(error instanceof wderror.NoSuchElementError)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we also need to handle StaleElement exceptions?

@heathkit
Copy link
Contributor

heathkit commented Jan 13, 2017

This has also come up in #3578 (comment), and there's an npm package out there to patch around the issue: https://github.com/tilmanpotthof/protractor-save-expected-conditions

In that 'protractor-save-expected-conditions' package, he masks NoSuchElement and StaleElement in textToBePresent, which is a little confusing to me since it looks like we already handle those, but maybe the way our isPresent() works still allows StaleElement errors to get through.

edit: Nevermind, I misread what he was doing. I do think we need to also catch StaleElement errors for the "false if missing" actions. That and the textToBePresent should make 'protractor-save-expected-conditions' unnecessary.

@heathkit
Copy link
Contributor

Also we could probably simplify ElementFinder.isPresent() as part of this.

…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.
@sjelin
Copy link
Contributor Author

sjelin commented Jan 13, 2017

@mgiambalvo all comments addressed

@sjelin
Copy link
Contributor Author

sjelin commented Jan 13, 2017

ack, I forgot to run tests. Give me a second to debug this

@tilmanschweitzer
Copy link
Contributor

@mgiambalvo Hi, the problem in the expected conditions is not the isPresent check. It happens, when an element is removed exactly in the moment after the isPresent check and before isDisplayed in visibilityOf. Although node is single threaded, the browser thread is not synchronised with the webdriver checks.

The possibility of these race conditions is very low for a single test, but I had it in almost every build with >600 test in 2-10 random test. This started after I replaced all browser.sleep usages to make my tests more reliable :-) With my temporary npm module they are really more reliable, but it would be great to see these changes in 5.0.1

@tilmanschweitzer
Copy link
Contributor

@sjelin I submitted a PR focusing on the expected conditions and ignoring the problems in the ElementFinder functions.

#4006

heathkit pushed a commit that referenced this pull request Jan 27, 2017
…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
@heathkit
Copy link
Contributor

Closing this as superceded by #4006

@heathkit heathkit closed this Jan 27, 2017
igniteram pushed a commit to igniteram/protractor that referenced this pull request Feb 21, 2017
…ibility (angular#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
angular#3972)

This problem was also referenced in
angular#3578 and
angular#3777
bodyduardU pushed a commit to bodyduardU/protractor that referenced this pull request Dec 5, 2022
…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
angular/protractor#3972)

This problem was also referenced in
angular/protractor#3578 and
angular/protractor#3777
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants