855146 - Add some IME automated tests

Status: RESOLVED INCOMPLETE

(Firefox for Android Graveyard :: Keyboards and IME, defect)

Description

[Chris Peterson [:cpeterson]]

Attachment: part-1-clickViewOnScreen.patch

Patch 1 extracts a clickViewOnScreen() method so I can reuse this code in a later patch's test case. I don't really like the "clickViewOnScreen" method name, but it follows Robotium's "OnScreen" naming conventions. But if you have a better suggestion, please let me know! <:)

Comment 1

[Chris Peterson [:cpeterson]]

Attachment: part-2-runOnUiThreadSync.patch

Extract runOnUiThreadSync() test method so I can reuse this code in a later patch's test case. Again, I don't really like the method name "runOnUiThreadSync", but it is a synchronous version of Android's Activity.runOnUiThread().

Comment 2

[Chris Peterson [:cpeterson]]

Attachment: part-3-testInputAwesomeBar.patch

Part 3: Add testInputAwesomeBar test for URL text.

This AwesomeBar test does not test Gecko's IME code, but it does confirm that Robocop can drive basic text editing on the test machines.

Comment 3

[Geoff Brown [:gbrown]]

Comment on attachment 729908 [details] [diff] [review]
part-3-testInputAwesomeBar.patch

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

I like this -- a great start to IME tests!

A few nits + we need to get this running reliably on all devices.

::: mobile/android/base/tests/testInputAwesomeBar.java.in
@@ +4,5 @@
> +import @ANDROID_PACKAGE_NAME@.*;
> +
> +import android.app.Activity;
> +import android.graphics.Rect;
> +import android.util.Log;

Some unused imports here

@@ +8,5 @@
> +import android.util.Log;
> +import android.view.View;
> +import android.widget.EditText;
> +
> +public final class testInputAwesomeBar extends BaseTest {

Add a brief header comment to call out the scope of the test.

@@ +9,5 @@
> +import android.view.View;
> +import android.widget.EditText;
> +
> +public final class testInputAwesomeBar extends BaseTest {
> +    private static final String LOGTAG = "testInputAwesomeBar";

Unused. Robocop tests use their own logging utilities.

@@ +81,5 @@
> +        // Wait for about:home to load
> +        waitForText("Browse all Fennec add-ons");
> +
> +        mActions.sendKeys("wx");
> +        assertAwesomeBarText("uv");

I don't think this check is valid. After you press BACK, the awesome screen is dismissed and we switch activities. assertAwesomeBarText is going to try to check a view that may no longer exist.

You could click on the awesome bar again, then check. But you want to make sure that you don't click on the awesome bar too soon -- you'll be racing those key events. Maybe just do without this check?

@@ +100,5 @@
> +        mActions.sendKeys("yz");
> +
> +        // FIXME BUG 855134: Clicking back to the AwesomeBar after the VKB has closed should
> +        // select all URL text. Instead it deselects the URL text and positions the cursor.
> +        mAsserter.todo_is(mTextElement.getText(), "yz", "AwesomeBar text");

This passes on Tegra, giving an UNEXPECTED-PASS failure. Consider commenting out the assertion and addressing in bug 855134.

Comment 4

[Geoff Brown [:gbrown]]

Comment on attachment 729906 [details] [diff] [review]
part-2-runOnUiThreadSync.patch

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

Thanks for the re-factor in FennecNativeElement -- I like it.

It seems a shame to duplicate code in FennecNativeElement and BaseTest, but I appreciate that may be difficult to avoid.

::: mobile/android/base/tests/BaseTest.java.in
@@ +573,5 @@
> +                    runnable.run();
> +                    try {
> +                        syncQueue.put(new Object());
> +                    } catch (InterruptedException e) {
> +                        FennecNativeDriver.log(FennecNativeDriver.LogLevel.ERROR, e);

nit -- use mAsserter.dumpLog() instead, in BaseTest.

@@ +580,5 @@
> +            });
> +        try {
> +            // Wait for the UiThread code to finish running
> +            if (syncQueue.poll(MAX_WAIT_MS, TimeUnit.MILLISECONDS) == null) {
> +                FennecNativeDriver.log(FennecNativeDriver.LogLevel.ERROR,

ditto

@@ +585,5 @@
> +                                       "time-out waiting for UI thread");
> +                FennecNativeDriver.logAllStackTraces(FennecNativeDriver.LogLevel.ERROR);
> +            }
> +        } catch (InterruptedException e) {
> +            FennecNativeDriver.log(FennecNativeDriver.LogLevel.ERROR, e);

ditto

Comment 5

[Geoff Brown [:gbrown]]

Comment on attachment 729905 [details] [diff] [review]
part-1-clickViewOnScreen.patch

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

Again, I appreciate the re-factoring. ...but, I question if we should be using this code at all. Robotium provides Solo.clickOnView(View), which looks to me to be equivalent and slightly safer. Solo.clickOnView (and Solo.clickOnScreen!) ends up calling Clicker.clickOnScreen. See https://github.com/jayway/robotium/blob/master/robotium-solo/src/main/java/com/jayway/android/robotium/solo/Clicker.java#L129

        public void clickOnScreen(View view, boolean longClick, int time) {
                if(view == null)
                        Assert.assertTrue("View is null and can therefore not be clicked!", false);
                int[] xy = new int[2];

                view.getLocationOnScreen(xy);

                final int viewWidth = view.getWidth();
                final int viewHeight = view.getHeight();
                final float x = xy[0] + (viewWidth / 2.0f);
                float y = xy[1] + (viewHeight / 2.0f);

                if (longClick)
                        clickLongOnScreen(x, y, time);
                else
                        clickOnScreen(x, y);
        }

Comment 6

[Chris Peterson [:cpeterson]]

Attachment: part-2-runOnUiThreadSync-v2.patch

What if I consolidate the duplicated runOnUiThreadSync() code into a RobocopUtils class with some static utility methods?

Comment 7

[Geoff Brown [:gbrown]]

Comment on attachment 730433 [details] [diff] [review]
part-2-runOnUiThreadSync-v2.patch

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

Awesome!

Comment 8

[Chris Peterson [:cpeterson]]

Landed part-2-runOnUiThreadSync-v2.patch:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9a0e1cdd47ce

Comment 9

[Chris Peterson [:cpeterson]]

Attachment: part-3-testInputAwesomeBar-v2.patch

Changes:
* Added test description comment.
* Removed unused imports.
* Removed test that tried to access AwesomeBar Activity after it was destroyed.
* Changed the about:home string we wait for so it will work regardless of build's channel name.
* Hacked intermittent UNEXPECTED-PASS test case so it shouldn't fail for now. I will revisit that test case in bug 855134.

Comment 10

[Geoff Brown [:gbrown]]

Comment on attachment 730897 [details] [diff] [review]
part-3-testInputAwesomeBar-v2.patch

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

Thanks!

Try looks good: https://tbpl.mozilla.org/?tree=Try&rev=1f552763a069

Comment 11

[Chris Peterson [:cpeterson]]

Attachment: bug-855146-part-4-waitForAlert.patch

Geoff, f? This is a simple test demonstrating a RoboCop test that finds, reads, and dismisses a JS alert() dialog. Do you see any potential pitfalls or opportunities for improvement? I have some follow-on test cases that use functionality.

Comment 12

[Chris Peterson [:cpeterson]]

Landed part-3-testInputAwesomeBar-v2.patch:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d65f7422e8bf

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9a0e1cdd47ce
https://hg.mozilla.org/mozilla-central/rev/d65f7422e8bf

Comment 14

[Geoff Brown [:gbrown]]

Comment on attachment 731366 [details] [diff] [review]
bug-855146-part-4-waitForAlert.patch

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

Interesting!

Are you thinking of using this as a generic mechanism for communicating between JS and Robocop? (I am looking at using events for that, but it's getting complicated...)

Be aware of bug 855978 - upgrade to robotium 4.0.

::: mobile/android/base/tests/BaseTest.java.in
@@ +360,5 @@
>          return rc;
>      }
>  
> +    protected final String waitForAlert() {
> +        final String ALERT_TITLE = "The page at http://mochi.test:8888 says:";

You might be able to use something like "The page at .* says:", to make it that much more generic.

@@ +369,5 @@
> +        }
> +
> +        // Grab the first view and then search for alert dialog's TextViews.
> +        ArrayList<View> views = mSolo.getCurrentViews();
> +        ArrayList<TextView> textViews = mSolo.getCurrentTextViews(views.get(0));

This seems like it might break. Does/could getCurrentViews() return multiple views here? I don't think it guarantees an order to the views...

Comment 15

[Chris Peterson [:cpeterson]]

(In reply to Geoff Brown [:gbrown] from comment #14)
> You might be able to use something like "The page at .* says:", to make it
> that much more generic.

Thanks! I didn't realize waitForText() took a regex parameter.


> > +        // Grab the first view and then search for alert dialog's TextViews.
> > +        ArrayList<View> views = mSolo.getCurrentViews();
> > +        ArrayList<TextView> textViews = mSolo.getCurrentTextViews(views.get(0));
> 
> This seems like it might break. Does/could getCurrentViews() return multiple
> views here? I don't think it guarantees an order to the views...

Yes, this code is probably fragile. getCurrentViews() does return multiple views and I was just grabbing, arbitrarily, the first view. The views *seem* to be ordered hierarchically, so the first is the outermost FrameLayout, allowing the getCurrentTextViews() to get all TextViews (including the alert() dialog).

Comment 16

[Geoff Brown [:gbrown]]

Attachment: proof of concept demonstrating Javascript -> Robocop messaging

This is what I was talking about in Comment 14. This works! Thanks to :jmaher for showing me how to use SpecialPowers.

Comment 17

[Chris Peterson [:cpeterson]]

Comment on attachment 732066 [details] [diff] [review]
proof of concept demonstrating Javascript -> Robocop messaging

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

Looks good. sendMessageToJava() looks simpler and more reliable than my alert() dialog approach.

Comment 18

[Wesley Johnston (:wesj)]

You can also send some data along with the page info. See:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/tests/testDistribution.java.in#108

Comment 19

[Geoff Brown [:gbrown]]

Attachment: do not remove special powers from robocop profile

Trivial patch to allow use of SpecialPowers in robocop tests.

Comment 20

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 733089 [details] [diff] [review]
do not remove special powers from robocop profile

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

oh cool, this will increase our overall test time, but it is worth it.

Comment 21

[Geoff Brown [:gbrown]]

Landed "do not remove special powers from robocop profile" patch:

https://hg.mozilla.org/integration/mozilla-inbound/rev/343da9f79907

Comment 22

[Chris Peterson [:cpeterson]]

Attachment: add-EventExpecter-reuse-checks.patch

GeckoEventExpecter instances can't be reused because blockForEvent() unregisters the event listener. This patch adds runtime checks to avoid a deadlock where a Robocop test might reuse an EventExpecter and not receive a test response.

Comment 23

[Chris Peterson [:cpeterson]]

Attachment: part-4-sendKeyCode.patch

Part 4: Add Actions.sendKeyCode() method for IME tests. In my next patch, I use sendKeyCode() to send KEYEVENT_DEL to delete some characters.

Comment 24

[Chris Peterson [:cpeterson]]

Attachment: part-5-testInputForm.patch

Part 5: Add testInputForm test for form input.

Comment 25

[Chris Peterson [:cpeterson]]

Here is a try build of the EventExpecter patch:
https://tbpl.mozilla.org/?tree=Try&rev=ac09c6bb2109

Comment 26

[Chris Peterson [:cpeterson]]

Here is a try build of the testInputForm test:
https://tbpl.mozilla.org/?tree=Try&rev=6f0e2ed0248a

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/343da9f79907

Comment 28

[Geoff Brown [:gbrown]]

Comment on attachment 733673 [details] [diff] [review]
add-EventExpecter-reuse-checks.patch

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

::: build/mobile/robocop/FennecNativeActions.java.in
@@ +109,5 @@
>          private String mEventData;
>          private static final int MAX_WAIT_MS = 90000;
>  
>          GeckoEventExpecter(String geckoEvent, Object[] registrationParams) {
> +            if (geckoEvent == null || geckoEvent.isEmpty()) {

try run says it all!

Probably TextUtils.isEmpty() was intended?

Comment 29

[Geoff Brown [:gbrown]]

Comment on attachment 733675 [details] [diff] [review]
part-5-testInputForm.patch

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

::: mobile/android/base/tests/testInputForm.java.in
@@ +7,5 @@
> +import android.os.SystemClock;
> +import android.util.Log;
> +import android.view.KeyEvent;
> +import android.view.View;
> +import android.widget.Button;

nit - remove unused imports

@@ +24,5 @@
> +        blockForGeckoReady();
> +
> +        expectMessage();
> +        loadUrl(url);
> +        //verifyUrl(url);

nit - clean up unused code

@@ +61,5 @@
> +            mActions.sendSpecialKey(Actions.SpecialKey.RIGHT);
> +            mSolo.sleep(1);
> +            mActions.sendKeyCode(KeyEvent.KEYCODE_DEL);
> +            mSolo.sleep(1);
> +        } while (SystemClock.uptimeMillis() < deadline);

There aren't any assertions in this section. How do we know that it worked?

Comment 30

[Chris Peterson [:cpeterson]]

(In reply to Geoff Brown [:gbrown] from comment #28)
> try run says it all!
> 
> Probably TextUtils.isEmpty() was intended?

oops. I didn't realize String.isEmpty() required API Level 9. It worked on my JB device! <:)

Comment 31

[Chris Peterson [:cpeterson]]

(In reply to Geoff Brown [:gbrown] from comment #29)
> > +            mActions.sendSpecialKey(Actions.SpecialKey.RIGHT);
> > +            mSolo.sleep(1);
> > +            mActions.sendKeyCode(KeyEvent.KEYCODE_DEL);
> > +            mSolo.sleep(1);
> > +        } while (SystemClock.uptimeMillis() < deadline);
> 
> There aren't any assertions in this section. How do we know that it worked?

I didn't add any assertions because it was just a stress test. I wanted to send many key events to see if we hit any IME deadlocks. But if all the characters and DEL events worked, then the result should always be the same. I'll add an assert for the end condition.

Comment 32

[Chris Peterson [:cpeterson]]

Landed Part 4: Add Actions.sendKeyCode() method for IME tests:
https://hg.mozilla.org/integration/mozilla-inbound/rev/722d438e1072

Comment 33

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/722d438e1072

Comment 34

[(inactive) Jim Chen [:jchen] [:darchons]]

Hey Chris, what's the status of your IME tests? I'm going to spend some time adding more IME tests and I want to see what you've done so far. Thanks!

Comment 35

[Chris Peterson [:cpeterson]]

Jim: Please feel free to change whatever you need. I landed patches 1-4, but not part-5-testInputForm.patch because I didn't finish debugging some intermittent Try-only failures with that patch. The patch has likely bitrotted after subsequent Robocop changes.

Comment 36

[BMO Automation]

We have completed our launch of our new Firefox on Android. The development of the new versions use GitHub for issue tracking. If the bug report still reproduces in a current version of [Firefox on Android nightly](https://play.google.com/store/apps/details?id=org.mozilla.fenix) an issue can be reported at the [Fenix GitHub project](https://github.com/mozilla-mobile/fenix/). If you want to discuss your report please use [Mozilla's chat](https://wiki.mozilla.org/Matrix#Connect_to_Matrix) server https://chat.mozilla.org and join the [#fenix](https://chat.mozilla.org/#/room/#fenix:mozilla.org) channel.