-
Notifications
You must be signed in to change notification settings - Fork 572
Add direct connect flag to be able to handle directConnectXxxxc #338
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
| By.IMAGE = MobileBy.IMAGE | ||
| By.CUSTOM = MobileBy.CUSTOM | ||
|
|
||
| def _update_command_executor(self, keep_alive): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should probably check that these values actually exist. just because someone has set the direct_connection param doesn't mean the server supports it, right?
up to you whether you think we should throw an error if one of these values is not present, or just silently ignore the request to direct connect.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice catch, I forgot it...
I'll show warning or error message rather than stopping test script since whether a server cannot handle the following requests or not depend on them.
appium/webdriver/webdriver.py
Outdated
| path=path | ||
| ) | ||
|
|
||
| logger.info('Update request endpoint to %s', executor) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated
appium/webdriver/webdriver.py
Outdated
| ) | ||
|
|
||
| if self.command_executor is not None: | ||
| if self.command_executor is not None: # pylint: disable=access-member-before-definition |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you could probably use hasattr to avoid pylint warning
appium/webdriver/webdriver.py
Outdated
| if (not {direct_protocol, direct_host, direct_port, direct_path}.issubset(set(self.capabilities))): | ||
| message = 'Direct connect capabilities from server were:\n'\ | ||
| '{protocol}: {protocol_get}, {host}: {host_get}, {port}: {port_get}, {path}: {path_get}'.format( | ||
| protocol=direct_protocol, protocol_get=self.capabilities.get(direct_protocol, ''), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
capabilities is already a dictionary, so we could in theory to something like **self.capabilities
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to show a blank in no key case like below, so I changed to for style
directConnectProtocol: 'http'
directConnectHost: 'localhost2'
directConnectPort: '4800'
directConnectPath: ''
This feature is the same as https://github.com/projectxyzio/web2driver#direct-connect-urls and https://www.rubydoc.info/github/appium/ruby_lib_core/Appium/Core/Driver#direct_connect-instance_method
Use case:
I've added
direct_connectionflag, and it is false by default, to be able to handle the flag.Different as SeleniumGrid, mobile apps tie to physical locations. Measuring performance including network latency is necessary to manage devices in various locations, for example. In such case, one proxy server and Appium servers in various locations make easy to manage the complexity. The proxy has device selection logic. The client only sends a session creation command to the proxy, no many URLs.
cc @jlipps