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

Add apple tvOS platform constant #1142

Merged
merged 4 commits into from
May 25, 2019
Merged

Add apple tvOS platform constant #1142

merged 4 commits into from
May 25, 2019

Conversation

ajeeshvl
Copy link
Contributor

@ajeeshvl ajeeshvl commented May 12, 2019

Change list

This is to add support for apple tvOS.

Types of changes

What types of changes are you proposing/introducing to Java client?

  • No changes in production code.
  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Details

This adds tvOS platform.
tvOS also uses xcuitest-driver.

Related PRs
appium/appium-xcuitest-driver#911
appium/appium#12401

extends AppiumDriver<T>
implements HidesKeyboardWithKeyName, HasIOSSettings, HasOnScreenKeyboard,
LocksDevice, FindsByIosNSPredicate<T>, FindsByIosClassChain<T>,
PushesFiles, CanRecordScreen, HasIOSClipboard {
Copy link
Member

Choose a reason for hiding this comment

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

Did you check all of commands work? (just a confirmation.)
Clipboad does not work on this list.

@mykola-mokhnach
Copy link
Contributor

For me it does not make sense to have a separate class, which, actually, adds nothing. We already had youidriver like that and removed it.

@ajeeshvl
Copy link
Contributor Author

@mykola-mokhnach sounds good. Since AppiumDriver is not abstract, we can use that. Do you think, platform name tvOS can still be added in MobilePlatform class?

@mykola-mokhnach
Copy link
Contributor

Do you think, platform name tvOS can still be added in MobilePlatform class?

Yes, this will be useful to have a constant for it

@ajeeshvl ajeeshvl changed the title Add apple tvOS driver support Add apple tvOS platform constant May 13, 2019
@ajeeshvl
Copy link
Contributor Author

@mykola-mokhnach @KazuCocoa @SrinivasanTarget removed TVOSDriver class.

Copy link
Member

@KazuCocoa KazuCocoa left a comment

Choose a reason for hiding this comment

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

thanks!

@@ -22,4 +22,5 @@
String IOS = "iOS";
String FIREFOX_OS = "FirefoxOS";
String WINDOWS = "Windows";
String TVOS = "tvOS";
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it make sense to have it inside AutomationName interface instead?

Copy link
Member

@KazuCocoa KazuCocoa May 14, 2019

Choose a reason for hiding this comment

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

No, I do not think so in this case.
It confuses users a bit since when we use tvOS, the capability should be like:

...
automationName: 'xcuites', # AutomationName.IOS_XCUI_TEST
platformName: 'tvOS'      # AutomationName.IOS_TVOS vs MobilePlatform.TVOS (or APPLE_TVOS is proper?)
...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unlike youiengine tvos uses xcuitest as automation name. I still think having a TVOSdriver would also be helpful. That would avoid confusion for users around what capability needs to be used for tvos.

Copy link
Member

Choose a reason for hiding this comment

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

I don't have a strong opinion about the new class.
I would respect @mykola-mokhnach, @saikrishna321 and @SrinivasanTarget

Copy link
Member

Choose a reason for hiding this comment

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

I would say not to have TVOSDriver class. We should just document what capability to use for TVOS.

@KazuCocoa KazuCocoa merged commit c559326 into appium:master May 25, 2019
@Ljo2017 Ljo2017 mentioned this pull request Jun 24, 2019
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.

5 participants