-
-
Notifications
You must be signed in to change notification settings - Fork 167
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
Adding apk-utilities and functional and unit tests for it #83
Conversation
log.errorAndThrow("activity and pkg is required for launching application"); | ||
} | ||
// initializing defaults | ||
_.defaults(startAppOptions, { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better clone the options before using defaults. It is modifying startAppOptions.
(https://lodash.com/docs#defaults)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jlipps and I discussed this. It ok to modify startAppOptions. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it's ok, makes it complicate to track side effects.
Few comments, otherwise 👍 |
let apiLevel = await this.getApiLevel(); | ||
let thirdparty = apiLevel >= 15 ? "-3" : ""; | ||
let stdout = await this.shell(['pm', 'list', 'packages', thirdparty, pkg]); | ||
let apkInstalledRgx = new RegExp('^package:' + pkg.replace(/(\.)/g, "\\$1") + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use an interpolated string here?
In general, looks good! |
26dc7d5
to
4c4fdde
Compare
log.errorAndThrow("Permission to start activity denied."); | ||
} | ||
if (startAppOptions.waitActivity) { | ||
if (startAppOptions.hasOwnProperty("waitDuration")) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can't we just get rid of this if/else?
await this.waitForActivity(startAppOptions.waitPkg, startAppOptions.waitActivity, startAppOptions.waitDuration);
if startAppOptions doesn't have a waitDuration, then it will pass undefined, which is exactly the same thing as not passing it at all.
made my comments |
Adding apk-utilities and functional and unit tests for it
@jlipps @sebv please review.