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.

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 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 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 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 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 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

(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.

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

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