feat : Add support for virtual devices on LambdaTest #1369
Conversation
Integrated LambdaTest
Added LT documentation link
Support for w3c=True
Added support for w3c=true
Defined variables in two lines
Changed the sessions api response structure
Added check for capabilities
Fixed indentation
Changed let to const
sync with upstream
support for virtual device app automation and attach to w3c sessions
eglitise
requested review from
jlipps,
KazuCocoa,
mykola-mokhnach and
eglitise
| @@ -347,15 +347,25 @@ export function newSession(caps, attachSessId = null) { | |||
| username = session.server.lambdatest.username || process.env.LAMBDATEST_USERNAME; | |||
| if (desiredCapabilities.hasOwnProperty.call(desiredCapabilities, 'lt:options')) { | |||
| desiredCapabilities['lt:options'].source = 'appiumdesktop'; | |||
| desiredCapabilities['lt:options'].isRealMobile = true; | |||
| if ( | |||
| desiredCapabilities['lt:options'].isRealMobile === undefined || | |||
There was a problem hiding this comment.
the condition might be extracted to avoid unnecessary duplication
| @@ -572,6 +582,9 @@ export function newSession(caps, attachSessId = null) { | |||
| let attachedSessionCaps = {}; | |||
| if (attachedSession) { | |||
| attachedSessionCaps = attachedSession.capabilities; | |||
| if (attachedSessionCaps.platformName === undefined) { | |||
There was a problem hiding this comment.
could you please describe the purpose of this change?
There was a problem hiding this comment.
When attaching to w3c sessions, capabilities are nested inside the values. As per the current logic, all the values are mapped as capabilities and in further lines, we set serverOpts.isIOS and serverOpts.isAndroid by performing regex match on this. Since in w3c session, capabilities are nested in values, appium inspector was giving cannot read property match of undefined error and attach to session fails.
This piece of code checks if platformName exists in attachedSessionCaps if not assign the capabilities nested inside.
There was a problem hiding this comment.
session data for reference
{ "value": { "capabilities": { "adbExecTimeout": 120000, "allowInvisibleElements": true, "autoAcceptAlerts": true, "autoGrantPermissions": true, "automationName": "uiautomator2", "chromeOptions": { "args": [] }, "desired": { "adbExecTimeout": 120000, "allowInvisibleElements": true, "autoAcceptAlerts": true, "autoGrantPermissions": true, "automationName": "uiautomator2", "chromeOptions": { "args": [] }, "headless": false, "nativeWebScreenshot": true, "newCommandTimeout": 0, "orientation": "PORTRAIT", "platformName": "android", "udid": "emulator-5554", "uiautomator2ServerLaunchTimeout": 60000, "waitForQuiescence": false }, "deviceScreenDensity": 320, "deviceScreenSize": "1080x2400", "headless": false, "javascriptEnabled": true, "locationContextEnabled": false, "nativeWebScreenshot": true, "networkConnectionEnabled": true, "newCommandTimeout": 0, "orientation": "PORTRAIT", "pixelRatio": 2, "platformName": "android", "platformVersion": "13", "statBarHeight": 48, "takesScreenshot": true, "uiautomator2ServerLaunchTimeout": 60000, "viewportRect": { "height": 2304, "left": 0, "top": 48, "width": 1080 }, "waitForQuiescence": false, "warnings": {}, "webStorageEnabled": false } "sessionId": "" } }
There was a problem hiding this comment.
@eglitise does it ring a bell for you? Also I suppose this is supposed to fix an issue which is different from the one this PR tries to address and should probably be addressed separately.
There was a problem hiding this comment.
I haven't used the attach to session functionality much, but since every session must have platformName anyway, I also don't see this breaking anything. Although, I'm wondering why the line below isn't attachedSession.value.capabilities, as this would match the reference session data above.
There was a problem hiding this comment.
@KazuCocoa we at lambdatest store session data of all running sessions triggered by user and return the session data when /sessions api is called.
It all depends on the session response from webdriveragent. In our case we WDA gives w3c response and we send this data in /sessions api.
I can share the api but I am afraid you will not be able to see the response unless you have running session with lambdatest.
There was a problem hiding this comment.
What I could share right now is please make sure the response follows https://appium.io/docs/en/latest/commands/base-driver/#getsessions format as the endpoint. Possibly your service might need to have an orchestration layer to modify the response.
It all depends on the session response from webdriveragent
Btw, the Appium/WDA is expected to be called by the Appium XCUITest driver in our current design. The direct response by WDA might not fit our Appium XCUITest driver response.
There was a problem hiding this comment.
Btw, the Appium/WDA is expected to be called by the Appium XCUITest driver in our current design. The direct response by WDA might not fit our Appium XCUITest driver response.
I agree on this and we follow the same, since WDA is the one that handles all the commands, I mentioned it as the response from WDA.
Possibly your service might need to have an orchestration layer to modify the response.
This is definitely one way of doing it but the reason we chose not to do it is that if we translate the session response from w3c to jsonwire we would have to translate the subsequent commands from appium inspector from jsonwire to w3c, which adds a significant overhead. We send the raw session response to appium inspector and let appium inspector decide the commands based on the session. We are open for suggestions here.
There was a problem hiding this comment.
Oh, so, you're suggesting/proposing a new specification for the /sessions endpoint, instead of current drivers' response, correct? (to detect which session is w3c, or mjsonwp for your platform because /sessions endpoint is not defined by W3C WebDriver protocol)
For example, the latest xcuitest driver's response of /sessions endpoint follows https://appium.io/docs/en/latest/commands/base-driver/#getsessions so the response of /sessions is like:
{
"value": [
{
"id": "fafef641-1d67-4460-95e8-0dd8c2bff7e6",
"capabilities": {
"platformName": "ios",...
}
}
]
}
but you're suggesting a new format to have nested capabilities like:
{
"value": [
{
"id": "fafef641-1d67-4460-95e8-0dd8c2bff7e6",
"capabilities": {
"capabilities": {
"platformName": "ios",...
}
}
}
]
}
which has nested capabilities key in case the session is w3c (which puts the response of a new session request into the first capabilities.)
I think it would be nice to move such a discussion in appium/appium#19856 or appium/appium#19857 where have discussing how to drop the /sessions endpoint as non-w3c.
Or I guess moving this implementation into a separate method for your platform only would help to clarify this behavior is for you and our future maintenance
There was a problem hiding this comment.
I agree with the suggestions and we were unaware of the discussions, thanks for sharing them.
As suggested, we will make the change to include this only for our platform.
Added support for virtual devices on LambdaTest