Skip to content
This repository was archived by the owner on Dec 7, 2019. It is now read-only.

Install extra APKs if received ( useful for Orchestrator )#159

Merged
artem-zinnatullin merged 6 commits intogojuno:masterfrom
CristianGM:orchestrator-apks
Oct 30, 2018
Merged

Install extra APKs if received ( useful for Orchestrator )#159
artem-zinnatullin merged 6 commits intogojuno:masterfrom
CristianGM:orchestrator-apks

Conversation

@CristianGM
Copy link
Copy Markdown
Contributor

@CristianGM CristianGM commented Oct 18, 2018

Now that we allow to use Orchestrator it would be nice to install the APKs required by Orchestrator to run.
Instead of being specific for Orchestrator this PR allows devs to install any extra APK they need.

There are no tests or documentation written yet because I want to validate the approach taken first, if it's ok I'll update the docs and add some tests ( suggestions on which ones are welcomed )

@CristianGM
Copy link
Copy Markdown
Contributor Author

I made it more generic, now any APK used by tests: orchestrator, test-butler-app, ... can be installed

what do you think @jonas-m- ?
I think it does exactly what you suggested on your PR ( with the help of the plugin is just magic )

val installApks: MutableCollection<Observable<Unit>> = mutableListOf(installAppApk, installTestApk)
if (args.runWithOrchestrator && args.orchestratorApks.isNotEmpty()) {
installApks.addAll(args.orchestratorApks.map {
if (args.extraApks.isNotEmpty()) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is this condition still needed?
It seems that if extraApks is empty then just nothing will be added in next line.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

You are absolutely right. I'll remove it.

@CristianGM
Copy link
Copy Markdown
Contributor Author

@yunikkk ?

@jonas-m-
Copy link
Copy Markdown
Contributor

I made it more generic, now any APK used by tests: orchestrator, test-butler-app, ... can be installed

what do you think @jonas-m- ?
I think it does exactly what you suggested on your PR ( with the help of the plugin is just magic )

Yep - looks good - liking the generic approach :)

val installTimeout = Pair(args.installTimeoutSeconds, TimeUnit.SECONDS)
val installAppApk = device.installApk(pathToApk = args.appApkPath, timeout = installTimeout)
val installTestApk = device.installApk(pathToApk = args.testApkPath, timeout = installTimeout)
val installApks: MutableCollection<Observable<Unit>> = mutableListOf(installAppApk, installTestApk)
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

nit: probably no need to specify type explicitly here?

@CristianGM CristianGM changed the title Install Orchestrator APKs if received Install extra APKs if received ( useful for Orchestrator ) Oct 26, 2018
Copy link
Copy Markdown
Collaborator

@artem-zinnatullin artem-zinnatullin left a comment

Choose a reason for hiding this comment

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

LGTM

README.md Outdated
* `--extra-apks`
* Apks to be installed for utilities. What you would typically declare in gradle as `androidTestUtil`
* Default: empty, only apk and test apk would be installed.
* Works great with orchestrator to install orchestrator & test services APKs.
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

nit: Orchestrator with capital O, can you please also make it a link to Android docs about it?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The link is just two lines before this, on the --with-orchestrator flag.

@trevjonez
Copy link
Copy Markdown

What machanism is used to install apks to the device? (I assume commander houses that functionality). I gave instant run another try this last week (still terrible) and noticed it used a single adb command to push all the apk files in one go. Not sure it would make any difference but might be worth looking at?

@artem-zinnatullin
Copy link
Copy Markdown
Collaborator

We use adb install if nothing changed. Instant run is not suitable for installs from scratch, it does push files and then uses tricks like custom classloader to hot swap to new stuff

@artem-zinnatullin
Copy link
Copy Markdown
Collaborator

@yunikkk, @ming13 let's merge and release an update? I have rights to do a release, but not to merge

@artem-zinnatullin artem-zinnatullin merged commit 984b64f into gojuno:master Oct 30, 2018
@yunikkk
Copy link
Copy Markdown
Contributor

yunikkk commented Oct 31, 2018

Guess ming13 added the merge permissions for you already...

@artem-zinnatullin
Copy link
Copy Markdown
Collaborator

Yep!

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants