Skip to content
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

common interface for android and ios #265

Closed
wants to merge 1 commit into from

Conversation

alizelzele
Copy link
Contributor

As discussed in #259 , this is a fork for having a common interface for both Android and Ios.
this PR contains the changes required to do the job. this Pr contains the following changes:
1- adding a new android project to sample-apps which is more like the IOS sample app version for testing.
2- changing code generator to support new system and generate files according to new requirement.
3- creating new common elements interfaces and implementing them for ios and android.
4- changing yaml reader to be able to read new style of yaml files.
5- adding some config properties for selecting platform and setting MobileElement annotation parameters from test suite.
6- changing config builder to support selenium grid with multiple appium instances with diferent platforms.
7- adding some test cases and a suite for testing if new system works properly.

Please let me know if any other change is required.

@mach6
Copy link
Contributor

mach6 commented Mar 23, 2016

@alizelzele Please update your PR. It needs to be able to compile. It looks like you are missing some files. Also, please squash your commits down to no more than 3 salient commits. Thanks!

@mach6
Copy link
Contributor

mach6 commented Mar 23, 2016

@alizelzele Also fyi, our commit comment format is as follows;

Short description on a single line. 
{empty line}
Detailed description. Can span many lines

@mach6
Copy link
Contributor

mach6 commented Mar 23, 2016

@alizelzele Was there no way to add the new android page objects functionality/needs into the existing app? How difficult would it be to do this? I'm trying to minimize confusion for anyone looking (developer, other) at our test apps.. To me, it is much less confusing to have one clear app for iOS and another for Android. At a minimum, can you give the new app a name (and corresponding top level directory) which better conveys the difference when compared to the pre-existing android page objects app?

@mach6
Copy link
Contributor

mach6 commented Mar 23, 2016

@kumaravel-jayakumar @fakrudeen78 @sundaramrajendran @sebady I'm going to need some help reviewing this one, please :)

@mach6
Copy link
Contributor

mach6 commented Mar 23, 2016

Note for reviewers:

[X] @alizelzele covered by the CLA. CLA signed.

@mach6
Copy link
Contributor

mach6 commented Mar 23, 2016

@alizelzele Please correct your [user.name] in your ~/.gitconfig. Your commits use

 "ali.kia" <ali.kia@symbio.com>

Or contribution policy asks that you set your [user.name] to be your full name with proper use of case and punctuation.. Here's a good writeup on how to set this - https://help.github.com/articles/setting-your-username-in-git/

to fix this in your existing commits, you will have to amend your commits and reset the author

@alizelzele
Copy link
Contributor Author

@mach6 Sorry, i did not know where to answer to your comments, so i add a comment here.
1- copyrights are changed and fixed.
2- sorry about compile problem. i forgot to add a test page file to git. it is added now.
3- i will squash them as soon as i solve naming with android app.
4- the new Android app is almost the same as ios version for testing porpose, if i had changed the previous app, there would be lots of changes and lots of test cases would need to be updated, so i decided to but it that way. but for sure, it could have a better name. i am not good with naming ( ;) ), do you have any suggestion for the name of new app, or you think it worth to make it only one app and change test cases according to that one?
5- after i squash them all into one commit, the user.name problem will be solved.

thank you.

@Override
public Map<String, String> getGuiMap(String locale, WebDriverPlatform platform) {
return getGuiMap(locale);
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we're ignoring platform param here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sebady from comment in com.paypal.selion.reader.YamlV1Reader(line 56), i figured out mobile platform does not need to be supported in yamV1 and i remember i saw somewhere yamlV1 is deprecated if i am not wrong. does it needs to support mobile platform?

@alizelzele
Copy link
Contributor Author

@mach6 i changed the old test cases to work with the new android app and removed the old one.
@sebady from com.paypal.selion.reader.YamlV1Reader(line 56) that says PageYAML V1 only needs to support platform = web and some where that i do not actually remember that was saying "Yaml V1 is deprecated" i figured out it is not required to add this functionality to this version and only V2 is enough. shall i add this functionality to V1 too?

@mach6
Copy link
Contributor

mach6 commented Mar 26, 2016

@alizelzele please do not update PageYaml V1.. Yes, it is deprecated (and undocumented) .. It will be removed in a future release.
also, thank you for updating the android tests.

@mach6
Copy link
Contributor

mach6 commented Mar 29, 2016

Hi @alizelzele fyi, I'm still waiting for your updates -- squashed commits, etc. cheers.

@alizelzele
Copy link
Contributor Author

@mach6 sorry i was waiting for review and comments before squashing them all to one.
i will do that ASAP.

@mach6
Copy link
Contributor

mach6 commented Mar 30, 2016

Thanks @alizelzele .. It makes it easier on the reviewer(s) when the changes are in a single (or 3 max) commits (and corresponding diffs) to go through each change.. It enables us to review them offline without also cross-referencing the output on github and the PR

@alizelzele
Copy link
Contributor Author

@mach6 It was a bit scary to force push to server, but it is done now. :-)
Hope i did not broke anything while force pushing :-)
It is my first time forking and pull requesting. I did not know about making that easier for reviewers and others by making it only one commit. Sorry for late reset.

@@ -111,8 +114,11 @@ public void setIosCustomElements(String[] elements) {
public void setAndroidCustomElements(String[] elements) {
androidCustomElements = Arrays.asList(elements);
}



Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alizelzele don't worry about this one

@@ -111,8 +114,11 @@ public void setIosCustomElements(String[] elements) {
public void setAndroidCustomElements(String[] elements) {
androidCustomElements = Arrays.asList(elements);
}



Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mach6 I amend commit the changes for code quality.

@@ -111,8 +114,11 @@ public void setIosCustomElements(String[] elements) {
public void setAndroidCustomElements(String[] elements) {
androidCustomElements = Arrays.asList(elements);
}



Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mach6 I solved all issues in codacy, but it still fails. Am i missing something?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you're okay. you fixed them all but this one. this reports show it -- https://www.codacy.com/app/SeLion/paypal-SeLion/pullRequest?prid=212622

Please don't fix this one. You followed the pattern the file had here. So, we will make an exception

private static final String ACTION_BUTTON_LOCATOR = "com.paypal.selion.pageobjectsdemoapp:id/action_button";
private static final String LONG_PRESS_BUTTON_LOCATOR = "com.paypal.selion.pageobjectsdemoapp:id/long_press_button";
private static final String TEXT_VIEW_LOCATOR = "com.paypal.selion.pageobjectsdemoapp:id/long_press_button_output";
private static final String ACTION_BUTTON_LOCATOR = "com.symbio.pageobject:id/btnNext";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please make these all com.paypal.selion

@mach6
Copy link
Contributor

mach6 commented Apr 12, 2016

@alizelzele will you please submit a PR for the doc updates that coincide with this PR's feature changes? Documentation is on the gh-pages branch. Thanks!

@alizelzele
Copy link
Contributor Author

@mach6 I changed all the parts you asked for except the documentation part.
is documentation essential for pull request? since it is in another branch i thought maybe it can be done later.

@mach6
Copy link
Contributor

mach6 commented Apr 13, 2016

@alizelzele Thanks. The documentation will be needed though it will not prevent this PR from moving forward. This said, I'd like to minimize the gap between merging this PR and the documentation updates. So, if you could please start the changes on your gh-pages branch and work towards opening a new PR, I would appreciate it. Also, pending doc changes could help those of us reviewing this PR understand the approach a bit better. :)

  1. adding a common interface for both android and ios objects
  2. changing yaml reader and code generator to  support new interface and yaml files.
  3. add more elements to Android elements.
  4. add parameters to config Mobile annotation parameters from suite and command line.
  5. creating new android test app that match with ios test app to be able to test common interface.
  6. adding test suites and test cases for testing common interface and updating old test cases to be able to work with new one.
*
* @parameter expression="${selion-code-generator.mobileCustomElements}"
*/
private List<String> mobileCustomElements;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alizelzele fyi, you will be impacted here in a minor way by #287 if/when it is merged.
We can fix when before we merge your PR.

@@ -0,0 +1,130 @@
package com.paypal.selion.mobile.sample.mobile;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs a copy of the copyright

@mach6
Copy link
Contributor

mach6 commented May 31, 2016

@alizelzele fyi - I'm working on this code. I have rebased / merged it with the latest changes on develop .. My goal is to move it to a branch on this repo for final review and merge it asap.

Here is the start of this work -- https://github.com/mach6/SeLion/commits/alizelzele-develop

I might reach out to you for some more help. Hope that is okay!

@alizelzele
Copy link
Contributor Author

@mach6 Sure, let me know how i can be of any help.

@mach6
Copy link
Contributor

mach6 commented Jun 3, 2016

@alizelzele @sebady Moved this PR to the local branch https://github.com/paypal/SeLion/tree/alizelzele-develop for final readiness. Also opened #296 for further tracking/review

@mach6 mach6 closed this Jun 3, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants