Closed Bug 990116 Opened 8 years ago Closed 8 years ago

Make UITest inherit from BaseRobocopTest

Categories

(Firefox for Android Graveyard :: Testing, defect)

All
Android
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 31

People

(Reporter: nalexander, Assigned: nalexander)

References

Details

(Whiteboard: [mentor=nalexander][lang=java][not good first bug])

Attachments

(7 files)

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.
This is not a good first (or even second, or third) because it requires try access to run the test suites.
Whiteboard: [mentor=nalexander][lang=java][not good first bug]
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
Attachment #8399616 - Flags: review?(michael.l.comella)
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.
Attachment #8399616 - Flags: review?(michael.l.comella) → review+
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.
Attachment #8399619 - Flags: review?(michael.l.comella) → review+
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?
Attachment #8399618 - Flags: review?(michael.l.comella) → review+
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?
Attachment #8399620 - Flags: review?(michael.l.comella) → review+
Attachment #8399621 - Flags: review?(michael.l.comella) → review+
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
Attachment #8399622 - Flags: review?(michael.l.comella) → review+
(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.
(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.
(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.
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.