Conversation
src/main/java/io/appium/java_client/remote/AndroidMobileOptions.java
Outdated
Show resolved
Hide resolved
titusfortner
force-pushed
the
options_classes
branch
from
84ffa10 to
0681576
Compare
|
So overall this approach is good? I'm ok to go through & add docstrings to |
Like I said I'm more into immutable data structures, but if Selenium is already doing it this way then there is no point for us not to follow it and provide different experience to users. @SrinivasanTarget Please share your thoughts on the feature |
|
Approach looks fine to me in terms of giving same experience as of Selenium. |
titusfortner
force-pushed
the
options_classes
branch
from
499a385 to
e6b7950
Compare
titusfortner
force-pushed
the
options_classes
branch
3 times, most recently
from
0661be1 to
5e8de63
Compare
titusfortner
force-pushed
the
options_classes
branch
from
eecafb2 to
686273c
Compare
|
All the tests are passing finally. Once I figure out what this is actually supposed to be: appium/appium#14133 I'll do a rebase to tidy up the commits. |
| */ | ||
| public Duration getAdbExecTimeout() { | ||
| Object duration = getCapability(AndroidMobileCapabilityType.ADB_EXEC_TIMEOUT); | ||
| if (duration.getClass().isAssignableFrom(Long.class)) { |
There was a problem hiding this comment.
return Duration.ofMillis(Long.valueOf("" + duration));
There was a problem hiding this comment.
same below.
Also, this could be extracted to a helper method to avoid unnecessary duplication
| * @see AndroidMobileCapabilityType#ALLOW_TEST_PACKAGES | ||
| */ | ||
| public boolean doesAllowTestPackages() { | ||
| return (boolean) getCapability(AndroidMobileCapabilityType.ALLOW_TEST_PACKAGES); |
There was a problem hiding this comment.
Above we return Integer (object type) for a number value, but here we do boolean (primitive type). I assume we should follow the common pattern everywhere and return either all primitive (in such case NPE will be thrown if the corresponding value is null) or object types
| * @return this MobileOptions, for chaining. | ||
| * @see AndroidMobileCapabilityType#ANDROID_DEVICE_READY_TIMEOUT | ||
| */ | ||
| public AndroidMobileOptions setAndroidDeviceReadyTimeout(Duration duration) { |
There was a problem hiding this comment.
It looks like this cap is not used anywhere in appium code: https://github.com/search?q=org%3Aappium+androidDeviceReadyTimeout&type=Code
| * @return this MobileOptions, for chaining. | ||
| * @see AndroidMobileCapabilityType#ADB_PORT | ||
| */ | ||
| public AndroidMobileOptions setAdbPort(Integer port) { |
There was a problem hiding this comment.
int port. It makes no sense to allow setting it to null
| * @see AndroidMobileCapabilityType#ANDROID_DEVICE_READY_TIMEOUT | ||
| */ | ||
| public Duration getAndroidDeviceReadyTimeout() { | ||
| Object duration = getCapability(AndroidMobileCapabilityType.ANDROID_DEVICE_READY_TIMEOUT); |
| * @return this MobileOptions, for chaining. | ||
| * @see AndroidMobileCapabilityType#ANDROID_INSTALL_PATH | ||
| */ | ||
| public AndroidMobileOptions setAndroidInstallPath(String path) { |
There was a problem hiding this comment.
this cap is not used anywhere in the code: https://github.com/search?q=org%3Aappium+androidInstallPath&type=Code
| * @return this MobileOptions, for chaining. | ||
| * @see AndroidMobileCapabilityType#ANDROID_NATURAL_ORIENTATION | ||
| */ | ||
| public AndroidMobileOptions setAndroidNaturalOrientation() { |
There was a problem hiding this comment.
this capability only works for the obsolete UIAutomator1 automation backend
| * @return this MobileOptions, for chaining. | ||
| * @see AndroidMobileCapabilityType#ANDROID_SCREENSHOT_PATH | ||
| */ | ||
| public AndroidMobileOptions setAndroidScreenshotPath(String path) { |
There was a problem hiding this comment.
this capability only works for the obsolete UIAutomator1 automation backend
| * @return this MobileOptions, for chaining. | ||
| * @see AndroidMobileCapabilityType#APP_ACTIVITY | ||
| */ | ||
| public AndroidMobileOptions setAppActivity(String activity) { |
There was a problem hiding this comment.
I think it makes sense to add a link to the documentation, because users quite often have difficulties understanding what activity name concept is: https://github.com/appium/appium/blob/master/docs/en/writing-running-appium/android/activity-startup.md
| * @return this MobileOptions, for chaining. | ||
| * @see AndroidMobileCapabilityType#APP_PACKAGE | ||
| */ | ||
| public AndroidMobileOptions setAppPackage(String pkg) { |
There was a problem hiding this comment.
⬆️ same as for activity name
| * @return this MobileOptions, for chaining. | ||
| * @see AndroidMobileCapabilityType#APP_WAIT_ACTIVITY | ||
| */ | ||
| public AndroidMobileOptions setAppWaitActivity(String activity) { |
| * @return this MobileOptions, for chaining. | ||
| * @see AndroidMobileCapabilityType#APP_WAIT_DURATION | ||
| */ | ||
| public AndroidMobileOptions setAppWaitDuration(Duration duration) { |
| * @see AndroidMobileCapabilityType#APP_WAIT_DURATION | ||
| */ | ||
| public Duration getAppWaitDuration() { | ||
| Object duration = getCapability(AndroidMobileCapabilityType.APP_WAIT_DURATION); |
| /** | ||
| * Set the Timeout used to wait for the appWaitActivity to launch. | ||
| * | ||
| * @param duration is the number of milliseconds to wait for the appWaitActivity to launch. |
There was a problem hiding this comment.
should be unit-agnostic now
| * @see AndroidMobileCapabilityType#AUTO_WEBVIEW_TIMEOUT | ||
| */ | ||
| public Duration getAutoWebviewTimeout() { | ||
| Object duration = getCapability(AndroidMobileCapabilityType.AUTO_WEBVIEW_TIMEOUT); |
| * @see AndroidMobileCapabilityType#AVD_LAUNCH_TIMEOUT | ||
| */ | ||
| public Duration getAvdLaunchTimeout() { | ||
| Object duration = getCapability(AndroidMobileCapabilityType.AVD_LAUNCH_TIMEOUT); |
| /** | ||
| * Set the wait time for an avd to finish its boot animations. | ||
| * | ||
| * @param duration is the number of milliseconds to wait for an avd to finish its boot animations. |
| * @see AndroidMobileCapabilityType#AVD_READY_TIMEOUT | ||
| */ | ||
| public Duration getAvdReadyTimeout() { | ||
| Object duration = getCapability(AndroidMobileCapabilityType.AVD_READY_TIMEOUT); |
| } | ||
|
|
||
| /** | ||
| * Get the path to webdriver executable. |
There was a problem hiding this comment.
|
|
||
| /** | ||
| * Set the directory to look for Chromedriver executables in, for automatic discovery of compatible Chromedrivers. | ||
| * |
| * @return ChromeOptions set for use with ChromeDriver. | ||
| * @see AndroidMobileCapabilityType#CHROME_OPTIONS | ||
| */ | ||
| public Object getChromeOptions() { |
| /** | ||
| * Set the timeout for device to become ready. | ||
| * | ||
| * @param duration is the number of seconds to wait for the device to become ready. |
| * @see AndroidMobileCapabilityType#DEVICE_READY_TIMEOUT | ||
| */ | ||
| public Duration getDeviceReadyTimeout() { | ||
| Object duration = getCapability(AndroidMobileCapabilityType.DEVICE_READY_TIMEOUT); |
|
@titusfortner I think it makes sense to split this PR into smaller parts, for example:
Then it would be much easier to review it |
|
@mykola-mokhnach thanks for your painstaking reviews of this; closing to open a new PR. |
3 participants
Change list
IOSMobileOptionsandAndroidMobileOptionsfor easier capability generationTypes of changes
What types of changes are you proposing/introducing to Java client?
Details
This is to follow the same approach that Selenium Java bindings have taken with their Browser Options classes. Since
DesiredCapabilitieswill soon be deprecated, users will either need to update toMutableCapabilitiesor otherwise use an Options class like this. An Options class has the advantage of encapsulating specific behaviors to IOS and Android.Note that this is a draft to get feedback before I implement all of the IOS & Android specific capabilities.
Honestly, this would be a lot less time consuming if we do some reflection magic with Java 9, but I don't think Appium is at the point where it is willing to require > 8?