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.

Sign up for GitHub

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

Jump to bottom

feat: Allowed detection of given firefox APK #1338

Merged
merged 1 commit into from
Jul 21, 2018
Merged

feat: Allowed detection of given firefox APK #1338

merged 1 commit into from
Jul 21, 2018

Conversation

tantawyk
Copy link
Contributor

This commit is to allow extension development on custom forks of
firefox for android, not just on firefox and fennec

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 19769e7 on khaled-cliqz:master into 5885500 on mozilla:master.

@coveralls
Copy link

coveralls commented Jun 25, 2018 •
edited
Loading

Coverage Status

Coverage remained the same at 100.0% when pulling 491a99b on khaled-cliqz:master into 5885500 on mozilla:master.

@tantawyk
Copy link
Contributor Author

tantawyk commented Jul 3, 2018

@rpl, could you please review this, and let me know if there's any questions or need any changes?

Rob--W
Rob--W reviewed Jul 3, 2018
View reviewed changes
src/util/adb.js Outdated
line.startsWith('org.mozilla.fennec') ||
line.startsWith('org.mozilla.firefox')
firefoxApk ?
line.startsWith(firefoxApk) :
Copy link
Member

Choose a reason for hiding this comment

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

Use line === firefoxApk instead of line.startsWith(firefoxApk).
In practice there is no difference because the list is filtered again in the end, but let's be more explicit that this an exact match is intended:

web-ext/src/extension-runners/firefox-android.js

Lines 340 to 367 in f9db35b

const packages = await adbUtils.discoverInstalledFirefoxAPKs(
selectedAdbDevice
);
if (packages.length === 0) {
throw new UsageError(
'No Firefox packages were found on the selected Android device');
}
const pkgsListMsg = (pkgs) => {
return pkgs.map((pkg) => ` - ${ pkg}`).join('\n');
};
if (!firefoxApk) {
log.info(`\nPackages found:\n${pkgsListMsg(packages)}`);
if (packages.length > 1) {
throw new UsageError('Select one of the packages using --firefox-apk');
}
// If only one APK has been found, select it even if it has not been
// specified explicitly on the comment line.
this.selectedFirefoxApk = packages[0];
log.info(`Selected Firefox for Android APK: ${this.selectedFirefoxApk}`);
return;
}
const filteredPackages = packages.filter((line) => line === firefoxApk);

tests/unit/test-util/test.adb.js Outdated
},
adbkitUtil: {
readAll: sinon.spy(() => {
return Promise.resolve(new Buffer(fakeADBPackageList));
Copy link
Member

Choose a reason for hiding this comment

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

The Buffer constructor is deprecated. Please use Buffer.from(fakeADBPackageList) instead.

tests/unit/test-util/test.adb.js Show resolved Hide resolved
tests/unit/test-util/test.adb.js Outdated
},
adbkitUtil: {
readAll: sinon.spy(() => {
return Promise.resolve(new Buffer(fakeADBPackageList));
Copy link
Member

Choose a reason for hiding this comment

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

Again, use the Buffer.from method.

rpl
rpl requested changes Jul 4, 2018
View reviewed changes
Copy link
Member

@rpl rpl left a comment

Choose a reason for hiding this comment

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

Hi @khaled-cliqz
Thanks, this looks like a reasonable change.
Follows some additional review comments (they are just some small tweaks that I would like to see applied before approving this PR for landing).

src/util/adb.js Outdated
@@ -111,8 +112,12 @@ export default class ADBUtils {
.map((line) => line.replace('package:', '').trim())
.filter((line) => {
return (
line.startsWith('org.mozilla.fennec') ||
line.startsWith('org.mozilla.firefox')
firefoxApk ?
Copy link
Member

Choose a reason for hiding this comment

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

Nit, do you mind to split this two conditions? just to enhance its readability a bit, something like:

// Look for an exact match if firefoxApk is defined.
if (firefoxApk) {
  return line === firefoxApk;
}

// Match any package name that starts with the package name of a Firefox for Android browser.
return (line.startsWith('org.mozilla.fennec') || ...);

tests/unit/test-util/test.adb.js Outdated
@@ -236,6 +238,52 @@ describe('utils/adb', () => {
sinon.assert.calledOnce(adb.util.readAll);
assert.deepEqual(packages, ['org.mozilla.fennec', 'org.mozilla.firefox']);
});

it('resolves only the given firefox APK', async () => {
Copy link
Member

Choose a reason for hiding this comment

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

It seems that these two new test cases are both testing same "exact match" scenario, it doesn't seem that we actually need both (at least if I'm not missing something that makes these two tests actually different).

If we feel that we need to test explicitly this scenario on both "org.mozilla.fennec" and "com.some.firefox.fork", we could at least extract one of these test functions into an helper function which takes the package name as a parameter and then we can call it for both these package names in the same test case.

@khaled-cliqz how that sounds to you?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds correct, I'll just remove this one.

@tantawyk tantawyk force-pushed the master branch 2 times, most recently from 7c7c189 to f796146 Compare July 4, 2018 16:28
feat: Allowed detection of given firefox APK
491a99b 
This commit is to allow extension development on custom forks of
firefox for android, not just on firefox and fennec
@tantawyk
Copy link
Contributor Author

tantawyk commented Jul 9, 2018

@rpl, could you help me with figuring out why this PR is failing?
I think that the test failing is unrelated:
should accept: --source-dir SRCDIR --api-url-prefix URL

@Rob--W
Copy link
Member

Rob--W commented Jul 9, 2018

@khaled-cliqz The failing test (web-ext sign) is a known issue, and @rpl is already looking into it.

@rpl
Copy link
Member

rpl commented Jul 9, 2018

@khaled-cliqz yeah, that is an unrelated intermittent, I'm going to take another look on this PR asap (and don't worry about that failure, I'm already fighting against it separately from this review).

@tantawyk
Copy link
Contributor Author

@Rob--W, @rpl, thanks guys and best of luck 👍

@rpl rpl merged commit 1cd88a1 into mozilla:master Jul 21, 2018
@caitmuenster
Copy link

Thank you, @khaled-cliqz! Your contribution has been added to our recognition wiki.

Would you be interested in creating an account on mozillians.org? I'd be happy to vouch for you!

@tantawyk
Copy link
Contributor Author

Thanks, @caitmuenster!
That would be great, here's the link: https://mozillians.org/de/u/BezrA/

@caitmuenster
Copy link

Yay! Your profile is vouched.

Thanks again for the patch, @khaled-cliqz! Hope to see you around the project. :)

This was referenced Oct 28, 2019
Open
Open
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Rob--W Rob--W Rob--W left review comments

@rpl rpl rpl approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@tantawyk @coveralls @Rob--W @rpl @caitmuenster