Add test for GeckoView API

NEW
Unassigned

Status

()

Firefox for Android
GeckoView
2 months ago
8 days ago

People

(Reporter: snorp, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

We have none right now and it's terrible.
Comment hidden (mozreview-request)
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.
Attachment #8933333 - Flags: feedback?(nalexander)

Comment 3

2 months ago
mozreview-review
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.
Attachment #8933333 - Flags: review-
(Reporter)

Comment 4

2 months ago
mozreview-review-reply
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 hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 7

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

https://reviewboard.mozilla.org/r/204684/#review210270
Attachment #8933755 - Flags: review?(nchen) → review+

Comment 8

2 months ago
mozreview-review
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.)
Attachment #8933755 - Flags: review+
(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 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
Attachment #8933333 - Flags: review?(nalexander) → review+
Comment on attachment 8933755 [details]
Bug 1422019 - Make resource://android/asset work again with GeckoView

oops.. recovery my mistake .. sorry..
Attachment #8933755 - Flags: review?(nalexander)
Attachment #8933755 - Flags: review+
You need to log in before you can comment on or make changes to this bug.