Closed Bug 912590 Opened 11 years ago Closed 10 years ago

[Follow-up] Testing: Update existing tests to use new Robotium 4.2 APIs

Categories

(Firefox for Android Graveyard :: General, defect)

26 Branch
ARM
Android
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 26

People

(Reporter: liuche, Assigned: liuche)

References

Details

Attachments

(2 files, 2 obsolete files)

This needs to be fleshed out a little more, but bug 908737 upgrades our robotium to 4.2 and adds APIs for better UI manipulation and timing. We probably want to update our current tests to use these APIs, instead of the current unstandardized hacks that we have.

(In any case, new tests should be written with the new APIs, and existing tests are what people often look to for modeling new tests.)
Depends on: 908737
Attached patch Patch: Start using some 4.2 APIs (obsolete) — Splinter Review
First pass over the main base classes.

Try push: https://tbpl.mozilla.org/?tree=Try&rev=6420ae290264
Assignee: nobody → liuche
Status: NEW → ASSIGNED
Updated patch to remove TabCounter casting.

Try: https://tbpl.mozilla.org/?tree=Try&rev=d6267ab6956e
Attachment #800513 - Attachment is obsolete: true
Attachment #800588 - Flags: feedback?(lucasr.at.mozilla)
Attachment #800588 - Flags: feedback?(gbrown)
Comment on attachment 800588 [details] [diff] [review]
Patch: Start using some 4.2 APIs v2

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

Looks good overall. But it would be nice if each aspect of the API was ported in separate patches. As in:
1. Port all waits to waitForCondition (which might mean removing waitForTest entirely)
2. Port all uses of findElement().getId() to find views (which might mean removing the Element bits entirely)
And so on...

::: mobile/android/base/tests/AboutHomeTest.java.in
@@ +97,5 @@
>  
>                  return (adapter.getCount() > 0);
>              }
> +        };
> +        boolean result = mSolo.waitForCondition(listWaitCondition, MAX_WAIT_MS);

nit: return mSolo.waitForConditin(...)
Attachment #800588 - Flags: feedback?(lucasr.at.mozilla) → feedback+
Comment on attachment 800588 [details] [diff] [review]
Patch: Start using some 4.2 APIs v2

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

::: mobile/android/base/tests/BaseTest.java.in
@@ +320,5 @@
>          }
>          // log out wait failure for diagnostic purposes only;
>          // a failed wait may be normal and does not necessarily
>          // warrant a test assertion/failure
>          mAsserter.dumpLog("waitForTest timeout after "+timeout+" ms");

This little log statement has been quite valuable when debugging and examining logs. I assume that Condition fails silently? Perhaps we should have our own waitForCondition that wraps Solo.waitForCondition and adds similar logging?

@@ +548,5 @@
> +        // This needs to be run on the UI thread to click on the views.
> +        runOnUiThreadSync(new Runnable() {
> +            @Override
> +            public void run() {
> +                mAsserter.ok(tabs.performClick(), "checking that tabs clicked", "tabs element clicked");

Why not use mSolo.clickOnView(mSolo.getView("tabs"))?
Attachment #800588 - Flags: feedback?(gbrown) → feedback+
Comment on attachment 800588 [details] [diff] [review]
Patch: Start using some 4.2 APIs v2

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

::: mobile/android/base/tests/BaseTest.java.in
@@ +473,5 @@
>      public final void selectMenuItem(String menuItemName) {
>          // build the item name ready to be used
>          String itemName = "^" + menuItemName + "$";
>          mActions.sendSpecialKey(Actions.SpecialKey.MENU);
> +        if (waitForEnabledText(itemName)) {

waitForEnabledText is a "stronger"/"safer" test in that it additionally waits for the item to be enabled. But keep in mind that waitForText (via Solo.waitForText) also has additional capabilities - like scrolling down when text is not found - that waitForEnabledText does not.
Attachment #800588 - Attachment is obsolete: true
Attachment #801015 - Flags: review?(gbrown)
Lucas, I've split the patches into two. I wanted to make the changes to the base tests first (AboutHomeTest and BaseTest) - I'll probably continue to add more patches, for porting the rest of the tests, but these should act as examples, should anyone else want to start porting tests to 4.2 :)

> >          mAsserter.dumpLog("waitForTest timeout after "+timeout+" ms");
> 
> This little log statement has been quite valuable when debugging and
> examining logs. I assume that Condition fails silently? Perhaps we should
> have our own waitForCondition that wraps Solo.waitForCondition and adds
> similar logging?

Done!

> @@ +548,5 @@
> > +        // This needs to be run on the UI thread to click on the views.
> > +        runOnUiThreadSync(new Runnable() {
> > +            @Override
> > +            public void run() {
> > +                mAsserter.ok(tabs.performClick(), "checking that tabs clicked", "tabs element clicked");
> 
> Why not use mSolo.clickOnView(mSolo.getView("tabs"))?

mSolo.clickOnView doesn't return a boolean, which this assertion requires.
Attachment #801016 - Flags: review?(gbrown)
Attachment #801016 - Flags: review?(gbrown) → review+
Attachment #801015 - Flags: review?(gbrown) → review+
(In reply to Chenxia Liu [:liuche] from comment #7)
> > Why not use mSolo.clickOnView(mSolo.getView("tabs"))?
> 
> mSolo.clickOnView doesn't return a boolean, which this assertion requires.

Those assertions do not add much value; I think it would be fine to remove them. But either way is OK.
Landed first two parts on fx-team:
https://hg.mozilla.org/integration/fx-team/rev/3500cc14c4a1
https://hg.mozilla.org/integration/fx-team/rev/8bceed557a76

Adding [leave-open] for the moment, since we might want to just keep adding patches for porting additional tests to 4.2; it seems less messy than a metabug with 4.2 API with specific API ports hanging off of it.
Whiteboard: [leave-open]
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [leave-open]
Target Milestone: --- → Firefox 26
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.