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.

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

Speed up permissions detection #356

Merged
merged 10 commits into from
Sep 25, 2018
Merged

Conversation

mykola-mokhnach
Copy link
Contributor

The idea behind this is the same as for #355
Package manager invocation is more expensive from performance perspective in comparison to dumpsys call

Copy link
Member

@KazuCocoa KazuCocoa left a comment

Choose a reason for hiding this comment

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

👍 dumpsys based way

Copy link
Member

@jlipps jlipps left a comment

Choose a reason for hiding this comment

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

LGTM, with a few comments. Also my only concern with this approach is that if dumpsys output format changes we will be screwed. Would be good to have some functional tests to catch this case for a new SDK release that might make such a breaking change

lib/helpers.js Outdated
const titleIndent = lines[0].search(indentPattern);
const permissionNamePattern = /android\.permission\.\w+/;
const grantedStatePattern = /\bgranted=(\w+)/;
const result = [];
Copy link
Member

Choose a reason for hiding this comment

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

i know it's valid but it seems weird to push to a const array

const stdout = cmdOutput || await this.shell(['dumpsys', 'package', pkg]);
const grantedPermissions = [];
for (const pattern of [
new RegExp(/^(\s*install permissions:[\s\S]+)/, 'm'),
Copy link
Member

Choose a reason for hiding this comment

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

might be more readable to make this array its own const

let match = new RegExp(/(install permissions:|User 0)([\s\S]*?)DUMP OF SERVICE activity:/g).exec(stdout);
if (!match) {
throw new Error('Unable to get denied permissions');
const stdout = cmdOutput || await this.shell(['dumpsys', 'package', pkg]);
Copy link
Member

Choose a reason for hiding this comment

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

this is all the same logic as above; maybe factor out one method that just takes a bool to get granted vs denied?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't want to change module interface. And there is also not much content to extract into a common function

@mykola-mokhnach
Copy link
Contributor Author

@jlipps I did some refactoring there and now the amount of redundant code has been slightly decreased. Thanks for the hint

Copy link
Member

@jlipps jlipps left a comment

Choose a reason for hiding this comment

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

thanks, that makes me happier :-)

@mykola-mokhnach mykola-mokhnach merged commit 285a92d into appium:master Sep 25, 2018
@mykola-mokhnach mykola-mokhnach deleted the perms branch October 10, 2018 17:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants