990116 - Make UITest inherit from BaseRobocopTest

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Testing, defect)

Description

[Nick Alexander :nalexander [he/him]]

Bug 986114 landed BaseRobocopTest, which factors out some of the Robocop shenanigans.  I didn't realize at the time that UITest does not inherit from BaseTest, and as such does not inherit from BaseRobocopTest.  This ticket tracks unifying the inheritance.

I found this out while reducing reflection in BaseTest, namely the ugly determination of the Fennec launcher activity.

Comment 1

[Nick Alexander :nalexander [he/him]]

This is not a good first (or even second, or third) because it requires try access to run the test suites.

Comment 2

[Nick Alexander :nalexander [he/him]]

What I thought would be an odd hour on Friday turned into several hours on Friday and Monday.  Let's see how I did:

https://tbpl.mozilla.org/?tree=Try&rev=4a0a543b9469

Comment 3

[Nick Alexander :nalexander [he/him]]

http://tbpl.mozilla.org/?tree=Try&rev=93c9400f7d28

Comment 4

[Nick Alexander :nalexander [he/him]]

Attachment: Pre: Remove UITest.mActivity. r=mcomella

Comment 5

[Nick Alexander :nalexander [he/him]]

Attachment: Part 1: Move UITest.Type to BaseRobocopTest. r=mcomella

Comment 6

[Nick Alexander :nalexander [he/him]]

Attachment: Part 2: Split BROWSER_INTENT_CLASS and BROWSER_INTENT_CLASS_NAME. r=mcomella

Comment 7

[Nick Alexander :nalexander [he/him]]

Attachment: Part 2: Use BROWSER_INTENT_CLASS in Robocop tests. r=mcomella

Comment 8

[Nick Alexander :nalexander [he/him]]

Attachment: Part 3: Use BROWSER_INTENT_CLASS in browser JUnit 3 tests. r=mcomella

Comment 9

[Nick Alexander :nalexander [he/him]]

Attachment: Part 4: Make UITest inherit from BaseRobocopTest. r=mcomella

Comment 10

[Nick Alexander :nalexander [he/him]]

Attachment: Post: Clean Robocop imports.

Comment 11

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

Comment on attachment 8399616 [details] [diff] [review]
Pre: Remove UITest.mActivity. r=mcomella

Review of attachment 8399616 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/tests/UITest.java
@@ +87,3 @@
>          final Intent intent = createActivityIntent(config);
>          setActivityIntent(intent);
> +        Activity activity = getActivity();

nit: final.

Comment 12

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

Comment on attachment 8399619 [details] [diff] [review]
Part 2: Split BROWSER_INTENT_CLASS and BROWSER_INTENT_CLASS_NAME. r=mcomella

Review of attachment 8399619 [details] [diff] [review]:
-----------------------------------------------------------------

r+ w/ nits

::: mobile/android/base/AppConstants.java.in
@@ +30,5 @@
> +     * This should always agree with <code>BROWSER_INTENT_CLASS_NAME</code>.
> +     */
> +    public static final Class<? extends Activity> BROWSER_INTENT_CLASS = @ANDROID_PACKAGE_NAME@.App.class;
> +    /**
> +     * The name of the Java class that launches the browser. 2

nit: Remove 2 from the EOL.

Comment 13

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

Comment on attachment 8399618 [details] [diff] [review]
Part 1: Move UITest.Type to BaseRobocopTest. r=mcomella

Review of attachment 8399618 [details] [diff] [review]:
-----------------------------------------------------------------

Touch all the files! r+ w/ nit.

::: mobile/android/base/tests/BaseRobocopTest.java
@@ +55,5 @@
>          mConfig = FennecNativeDriver.convertTextToTable(configFile);
>          mLogFile = (String) mConfig.get("logfile");
>  
>          // Initialize the asserter.
> +        if (getTestType() == Type.TALOS) {

nit: Maybe use `switch` instead, just so we can throw on the default case? Your call.

::: mobile/android/base/tests/testBrowserProviderPerf.java
@@ +76,5 @@
>      }
>  
>      @Override
> +    protected Type getTestType() {
> +        return Type.TALOS;

Does this not require an import?

Comment 14

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

Comment on attachment 8399620 [details] [diff] [review]
Part 2: Use BROWSER_INTENT_CLASS in Robocop tests. r=mcomella

Review of attachment 8399620 [details] [diff] [review]:
-----------------------------------------------------------------

Just want to let you know that this is one of two part 2s. r+ w/ nit.

::: mobile/android/base/tests/UITest.java
@@ +60,2 @@
>      public UITest() {
> +        super((Class<Activity>) AppConstants.BROWSER_INTENT_CLASS);

nit: Why not allow the default constructor to do this for you, like you do in BaseTest?

Comment 15

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

Comment on attachment 8399622 [details] [diff] [review]
Part 4: Make UITest inherit from BaseRobocopTest. r=mcomella

Review of attachment 8399622 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/tests/BaseRobocopTest.java
@@ +21,5 @@
>          MOCHITEST,
>          TALOS
>      }
>  
>      protected Assert mAsserter;

Unrelated nit: We decided to move away from this naming convention, right? I'm assuming this can be a followup? [mentor=?] :P

Comment 16

[Nick Alexander :nalexander [he/him]]

(In reply to Michael Comella (:mcomella) from comment #14)
> Comment on attachment 8399620 [details] [diff] [review]
> Part 2: Use BROWSER_INTENT_CLASS in Robocop tests. r=mcomella
> 
> Review of attachment 8399620 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Just want to let you know that this is one of two part 2s. r+ w/ nit.
> 
> ::: mobile/android/base/tests/UITest.java
> @@ +60,2 @@
> >      public UITest() {
> > +        super((Class<Activity>) AppConstants.BROWSER_INTENT_CLASS);
> 
> nit: Why not allow the default constructor to do this for you, like you do
> in BaseTest?

mcomella and I discussed this in IRC.  For posterity: the next patch makes UITest inherit from BaseRobocopTest, so the more general constructor gets inherited.

Comment 17

[Nick Alexander :nalexander [he/him]]

(In reply to Michael Comella (:mcomella) from comment #15)
> Comment on attachment 8399622 [details] [diff] [review]
> Part 4: Make UITest inherit from BaseRobocopTest. r=mcomella
> 
> Review of attachment 8399622 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/tests/BaseRobocopTest.java
> @@ +21,5 @@
> >          MOCHITEST,
> >          TALOS
> >      }
> >  
> >      protected Assert mAsserter;
> 
> Unrelated nit: We decided to move away from this naming convention, right?
> I'm assuming this can be a followup? [mentor=?] :P

I follow per-file conventions.  I don't care to file a follow-up, but you're welcome to.

Comment 18

[Nick Alexander :nalexander [he/him]]

(In reply to Michael Comella (:mcomella) from comment #13)
> Comment on attachment 8399618 [details] [diff] [review]
> Part 1: Move UITest.Type to BaseRobocopTest. r=mcomella
> 
> Review of attachment 8399618 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Touch all the files! r+ w/ nit.
> 
> ::: mobile/android/base/tests/BaseRobocopTest.java
> @@ +55,5 @@
> >          mConfig = FennecNativeDriver.convertTextToTable(configFile);
> >          mLogFile = (String) mConfig.get("logfile");
> >  
> >          // Initialize the asserter.
> > +        if (getTestType() == Type.TALOS) {
> 
> nit: Maybe use `switch` instead, just so we can throw on the default case?
> Your call.
> 
> ::: mobile/android/base/tests/testBrowserProviderPerf.java
> @@ +76,5 @@
> >      }
> >  
> >      @Override
> > +    protected Type getTestType() {
> > +        return Type.TALOS;
> 
> Does this not require an import?

In my tree, this patch has the import.  In any case, the final push should be fine.

Comment 19

[Nick Alexander :nalexander [he/him]]

https://hg.mozilla.org/integration/fx-team/rev/77e6546f5460
https://hg.mozilla.org/integration/fx-team/rev/8fff417c7831
https://hg.mozilla.org/integration/fx-team/rev/95a86df2317f
https://hg.mozilla.org/integration/fx-team/rev/ee4e3900536c
https://hg.mozilla.org/integration/fx-team/rev/49cb824b0da4
https://hg.mozilla.org/integration/fx-team/rev/3a9374b328cf
https://hg.mozilla.org/integration/fx-team/rev/523e67cf4b9e

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/77e6546f5460
https://hg.mozilla.org/mozilla-central/rev/8fff417c7831
https://hg.mozilla.org/mozilla-central/rev/95a86df2317f
https://hg.mozilla.org/mozilla-central/rev/ee4e3900536c
https://hg.mozilla.org/mozilla-central/rev/49cb824b0da4
https://hg.mozilla.org/mozilla-central/rev/3a9374b328cf
https://hg.mozilla.org/mozilla-central/rev/523e67cf4b9e