-
-
Notifications
You must be signed in to change notification settings - Fork 758
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
Added You.i Engine Support #429
Conversation
We will squash these commits. |
Hi @pfoster-youitv @philgreenyouilabs |
Thanks @TikhomirovSergey! |
Please and thank you! @TikhomirovSergey |
Hi @TikhomirovSergey any movement on reviewing this? |
@jlipps @pfoster-youitv @philgreenyouilabs Ok. There are remarks. |
The first remark is related to checkstile issues. The current report: site.zip
|
ok. Other remarks and concerns:
New youi engines could be included to the new hierarchy conveniently. What do you think?
https://github.com/appium/java-client/pull/429/files#diff-ce6f39a92ea9fa433bdd2f3f2945f427R79 |
@SrinivasanTarget could you review it too if you can? |
@pfoster-youitv @philgreenyouilabs I agree with @TikhomirovSergey comments.Here are my remarks, 1.Add I enforce again to strictly follow code styles.Will run the tests post all the remarks are addressed. Thanks for your patience. |
The first remark is related to checkstile issues. Answer: We ran the StyleChecker before but it looks like it wasn't run for the last few commits. 1) It seems there is lack of new functions. Are any other features going to be added? Is there a new searching engine? How does the searching by accessibility id feel with it? Answer: The ‘new’ functionality this provides is a unified API that test writers can code against. The reason this feature is important is because most, if not all apps written by our users target multiple platforms (We are a cross-platform engine.) Apps that target the same form factor (iOS and Android handsets and tablets for example) will be indistinguishable from a testing perspective. As for new features, for now we need to catch up on the functionality already provided in other drivers. You can find a full list of currently supported commands here: https://github.com/YOU-i-Labs/appium-youiengine-driver. 2) I do not like that the one AppiumDriver subclass has commands for both iOS and Android. It looks like AppiumDriver looked two years ago. It was containing features for both iOS and Android. It was causing confusions and errors. There were any other things. So I and @Jonahss decided to split it to AndroidDriver and IOSDriver. But the problem is not completely resolved. There is an issue: #398 Answer: A You.i Engine app is developed from a single code-base and deployed to multiple platforms on which it is almost identical in appearance and behaviour. This allows our users to use the same tests across multiple platforms. To facilitate this we would like to provide a single driver API across multiple platforms. Under the hood we will use the existing iOS and Android drivers whenever possible. 3) I have not tried to run the proposed code. But I'm sure that it may not work because these commands are not included to the MobileCommand repository. Answer: Right, we did have issues with these commands. We will clean this up. 1.Add AppiumServiceBuilder in tests. Answer: OK, we will do that 2.I see lot of commented codes here in tests which needs to be removed. https://github.com/YOU-i-Labs/appium-java-client/blob/94992de7988fe9d0699d41a0400ff43e898869ed/src/test/java/io/appium/java_client/YouiEngine/SanityTest.java#L470 Answer: OK, we will do that 3.I suggest to document findBy strategies that appium-youiengine-driver supports. Answer: OK, we will do that 4.Do we really need YouiEngineElement class, If so how is it gonna be different from MobileElement? Answer: I think we just defined YouiEngineElement using AndroidElement or IOSElement as a template. Doesn’t look like we define anything into it... [Edit]: MobileElement is defined as abstract therefore we need to define YouiEngineElement... |
@simongranger So let go on You said
Ok. But the proposed code looks like the unified functionality is valid for the one supported platform and not valid for another. From this string till the end of the file. So I cant get the reason for the such unifying. Are these commands going to be working for both supported platforms? Do I clear understand that these changes have not large impact? The specific team and related teams?
Map<String, CommandInfo> yourMap = new HashMap<>();
yourMap.putAll(MobileCommand.commandRepository);
yourMap.put("yourAdditionalCommand", MobileCommand.postC("/session/:sessionId/the/path/to/the command"));
AppiumCommandExecutor executor = new AppiumCommandExecutor(yourMap, new URL("your URL"));
driver = new IOSDriver<>(executor, capabilities);
//and then you are free to
ImmutableMap.Builder builder = ImmutableMap.builder();
...
execute("yourAdditionalCommand", builder.build()); I thing it is easy to be facilitated and it is flexible. Thanks for the patience. Maybe I just missing something. But it is important to make all thing clear and publish the code as robust as it possible. I'm waiting for your answer/suggestions. |
Lots of good comments here. I'm sorry I haven't responded, I've been on vacation for the last week. I'll catch up with Simon and see what needs to be done. To be clear - the goal is to have test authors write a single test for an app (not device specific calls) once and be able to use it on whatever platform our You.i Engine automation layer supports. We don't want our test authors to need to write different tests for different platforms to test functionality that will be cross platform. We are not talking to Android's automation layer and we are not talking to iOS's automation layer for these tests so there should not be a need to distinguish between the two - just talk directly to our own automation layer. The current disconnect between iOS and Android (in our implementation) are around toggling network settings but we might be able to get around this by calling into our own layer and having it do this instead of trying to use the native automation layers. I'll talk with the team again about this. |
-Fixed Style Checker warnings and errors -Fixed issues reported by codacy-bot -Removed commented code -Synced with MobileCommand.java --Removed toggleWiFi --Removed toggleData --Removed toggleAirplane --Added support for toggle Wifi, Data, Airplane, All via NetworkConnection --Fixed shake --Added pressKeyCode --Added longPressKeyCode
Addressed PR comments
Fixed style issues
Updated SanityTest.java now that backgrounding works
Addressed codacy-bot comments
@TikhomirovSergey We have removed the references to iOS and Android in our java driver and will let the Server return an exception for any commands that are not supported. Does that satisfy your requirements? |
@SrinivasanTarget For now, we don't see the use case for the AppiumServiceBuilder and will consider adding it later if necessary. |
Other than the 2 comments just above. We believe we have addressed all the comments. We'd like to get this PR merge before the end of the week :) |
Hi everyone, I've been following this thread off and on and it seems to me that the proposed changes are reasonable. Would be great to get this out and let youi's users start automating via appium if there are no more objections! |
Moved tests for Size and Location into SanityTest
only `getId()` method is actually used
Now it is needed to add the listenable objet factory and cover it with tests
Also: - improvement of the WIKI chapter - update to org.springframework spring-context v4.3.1.RELEASE
It was found during the self-review.
The bug which has not been caught previously was worked out.
Also update to Selenium 2.53.1
I decided to delay the build 4.1.0 for one day. There is an enhancement (#445) that make access to commands easier. Also:
I'm ok if the new driver would look like https://github.com/TikhomirovSergey/java-client/tree/YOU-i-Labs-develop/src/main/java/io/appium/java_client/youiengine. I would like to merge it at this case. Also there are plans for the next (after 4.1.0) release:
What do uou think about this? ping @SrinivasanTarget @jlipps |
I totally agree with @TikhomirovSergey comments. 👍 |
Depending on what we see later today, this PR will be closed or changed (to similar to what you've noted). YouiEngine code is in the server and there is an existing driver already published in NPM. The SanityTest passes on iOS 100% and nearly 100% on Android with these changes. The android exceptions are logged internally to be addressed. The java client is the only part that is holding back the release and we don't want to have it as an external class to be imported, we want the customers to just be able to get the official Appium files and use them - no extra work. You would prefer they be a minimal subclass from AppiumDriver and other classes? I thought pulling in the platform specific calls would make it easier for users to call. I can remove that if that's the common practice. |
Command executor helper changes
Closing this one - there will be another based on conversation with @TikhomirovSergey |
Change list
Added new io.appium.java_client.YouiEngine package to enable Appium to automate interactions with apps built using You.i Engine.
Package includes:
Under the test side of the package:
isAndroid = false
isAndroid = true
No changes made to existing files, only additions to specific to You.i Engine.
Google coding standards were applied with all error-level violations being corrected.
Types of changes
Details
The only difference between writing a test for You.i Engine and any other Appium platform is the use of
automationName = "youiengine"
and the presence ofyouiEngineAppAddress
. TheyouiEngineAppAddress
must be set to the device's IP address.Review the
YouiEngineAppiumSampleTest.java
for one way to write tests against a Youi Engine app using this Appium client. Adjust the capabilities found within this class to suit the environment the test will be run in, including the target device. The same test class can be used for Android or iOS by toggling theisAndroid
boolean accordingly.Supported Platforms
Android and iOS are currently supported.
Requirements
This client requires the
appium-youiengine-driver
.Available Interactions
Following is a list of available interactions by supported platforms.
Common
* currently supports
className
,id
andname
By
strategies .** currently supports
name
attribute only.*** special key code support not available.
Exclusive to Android
* this implementation takes in the number of seconds to lock the device for.
Exclusive to iOS
* this implementation takes in the number of seconds to lock the device for.
NOTE: Unlock calls will only work when unlocking is set to 'swipe to unlock'.