-
-
Notifications
You must be signed in to change notification settings - Fork 768
feat: add directConnect feature #1747
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
src/main/java/io/appium/java_client/remote/AppiumCommandExecutor.java
Outdated
Show resolved
Hide resolved
src/main/java/io/appium/java_client/remote/AppiumCommandExecutor.java
Outdated
Show resolved
Hide resolved
src/main/java/io/appium/java_client/remote/AppiumCommandExecutor.java
Outdated
Show resolved
Hide resolved
src/main/java/io/appium/java_client/remote/AppiumCommandExecutor.java
Outdated
Show resolved
Hide resolved
src/main/java/io/appium/java_client/remote/AppiumCommandExecutor.java
Outdated
Show resolved
Hide resolved
src/main/java/io/appium/java_client/remote/AppiumCommandExecutor.java
Outdated
Show resolved
Hide resolved
src/main/java/io/appium/java_client/remote/AppiumCommandExecutor.java
Outdated
Show resolved
Hide resolved
src/main/java/io/appium/java_client/remote/AppiumCommandExecutor.java
Outdated
Show resolved
Hide resolved
src/main/java/io/appium/java_client/remote/AppiumCommandExecutor.java
Outdated
Show resolved
Hide resolved
I assume a vanilla redirect (code 302) would work better for such purpose than all this logic with response parsing and multiple values that eventually construct the same resulting url |
I agree with your intention basically, but I think the behavior was a bit different by this.: |
Thanks for the explanation. In such case redirect is probably won't work, although I still don't see an actual need to provide 4 different values if we still need to combine them into a single url. It would probably too late to change the current implementation as it is already present in other clients |
thanks. |
Changed to draft to update reviews |
Updated. #1779 includes UA stuff as another PR |
|
||
private final AppiumClientConfig appiumClientConfig; | ||
|
||
private static class DirectConnect { |
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 would say it is ok to have this class public. Also, you could use lombok for this purpose, which greatly reduces the amount of boilerplate code for POJOs
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.
also having it public simplifies unit testing
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.
Moved to a new public class file. I'll try the lambok later as another pr after the UA change (So far, I've left a TODO comment)
src/main/java/io/appium/java_client/remote/AppiumCommandExecutor.java
Outdated
Show resolved
Hide resolved
src/main/java/io/appium/java_client/remote/AppiumCommandExecutor.java
Outdated
Show resolved
Hide resolved
private static final String DIRECT_CONNECT_HOST = "directConnectHost"; | ||
private static final String DIRECT_CONNECT_PORT = "directConnectPort"; | ||
|
||
public final String 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.
please create public accessors rather than making properties public (e.g. protocol -> getProtocol()). Later we could simplify that code with lombok
class DirectConnectTest { | ||
|
||
@Test | ||
void hasValidDirectConnectValuesWithoutAppiumPrefix() throws MalformedURLException { |
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.
👍
} | ||
|
||
@Test | ||
void hasValidDirectConnectInvalid() { |
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.
this test name does not make much sense to me
|
||
@Override | ||
public AppiumClientConfig readTimeout(Duration timeout) { | ||
ClientConfig clientConfig = super.connectionTimeout(timeout); |
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 should call super.readTimeout(timeout)
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.
Thank you, fixed in #1959
Change list
I'd like to add https://appiumpro.com/editions/86-connecting-directly-to-appium-hosts-in-distributed-environments for Java client. Other clients such as webdriverio, Ruby and Python already have them for long. If the response had directConnect capabilities, the appium client attempt to send requests to the directConnect one instead of the original URL.
or
appium:
prefixed ones.Types of changes
What types of changes are you proposing/introducing to Java client?
Put an
x
in the boxes that applyDetails
The main change is
setDirectConnect
in src/main/java/io/appium/java_client/remote/AppiumCommandExecutor.javaTo allow users to turn it on/off, I've added
AppiumClientConfig
and give the instance to the driver.The example code is like...
By inheriting
ClientConfig
, we can set a custom User Agent as well. This PR includes the way to set the UA likeappium/8.3.0 (selenium/4.5.0 (java mac))
The naming and implementation methods in AppiumClientConfig follow ClientConfig as possible.
ImeHandler
removal in the test code was selenium 4.5.0 removed it.