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

RESOLVED FIXED in Firefox 26

Status

()

RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: liuche, Assigned: liuche)

Tracking

26 Branch
Firefox 26
ARM
Android
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

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
Created attachment 800513 [details] [diff] [review]
Patch: Start using some 4.2 APIs

First pass over the main base classes.

Try push: https://tbpl.mozilla.org/?tree=Try&rev=6420ae290264
Assignee: nobody → liuche
Status: NEW → ASSIGNED
Created attachment 800588 [details] [diff] [review]
Patch: Start using some 4.2 APIs v2

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.
Created attachment 801015 [details] [diff] [review]
Part 1: Use getView from 4.2 APIs on base tests
Attachment #800588 - Attachment is obsolete: true
Attachment #801015 - Flags: review?(gbrown)
Created attachment 801016 [details] [diff] [review]
Part 2: Use Condition on base tests v1

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)

Updated

5 years ago
Attachment #801016 - Flags: review?(gbrown) → review+

Updated

5 years ago
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
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [leave-open]
Target Milestone: --- → Firefox 26
You need to log in before you can comment on or make changes to this bug.