Introduce apks support #367

mykola-mokhnach · 2018-10-16T12:58:25Z

This PR continues what was started by @KazuCocoa in #366

the main idea behind the current implementation is to introduce .apks format support without making additional changes in the depending driver modules (aka it should simply work).

imurchie · 2018-10-16T17:14:39Z

lib/helpers.js

-async function unzipFile (zipPath) {
-  log.debug(`Unzipping ${zipPath}`);
+async function unzipFile (zipPath, dstRoot = null) {
+  dstRoot = dstRoot || path.dirname(zipPath);


This can be done in the default parameter

async function unzipFile (zipPath, dstRoot = path.dirname(zipPath)) {

Is it ok to use other arguments in default value calculation?

Yes, as long as they are later in the argument list. So

function a (b = c, c) { console.log(b, c); }

would fail if you called a(undefined, 1), with a reference error, because c would not have been defined when b is initialized to it.

But, for every situation, the follow is fine:

function a (b, c = b) { console.log(b, c); }

mykola-mokhnach · 2018-10-16T17:36:45Z

@KazuCocoa Would you like to run some tests on this PR?

…into apks_support # Conflicts: # lib/tools/apk-signing.js

KazuCocoa

Yes. Of course. I'll take a look & test

KazuCocoa · 2018-10-17T00:12:27Z

lib/tools/apk-utils.js

                `the current API level ${apiLevel} does not support applications ` +
                `permissions customization`);
    } else {
      additionalArgs.push('-g');
    }
  }

+  if (appPath.endsWith(APKS_EXTENSION)) {


👍
The installation depends on device environment. Configuring device env, including language preference, is called in first step in startAndroidSession. The bundle-install command doesn't install unused languages resources.
https://github.com/appium/appium-android-driver/blob/2bac1672019be24c74fec7b4b5e4301558bfcb66/lib/driver.js#L234

in theory we could additionally install these language resources manually or change the setup flow sequence. Which way does look more convenient to you?

@KazuCocoa I've changed the way we install apks. Now it is much more customizable.

jlipps · 2018-10-18T05:41:50Z

lib/helpers.js

+ * @returns {string} The calculated hash as hexadecimal string
+ * @throws {Error} If hash calculation fails
+ */
+async function fileHash (filePath, algorithm = 'sha1') {


feels like this should be in appium-support?

into which submodule of appium-support it would be better to put it? util or fs?

jlipps · 2018-10-18T05:46:03Z

lib/tools/apk-utils.js

-      additionalArgs.push('-g');
-    }
-  }
+  options = Object.assign({


Object.assign mutates, dunno if that changes how you'd want to use it here

here we just assign the default values to options. The argument dictionary itself is not mutated, but a new copy is created

oh right, only the target is mutated and in this case it's an object literal

jlipps · 2018-10-18T05:47:02Z

lib/tools/apks-utils.js

+ *
+ * @param {string} apks - The full path to the .apks file
+ * @param {string|Array<String>} dstPath - The relative path to the destination file,
+ * which is going to be extract3ed, where each path component is an array item


KazuCocoa

Thanks! Looks good to me. I'll install real apks after my work.

KazuCocoa · 2018-10-18T06:22:16Z

lib/tools/apks-utils.js

+    await this.execBundletool(args, `Cannot extract the application bundle at '${apks}'`);
+    const installArgs = buildInstallArgs(await this.getApiLevel(), options);
+    const apksToInstall = (await fs.readdir(tmpRoot))
+      .filter((name) => name.endsWith('.apk'))


[nits]
Is it better to define .apk const like APKS_EXTENSION?

makes sense

KazuCocoa · 2018-10-18T06:36:29Z

lib/tools/apks-utils.js

+      .filter((name) => name.endsWith('.apk'))
+      .map((name) => path.resolve(tmpRoot, name));
+    log.debug(`Got the following apk files to install: ${JSON.stringify(apksToInstall)}`);
+    const output = await this.adbExec(['install-multiple', ...installArgs, ...apksToInstall], {


If the install apks has unsuitable architecture, installing apks fail. (adb install failed before. It happen if users use ML related libraries, for example.)

base-vi.apk 11K base-x86.apk 483K base-x86_64.apk 453K base-xhdpi.apk 2.4M

~~What about add considering abis?~~
=> ah, we use '--device-spec', specPath,. nice 👍

yep. also this strategy allows to alter the actual spec set if, for example, we need to install more locales, by just modifying the spec json

KazuCocoa · 2018-10-18T06:58:47Z

lib/tools/adb-commands.js

+ */
+methods.initBundletool = async function () {
+  try {
+    const bundletoolPath = await fs.which('bundletool.jar');


We should export the path to 'bundletool.jar', right? 👀

yes. bundletool.jar binary should be in any of folders enumerated in system PATH

KazuCocoa · 2018-10-18T14:13:01Z

I've implemented https://github.com/KazuCocoa/AppBundleSample/blob/master/test.rb and run it to make sure the behaviour. Then, I noticed below three points.

If a test app(only english resource) already has been installed, it remains. Then, 2nd test scenario failed since 2nd scenario would like to run tests against Japanese resource
- https://github.com/KazuCocoa/AppBundleSample/blob/master/test.rb
  1. Can we make sure the installed apk's resource and try re-install if we need?
  2. Or use standalones/standalone-xxxhdpi.apk?
  3. (Or we'll guide full reset for .apks at first?)
    - If we can do 1st or 2nd way, it's better, I think. 3 is reasonable so far, I also think since buildtools also isn't 1.x yet.
We need to relax APP_EXTENSION for configureApp to allow .apks extension. We need to extend configureApp to allow multiple extension.
- https://github.com/search?p=2&q=org%3Aappium+configureApp&type=Code
  - I can work them when I have a time, btw.
https://github.com/appium/appium-adb/pull/367/files#r226311748

mykola-mokhnach · 2018-10-18T14:23:39Z

@KazuCocoa

Regarding p1 I will try to do some internal testing and check on what can be done there.
Btw, does it help if fullReset is enabled?

For p2 should be an easy fix.

Many thanks for help on this

KazuCocoa · 2018-10-18T15:05:41Z

Btw, does it help if fullReset is enabled?

Yes, but it's the final way, I believe.
If we can't do anything to identify when Appium must re-install an apk, it's helpful. But if we enable fullReset everytime, below case happens. (Your heavy improvement for creating session doesn't work this case.)

Create a session with English resource
Create a session with English resource as another test case <= We don't need to re-install, but re-install happens if we enable fullReset

mykola-mokhnach · 2018-10-18T15:27:51Z

I would just assume this is the main purpose of bundling, that we only install the necessary resources. So it would be logically to assume we need to reinstall the app in order to get the new locale from the bundle (real users would also have to do the same in case of apks). Also, sometimes full reset might be even faster than soft reset, since it does not require to set permissions on the package (these are set during installation)

KazuCocoa · 2018-10-18T15:59:28Z

sometimes full reset might be even faster than soft reset, since it does not require to set permissions on the package (these are set during installation)

ah, it's reasonable to enable fullReset for .apks. I agree with the trying fullReset way.

mykola-mokhnach · 2018-10-19T08:09:47Z

@KazuCocoa Are you fine with merging this PR in the current state or you'd like to add something?

KazuCocoa · 2018-10-19T10:18:43Z

Let me run the test again once with fullRest cap

KazuCocoa

Worked perfectly 👍 Thanks!

Introduce apks support

adc142e

mykola-mokhnach requested review from dpgraham, imurchie and KazuCocoa October 16, 2018 12:58

add unit tests

cc26618

imurchie reviewed Oct 16, 2018

View reviewed changes

Mykola Mokhnach added 2 commits October 16, 2018 21:07

Address comments

4b55dda

Merge branch 'master' of https://github.com/mykola-mokhnach/appium-adb …

461daaf

…into apks_support # Conflicts: # lib/tools/apk-signing.js

KazuCocoa reviewed Oct 17, 2018

View reviewed changes

Mykola Mokhnach added 5 commits October 17, 2018 08:35

Add hashing

013763c

Change algorithm to sha1

8107740

Get rid of the unused stuff

0e0c737

Customize apks install logic

1c93c98

Update unit tests

5547a5f

jlipps approved these changes Oct 18, 2018

View reviewed changes

KazuCocoa approved these changes Oct 18, 2018

View reviewed changes

Mykola Mokhnach added 2 commits October 18, 2018 11:36

Address comments

40171f7

Improve the condition

b8ef130

Remove the extra parameter

0bf94d0

mykola-mokhnach mentioned this pull request Oct 18, 2018

Make it possible to pass multiple supported extensions to configureApp helper appium/appium-base-driver#271

Merged

Reuse the stuff from appium-support lib

6be358e

appium deleted a comment from KazuCocoa Oct 19, 2018

KazuCocoa approved these changes Oct 19, 2018

View reviewed changes

mykola-mokhnach merged commit 9fa581d into appium:master Oct 19, 2018

mykola-mokhnach deleted the apks_support branch October 20, 2018 07:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce apks support #367

Introduce apks support #367

mykola-mokhnach commented Oct 16, 2018

imurchie Oct 16, 2018

mykola-mokhnach Oct 16, 2018

imurchie Oct 16, 2018

mykola-mokhnach Oct 16, 2018

mykola-mokhnach commented Oct 16, 2018

KazuCocoa left a comment

KazuCocoa Oct 17, 2018

mykola-mokhnach Oct 17, 2018

mykola-mokhnach Oct 17, 2018

jlipps Oct 18, 2018

mykola-mokhnach Oct 18, 2018

jlipps Oct 18, 2018

mykola-mokhnach Oct 18, 2018

jlipps Oct 18, 2018

jlipps Oct 18, 2018

KazuCocoa left a comment

KazuCocoa Oct 18, 2018

mykola-mokhnach Oct 18, 2018

KazuCocoa Oct 18, 2018

mykola-mokhnach Oct 18, 2018 •

edited

Loading

KazuCocoa Oct 18, 2018

mykola-mokhnach Oct 18, 2018 •

edited

Loading

KazuCocoa commented Oct 18, 2018

mykola-mokhnach commented Oct 18, 2018

KazuCocoa commented Oct 18, 2018

mykola-mokhnach commented Oct 18, 2018

KazuCocoa commented Oct 18, 2018

mykola-mokhnach commented Oct 19, 2018

KazuCocoa commented Oct 19, 2018 •

edited

Loading

KazuCocoa left a comment

Introduce apks support #367

Introduce apks support #367

Conversation

mykola-mokhnach commented Oct 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mykola-mokhnach commented Oct 16, 2018

KazuCocoa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KazuCocoa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mykola-mokhnach Oct 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mykola-mokhnach Oct 18, 2018 • edited Loading

Choose a reason for hiding this comment

KazuCocoa commented Oct 18, 2018

mykola-mokhnach commented Oct 18, 2018

KazuCocoa commented Oct 18, 2018

mykola-mokhnach commented Oct 18, 2018

KazuCocoa commented Oct 18, 2018

mykola-mokhnach commented Oct 19, 2018

KazuCocoa commented Oct 19, 2018 • edited Loading

KazuCocoa left a comment

Choose a reason for hiding this comment

mykola-mokhnach Oct 18, 2018 •

edited

Loading

mykola-mokhnach Oct 18, 2018 •

edited

Loading

KazuCocoa commented Oct 19, 2018 •

edited

Loading