Closed
Bug 990116
Opened 10 years ago
Closed 10 years ago
Make UITest inherit from BaseRobocopTest
Categories
(Firefox for Android Graveyard :: Testing, defect)
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)
2.73 KB,
patch
|
mcomella
:
review+
|
Details | Diff | Splinter Review |
56.07 KB,
patch
|
mcomella
:
review+
|
Details | Diff | Splinter Review |
9.25 KB,
patch
|
mcomella
:
review+
|
Details | Diff | Splinter Review |
11.12 KB,
patch
|
mcomella
:
review+
|
Details | Diff | Splinter Review |
1.58 KB,
patch
|
mcomella
:
review+
|
Details | Diff | Splinter Review |
6.52 KB,
patch
|
mcomella
:
review+
|
Details | Diff | Splinter Review |
79.97 KB,
patch
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•10 years ago
|
||
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]
Assignee | ||
Comment 2•10 years ago
|
||
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
Assignee | ||
Comment 3•10 years ago
|
||
http://tbpl.mozilla.org/?tree=Try&rev=93c9400f7d28
Assignee | ||
Comment 4•10 years ago
|
||
Attachment #8399616 -
Flags: review?(michael.l.comella)
Assignee | ||
Comment 5•10 years ago
|
||
Attachment #8399618 -
Flags: review?(michael.l.comella)
Assignee | ||
Comment 6•10 years ago
|
||
Attachment #8399619 -
Flags: review?(michael.l.comella)
Assignee | ||
Comment 7•10 years ago
|
||
Attachment #8399620 -
Flags: review?(michael.l.comella)
Assignee | ||
Comment 8•10 years ago
|
||
Attachment #8399621 -
Flags: review?(michael.l.comella)
Assignee | ||
Comment 9•10 years ago
|
||
Attachment #8399622 -
Flags: review?(michael.l.comella)
Assignee | ||
Comment 10•10 years ago
|
||
Comment 11•10 years ago
|
||
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 12•10 years ago
|
||
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 13•10 years ago
|
||
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 14•10 years ago
|
||
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+
Updated•10 years ago
|
Attachment #8399621 -
Flags: review?(michael.l.comella) → review+
Comment 15•10 years ago
|
||
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+
Assignee | ||
Comment 16•10 years ago
|
||
(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.
Assignee | ||
Comment 17•10 years ago
|
||
(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.
Assignee | ||
Comment 18•10 years ago
|
||
(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.
Assignee | ||
Comment 19•10 years ago
|
||
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
Assignee: nobody → nalexander
Status: NEW → ASSIGNED
Comment 20•10 years ago
|
||
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
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 31
Updated•3 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•