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.

Sign up for GitHub

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

Jump to bottom

feat: start wda process via Xctest in a real device #687

Merged
merged 14 commits into from
May 2, 2023
Merged

feat: start wda process via Xctest in a real device #687

merged 14 commits into from
May 2, 2023

Conversation

KazuCocoa
Copy link
Member

@KazuCocoa KazuCocoa commented Apr 25, 2023 •
edited
Loading

For appium/appium-xcuitest-driver#1609 to start a WDA process via Xctest in appoum-ios-device. The XCUITest PR will introduce 2 capabilities, usePreinstalledWDA and preInstalledWDABundleId.

preInstalledWDABundleId is to detect which bundle id is preinstalled wda the session want to start to.
usePreinstalledWDA is if a session uses XCTest sessions via appium-ios-device instead of xcodebuild to handle WDA.

One good thing in this aspect is this method can start a WDA process with free apple id account's one.

example capabilities:

{
  "appium:udid": "udid",
  "platformName": "ios",
  "appium:automationName": "xcuitest",
  "appium:usePreinstalledWDA": "true",
  "appium:preInstalledWDABundleId": "com.kazucocoa.WebDriverAgentRunner.xctrunner"
}

I'll try to write test code as a followup pr.

Current CI failures are not related to this change. Maybe the host machine was slow?

@KazuCocoa KazuCocoa mentioned this pull request Apr 25, 2023
Merged
@KazuCocoa KazuCocoa changed the title [wip] feat: start wda process via Xctest in a real device feat: start wda process via Xctest in a real device Apr 25, 2023
@KazuCocoa KazuCocoa marked this pull request as ready for review April 25, 2023 09:04
@KazuCocoa KazuCocoa marked this pull request as draft April 25, 2023 16:38
@KazuCocoa KazuCocoa marked this pull request as ready for review April 26, 2023 07:55
@KazuCocoa KazuCocoa requested review from jlipps and mykola-mokhnach and removed request for jlipps April 26, 2023 07:57
@KazuCocoa KazuCocoa marked this pull request as draft April 29, 2023 08:48
@KazuCocoa KazuCocoa marked this pull request as ready for review April 29, 2023 08:56
@KazuCocoa
Copy link
Member Author

Tested with appium/appium-xcuitest-driver#1609

@KazuCocoa
Copy link
Member Author

updated

@KazuCocoa KazuCocoa mentioned this pull request Apr 30, 2023
Closed
mykola-mokhnach
mykola-mokhnach reviewed Apr 30, 2023
View reviewed changes
lib/webdriveragent.js Outdated
this.log.info('Launching WebDriverAgent on the device without xcodebuild');
this.xctestSessionForWDA = new Xctest(this.device.udid, this.bundleIdForXctest, null, {env: xctestEnv});

await this.xctestSessionForWDA.start();
Copy link

Choose a reason for hiding this comment

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

should we reset this.xctestSessionForWDA if this call or any call below throws an exception?

Copy link
Member Author

Choose a reason for hiding this comment

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

Does reset means this.xctestSessionForWDA = null for example? We can re-call .start unless the target udid hasn't changed. The caller, in xcuitest driver, calls stop in the case, so the this.xctestSessionForWDA.stop will be called in such exception case

mykola-mokhnach
mykola-mokhnach reviewed Apr 30, 2023
View reviewed changes
lib/constants.js Outdated
@@ -1,6 +1,7 @@
import path from 'path';

const WDA_RUNNER_BUNDLE_ID = 'com.facebook.WebDriverAgentRunner';
const WDA_RUNNER_BUNDLE_ID_FOR_XCTEST = 'com.facebook.WebDriverAgentRunner.xctrunner';
Copy link

Choose a reason for hiding this comment

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

const WDA_RUNNER_BUNDLE_ID_FOR_XCTEST = `${WDA_RUNNER_BUNDLE_ID}.xctrunner`;

Copy link

@mykola-mokhnach mykola-mokhnach Apr 30, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

not sure if we need a separate constant for it though if only the suffix differs

Copy link
Member Author

Choose a reason for hiding this comment

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

Either is fine, I wanted to explicitly keep the xctrunner as xctest bundle's bundle id

@KazuCocoa
Copy link
Member Author

updated ✅

mykola-mokhnach
mykola-mokhnach reviewed May 1, 2023
View reviewed changes
lib/webdriveragent.js Outdated
@@ -59,6 +61,10 @@ class WebDriverAgent {

this.updatedWDABundleId = args.updatedWDABundleId;

this.usePreinstalledWDA = args.usePreinstalledWDA;
this.preInstalledWDABundleId = args.preInstalledWDABundleId;
Copy link

Choose a reason for hiding this comment

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

Do we need this capability in general? Can its value be calculated as ${updatedWDABundleId}.xctrunner instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

Could be with additional a few changes. (At least needed to add if condition in a few places) I initially re-used the capability, but this PR itself does not update bundle id itself. So the naming could be confusing for the behavior.
So i defined a new capability to simplify the implementation and purpose at this time.

Copy link

Choose a reason for hiding this comment

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

my point is the less capabilities we have the better it is (easier to maintain)

Copy link
Member Author

@KazuCocoa KazuCocoa May 1, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

yea, understood this point as well. I like less if condition for this case, but then, 211a10f and appium/appium-xcuitest-driver@d2bfc15 help to work with updatedWDABundleId. Then, ignoring xcodebuild instance itself might be safe (rather than adding if condition in xcodebuild instance to avoid unnecessary updatedWDABundleId related procedure).

Copy link
Member Author

@KazuCocoa KazuCocoa May 1, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Also, I addressed the WDA must have been a package built with over Xcode 12 in appium/appium-xcuitest-driver@d2bfc15 for safe to follow xctrunner rule. (We do not need to take care the case lower version case with current our requirement, but for safe as this implementation's requirement.)

KazuCocoa
KazuCocoa commented May 1, 2023
View reviewed changes
lib/webdriveragent.js Outdated
// FIXME: maybe 'this.webDriverAgentUrl' also can ignore
// the xcodebuild instance itself.
if (this.usePreinstalledWDA) {
this.xcodebuild = null;
Copy link
Member Author

Choose a reason for hiding this comment

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

do not return immediately here so that we may write something after this if/else clause in the future. (do not restrict the order)

Copy link

Choose a reason for hiding this comment

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

a ternary operator would probably be better readable here

@KazuCocoa KazuCocoa requested a review from mykola-mokhnach May 1, 2023 16:28
mykola-mokhnach
mykola-mokhnach reviewed May 1, 2023
View reviewed changes
lib/webdriveragent.js Outdated
@@ -286,6 +308,25 @@ class WebDriverAgent {
return await this.getStatus();
}

if (this.isRealDevice && this.usePreinstalledWDA) {
Copy link

Choose a reason for hiding this comment

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

I think we should verify it in xcuitest driver if usePreinstalledWDA cap is set to true and a simulator is selected. In such case a validation error must be thrown

Copy link

Choose a reason for hiding this comment

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

I find the current method a bit to long. Perhaps, it makes sense to move this code block into a separate method?

Copy link
Member Author

Choose a reason for hiding this comment

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

ah, it makes sense

@KazuCocoa KazuCocoa merged commit e1c0f83 into master May 2, 2023
@KazuCocoa KazuCocoa deleted the launch-preinstalled-wda branch May 2, 2023 06:48
github-actions bot pushed a commit that referenced this pull request May 2, 2023
@semantic-release-bot
chore(release): 4.14.0 [skip ci]
d280750 
## [4.14.0](v4.13.2...v4.14.0) (2023-05-02)

### Features

* start wda process via Xctest in a real device ([#687](#687)) ([e1c0f83](e1c0f83))
@github-actions
Copy link

github-actions bot commented May 2, 2023

🎉 This PR is included in version 4.14.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mykola-mokhnach mykola-mokhnach mykola-mokhnach approved these changes

@jlipps jlipps Awaiting requested review from jlipps

Assignees
No one assigned
Labels
released
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@KazuCocoa @mykola-mokhnach