Add a separate proxy for WDA commands without session ID #395
Conversation
There was a problem hiding this comment.
This looks like a good way to handle future no session commands from WDA. Nice thinking, @mykola-mokhnach. I had one question otherwise LGTM.
lib/webdriveragent.js
Outdated
| this.setupProxy(sessionId); | ||
| return this.webDriverAgentUrl; | ||
| } | ||
|
|
||
| log.info('Launching WebDriverAgent on the device'); | ||
|
|
||
| this.setupNoSessionProxy(); |
There was a problem hiding this comment.
why do we set up the noSessionProxy twice?
There was a problem hiding this comment.
we use this.url instance variable inside setupNoSessionProxy method and it is mandatory, that this variable is properly set before we call it.
There was a problem hiding this comment.
Is it needed before line 165 (where we initialize the other proxy)? If not, I would vote for just putting the no-session proxy initialization into setupProxy.
There was a problem hiding this comment.
unfortunately it is not possible to combine proxies init into single method, since noSessionProxy is used for getting driver status to detect whether WDA is running after xcodebuild has been executed. And the jwproxy can only be initialised after sessionId is known (which is known only after session init)
There was a problem hiding this comment.
We pass sessionId into the launch method. The setupProxy method is also called before the xcodebuild command is run.
lib/commands/proxy-helper.js
Outdated
| @@ -7,7 +7,7 @@ const GET_STATUS_COMMAND = 'getStatus'; | |||
|
|
|||
| let helpers = {}, extensions = {}; | |||
|
|
|||
| helpers.proxyCommand = async function (endpoint, method, body) { | |||
| helpers.proxyCommand = async function (endpoint, method, body, isSessionCommand=true) { | |||
There was a problem hiding this comment.
I don't think we've documented this appropriately in our style guide, but we put spaces around = in default parameter values like this.
There was a problem hiding this comment.
ok, will do
lib/webdriveragent.js
Outdated
| throw new Error(`Received non-zero status code from WDA server: '${res.status}'`); | ||
| const proxyTimeout = this.noSessionProxy.timeout; | ||
| try { | ||
| this.noSessionProxy.timeout = this.wdaLaunchTimeout < 5000 ? this.wdaLaunchTimeout / 2 : 5000; |
There was a problem hiding this comment.
this is a bit cryptic, maybe it deserves a comment? why would we halve the proxy timeout if the launch timeout is less than 5s?
There was a problem hiding this comment.
yep, I'm not sure about the actual timeout value here. Probably, it would be smart to set the timeout to the lowest value possible, so it does not delay retries?
lib/webdriveragent.js
Outdated
| this.setupProxy(sessionId); | ||
| return this.webDriverAgentUrl; | ||
| } | ||
|
|
||
| log.info('Launching WebDriverAgent on the device'); | ||
|
|
||
| this.setupNoSessionProxy(); |
There was a problem hiding this comment.
Is it needed before line 165 (where we initialize the other proxy)? If not, I would vote for just putting the no-session proxy initialization into setupProxy.
lib/webdriveragent.js
Outdated
| @@ -25,6 +24,37 @@ const WDA_AGENT_PORT = 8100; | |||
| const WDA_BASE_URL = 'http://localhost'; | |||
| const BUILD_TEST_DELAY = 1000; | |||
|
|
|||
|
|
|||
| class NoSessionProxy extends JWProxy { | |||
There was a problem hiding this comment.
I'd rather this be in its own file.
There was a problem hiding this comment.
I tried, but got circular dependency error as a result :(
I don't know the other way how to get rid of it except of putting the class together with main WDA class
There was a problem hiding this comment.
Weird. I just tried and had no problems.
There was a problem hiding this comment.
I'd be happy if you could show me how to implement it properly
There was a problem hiding this comment.
Like so should work: https://gist.github.com/imurchie/c9e0f9136d922db537cd87a4be66d8b9
There was a problem hiding this comment.
many thanks. Shall I also imp[ort util module from appium-support there?
Will it work if I import it like this, assuming I save this file as "no-session-proxy.js" into same folder
import { NoSessionProxy } from "./no-session-proxy"
?
There was a problem hiding this comment.
Ah, yes, you'll need the util module from appium-support.
If it is in the same directory you should be able to import with either of the two:
import NoSessionProxy from './no-session-proxy';
import { NoSessionProxy } from './no-session-proxy';
|
yep, it works! thanks @imurchie |
|
The CI failure is fixed in a different PR. |
Some WDA commands don't require session ID to be set. Such commands can be executed even if the application under test is not active. This PR adds support for such endpoints to WDA driver