fix: handle safari alerts during atom execution #1176
Conversation
| extensions.checkForAlert = async function checkForAlert () { // eslint-disable-line require-await | ||
| extensions.checkForAlert = async function checkForAlert () { | ||
| try { | ||
| await this.getAlert(); |
There was a problem hiding this comment.
have you checked how this change affects the performance?
There was a problem hiding this comment.
maybe we could make this check configurable via settings, so it does not slow down tests, which don't face such issue
There was a problem hiding this comment.
There is very little scope for performance effects. Most of the atoms, which do not cause an alert to come up, return well within the 500ms timeout before we check the first alert. If they take longer, it will be extended a very small amount while the alert it checked.
In the case of alerts actually arising, this will be much faster than the current execution, which hangs until the operation times out.
I did a little check, with the 'element handling' tests from the appium-xcuitest-driver Safari suite, which consists of 16 tests that involve running a lot of atoms. Running 5 times, the old strategy took 79767ms on average, and this took 80591ms.
If performance problems do arise, I'm happy to figure out something. But I don't think it is warranted at the moment.
There was a problem hiding this comment.
cool, thanks for checking it
| } | ||
|
|
||
| let res = await promise; | ||
| if (error) { |
There was a problem hiding this comment.
this if is redundant
| // check if there is an alert | ||
| if (await this.checkForAlert()) { | ||
| // we found an alert, and should just return control | ||
| return ''; |
There was a problem hiding this comment.
Does this empty string have some special meaning?
|
Weird. A commit on the forked branch didn't get into this. It's just clean up, so I'll do it elsewhere. |
1 similar comment
|
Weird. A commit on the forked branch didn't get into this. It's just clean up, so I'll do it elsewhere. |
* fix: handle safari alerts during atom execution * fix: make sure alerts are waited for, and run tests in travis
Actually do the check for an alert. The atom will not actually complete until the alert is closed in the native layer, so returning is a must.
The tests were passing because all our alert tests use
nativeWebTap, which works.Fixes appium/appium#13979 and appium/appium#13549