feat: Support for android build variants without requiring a fully qualified value in the `--apk-component` flag #1941

MikeM711 · 2020-06-26T01:23:33Z

Fixes #1935

Desired behavior from the original issue:

web-ext run -t firefox-android --android-device=<device ID here> --firefox-apk=org.mozilla.fenix.performancetest --firefox-apk-component=HomeActivity

…alified value in the `--apk-component` flag

coveralls · 2020-06-26T01:29:59Z

Coverage remained the same at 100.0% when pulling 819d5a2 on MikeM711:pr/support-variants-without-fully-qualified-value into 515e8cf on mozilla:master.

Rob--W · 2020-06-26T20:28:25Z

tests/unit/test-util/test.adb.js

-        'org.mozilla.geckoview_example',
-        'org.mozilla.geckoview_example.GeckoViewActivity', // firefoxApkComponent
+        'org.mozilla.geckoview_example.release',
+        'GeckoViewActivity', // firefoxApkComponent


Those tests should not be modified (because the old syntax should still work).

Please add a new test.

Sure thing - I'll add a new test for org.mozilla.fenix.nightly

Rob--W · 2020-06-26T20:30:59Z

src/firefox/package-identifiers.js

+
+export const browserList = [
+  'org.mozilla.fennec',
+  'org.mozilla.fenix',


Could you run an actual test with org.mozilla.fenix.nightly to verify that it works, and if it does work, add a unit test for it? It is not entirely obvious that org.mozilla.fenix.nightly ought to use org.mozilla.fenix as the name.

Yup, just added the test in the new commit

Rob--W · 2020-06-26T20:32:03Z

src/firefox/package-identifiers.js

@@ -0,0 +1,10 @@
+/* @flow */
+
+export const browserList = [


Rename to packageIdBases ?

or just export default. The file name, packageIdentifiers, is already a nice name on its own.

Rob--W · 2020-06-26T20:34:12Z

src/util/adb.js

-    if (!apkComponent) {
-      apkComponent = '.App';
-    } else if (!apkComponent.includes('.')) {
-      apkComponent = `.${apkComponent}`;


This original logic should be kept. It allows users to use a custom app that isn't in the hardcoded list. For example if they develop their own GeckoView-based app.

The change that you need is to check if apk starts with any of the known prefixes, and if it is, prepend that prefix before .${apkComponent}.

…to support full and shortened apkComponents, added org.mozilla.fenix.nightly test

MikeM711 · 2020-07-01T01:05:43Z

src/util/adb.js

+        break;
+      }
+    }
+


Hopefully this isn't too hacky - it's mainly due to wanting to leave the loop if geckoview_example has been checked, so it doesn't check again on geckoview

break after finding the matching apk is not a hack, it looks normal.

Rob--W · 2020-07-09T13:24:13Z

src/util/adb.js

+        break;
+      }
+    }
+


break after finding the matching apk is not a hack, it looks normal.

Rob--W · 2020-07-09T13:27:03Z

src/util/adb.js

@@ -271,6 +272,17 @@ export default class ADBUtils {
      value: `-profile ${deviceProfileDir}`,
    }];

+    for (const browser of packageIdentifiers) {
+      if (apk.startsWith(browser) && apkComponent) {
+        if (apk !== browser && !apkComponent.includes('.')) {


This logic should be after the following block below, to make sure that the browser is also prepended by default (.App):

if (!apkComponent) { apkComponent = '.App'; } else if (!apkComponent.includes('.')) { apkComponent = `.${apkComponent}`; }

It would be nice to have unit tests to cover these cases.

And then the logic should be like this:

if (apkComponent.startsWith(".")) { // loop over packageIdentifiers, // and if apk is either identical to the package, // or apk starts with `${package}.`, // prepend the package identifier before `apkComponent` // and break. }

or apk starts with ${package}.

Ah! That's a good idea for the whole "geckoview" vs "geckoview_example"

…upport-variants-without-fully-qualified-value

MikeM711 · 2020-07-22T19:16:59Z

src/util/adb.js

+    // before `apkComponent`
+    if (apkComponent.startsWith('.')) {
+      for (const browser of packageIdentifiers) {
+        if (apk.startsWith(`${browser}.`)) {


Quick note (before I move on to adding tests):

In the previous comment, there was the condition:

if apk is either identical to the package, or apk starts with `${package}.`

For the tests to work, I took out the apk is identical to the package part of the or condition.

The offended test was: starts a given APK component without a period

Incoming apk: org.mozilla.geckoview_example

Incoming apkComponent: GeckoViewActivity

Expected result: org.mozilla.geckoview_example/.GeckoViewActivity

Result received: org.mozilla.geckoview_example/org.mozilla.geckoview_example.GeckoViewActivity

Would you like me to expand out the Expected Result of this test to match the received result and use the apk is identical to the package condition on this line?

For ADB the two results are equivalent.

The behavior of the received result is probably more desirable to support the .debug/. performancetest scenarios.

MikeM711 · 2020-08-12T22:03:58Z

Hey, sorry I've been spotty on this PR - backlogged with work and class.

On Aug 23rd I should have a nice break - this PR is a simple fix, but I like to have full attention for it before proceeding. Should be good then.

Rob--W · 2020-08-12T22:27:16Z

Thanks for the heads-up @MikeM711 and no worries! Your contributions are appreciated, regardless of how much time it takes.

…upport-variants-without-fully-qualified-value

…the respective test.adb case

MikeM711 · 2020-08-24T01:43:39Z

Assuming that the previous commit looks good to you,

For APK tests we currently have:

Test name	Incoming apk	Incoming apkComponent
rejects an UsageError on adb binary not found'	org.mozilla.firefox_mybuild	undefined
starts Firefox APK on a custom profile (only used by Fennec)	org.mozilla.firefox_mybuild	undefined
starts a given APK component without a period	org.mozilla.geckoview_example	GeckoViewActivity
starts a given APK component with a period	org.mozilla.geckoview_example	org.mozilla.geckoview_example.GeckoViewActivity
starts a given APK component on fenix.nightly	org.mozilla.fenix.nightly	HomeActivity

To take care of, #1941 (comment), what do you think about these few tests?

Test name	Incoming apk	Incoming apkComponent
~~starts an undefined APK component~~ starts without specifying an APK component	org.mozilla.geckoview_example	undefined
starts a fully-qualified APK component on the build-variant: fenix.nightly	org.mozilla.fenix.nightly	org.mozilla.fenix.HomeActivity
starts a given APK component that begins with a period	org.mozilla.fenix.nightly	.HomeActivity

Rob--W · 2020-08-25T14:58:38Z

The proposed tests look good. Thanks for the table with the clear summarizing overview.

starts an undefined APK component

I would make this "starts without specifying an APK component"

…upport-variants-without-fully-qualified-value

MikeM711 · 2020-08-27T21:58:54Z

No problem! Let me know how it looks

Rob--W

LGTM

feat: Support for android build variants without requiring a fully qu…

594d724

…alified value in the `--apk-component` flag

Rob--W requested changes Jun 26, 2020

View reviewed changes

MikeM711 added 2 commits June 30, 2020 20:32

package-identifiers.js imported as packageIdentifiers, updated logic …

834bba9

…to support full and shortened apkComponents, added org.mozilla.fenix.nightly test

pull updated repo

1783b63

MikeM711 commented Jul 1, 2020

View reviewed changes

Rob--W requested changes Jul 9, 2020

View reviewed changes

MikeM711 added 2 commits July 22, 2020 12:56

Merge branch 'master' of https://github.com/mozilla/web-ext into pr/s…

4a5c793

…upport-variants-without-fully-qualified-value

Modified browser prepend logic

2398017

MikeM711 commented Jul 22, 2020

View reviewed changes

rpl added the priority: enhancement label Jul 23, 2020

MikeM711 added 2 commits August 23, 2020 19:02

Merge branch 'master' of https://github.com/mozilla/web-ext into pr/s…

c0be414

…upport-variants-without-fully-qualified-value

Added “if apk is identical to the package” OR condition, and updated …

aff4424

…the respective test.adb case

MikeM711 added 2 commits August 27, 2020 14:19

Merge branch 'master' of https://github.com/mozilla/web-ext into pr/s…

17779ce

…upport-variants-without-fully-qualified-value

Added 3 tests for adb.js

819d5a2

Rob--W approved these changes Sep 1, 2020

View reviewed changes

Rob--W added the state: pull request ready label Sep 1, 2020

Rob--W merged commit c083c07 into mozilla:master Sep 17, 2020

Rob--W mentioned this pull request Mar 18, 2021

Wrong activity being started for Fenix #2206

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Support for android build variants without requiring a fully qualified value in the `--apk-component` flag #1941

feat: Support for android build variants without requiring a fully qualified value in the `--apk-component` flag #1941

MikeM711 commented Jun 26, 2020

coveralls commented Jun 26, 2020 •

edited

Loading

Rob--W Jun 26, 2020

MikeM711 Jun 30, 2020

Rob--W Jun 26, 2020

MikeM711 Jul 1, 2020 •

edited

Loading

Rob--W Jun 26, 2020

Rob--W Jun 26, 2020

MikeM711 Jul 1, 2020

Rob--W Jul 9, 2020

Rob--W Jul 9, 2020

Rob--W Jul 9, 2020

MikeM711 Jul 22, 2020

MikeM711 Jul 22, 2020 •

edited

Loading

Rob--W Jul 22, 2020

MikeM711 commented Aug 12, 2020

Rob--W commented Aug 12, 2020

MikeM711 commented Aug 24, 2020 •

edited

Loading

Rob--W commented Aug 25, 2020

MikeM711 commented Aug 27, 2020 •

edited

Loading

Rob--W left a comment

feat: Support for android build variants without requiring a fully qualified value in the --apk-component flag #1941

feat: Support for android build variants without requiring a fully qualified value in the --apk-component flag #1941

Conversation

MikeM711 commented Jun 26, 2020

coveralls commented Jun 26, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MikeM711 Jul 1, 2020 • edited Loading

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

MikeM711 Jul 22, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MikeM711 commented Aug 12, 2020

Rob--W commented Aug 12, 2020

MikeM711 commented Aug 24, 2020 • edited Loading

Rob--W commented Aug 25, 2020

MikeM711 commented Aug 27, 2020 • edited Loading

Rob--W left a comment

Choose a reason for hiding this comment

feat: Support for android build variants without requiring a fully qualified value in the `--apk-component` flag #1941

feat: Support for android build variants without requiring a fully qualified value in the `--apk-component` flag #1941

coveralls commented Jun 26, 2020 •

edited

Loading

MikeM711 Jul 1, 2020 •

edited

Loading

MikeM711 Jul 22, 2020 •

edited

Loading

MikeM711 commented Aug 24, 2020 •

edited

Loading

MikeM711 commented Aug 27, 2020 •

edited

Loading