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

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Chenxia Liu [:liuche]]

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

Comment 1

[Chenxia Liu [:liuche]]

Attachment: Patch: Start using some 4.2 APIs

First pass over the main base classes.

Try push: https://tbpl.mozilla.org/?tree=Try&rev=6420ae290264

Comment 2

[Chenxia Liu [:liuche]]

Attachment: Patch: Start using some 4.2 APIs v2

Updated patch to remove TabCounter casting.

Try: https://tbpl.mozilla.org/?tree=Try&rev=d6267ab6956e

Comment 3

[Lucas Rocha (:lucasr)]

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

Comment 4

[Geoff Brown [:gbrown]]

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"))?

Comment 5

[Geoff Brown [:gbrown]]

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.

Comment 6

[Chenxia Liu [:liuche]]

Attachment: Part 1: Use getView from 4.2 APIs on base tests

Comment 7

[Chenxia Liu [:liuche]]

Attachment: 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.

Comment 8

[Geoff Brown [:gbrown]]

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

Comment 9

[Chenxia Liu [:liuche]]

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.

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3500cc14c4a1