Closed Bug 1439410 Opened 5 years ago Closed 5 years ago

Add headless GeckoSession tests

Categories

(GeckoView :: General, enhancement)

Product:
GeckoView
Component:
General
Platform:
All
Android
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(firefox60 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla60
Tracking Flags:
Tracking Status
firefox60 --- fixed

People

(Reporter: jchen, Assigned: jchen)

References

Details

Attachments

(6 files)

Bug 1439410 - 1. Enable Kotlin for geckoview tests;
59 bytes, text/x-review-board-request
nalexander
: review+
Details
Bug 1439410 - 2. Add copy constructor for GeckoSessionSettings;
59 bytes, text/x-review-board-request
snorp
: review+
Details
Bug 1439410 - 3a. Make GeckoSessionSettings.Key<> public;
59 bytes, text/x-review-board-request
snorp
: review+
Details
Bug 1439410 - 4. Add JUnit4 rule for testing GeckoSession;
59 bytes, text/x-review-board-request
snorp
: review+
Details
Bug 1439410 - 5. Add test for GeckoSessionTestRule;
59 bytes, text/x-review-board-request
snorp
: review+
Details
Bug 1439410 - 3b. Add GeckoSession.getScrollListener();
59 bytes, text/x-review-board-request
jchen
: review+
Details 
I've been playing around getting some headless GeckoSession tests going.
Comment on attachment 8952808 [details]
Bug 1439410 - 2. Add copy constructor for GeckoSessionSettings;

https://reviewboard.mozilla.org/r/222030/#review227926
Attachment #8952808 - Flags: review?(snorp) → review+
These patches are all based on snorp and nalexander's geckoview androidTest patches. Part 5 is a test for the test framework that also demonstrates some of its functionalities.
Comment on attachment 8952809 [details]
Bug 1439410 - 3a. Make GeckoSessionSettings.Key<> public;

https://reviewboard.mozilla.org/r/222032/#review227928
Attachment #8952809 - Flags: review?(snorp) → review+
Comment on attachment 8952807 [details]
Bug 1439410 - 1. Enable Kotlin for geckoview tests;

https://reviewboard.mozilla.org/r/222028/#review227936

I have questions and one small refactoring request, but nothing major.  Roll on!

::: build.gradle:56
(Diff revision 1)
>      }
>  
>      dependencies {
>          classpath 'com.android.tools.build:gradle:3.0.1'
>          classpath 'com.getkeepsafe.dexcount:dexcount-gradle-plugin:0.8.2'
> +        classpath 'org.jetbrains.kotlin:kotlin-gradle-plugin:1.1.51'

Let's put the Kotlin version in the shared `ext`, around https://searchfox.org/mozilla-central/source/build.gradle#16.

::: mobile/android/geckoview/build.gradle:155
(Diff revision 1)
>  
>  dependencies {
>      implementation "com.android.support:support-v4:${mozconfig.substs.ANDROID_SUPPORT_LIBRARY_VERSION}"
>      implementation "com.android.support:palette-v7:${mozconfig.substs.ANDROID_SUPPORT_LIBRARY_VERSION}"
>  
> +    testImplementation 'org.jetbrains.kotlin:kotlin-stdlib:1.1.51'

I see at https://kotlinlang.org/docs/reference/using-gradle.html#targeting-android that there are `kotlin-stdlib-jdk{7,8}` libraries as well.  Should we be using one of those?

::: mobile/android/geckoview/build.gradle:271
(Diff revision 1)
>          // Javadoc and sources for developer ergononomics.
>          archives javadocJarOfficialWithGeckoBinariesNoMinApiRelease
>          archives sourcesJarOfficialWithGeckoBinariesNoMinApiRelease
>      }
> +
> +    // For now, ensure Kotlin is only used in tests.

Is a missing `implementation` stanza not sufficient to prevent using Kotlin in `src/main`?  Have you verified that the `kotlin-android` plugin isn't secretly addig the `kotlin-stdlib` dependency behind your back?
Attachment #8952807 - Flags: review?(nalexander) → review+
Comment on attachment 8952810 [details]
Bug 1439410 - 4. Add JUnit4 rule for testing GeckoSession;

https://reviewboard.mozilla.org/r/222034/#review227942

::: mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/rule/GeckoSessionTestRule.java:59
(Diff revision 1)
> +
> +    public final String APK_URI_PREFIX = "resource://android" ;
> +    public final String HELLO_URI_SUFFIX = "/assets/www/hello.html";
> +    public final String HELLO_URI = APK_URI_PREFIX + HELLO_URI_SUFFIX;
> +    public final String HELLO2_URI_SUFFIX = "/assets/www/hello2.html";
> +    public final String HELLO2_URI = APK_URI_PREFIX + HELLO2_URI_SUFFIX;

We're going to have many such pages eventually. This should probably just live wherever the test needs it instead of the rule.

::: mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/rule/GeckoSessionTestRule.java:428
(Diff revision 1)
> +
> +        mSession = new GeckoSession(settings);
> +
> +        for (final Class<?> cls : CALLBACK_CLASSES) {
> +            if (cls != null) {
> +                getCallbackSetter(cls).invoke(mSession, proxy);

This attaches a default listener/delegate for all interfaces in order to do the call counting, playback, etc. Do we want to be able to run tests with fewer (or zero) delegates registered?

::: mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/rule/GeckoSessionTestRule.java:435
(Diff revision 1)
> +        }
> +
> +        mSession.openWindow(mInstrumentation.getTargetContext());
> +
> +        if (settings.getBoolean(GeckoSessionSettings.USE_MULTIPROCESS)) {
> +            // Under e10s, we receive an initial about:blank load; don't expose that to the test.

Are you still getting that with my patches applied? If so, we need to get a followup filed for that.

::: mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/rule/GeckoSessionTestRule.java:660
(Diff revision 1)
> +     * to a GeckoSession listener are supported.
> +     *
> +     * @param listener Target listener object; must implement one or more interfaces
> +     *                 under GeckoSession.
> +     */
> +    public void forCallbacksDuringWait(final @NonNull Object listener) {

Wow, this is brilliant!

We can't use this if we want to do performance timings (like measure time between onPageStart() and onPageStop()), but a lot of the other stuff in this rule is useful for that. Maybe it would be good to split some of it up?
Attachment #8952810 - Flags: review?(snorp) → review+
Comment on attachment 8952810 [details]
Bug 1439410 - 4. Add JUnit4 rule for testing GeckoSession;

https://reviewboard.mozilla.org/r/222034/#review227960

::: mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/util/Dummy.kt:9
(Diff revision 1)
> +package org.mozilla.geckoview.test.util
> +
> +import org.mozilla.gecko.GeckoSession
> +import org.mozilla.gecko.util.GeckoBundle
> +
> +class Dummy private constructor() {

I think we might need a better name, since we override delegate functions from here to use waitUntilCalled(). Maybe All, Delegates, or Listeners?
Comment on attachment 8952811 [details]
Bug 1439410 - 5. Add test for GeckoSessionTestRule;

https://reviewboard.mozilla.org/r/222026/#review227964

::: mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/GeckoSessionTestRuleTest.kt:112
(Diff revision 1)
> +                                          securityInfo: GeckoSession.ProgressListener.SecurityInformation) {
> +                counter++
> +            }
> +        })
> +
> +        assertEquals("Invocation count should be correct", 1, counter)

Shouldn't this be 3? Or it 1 because we're only waiting for one method to be called above in waitUntilCalled()?
Attachment #8952811 - Flags: review?(snorp) → review+
Comment on attachment 8952811 [details]
Bug 1439410 - 5. Add test for GeckoSessionTestRule;

https://reviewboard.mozilla.org/r/222026/#review227964

> Shouldn't this be 3? Or it 1 because we're only waiting for one method to be called above in waitUntilCalled()?

Yeah without any @AssertCalled annotation, it just waits for any call.
Comment on attachment 8952810 [details]
Bug 1439410 - 4. Add JUnit4 rule for testing GeckoSession;

https://reviewboard.mozilla.org/r/222034/#review227960

> I think we might need a better name, since we override delegate functions from here to use waitUntilCalled(). Maybe All, Delegates, or Listeners?

"Callbacks"? I've been using that to mean "listener or delegate"
Comment on attachment 8952810 [details]
Bug 1439410 - 4. Add JUnit4 rule for testing GeckoSession;

https://reviewboard.mozilla.org/r/222034/#review227942

> This attaches a default listener/delegate for all interfaces in order to do the call counting, playback, etc. Do we want to be able to run tests with fewer (or zero) delegates registered?

I think we can add a @Disable or something in a follow-up

> Are you still getting that with my patches applied? If so, we need to get a followup filed for that.

Still getting it. I think it might have to do with the extra step of adding "remote=true" to `<browser>` for e10s that generates an extra load.

> Wow, this is brilliant!
> 
> We can't use this if we want to do performance timings (like measure time between onPageStart() and onPageStop()), but a lot of the other stuff in this rule is useful for that. Maybe it would be good to split some of it up?

I think with custom delegates we can do some timing stuff. We just have to make sure the test doesn't block the UI thread and throw off the timing.
Comment on attachment 8952807 [details]
Bug 1439410 - 1. Enable Kotlin for geckoview tests;

https://reviewboard.mozilla.org/r/222028/#review227936

> Let's put the Kotlin version in the shared `ext`, around https://searchfox.org/mozilla-central/source/build.gradle#16.

I put it under 'buildscript' because 'allprojects' didn't work for some reason.

> I see at https://kotlinlang.org/docs/reference/using-gradle.html#targeting-android that there are `kotlin-stdlib-jdk{7,8}` libraries as well.  Should we be using one of those?

I'm not sure of the exact differences, but I think 'jdk7/8' may be referring to running in a desktop environment.

> Is a missing `implementation` stanza not sufficient to prevent using Kotlin in `src/main`?  Have you verified that the `kotlin-android` plugin isn't secretly addig the `kotlin-stdlib` dependency behind your back?

Right, Kotlin will still compile without stdlib, so we need the extra check.
Comment on attachment 8953277 [details]
Bug 1439410 - 3b. Add GeckoSession.getScrollListener();

https://reviewboard.mozilla.org/r/222554/#review228448
Attachment #8953277 - Flags: review+
Pushed by nchen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6a8a4bc838ab
1. Enable Kotlin for geckoview tests; r=nalexander
https://hg.mozilla.org/integration/autoland/rev/6dfd2228e3fd
2. Add copy constructor for GeckoSessionSettings; r=snorp
https://hg.mozilla.org/integration/autoland/rev/bcda392921ff
3a. Make GeckoSessionSettings.Key<> public; r=snorp
https://hg.mozilla.org/integration/autoland/rev/6ff70c4ce49c
3b. Add GeckoSession.getScrollListener(); r=jchen
https://hg.mozilla.org/integration/autoland/rev/275ec40988ba
4. Add JUnit4 rule for testing GeckoSession; r=snorp
https://hg.mozilla.org/integration/autoland/rev/197404819c14
5. Add test for GeckoSessionTestRule; r=snorp
Pushed by nchen@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e3960334b2f4
6a. Follow-up to fix import package; r=me
https://hg.mozilla.org/integration/mozilla-inbound/rev/e12ed9dcb464
6b. Follow-up to add some small improvements; r=me
Product: Firefox for Android → GeckoView
Target Milestone: Firefox 60 → mozilla60
You need to log in before you can comment on or make changes to this bug.