Fix visible commands for Appium. #3566
Conversation
|
This PR does not consider BrowserStack App Automate runs and thus, visible commands still fail over there. |
lib/element/locator.js
Outdated
| @@ -52,6 +53,10 @@ class LocateElement { | |||
|
|
|||
| const elementInstance = Element.createFromSelector(selector, strategy); | |||
|
|
|||
| if (isAppiumClient && strategy === 'id') { | |||
| return new By(strategy, selector); | |||
| } | |||
There was a problem hiding this comment.
This change is for the following reason:
For waitForElementVisible command and expect.element().visible assertion, when we pass {selector: 'com.app:id/web-login', locateStrategy: 'id'} as selector, it gets modified to {using: 'css selector', value: '*[id="com\\.app\\:id\\/web-login"]' when searching for the element.
This fails in Appium v1.17.0 (default version of Appium in BrowserStack) since Appium v1.17.0 does not support css selector locate strategy, but works fine on the latest version of Appium 1.x.
So, I made this change to keep the locate strategy as id only when working with Appium.
| @@ -367,7 +367,7 @@ module.exports = class MethodMappings { | |||
| }, | |||
|
|
|||
| async locateMultipleElements(element) { | |||
| const locator = Locator.create(element); | |||
| const locator = Locator.create(element, this.transport.api.isAppiumClient()); | |||
There was a problem hiding this comment.
I suggest to create a subclass of Locator which overrides the create() method. That way we don't have to pass the isAppium flag around and pollute the Locator class.
There was a problem hiding this comment.
Please check 42d08a7.
I couldn't think of any other good way of doing it, since I need all the functionality of LocateElement.create() method except just the last line where it converts {using: 'id', value: 'org.wikipedia:id/search_container'} to {using: 'css selector', value: '*[id="org\\.wikipedia\\:id\\/search_container"]'}.
Other way could be create a sub-class of By itself, but then we'd have to somehow pass the value of isAppiumClient into By class for that to work.
There was a problem hiding this comment.
How about adding a Locator.createAppium(element) method?
There was a problem hiding this comment.
Please check the implementation now. Also tested it by running .waitForElementVisible command on both Appium Client and normal desktop Chrome browser.
|
|
||
| // Handle BrowserStack case | ||
| // (BrowserStack always returns platformName in capabilities) | ||
| const isMobile = this.__isPlatformName('android') || this.__isPlatformName('ios'); |
There was a problem hiding this comment.
Cannot use isMobile from utils here because that method only considers desiredCapabilites whereas over here we need to check is android or ios is present as platformName inside the capabilities returned by BrowserStack.
This is necessary because users might not explicitly specify platformName in their desiredCapabilites (for example, when testing on mobile browser on BrowserStack, platformName in desired capabilites is not necessary) but BrowserStack would always send back the platformName in capabilities.
There was a problem hiding this comment.
We would also need to tweek isAndroid() and isIOS() methods available in our API accordingly for it to consider capabilities as well, but that is out of scope of this PR and I'll create a separate issue for that.
There was a problem hiding this comment.
Created an issue here: #3603
| @@ -531,11 +531,14 @@ module.exports = class MethodMappings { | |||
| return `/element/${id}/location_in_view`; | |||
| }, | |||
|
|
|||
| async isElementDisplayed(webElementOrId) { | |||
| isElementDisplayed(webElementOrId) { | |||
| if (this.transport.api.isAppiumClient()) { | |||
There was a problem hiding this comment.
We should not use api methods here. We can move isAppium client to util and api method can use take use of that util function
There was a problem hiding this comment.
Following up on the above response, this method cannot be moved to utils in its current form since for handling the BrowserStack case, we'd need the capabilities returned by BrowserStack after a session is established with it, and those capabilities are only made available in client.js as this.capabilities.
garg3133
force-pushed
the
appium-visible
branch
from
42d08a7
to
35bfe4f
Compare
Fixes: #3523