1422019 - Add test for GeckoView API

Status: RESOLVED FIXED

(GeckoView :: General, enhancement)

Description

[James Willcox (:snorp) ([e-mail]) (he/him)]

We have none right now and it's terrible.

Comment 1

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Bug 1422019 - Stand up initial GeckoView tests

Review commit: https://reviewboard.mozilla.org/r/204248/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/204248/

Comment 2

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8933333 [details]
Bug 1422019 - Stand up initial GeckoView tests

This is a first cut at getting some basic tests going. It uses Espresso/JUnit against an activity defined in a separate geckoview_test module. I tried to make it all work within the geckoview module, but ran into problems due to it being a lib module.

I think this will need a lot more refinement around the infrastructure to write tests. Specifically, I think we'll need Marionette to do things like click buttons on the test pages. Espresso has some built-in support for this, so it would be great if we can just make that work somehow.

Comment 3

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8933333 [details]
Bug 1422019 - Stand up initial GeckoView tests

https://reviewboard.mozilla.org/r/204248/#review209888

r- but f+, of course -- this is looking great!  Can you give more details on what's happening when you push this into the main geckoview project?  I'd really like to figure that out if possible.  I can try to investigate myself eventually.

::: mobile/android/geckoview_test/.gitignore:1
(Diff revision 1)
> +/build

Eh?  Don't do this, put the build directory under $topobjdir/gradle/build.  Basically, cult more from any of the other build.gradle files.

::: mobile/android/geckoview_test/build.gradle:29
(Diff revision 1)
> +        }
> +    }
> +
> +    project.configureProductFlavors.delegate = it
> +    project.configureProductFlavors()
> +    compileOptions {

nit: newline.

::: mobile/android/geckoview_test/build.gradle:36
(Diff revision 1)
> +        targetCompatibility JavaVersion.VERSION_1_8
> +    }
> +}
> +
> +dependencies {
> +    implementation fileTree(include: ['*.jar'], dir: 'libs')

Drop this -- we don't want it.

::: mobile/android/geckoview_test/build.gradle:37
(Diff revision 1)
> +    }
> +}
> +
> +dependencies {
> +    implementation fileTree(include: ['*.jar'], dir: 'libs')
> +    // AndroidJUnitRunner and JUnit Rules

nit: full sentences, here and below.

::: mobile/android/geckoview_test/build.gradle:42
(Diff revision 1)
> +    // AndroidJUnitRunner and JUnit Rules
> +    androidTestImplementation 'com.android.support.test:runner:0.5'
> +    androidTestImplementation 'com.android.support.test:rules:0.5'
> +    // Espresso dependencies
> +    androidTestImplementation 'com.android.support.test.espresso:espresso-core:2.2.2'
> +    androidTestImplementation 'com.android.support:support-annotations:23.4.0'

This should be our subst, not 23.4.0.  If it's special, comment extensively.

::: mobile/android/geckoview_test/src/androidTest/java/org/mozilla/geckoview/test/BaseGeckoViewTest.java:1
(Diff revision 1)
> +package org.mozilla.geckoview.test;

nit: CC license, throughout.

::: mobile/android/geckoview_test/src/androidTest/java/org/mozilla/geckoview/test/BaseGeckoViewTest.java:56
(Diff revision 1)
> +    }
> +
> +
> +    protected void loadTestPage(final String startUrl, Runnable finished) {
> +        mSession.setProgressListener(new GeckoSession.ProgressListener() {
> +            private boolean mNotBlank;

It's generally better to explicitly collect events, or to at least anticipate receiving multiple completely unexpected events.

::: mobile/android/geckoview_test/src/androidTest/java/org/mozilla/geckoview/test/BaseGeckoViewTest.java:86
(Diff revision 1)
> +        });
> +
> +        mSession.loadUri(startUrl);
> +    }
> +
> +    private class ExplicitIdlingResource implements IdlingResource {

I remember reading about this, and I agree, it's odd.  We'll want to connect this to Marionette in some way; I just don't know how much state Marionette exposes.

::: mobile/android/geckoview_test/src/androidTest/java/org/mozilla/geckoview/test/NavigationTests.java:142
(Diff revision 1)
> +                assertTrue(securityInfo.isSecure);
> +                assertEquals(securityInfo.securityMode, SecurityInformation.SECURITY_MODE_IDENTIFIED);
> +            }
> +        });
> +
> +        mSession.loadUri("https://mozilla-modern.badssl.com/");

Hmm.  Such tests are hard to do without a local server, which I really, really, really hope not to require.  (Or we annotate those that require a local server.)

Running a local server makes it so much harder to test these things in the IDE and on devices.  We've had no end of pain with hostutils and local WiFi connections.

::: mobile/android/geckoview_test/src/main/java/org/mozilla/geckoview/test/TestRunnerActivity.java:42
(Diff revision 1)
> +        } catch (Exception e) {
> +            Log.e(LOGTAG, "Failed to unpack assets", e);
> +        }
> +    }
> +
> +    private void unpackPath(String path) throws Exception {

We have an `android://assets/` protocol handler already, don't we?  I'd really hope we can make that work in GeckoView.  See https://bugzilla.mozilla.org/show_bug.cgi?id=948465.

::: mobile/android/geckoview_test/src/main/res/values/colors.xml:1
(Diff revision 1)
> +<?xml version="1.0" encoding="utf-8"?>

Do these need licenses, too?  (I'm not sure what we do for resources like this.)

::: settings.gradle:31
(Diff revision 1)
>  // local.properties (first 'sdk.dir', then 'android.dir') and then the
>  // environment variable ANDROID_HOME will override this.  That's unfortunate,
>  // but it's hard to automatically arrange better.
>  System.setProperty('android.home', json.substs.ANDROID_SDK_ROOT)
>  
> -include ':app'
> +include ':app', ':geckoview_test'

Split lines.

Comment 4

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8933333 [details]
Bug 1422019 - Stand up initial GeckoView tests

https://reviewboard.mozilla.org/r/204248/#review209888

The problem I had is that Studio popped up the "no module" dialog when I tried to run the geckoview tests, and I couldn't select it from the dialog either.

> Eh?  Don't do this, put the build directory under $topobjdir/gradle/build.  Basically, cult more from any of the other build.gradle files.

Looks like Studio did that itself, whoops. Removed.

> It's generally better to explicitly collect events, or to at least anticipate receiving multiple completely unexpected events.

I don't think I'm following. The problem here is that GV loads about:blank as the initial page and we sometimes get progress events related to that. I would like to just make it not do that, but we need this workaround for now.

> I remember reading about this, and I agree, it's odd.  We'll want to connect this to Marionette in some way; I just don't know how much state Marionette exposes.

Yeah, the Marionette/WebDriver situation is a bit of a mess, it seems. I don't know if we're going to be able to use that. I have another idea that I want to talk over soon.

> Hmm.  Such tests are hard to do without a local server, which I really, really, really hope not to require.  (Or we annotate those that require a local server.)
> 
> Running a local server makes it so much harder to test these things in the IDE and on devices.  We've had no end of pain with hostutils and local WiFi connections.

Yeah, I think we'd annotate the ones that require network and just skip them in automation if necessary. We could run a web server on the device, though, and I think that would be very reliable.

> We have an `android://assets/` protocol handler already, don't we?  I'd really hope we can make that work in GeckoView.  See https://bugzilla.mozilla.org/show_bug.cgi?id=948465.

I totally forgot about that! I'll see if I can make it work.

> Do these need licenses, too?  (I'm not sure what we do for resources like this.)

We apparently do put licenses in these. What a PITA.

Comment 5

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Bug 1422019 - Make resource://android/asset work again with GeckoView

Review commit: https://reviewboard.mozilla.org/r/204684/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/204684/

Comment 6

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8933333 [details]
Bug 1422019 - Stand up initial GeckoView tests

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/204248/diff/1-2/

Comment 7

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8933755 [details]
Bug 1422019 - Make resource://android/asset work again with GeckoView

https://reviewboard.mozilla.org/r/204684/#review210270

Comment 8

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8933755 [details]
Bug 1422019 - Make resource://android/asset work again with GeckoView

https://reviewboard.mozilla.org/r/204684/#review210276

::: mobile/android/components/geckoview/GeckoViewStartup.js:22
(Diff revision 1)
>  GeckoViewStartup.prototype = {
>    classID: Components.ID("{8e993c34-fdd6-432c-967e-f995d888777f}"),
>  
>    QueryInterface: XPCOMUtils.generateQI([Ci.nsIObserver]),
>  
> +  /**

Is there any security issue with letting consumers have read-only access to the APK?  I guess not, since they run arbitrary Java or native code and can do whatever they want.

This doesn't leak to web content, right?  (If it did, we'd already be insecure in Fennec.)

Comment 9

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Nick Alexander :nalexander (less responsive until Jan 3, 2018) from comment #8)
> Comment on attachment 8933755 [details]
> Bug 1422019 - Make resource://android/asset work again with GeckoView
> 
> https://reviewboard.mozilla.org/r/204684/#review210276
> 
> ::: mobile/android/components/geckoview/GeckoViewStartup.js:22
> (Diff revision 1)
> >  GeckoViewStartup.prototype = {
> >    classID: Components.ID("{8e993c34-fdd6-432c-967e-f995d888777f}"),
> >  
> >    QueryInterface: XPCOMUtils.generateQI([Ci.nsIObserver]),
> >  
> > +  /**
> 
> Is there any security issue with letting consumers have read-only access to
> the APK?  I guess not, since they run arbitrary Java or native code and can
> do whatever they want.
> 
> This doesn't leak to web content, right?  (If it did, we'd already be
> insecure in Fennec.)

The resource:// url gets converted to a file:// one, so all of the normal rules for that apply. This means that the user could enter resource:// or a page could link to it, but you couldn't, say, xhr a resource:// from a page loaded over http.

Comment 10

[Nevin Chen(Not active on Bugzilla)]

Comment on attachment 8933333 [details]
Bug 1422019 - Stand up initial GeckoView tests

I've tested the patch(locally) and it works great.

1. I also like the method "loadTestPage" cause it separate "How to test" and "What to test". Maybe we can move this to a seperate class in the future.

2. We call mSession.loadUri(startUrl); directly instead of finding the element in UI and type the url. It should be fine since there's no such UI in TestRunnerActivity. 

3. Testing valid TLS is hard for local static files. I'll survey if this works for us and report back if I have the time[1]

4. Work with CI : As Nick's suggestion we can leverage robocop to run Espresso test for us[2] just by sepcifying the test runner and the test to run(Maybe like this[3])

[1] https://github.com/square/okhttp/tree/master/mockwebserver
[2] https://searchfox.org/mozilla-central/source/testing/mochitest/runrobocop.py#441
[3] https://medium.com/@satyajit/running-subset-of-espresso-tests-locally-and-gcloud-4713931da56

Comment 11

[Nevin Chen(Not active on Bugzilla)]

Comment on attachment 8933755 [details]
Bug 1422019 - Make resource://android/asset work again with GeckoView

oops.. recovery my mistake .. sorry..

Comment 12

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8933755 [details]
Bug 1422019 - Make resource://android/asset work again with GeckoView

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/204684/diff/1-2/

Comment 13

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8933333 [details]
Bug 1422019 - Stand up initial GeckoView tests

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/204248/diff/2-3/

Comment 14

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8933333 [details]
Bug 1422019 - Stand up initial GeckoView tests

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/204248/diff/3-4/

Comment 15

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8933333 [details]
Bug 1422019 - Stand up initial GeckoView tests

https://reviewboard.mozilla.org/r/204248/#review225780

r+ for non-build bits. Does the Android Studio problem still happen with gradle plugin 3 (i.e. can we make this part of the geckoview project with gradle plugin 3?)

Also I feel like these tests could all be headless, which should make them much faster to run than activity tests.

::: mobile/android/geckoview_test/src/androidTest/java/org/mozilla/geckoview/test/BaseGeckoViewTest.java:67
(Diff revision 4)
> +    protected void loadTestPage(final String url, Runnable finished) {
> +        final String path = Uri.parse(url).getPath();
> +        mSession.setProgressListener(new GeckoSession.ProgressListener() {
> +            @Override
> +            public void onPageStart(GeckoSession session, String loadingUrl) {
> +                assertTrue("Unexpected url load: " + loadingUrl, loadingUrl.endsWith(path));

I think these assert messages are usually "positive", e.g. "Loaded url should match" for this case.

::: mobile/android/geckoview_test/src/androidTest/java/org/mozilla/geckoview/test/BaseGeckoViewTest.java:99
(Diff revision 4)
> +        @Override
> +        public boolean isIdleNow() {
> +            return mIsIdle;
> +        }
> +
> +        public void done() {

We should probably assert this is called on the main thread.

::: mobile/android/geckoview_test/src/androidTest/java/org/mozilla/geckoview/test/NavigationTests.java:36
(Diff revision 4)
> +    @Test
> +    public void testGoBack() {
> +        final String startPath = "hello.html";
> +        loadTestPath(startPath, () -> {
> +            loadTestPath("hello2.html", () -> {
> +                mSession.setNavigationListener(new GeckoSession.NavigationListener() {

This overrides the listener set in TestRunnerActivity, right?

Comment 16

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8933333 [details]
Bug 1422019 - Stand up initial GeckoView tests

https://reviewboard.mozilla.org/r/204248/#review225780

Headless would be great. I spent some time trying to get that working, but the problem is that JUnit does not really have a way to run an asynchronous test without Espresso. It could work if we were able to use GeckoSession off the main thread, which seems like it should be possible. I'll file a followup bug about getting headless going.

> I think these assert messages are usually "positive", e.g. "Loaded url should match" for this case.

I think you're right. Fixed.

> This overrides the listener set in TestRunnerActivity, right?

That's correct, but in this case that's what we want. The default listener in TestRunnerActivity is really for the mochitest/reftest. If there is more divergence later on we can split it out into a separate Activity with no penalty; mozharness will look for org.mozilla.geckoview.test/.App which is currently aliased to TestRunnerActivity.

Comment 17

[Nick Alexander :nalexander [he/him]]

Attachment: Bug 1422019 - Make resource://android/asset work again with GeckoView

Review commit: https://reviewboard.mozilla.org/r/220702/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/220702/

Comment 18

[Nick Alexander :nalexander [he/him]]

Attachment: Bug 1422019 - Stand up initial GeckoView tests

This adds a new geckoview_test module, which contains a
test runner Activity. We can then use JUnit4 + Espresso to
exercise the GeckoView APIs (such as GeckoSession).

Review commit: https://reviewboard.mozilla.org/r/220704/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/220704/

Comment 19

[Nick Alexander :nalexander [he/him]]

Attachment: Bug 1422019 - REVIEW COMMENTS -- move everything into GV project.

To be folded with the follow-up to remove Java 8-isms into snorp's
original patch.

Review commit: https://reviewboard.mozilla.org/r/220706/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/220706/

Comment 20

[Nick Alexander :nalexander [he/him]]

Attachment: Bug 1422019 - Pre: Fix diagnostic requiring |mach package|.

No idea when this broke -- probably the transition to Gradle 4 -- but
this seems to work.  As suggested at
https://discuss.gradle.org/t/copy-task-how-to-fail-on-no-source/25581/2.

Review commit: https://reviewboard.mozilla.org/r/220708/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/220708/

Comment 21

[Nick Alexander :nalexander [he/him]]

Attachment: Bug 1422019 - Pre: Fix errors in |mach android archive-geckoview|.

Review commit: https://reviewboard.mozilla.org/r/220710/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/220710/

Comment 22

[Nick Alexander :nalexander [he/him]]

Attachment: Bug 1422019 - Post: Don't require Java 8 in GeckoView androidTest code.

There are two reasons to do this.  The first is significant: it
appears that Java 7 projects do not always play nicely with dependent
projects that require Java 8; since GeckoView is primarily for
external consumers, it should accept some inconvenience in order to
ease the lives of consumers.  It appears to not be possible to set
different compile options for the main/ and the androidTest/ Java
code, which is unfortunate.  (A separate testing project would allow
using different settings for the test code, but is less pleasant for
other reasons.)

The second is insignificant, but annoying: using Java 8 exposes a bug
or incompatibility with the current Gradle configuration that uses the
Java compile bootClasspath to run Mozilla's JNI wrapper generation
annotation processor.  That configuration grabs the location of
android.jar for use by the annotation processor; the issue manifests
itself as a NullPointerException.  A quick examination didn't show a
work-around, and it's worth noting that it took a long time for
nalexander to discover a way to fish the location of android.jar from
Gradle when it was implemented.

Review commit: https://reviewboard.mozilla.org/r/220712/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/220712/

Comment 23

[Nick Alexander :nalexander [he/him]]

snorp: I rebased bits of the required commit sequence and your patches onto a recent central.  I then reworked your patches to not have a separate testing project, incorporating my feedback and working through several issues :/  See the commit messages.

If this is good by you, move the Pre: parts to the head of the sequence and fold my changes in, and r=nalexander if you want it like that.

Comment 24

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8951417 [details]
Bug 1422019 - Make resource://android/asset work again with GeckoView

https://reviewboard.mozilla.org/r/220702/#review226892

::: mobile/android/components/geckoview/GeckoViewStartup.js:29
(Diff revision 1)
> +   */
> +  setResourceSubstitutions: function() {
> +    let registry = Cc["@mozilla.org/chrome/chrome-registry;1"].getService(Ci.nsIChromeRegistry);
> +    // Like jar:jar:file:///data/app/org.mozilla.geckoview.test.apk!/assets/omni.ja!/chrome/geckoview/content/geckoview.js
> +    let url = registry.convertChromeURL(Services.io.newURI("chrome://geckoview/content/geckoview.js")).spec;
> +    // Like jar:file:///data/app/org.mozilla.geckoview.test!/

"Like jar:file:///data/app/org.mozilla.geckoview.test.apk!/"

Comment 25

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8951418 [details]
Bug 1422019 - Stand up initial GeckoView tests

https://reviewboard.mozilla.org/r/220704/#review226894

See comment 15

Comment 26

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8951422 [details]
Bug 1422019 - Post: Don't require Java 8 in GeckoView androidTest code.

https://reviewboard.mozilla.org/r/220712/#review227538

This makes writing tests a bit more annoying, but definitely necessary now that they are in the main geckoview module.

Comment 27

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8951421 [details]
Bug 1422019 - Pre: Fix errors in |mach android archive-geckoview|.

https://reviewboard.mozilla.org/r/220710/#review227540

Comment 28

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8951420 [details]
Bug 1422019 - Pre: Fix diagnostic requiring |mach package|.

https://reviewboard.mozilla.org/r/220708/#review227542

Comment 29

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8951419 [details]
Bug 1422019 - REVIEW COMMENTS -- move everything into GV project.

https://reviewboard.mozilla.org/r/220706/#review227546

Comment 30

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Bug 1422019 - Fix diagnostic requiring |mach package|.

No idea when this broke -- probably the transition to Gradle 4 -- but
this seems to work.  As suggested at
https://discuss.gradle.org/t/copy-task-how-to-fail-on-no-source/25581/2.

Review commit: https://reviewboard.mozilla.org/r/221776/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/221776/

Comment 31

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Bug 1422019 - Fix errors in |mach android archive-geckoview|.

Review commit: https://reviewboard.mozilla.org/r/221778/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/221778/

Comment 32

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8933755 [details]
Bug 1422019 - Make resource://android/asset work again with GeckoView

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/204684/diff/2-3/

Comment 33

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8933333 [details]
Bug 1422019 - Stand up initial GeckoView tests

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/204248/diff/4-5/

Comment 34

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8952544 [details]
Bug 1422019 - Fix diagnostic requiring |mach package|.

https://reviewboard.mozilla.org/r/221776/#review227672

Comment 35

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8952545 [details]
Bug 1422019 - Fix errors in |mach android archive-geckoview|.

https://reviewboard.mozilla.org/r/221778/#review227674

Comment 36

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0f10d22a791e
Make resource://android/asset work again with GeckoView r=jchen
https://hg.mozilla.org/integration/mozilla-inbound/rev/403529fe19ff
Fix diagnostic requiring |mach package|. r=snorp
https://hg.mozilla.org/integration/mozilla-inbound/rev/1687a5347f75
Fix errors in |mach android archive-geckoview|. r=snorp
https://hg.mozilla.org/integration/mozilla-inbound/rev/f6b12d0f6505
Stand up initial GeckoView tests r=nalexander,jchen

Comment 37

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/0f10d22a791e
https://hg.mozilla.org/mozilla-central/rev/403529fe19ff
https://hg.mozilla.org/mozilla-central/rev/1687a5347f75
https://hg.mozilla.org/mozilla-central/rev/f6b12d0f6505

Comment 38

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/99f373203aea
Follow-up to fix import package; r=me

Comment 39

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/99f373203aea