fix: uninstall wda with '.xctrunner' suffix for real device #1052

KazuCocoa · 2019-08-26T17:08:39Z

Will take a look again tomorrow.
(Draft for now)

Built by over Xcode 11:
- On simulator => 'com.facebook.WebDriverAgentRunner.xctrunner'
- On real device => 'xxxxx.xctrunner'. xxxx is this.opts.updatedWDABundleId
Buiild by Xcode 10-
- Only 'com.apple.test.WebDriverAgentRunner-Runner' (both real device and simulator)

imurchie · 2019-08-26T17:28:21Z

lib/wda/webdriveragent.js

+   *
+   * @param {string} updatedWDABundleId The bundle id which will be uninstalled
+   */
+  async uninstall (updatedWDABundleId = WDA_RUNNER_BUNDLE_ID) {


Isn't updatedWDABundleId already sent in as an argument to the constructor? Why not just save that as an instance variable and use it?

imurchie · 2019-08-26T17:29:43Z

lib/wda/webdriveragent.js

+      } else {
+        // On simulator, the bundle id should be 'com.facebook.WebDriverAgentRunner.xctrunner'
+        // even if 'updatedWDABundleId' was given
+        const bundle_id = `${this.realDevice ? updatedWDABundleId : WDA_RUNNER_BUNDLE_ID}${WDA_XCTRUNNER_SUFFIX}`;


Should probably keep with camel case for variable names. I personally would also prefer the conditional to figure out the bundle id, then a single call to removeApp.

mykola-mokhnach · 2019-08-26T18:32:53Z

lib/wda/webdriveragent.js

    try {
-      await this.device.removeApp(WDA_BUNDLE_ID);
+      if (this.xcodeVersion.major < 11) {


it might be that xcode version is unknown for real devices

ah...
Let me check again...

this.xcodeVersion had Xcode version on a real device.
The value might be undefined if this.opts.webDriverAgentUrl was also provided in createSession

The value might be undefined if this.opts.webDriverAgentUrl was also provided in createSession

exactly

mykola-mokhnach · 2019-08-26T18:35:22Z

lib/wda/webdriveragent.js

+   * @param {string} updatedWDABundleId The bundle id which will be uninstalled
+   */
+  async quitAndUninstall (updatedWDABundleId = WDA_RUNNER_BUNDLE_ID) {
+    const status = await this.getStatus();


Can we fallback to read the id from the actual plist bundle since status might not be available at this stage?

Sounds good.
Here is only called by 'useNewWDA' is true

umutuzgur · 2019-08-29T08:36:10Z

@KazuCocoa I have a simpler solution. We can look or add a certain identifier to the Info.plist of the webdriveragent project. This would help us to find what we want to uninstall and we can avoid bundleid based search

KazuCocoa · 2019-08-29T09:26:58Z

Do you mean like https://github.com/appium/appium-xcuitest-driver/pull/1052/files#diff-a0e3f2ac971dfbd2354c4efa8121165aR222-R234 ? It gets CFBundleIdentifier in Info.plist. The value is exactly follow #1052 (comment) rule.

Yeah, it should work.

Below are cases I considered.
I concerned the 4, so considered to refer productBundleIdentifier. But I thought yesterday we could ignore the potential case since the case was maybe very rare. Not so common environment.

With useXctestrunFile
- We can get Info.Plist with current implementation this
With usePrebuiltWDA
- Should work with this. It refers derivedDataPath.
With webDriverAgentUrl
- We do not uninstall WDA since handing WDA is out of scope by Appium
Our built-in cache (re-use running WDA) feature and useNewWDA
- It means Appium re-use running WDA on the device.
  - Potentially, failed to uninstall the running WDA happen if running WDA has different bundleid from bundleId which Appium has.

Will work to simplify today. (maybe, a couple of hours later)

umutuzgur · 2019-08-29T09:40:36Z

Something like that. We can get a list of the applications from the phone and check for an entry like IsAppiumWDA=true. This would help us remove the code for checking if wda is removable without caring about any other configurations. On top of this, I really don't wanna deal with xcode changing the bundle id format of xctrunners

umutuzgur · 2019-08-29T09:43:38Z

We can modify this method https://github.com/appium/appium-ios-simulator/blob/master/lib/simulator-xcode-6.js#L182 and list all apps with their Info.plist parsed. This would do the same thing for real devices https://github.com/appium/appium-ios-device/blob/master/lib/installation-proxy/index.js#L39

KazuCocoa · 2019-08-29T12:28:32Z

That's cool!
I've added custom key as Info.Plist, but it did not appear as the returned value.
I think filtered by value.CFBundleName === 'WebDriverAgentRunner-Runner' or value. CFBundleDisplayName === 'WebDriverAgentRunner-Runner' are enough for real devices.
https://gist.github.com/KazuCocoa/ed4963fe5807e5f33e8944b8e54df92a is the result of Xcode 10.3 and 11 beta

umutuzgur · 2019-08-29T13:39:07Z

XCode might be building it weirdly and stripping stuff from the Info.plist but CFBundleName also sounds cool

lib/real-device-management.js

lib/simulator-management.js

lib/wda/webdriveragent.js

mykola-mokhnach · 2019-08-29T14:48:35Z

lib/wda/webdriveragent.js

+   */
+  async getInstalledWDBundleId () {
+    if (this.isRealDevice) {
+      return await getIBundleIdsByCFBundleName(this.device, WDA_CF_BUNDLE_NAME);


I assume this function and the next one should be methods of this.device object

lib/real-device-management.js

mykola-mokhnach · 2019-08-29T16:24:32Z

lib/simulator-management.js

@@ -216,11 +217,11 @@ async function shutdownOtherSimulators (currentDevice) {

 async function getInstalledBundleIds (device, candidateBundleIds) {
  const bundleIds = [];
-  for (const bundleId of candidateBundleIds) {
+  await B.filter(candidateBundleIds, async (bundleId) => {
    if (await device.isAppInstalled(bundleId)) {


not really - the filter condition should be async (bundleId) => await device.isAppInstalled(bundleId)

and the result could be returned immediately. There is no need for the intermediate accumulator

mykola-mokhnach · 2019-08-29T16:25:07Z

lib/wda/webdriveragent.js

@@ -422,7 +423,7 @@ class WebDriverAgent {
   */
  async getInstalledWDBundleId () {
    return this.isRealDevice
-      ? await getIBundleIdsByCFBundleName(this.device, WDA_CF_BUNDLE_NAME)


so how about moving these methods into corresponding classes and make it polymorphic?

yes. I also thought we can get CFBundleNam in simulator case (ios-simulator does not have such a method for now tho) as future work

umutuzgur · 2019-08-29T15:43:19Z

lib/simulator-management.js

@@ -214,5 +215,15 @@ async function shutdownOtherSimulators (currentDevice) {
  }
 }

+async function getInstalledBundleIds (device, candidateBundleIds) {


Should we actually change this in appium-ios-simulator that we get a similar api like what the real devices have? This would help us get rid of having a list of potential bundle ids

we can either add this call to ios simulator or we can attach it to the object dynamically right in this module:

this.device.getInstalledBundleIds = getInstalledBundleIds.bind(this.device);

Then this context in this method will be pointing to device instance if called from this.device.getInstalledBundleIds(...)

I'm talking about something else. I'm talking about making this method similar as the getBundleIdsByBundleName (device, bundleName)

^
This one as well. Otherwise how can it be polymorphic?

mykola-mokhnach · 2019-08-30T05:24:38Z

lib/wda/webdriveragent.js

+      }
+
+      log.debug(`Uninstalling WDAs: '${bundleIds}'`);
+      await B.each(bundleIds, async (bundleId) => {


and here I would revert back to for of. each call does not provide any preferences over a normal loop

mykola-mokhnach · 2019-08-30T05:48:06Z

lib/real-device-management.js

+ * @returns {Array<string>} A list of User level apps' bundle ids which has
+ *                          'CFBundleName' attribute as 'bundleName'.
+ */
+async function getBundleIdsByBundleName (device, bundleName) {


yep, lets move this method into IOSDeploy class

mykola-mokhnach · 2019-08-30T05:49:34Z

lib/simulator-management.js

@@ -214,5 +215,15 @@ async function shutdownOtherSimulators (currentDevice) {
  }
 }

+async function getInstalledBundleIds (device, candidateBundleIds) {


since this is an oneliner then we could simply move it to getInstalledWDBundleId

mykola-mokhnach

I like it

umutuzgur

Nice work!

) * fix: Follow the new bundleid if running env on Xcode 11 * get bundleid via ios-device, revert some implementations * move getBundleIdsByBundleName to iosdeploy * call getUserInstalledBundleIdsByBundleName from device * add unittests for uninstall

KazuCocoa added 2 commits August 27, 2019 01:08

fix: Follow the new bundleid if running env on Xcode 11

56595d4

tweak quit

d2c5fb1

imurchie reviewed Aug 26, 2019

View reviewed changes

mykola-mokhnach reviewed Aug 26, 2019

View reviewed changes

KazuCocoa added 6 commits August 27, 2019 23:23

simplify uninstall

e16a362

remove args from quitAndUninstall

6588ae0

remove arg for setupCaching

5b18d3e

add parsing plist

9ab2c3e

Merge branch 'master' into km/fix-differnet-bundleid

9186503

remove a line

7c5e41d

KazuCocoa marked this pull request as ready for review August 27, 2019 16:04

get bundleid via ios-device, revert some implementations

24078cd

add get bundlids for simulator

6c8523b