Skip to content

Conversation

@TikhomirovSergey
Copy link
Contributor

I have made up some interfaces.

There are interfaces that declare general behavior and specific for Android and iOS.

They unite optional methods, related getters/setters and methods that logically related (my point of view). Some ones declare single methods, but I think it is possible that new optional method would be there.

Some methods of AppiumDriver marked by this comment

//Should be moved to the subclass

If there is ok, it is the beginning of fixing #105.

@Jonahss
Copy link
Member

Jonahss commented Sep 11, 2014

Cool!
Do you think you're going overboard a little bit with all the interfaces? It's quite a lot. Do you anticipate having many more implementations than just iOS and Android? I guess things like wearables and such could benefit...

Just don't want to overcomplicate too much. I fee like the Selenium project is a bit over-generalized. If you explain your reasoning, I think I will agree with you :)

A couple thoughts:

  • Can we combine some of the interfaces? Like PullsFiles and PushesFiles could be one InteractsWithFiles.
  • Maybe we could put all the TouchAction convenience methods into one? So Pinch, Swipe, Tap, Zoom could all be in a TouchShortcuts interface.

Could you push to a new branch and we keep all the changes there until we are ready to release v2.0.0?

@bootstraponline
Copy link
Member

What about only 3 interfaces: Common, Android, and IOS?

@Jonahss
Copy link
Member

Jonahss commented Sep 11, 2014

The way the selenium project is already implemented, having many interfaces is more in the style, so I'm ok with that :)

Also, the TouchAction interface is great, because we can merge that directly to Selenium right away. For webdriver3

@bootstraponline
Copy link
Member

ok 👍

@TikhomirovSergey
Copy link
Contributor Author

Ok!
I combined some interfaces.

I don't know how new interfaces can be implemented someway else by Apppium APIs(except AppiumDriver, AndroidDriver and IOSDriver). But it is possible, that they can be useful outside.

For example.

...
public class MyScreenObject implements TouchShortcuts{
...

My point. For this page/screen object generalized interfaces such as

General
Android
IOS

would be excessive. So, this is flexible and logically final. Declared methods of each new interface are enough for possible user components or methods. These components will be easy to synchronize with Appium modifications.

Of course, I think this interface model will be useful for Appium in future.

@TikhomirovSergey
Copy link
Contributor Author

Like PullsFiles and PushesFiles could be one InteractsWithFiles.

I am confused by comment

/**
   * Save base64 encoded data as a file on the remote mobile device.
   * This is an Android only method.
   * @param remotePath Path to file to write data to on remote device
   * @param base64Data Base64 encoded byte array of data to write to remote device
   */
  public void pushFile(String remotePath, byte[] base64Data) {

If it is fake there can be one interface instead of two.

@Jonahss
Copy link
Member

Jonahss commented Sep 12, 2014

Ah, I didn't catch that it's an Android-only method. Guess we have to keep it as two then.

@Jonahss
Copy link
Member

Jonahss commented Sep 12, 2014

Cool, do you want to continue, and move the functions with the // should be moved to subclass comments?

@TikhomirovSergey
Copy link
Contributor Author

Cool, do you want to continue, and move the functions with the // should be moved to subclass >comments?

I is the notification when refactor will be started.

I can finish this work from end to end. I would not mind if you want to make something. We can divide the work at least. :)

If there if all right with interfaces, I think they can be added to master or some branch.

@TikhomirovSergey
Copy link
Contributor Author

We can divide the work at least. :)

I can implement AndroidDriver and move methods marked by comment. I have some problems with iOS (hackintosh). And I dont know how to implement new scrolls

@Jonahss
Copy link
Member

Jonahss commented Sep 12, 2014

cool! I'll merge this PR into a branch, and then you can work on AndroidDriver. I'll be mostly busy until the middle of next week, but can work on IOSDriver then. I can work out the scrolling methods too, they will use UiScrollable and stuff :)

@Jonahss
Copy link
Member

Jonahss commented Sep 12, 2014

the lock() method is iOS only. just found out.

@Jonahss
Copy link
Member

Jonahss commented Sep 12, 2014

created branch interfacesRefactor

@TikhomirovSergey
Copy link
Contributor Author

Thanks, @Jonahss!

After a break (one or two days) I will continue. There will be few pull-requests

@Jonahss
Copy link
Member

Jonahss commented Sep 15, 2014

Awesome ^.^

On Sat, Sep 13, 2014 at 9:08 AM, Sergey Tikhomirov <notifications@github.com

wrote:

Thanks, @Jonahss https://github.com/Jonahss!

After a break (one or two days) I will continue. There will be few
pull-requests


Reply to this email directly or view it on GitHub
#108 (comment).

@TikhomirovSergey
Copy link
Contributor Author

Hi!
I have a question.

Are methods

isAppInstalled
installApp
removeApp

general? Am little bit confused because isAppInstalled is checked by

MobileDriverAndroidTest.java

but is not checked by MobileDriverIOSTest.java.

AdroidDriver has been implemented and now it is under test. I think the new commit will be today evening (Moscow time).

@0x1mason
Copy link
Contributor

lock/unlock/isLocked are all awaiting merge for Android, if that makes any difference

@TikhomirovSergey
Copy link
Contributor Author

...and one more question

Is something wrong with my environment?
Win7
Genymotiom (virtual machine) instead of Android SDK emulator
Appium node server 1.2.2

There are 3 tests that fails in my fork and in master (I have cloned it for comparison).
Tests

MobileDriverAndroidTest

@test
public void ignoreUnimportantViews()

@test
public void startActivityInThisAppTest()

@test
startActivityInAnotherAppTest()

Exception (is taken from cloned master):

org.openqa.selenium.UnsupportedCommandException: That URL did not map to a valid JSONWP resource
Command duration or timeout: 5 milliseconds
Build info: version: '2.42.2', revision: '6a6995d31c7c56c340d6f45a76976d43506cd6cc', time: '2014-06-03 10:52:47'
System info: host: 'xxxx', ip: 'xxxx', os.name: 'Windows 7', os.arch: 'amd64', os.version: '6.1', java.version: '1.8.0_05'
Driver info: io.appium.java_client.AppiumDriver
Capabilities [{app=D:\java-client\src\test\java\io\appium\java_client\ApiDemos-debug.apk, networkConnectionEnabled=true, warnings={}, databaseEnabled=false, deviceName=Android Emulator, platform=LINUX, desired={app=D:\java-client\src\test\java\io\appium\java_client\ApiDemos-debug.apk, newCommandTimeout=120, browserName=, platformName=Android, deviceName=Android Emulator}, newCommandTimeout=120, platformVersion=4.1, webStorageEnabled=false, locationContextEnabled=false, browserName=, takesScreenshot=true, javascriptEnabled=true, platformName=Android}]
Session ID: e6ee1bf3-9e40-46b0-bd6d-76593c1f354a
    at sun.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method)
    at sun.reflect.NativeConstructorAccessorImpl.newInstance(Unknown Source)
    at sun.reflect.DelegatingConstructorAccessorImpl.newInstance(Unknown Source)
    at java.lang.reflect.Constructor.newInstance(Unknown Source)
    at org.openqa.selenium.remote.ErrorHandler.createThrowable(ErrorHandler.java:204)
    at org.openqa.selenium.remote.ErrorHandler.throwIfResponseFailed(ErrorHandler.java:156)
    at org.openqa.selenium.remote.RemoteWebDriver.execute(RemoteWebDriver.java:599)
    at io.appium.java_client.AppiumDriver.execute(AppiumDriver.java:102)
    at io.appium.java_client.AppiumDriver.startActivity(AppiumDriver.java:204)
    at io.appium.java_client.MobileDriverAndroidTest.startActivityInAnotherAppTest(MobileDriverAndroidTest.java:152)
    at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
    at sun.reflect.NativeMethodAccessorImpl.invoke(Unknown Source)
    at sun.reflect.DelegatingMethodAccessorImpl.invoke(Unknown Source)
    at java.lang.reflect.Method.invoke(Unknown Source)
    at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:47)
    at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
    at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:44)
    at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)
    at org.junit.internal.runners.statements.RunBefores.evaluate(RunBefores.java:26)
    at org.junit.internal.runners.statements.RunAfters.evaluate(RunAfters.java:27)
    at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:271)
    at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:70)
    at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:50)
    at org.junit.runners.ParentRunner$3.run(ParentRunner.java:238)
    at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:63)
    at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:236)
    at org.junit.runners.ParentRunner.access$000(ParentRunner.java:53)
    at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:229)
    at org.junit.runners.ParentRunner.run(ParentRunner.java:309)
    at org.eclipse.jdt.internal.junit4.runner.JUnit4TestReference.run(JUnit4TestReference.java:50)
    at org.eclipse.jdt.internal.junit.runner.TestExecution.run(TestExecution.java:38)
    at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.runTests(RemoteTestRunner.java:467)
    at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.runTests(RemoteTestRunner.java:683)
    at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.run(RemoteTestRunner.java:390)
    at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.main(RemoteTestRunner.java:197)

@TikhomirovSergey
Copy link
Contributor Author

I have added AndroidDriver. You can review this.

@Jonahss
Copy link
Member

Jonahss commented Sep 19, 2014

Cool!
Those tests fail for you because you're on v1.2.2 instead of master.

@Jonahss
Copy link
Member

Jonahss commented Sep 19, 2014

Looks awesome. now we just need iOS driver. I'll see about adding it :)

@TikhomirovSergey
Copy link
Contributor Author

Hi, @Jonahss!
I have implemented IOSDriver. So it can be pushed to interfacesRefactor branch or master if it is OK. Also I had thought and decided to add interface InteractsWithApps. It is implemented.

Now we have ability to implement the same behavior differently for each mobile platform. :)

@0x1mason

lock/unlock/isLocked are all awaiting merge for Android, if that makes any difference

I didn't anything. I refactored the present functionality. But in the future there can be made up a new interface (PerformsLock, for example), which will be implemented differently for iOS and Android.

@TikhomirovSergey
Copy link
Contributor Author

I want to ask a question. What about FirefoxOSDriver? Maybe WindowsMobileDriver,
if some features are going to be made for Appium server. Are there these plans?

@0x1mason
Copy link
Contributor

Windows phone is something we'd like Appium to support, but AFAIK no
timetable has been set for integration. Whenever we add Win phone support
to Appium, client support will follow quickly. There's a C# WebDriver lib
for Win phone on codeplex if you're comfortable with Mono or .Net. I have
no idea how well it works tho.

Iiuc, FF driver is out of sync with the latest Firefoxos changes, but Jonah
may know more.

On Sunday, September 21, 2014, Sergey Tikhomirov notifications@github.com
wrote:

I want to ask a question. What about FirefoxOSDriver? Maybe
WindowsMobileDriver,
if some features are going to be made for Appium server. Are there these
plans?


Reply to this email directly or view it on GitHub
#108 (comment).

@Jonahss
Copy link
Member

Jonahss commented Sep 22, 2014

Awesome, I will merge it and run all the tests today 👍

When windows phone support comes along, we can just add a new WindowsDriver. The work you've done makes this easy :D

Appium supported FirefoxOS a long time ago. It technically still does, but nobody has tried to run that code in a long time! We don't test it. At this point it's hard to tell what works and what doesn't. If somebody wants it to work we could help them through the process of submitting a pull request.

@TikhomirovSergey
Copy link
Contributor Author

Ok! Thanks. I am waiting for the result :)

As for Firefox OS... I could try it later. Appiun server has to be rebuild (I found the posted issue #3646)

@Jonahss
Copy link
Member

Jonahss commented Sep 22, 2014

Workin on the refactor stuff. Moved around a couple functions, and
organizing the tests...

On Mon, Sep 22, 2014 at 12:18 PM, Sergey Tikhomirov <
notifications@github.com> wrote:

Ok! Thanks. I am waiting for the result :)

As for Firefox OS... I could try it later. Appiun server has to be rebuild
(I found the posted issue, milestone is 1.5 or 2.0)


Reply to this email directly or view it on GitHub
#108 (comment).

@TikhomirovSergey
Copy link
Contributor Author

...so it could be closed.

Jonahss pushed a commit to Jonahss/java-client that referenced this pull request Sep 26, 2014
Jonahss pushed a commit to Jonahss/java-client that referenced this pull request Sep 26, 2014
Jonahss pushed a commit to Jonahss/java-client that referenced this pull request Sep 26, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants