968409 - Add basic InputConnection tests

Status: RESOLVED FIXED

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

Comment 1

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

Attachment: Add assertNotEquals method (v1)

assertNotEquals is useful for some cases, and we have negative versions of other asserts, like assertNotSame and assertNotNull.

Comment 2

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

Attachment: Add ReflectionHelper for UITests (v1)

We need some reflection to isolate ourselves from the system IME, so the system IME doesn't interfere with our test. These helpers may not be necessary because I don't know if our other testing libraries already provide reflection helpers.

Comment 3

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

Attachment: Add GeckoViewComponent for UITests (v1)

The GeckoViewComponent represents the GeckoView and has a TextInput inner class to represent text input operations within the GeckoView.

Comment 4

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

Attachment: Add TextInputHelper for UITests (v1)

Helpers for testing the InputConnection interface

Comment 5

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

Attachment: Add testInputConnection with basic tests (v1)

Basic tests to test our InputConnection interface by calling its methods and looking at their effects.

Comment 6

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8372612 [details] [diff] [review]
Add assertNotEquals method (v1)

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

This looks fine if we add `assertNotEquals`, however, I believe the reason that `assertNotEquals` DNE in the JUnit API [1] is to encourage better testing behavior.

For example, when you're getting data from a DB and want to make sure it's not empty, you can say `assertNotEquals(0, cursor.getCount())`, however, for a little extra work you can verify what's actually in the DB which, imo, is a better test, i.e. `assertNotEquals(rowCount, cursor.getCount())`.

As such, I'd prefer not to put this in the API if we don't have to to avoid encouraging such behavior. For your use case, I think `assertTrue(selectionStart != selectionEnd, message)` is sufficient (and you can even log the values before it, if you want to, to replicate the assertNotEquals functionality). I think your extra effort here is for the good of the API.

Please let me know if you disagree.

[1]: http://junit.sourceforge.net/javadoc/org/junit/Assert.html

::: mobile/android/base/tests/helpers/AssertionHelper.java
@@ +61,5 @@
>      public static void assertEquals(final String message, final Object expected, final Object actual) {
>          sAsserter.is(actual, expected, message);
>      }
>  
> +    public static void assertNotEquals(final String message, final double notexpected, final double actual, final double delta) {

nit: notexpected -> unexpected, to stay consistent with assertNotSame.

Comment 7

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8372613 [details] [diff] [review]
Add ReflectionHelper for UITests (v1)

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

We're trying to remove reflection from our tests (bug 916507 and its blocking bugs) because it prevents ProGuard from optimizing the code entirely, and necessitates the hairy maintenance of various annotations. Do you need this?

Typically, you can use dependency-injection style coding and pass in a mock of the class. Given our code architecture (and the lack of forethought to these ideas), however, this may be difficult. Take a look at it, and tell me if you think it's possible, or see any alternatives.

Comment 8

[Chris Peterson [:cpeterson]]

Comment on attachment 8372617 [details] [diff] [review]
Add testInputConnection with basic tests (v1)

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

LGTM!

::: mobile/android/base/tests/testInputConnection.java
@@ +34,5 @@
> +            // Test setSelection
> +            ic.setSelection(0, 3);
> +            TextInputHelper.assertSelection("Cannot set selection", ic, 0, 3);
> +            ic.setSelection(3, 3);
> +            TextInputHelper.assertSelection("Cannot collapse selection", ic, 3);

Maybe add tests for invalid input like:
• setSelection(2,1)   // equivalent to setSelection(1, 2)?
• setSelection(-1,2)  // equivalent to setSelection(0, 2)?
• setSelection(8,9)   // equivalent to setSelection(3, 3)?
• setSelection(1,9)   // equivalent to setSelection(1, 3)?
• setSelection(-1,9)  // equivalent to setSelection(0, 3)?

@@ +40,5 @@
> +            // Test commitText
> +            ic.commitText("bar", 1); // Selection at end of new text
> +            TextInputHelper.assertTextAndSelection("Cannot commit text (select after)",
> +                ic, "foobar", 6);
> +            ic.commitText("foo", -1); // Selection at start of new text

Maybe add tests for commitText("", 0) or commitText("whatever", -99)?

@@ +51,5 @@
> +                ic, "foobrfoo", 4);
> +            ic.deleteSurroundingText(1, 1);
> +            TextInputHelper.assertTextAndSelection("Cannot delete text before/after",
> +                ic, "foofoo", 3);
> +            ic.deleteSurroundingText(0, 10);

Maybe add tests for invalid parameters like deleteSurroundingText(0, 0), deleteSurroundingText(-1, -1), and deleteSurroundingText(99, 99)?

@@ +59,5 @@
> +            // Test setComposingText
> +            ic.setComposingText("foo", 1);
> +            TextInputHelper.assertTextAndSelection("Cannot start composition",
> +                ic, "foofoo", 6);
> +            ic.setComposingText("bar", 1);

Maybe add tests for setComposingText("", 1) or setComposingText("whatever", 99)?

@@ +64,5 @@
> +            TextInputHelper.assertTextAndSelection("Cannot update composition",
> +                ic, "foobar", 6);
> +
> +            // Test finishComposingText
> +            ic.finishComposingText();

What happens if you call deleteSurroundingText() on an active composition string before calling finishComposingText()?
What happens if you call finishComposingText() without an active composition string?
What happens if you call commitText() when you have an active composition string?

Comment 9

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

Attachment: Add assertNotEquals method (v2)

So looking at [1], I didn't get the impression that assertNotEquals was intentionally excluded, and one answer mentioned assertNotEquals has been added in the latest versions of JUnit [2].

I agree that not-equals has fewer proper use cases than equals, but I think we should have it for completeness. I don't really think leaving it out will encourage people to write better tests; they will just write "assertTrue(0 != cursor.getCount())" instead.

[1] http://stackoverflow.com/questions/1096650/why-doesnt-junit-provide-assertnotequals-methods
[2] http://junit.org/javadoc/latest/org/junit/Assert.html

(In reply to Michael Comella (:mcomella) from comment #6)
> Comment on attachment 8372612 [details] [diff] [review]
> Add assertNotEquals method (v1)
> 
> Review of attachment 8372612 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> nit: notexpected -> unexpected, to stay consistent with assertNotSame.

Fixed

Comment 10

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8373475 [details] [diff] [review]
Add assertNotEquals method (v2)

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

(In reply to Jim Chen [:jchen :nchen] from comment #9)
> I agree that not-equals has fewer proper use cases than equals, but I think
> we should have it for completeness. I don't really think leaving it out will
> encourage people to write better tests; they will just write "assertTrue(0
> != cursor.getCount())" instead.

I'd argue that this route leads to C++. ;) If someone wrote anyway `assertTrue(0 != cursor.getCount())`, they were at least encouraged to stop and think about why they couldn't write `assertNotEquals` (which *might* lead to better tests).

In any case, this is fairly trivial to catch in reviews, so r+.

Comment 11

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

Comment on attachment 8372613 [details] [diff] [review]
Add ReflectionHelper for UITests (v1)

(In reply to Michael Comella (:mcomella) from comment #7)
> Comment on attachment 8372613 [details] [diff] [review]
> Add ReflectionHelper for UITests (v1)
> 
> Review of attachment 8372613 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> We're trying to remove reflection from our tests (bug 916507 and its
> blocking bugs) because it prevents ProGuard from optimizing the code
> entirely, and necessitates the hairy maintenance of various annotations. Do
> you need this?
> 
> Typically, you can use dependency-injection style coding and pass in a mock
> of the class. Given our code architecture (and the lack of forethought to
> these ideas), however, this may be difficult. Take a look at it, and tell me
> if you think it's possible, or see any alternatives.

The IME test uses reflection to inject a mock Context class into the Android View class inherited by GeckoView. This is necessary to isolate our test environment from the actual IME running on the test system, which will interfere with our test. I tried looking at other ways but using reflection seems to be the best way, and because we're changing an Android class, we don't need to worry about ProGuard performance or annotations. It is true that ReflectionHelper should be used very sparingly.

Comment 12

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8372613 [details] [diff] [review]
Add ReflectionHelper for UITests (v1)

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

Pretty close - mostly just comment complaints. Note that I haven't looked into any alternatives to using reflection just yet, so I might end up rejecting this if we come up with something better.

Push this, and patches that use it, to try so we know we're not accidentally reflecting on something that ProGuard will break.

nit: You can use `final` a bit more on your local vars, but I'm too pedantic so I won't r- you for it. :P

::: mobile/android/base/tests/helpers/ReflectionHelper.java
@@ +8,5 @@
> +
> +import java.lang.reflect.Field;
> +
> +/**
> + * Provides helper functions for accessing the underlying Gecko engine.

Add a comment here saying that this class should NEVER be used to reflect onto any code we write because of ProGuard.

@@ +10,5 @@
> +
> +/**
> + * Provides helper functions for accessing the underlying Gecko engine.
> + */
> +public final class ReflectionHelper {

Perhaps we should call this class something to mislead and discourage its use (e.g. AndroidFrameworkReflectionHelper). What do you think?

@@ +23,5 @@
> +            } catch (NoSuchFieldException e) {
> +                cls = cls.getSuperclass();
> +            }
> +        } while (cls != null);
> +        return cls.getField(field); // throw NoSuchFieldException

I believe the return would actually be a NullPointerException. In any case, I think this code block might be clearer as:

while (true) {
  try {
    return cls.getDeclaredField(field);
  } catch (NoSuchFieldException e) {
    cls = cls.getSuperClass();
    if (cls == null) {
      throw e;
    }
  }
}

@@ +35,5 @@
> +            Object ret = fld.get(obj);
> +            fld.setAccessible(accessible);
> +            return ret;
> +        } catch (NoSuchFieldException e) {
> +            AssertionHelper.fail(e.toString());

Import AssertionHelper.* statically, and drop the `AssertionHelper.` here and below.

Add a comment explaining that we fail here, rather than throwing the exception to the caller, because anything that you're reflecting on should not be in code we write and thus should be guaranteed to exist. Perhaps add a tad of this to the fail message.

@@ +37,5 @@
> +            return ret;
> +        } catch (NoSuchFieldException e) {
> +            AssertionHelper.fail(e.toString());
> +        } catch (IllegalAccessException e) {
> +            AssertionHelper.fail(e.toString());

Add a comment saying this should never happen (assuming it shouldn't though what happens if the class we're reflecting on is declared private?).

@@ +39,5 @@
> +            AssertionHelper.fail(e.toString());
> +        } catch (IllegalAccessException e) {
> +            AssertionHelper.fail(e.toString());
> +        }
> +        return null;

This is javac being a jerk, right? If so, add a comment saying so here and below.

Comment 13

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8372613 [details] [diff] [review]
Add ReflectionHelper for UITests (v1)

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

::: mobile/android/base/tests/helpers/ReflectionHelper.java
@@ +8,5 @@
> +
> +import java.lang.reflect.Field;
> +
> +/**
> + * Provides helper functions for accessing the underlying Gecko engine.

Update this JavaDoc - it reminds me too much of GeckoHelper. ;)

Comment 14

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8372616 [details] [diff] [review]
Add TextInputHelper for UITests (v1)

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

Close: mostly nitty stuff.

::: mobile/android/base/tests/helpers/TextInputHelper.java
@@ +10,5 @@
> +import android.view.inputmethod.ExtractedTextRequest;
> +import android.view.inputmethod.InputConnection;
> +
> +/**
> + * Provides helper functions for accessing the underlying Gecko engine.

Update this javadoc.

@@ +16,5 @@
> +public final class TextInputHelper {
> +
> +    private TextInputHelper() { /* To disallow instantiation. */ }
> +
> +    public static ExtractedText getExtractedText(final InputConnection ic) {

nit: private? Is this needed elsewhere?

@@ +29,5 @@
> +
> +    public static void assertText(final String message,
> +                                  final InputConnection ic,
> +                                  final String text) {
> +        AssertionHelper.assertEquals(message, text, getText(ic));

Import statically and just use `assertEquals(...` directly.

@@ +36,5 @@
> +    public static int getSelectionStart(final InputConnection ic) {
> +        return getExtractedText(ic).selectionStart;
> +    }
> +
> +    public static int getSelectionEnd(final InputConnection ic) {

Is this (and `getSelectionStart`) used?

If not, you should probably either write a test using it (painfully simple is fine), just to ensure it works, or remove it. If it doesn't work (for whatever possible miniscule edge case), it would be frustrated as a test developer attempting to use the method and your test breaks because the framework is broken.

Similarly, there seem to be a few more methods not used in your test in the other patch - the same applies to them.

@@ +49,5 @@
> +        AssertionHelper.assertEquals(message, start, extract.selectionStart);
> +        AssertionHelper.assertEquals(message, end, extract.selectionEnd);
> +    }
> +
> +    public static void assertSelection(final String message,

Would this better known as `assertNoSelection`, `assertSinglePointSelection`, or similar?

@@ +55,5 @@
> +                                       final int value) {
> +        assertSelection(message, ic, value, value);
> +    }
> +
> +    public static void assertNonEmptySelection(final String message,

This seems like an example for a less than stellar assertion. Why would we want to assertNonEmpty, when we can put deliberate data into a selection, and assert that instead?

@@ +78,5 @@
> +        AssertionHelper.assertEquals(message, start, extract.selectionStart);
> +        AssertionHelper.assertEquals(message, end, extract.selectionEnd);
> +    }
> +
> +    public static void assertTextAndSelection(final String message,

Same as `assertSelection`.

Comment 15

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8372614 [details] [diff] [review]
Add GeckoViewComponent for UITests (v1)

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

I think you should extend UITest (e.g. `TextInputTest`). Any reason not to?

::: mobile/android/base/tests/UITest.java
@@ +58,5 @@
>      private String mBaseIpUrl;
>  
>      protected AboutHomeComponent mAboutHome;
>      protected ToolbarComponent mToolbar;
> +    protected GeckoViewComponent mGeckoView;

nit: Alphabetize.

@@ +121,5 @@
>  
>      private void initComponents() {
>          mAboutHome = new AboutHomeComponent(this);
>          mToolbar = new ToolbarComponent(this);
> +        mGeckoView = new GeckoViewComponent(this);

nit: Alphabetize.

@@ +168,5 @@
>              case TOOLBAR:
>                  return mToolbar;
>  
> +            case GECKOVIEW:
> +                return mGeckoView;

nit: Alphabetize.

::: mobile/android/base/tests/UITestContext.java
@@ +21,5 @@
>  
>      public static enum ComponentType {
>          ABOUTHOME,
> +        TOOLBAR,
> +        GECKOVIEW,

nit: Alphabetize.

::: mobile/android/base/tests/components/GeckoViewComponent.java
@@ +25,5 @@
> + * A class representing any interactions that take place on GeckoView.
> + */
> +public class GeckoViewComponent extends BaseComponent {
> +
> +    public TextInput mTextInput;

Add an accessor and make private.

@@ +37,5 @@
> +     * Returns the GeckoView.
> +     */
> +    private View getView() {
> +        View v = mSolo.getView(R.id.layer_view);
> +        AssertionHelper.assertNotNull("No GeckoView", v);

Drop the `AssertionHelper` (you've already imported so no need to import; drop it below).

@@ +64,5 @@
> +            return getInputMethodManager().isActive(getView());
> +        }
> +
> +        public TextInput assertActive() {
> +            AssertionHelper.assertTrue("No active view", isActive());

"View is active".

@@ +69,5 @@
> +            return this;
> +        }
> +
> +        public TextInput waitForActive() {
> +            WaitHelper.waitFor("Active view", new Condition() {

nit: "view to be active" (waitFor messages are prepended by "Waiting for ").

@@ +90,5 @@
> +            return imm.isActive(getView()) && imm.isAcceptingText();
> +        }
> +
> +        public TextInput assertInputConnection() {
> +            AssertionHelper.assertTrue("No InputConnection", hasInputConnection());

"InputConnection is available".

I've been writing these messages as what is expected, rather than the actual state of things when the assertion fails.

@@ +95,5 @@
> +            return this;
> +        }
> +
> +        public TextInput waitForInputConnection() {
> +            WaitHelper.waitFor("Active InputConnection", new Condition() {

"InputConnection to be active"

@@ +107,5 @@
> +
> +        public TextInput testInputConnection(final InputConnectionTest test)
> +                throws InterruptedException {
> +
> +            /* Starts an InputConnectionTest. An InputConnectionTest must run on the

Why not javadoc?

@@ +114,5 @@
> +             * be temporarily disabled to prevent the system IME from interfering with our
> +             * tests. We disable the service by override the GeckoView's context with one
> +             * that returns a null InputMethodManager service. */
> +
> +            AssertionHelper.assertNotNull("No test", test);

"Test should not be null"

@@ +128,5 @@
> +
> +            final Context oldContext = (Context)ReflectionHelper.getField(v, "mContext");
> +
> +            // Switch to a no-InputMethodManager context to avoid interference
> +            mTestContext.getInstrumentation().runOnMainSync(new Runnable() {

`setUp()`?

@@ +167,5 @@
> +                test.wait();
> +            }
> +
> +            // Restore the old context
> +            mTestContext.getInstrumentation().runOnMainSync(new Runnable() {

`tearDown`?

Comment 16

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8372617 [details] [diff] [review]
Add testInputConnection with basic tests (v1)

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

::: mobile/android/base/tests/robocop.ini
@@ +98,5 @@
>  # Using UITest
>  [testAboutHomePageNavigation]
>  [testAboutHomeVisibility]
>  [testSessionHistory]
> +[testInputConnection]

nit: Alphabetize.

::: mobile/android/base/tests/robocop_input.html
@@ +8,5 @@
> +    <input id="input">
> +    <script>
> +      function fillInput() {
> +        var input = document.getElementById("input");
> +        input.value = window.location.hash.slice(1); // remove leading #

Mention that this assumes we load a page with an anchor.

::: mobile/android/base/tests/testInputConnection.java
@@ +17,5 @@
> +        GeckoHelper.blockForReady();
> +
> +        NavigationHelper.enterAndLoadUrl(StringHelper.ROBOCOP_INPUT_URL + "#foo");
> +        mToolbar.assertTitle(StringHelper.ROBOCOP_INPUT_TITLE);
> +        mAboutHome.assertNotVisible();

nit: This assertion should not be necessary as the implicit state is checked by enterAndLoadUrl. However, if you want it for peace of mind, I suppose that's fine: just add a comment.

@@ +28,5 @@
> +    private class BasicInputConnectionTest implements InputConnectionTest {
> +        @Override
> +        public void test(InputConnection ic, EditorInfo info) {
> +            // Test initial text
> +            TextInputHelper.assertText("Wrong initial text", ic, "foo");

"Initial text is correct". Maybe assertText should take in a `String when` and it will take on " text is correct" e.g. `assertText("Initial", ic, "foo");`. Similarly with other log messages.

Add a comment about how this initial text got there.

"foo" should be a constant.

Comment 17

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8372617 [details] [diff] [review]
Add testInputConnection with basic tests (v1)

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

Knowing how little I do about the IME code, lgtm.

Comment 18

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

Attachment: Add FrameworkHelper for UITests (v2)

I ended up renaming this to FrameworkHelper (either that or SystemHelper), and now it only exposes specific functionality (e.g. getting/setting a View's Context), instead of exposing generic reflection methods. Also addressed most of the comments.

(In reply to Michael Comella (:mcomella) from comment #12)
> Comment on attachment 8372613 [details] [diff] [review]
> Add ReflectionHelper for UITests (v1)
> 
> @@ +23,5 @@
> > +            } catch (NoSuchFieldException e) {
> > +                cls = cls.getSuperclass();
> > +            }
> > +        } while (cls != null);
> > +        return cls.getField(field); // throw NoSuchFieldException
> 
> I believe the return would actually be a NullPointerException. In any case,
> I think this code block might be clearer as:

Fixed NPE. We want to call getField in the end as a last resort.

> @@ +39,5 @@
> > +            AssertionHelper.fail(e.toString());
> > +        } catch (IllegalAccessException e) {
> > +            AssertionHelper.fail(e.toString());
> > +        }
> > +        return null;
> 
> This is javac being a jerk, right? If so, add a comment saying so here and
> below.

Changed to throwing an unchecked exception.

Comment 19

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

Attachment: Add TextInputHelper for UITests (v2)

Addressed review comments, and removed unused methods.

(In reply to Michael Comella (:mcomella) from comment #14)
> Comment on attachment 8372616 [details] [diff] [review]
> Add TextInputHelper for UITests (v1)
> 
> @@ +49,5 @@
> > +        AssertionHelper.assertEquals(message, start, extract.selectionStart);
> > +        AssertionHelper.assertEquals(message, end, extract.selectionEnd);
> > +    }
> > +
> > +    public static void assertSelection(final String message,
> 
> Would this better known as `assertNoSelection`,
> `assertSinglePointSelection`, or similar?

Changed to assertSelectionAt

Comment 20

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

Attachment: Add GeckoViewComponent for UITests (v2)

Addressed most comments.

(In reply to Michael Comella (:mcomella) from comment #15)
> Comment on attachment 8372614 [details] [diff] [review]
> Add GeckoViewComponent for UITests (v1)
> 
> I think you should extend UITest (e.g. `TextInputTest`). Any reason not to?

First, the InputConnection test doesn't really fall into JUnit's setUp/test/tearDown model because the test requires running on a specific thread. Therefore, there's little advantage in having a TextInputTest that extends UITest versus having a component.

Second, similar to why AboutHomeComponent is a component and not an AboutHomeTest, I want a GeckoViewComponent that it can be expanded and reused in the future for other IME/non-IME tests. For example, it will have methods to test key presses in content, etc. 

> ::: mobile/android/base/tests/components/GeckoViewComponent.java
> @@ +25,5 @@
> > + * A class representing any interactions that take place on GeckoView.
> > + */
> > +public class GeckoViewComponent extends BaseComponent {
> > +
> > +    public TextInput mTextInput;
> 
> Add an accessor and make private.

I like having a public field here. That way, in your test, you write "mGeckoView.mTextInput.foo", which makes it clear that you're using the GeckoView component and TextInput subcomponent. With an accessor, it looks like "mGeckoView.getTextInput().foo", which IMO is not as clear about the relationship between TextInput and GeckoView.

> @@ +90,5 @@
> > +            return imm.isActive(getView()) && imm.isAcceptingText();
> > +        }
> > +
> > +        public TextInput assertInputConnection() {
> > +            AssertionHelper.assertTrue("No InputConnection", hasInputConnection());
> 
> "InputConnection is available".
> 
> I've been writing these messages as what is expected, rather than the actual
> state of things when the assertion fails.

Mochitest assertion messages are supposed to be error messages [1], because people only read these messages when the assertions fail, and it's simpler to just tell them what the errors are. To avoid confusion and to keep consistency, I think we should treat assertion messages as error messages here as well.

[1] https://developer.mozilla.org/en-US/docs/Mochitest#Test_functions

> @@ +128,5 @@
> > +
> > +            final Context oldContext = (Context)ReflectionHelper.getField(v, "mContext");
> > +
> > +            // Switch to a no-InputMethodManager context to avoid interference
> > +            mTestContext.getInstrumentation().runOnMainSync(new Runnable() {
> 
> `setUp()`?
> 
> @@ +167,5 @@
> > +                test.wait();
> > +            }
> > +
> > +            // Restore the old context
> > +            mTestContext.getInstrumentation().runOnMainSync(new Runnable() {
> 
> `tearDown`?

I want to keep these inside GeckoViewComponent. Using setUp/tearDown would require moving this code to the test itself.

Comment 21

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

Attachment: Run InputConnection tests on IC thread (v1)

InputConnection tests should run on the InputConnection thread, so if we're being called by a test, we need to return the IC thread handler.

Comment 22

[Chris Peterson [:cpeterson]]

Comment on attachment 8378614 [details] [diff] [review]
Run InputConnection tests on IC thread (v1)

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

LGTM

::: mobile/android/base/GeckoInputConnection.java
@@ +544,5 @@
>                  return true;
>              }
> +            if ("testInputConnection".equals(frame.getMethodName()) &&
> +                ("org.mozilla.gecko.tests.components." +
> +                    "GeckoViewComponent$TextInput").equals(frame.getClassName())) {

Instead of breaking a long string over two lines because of our arbitrary line length limit, I would recommend adding a `private static final String` constant with a shorter name.

Comment 23

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8378355 [details] [diff] [review]
Add FrameworkHelper for UITests (v2)

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

I like. Very much I like.

::: mobile/android/base/tests/helpers/FrameworkHelper.java
@@ +35,5 @@
> +                cls = cls.getSuperclass();
> +            }
> +        } while (cls != null);
> +        /* We tried getDeclaredField before; now try getField
> +           instead and let the exception propagate if that fails. */

nit: Multiple lines with

// Line 1
// Line 2

Here and below.

@@ +36,5 @@
> +            }
> +        } while (cls != null);
> +        /* We tried getDeclaredField before; now try getField
> +           instead and let the exception propagate if that fails. */
> +        return clazz.getField(field);

Silly me, I missed the `DeclaredField` distinction before. But why not just call getField?

@@ +39,5 @@
> +           instead and let the exception propagate if that fails. */
> +        return clazz.getField(field);
> +    }
> +
> +    private static Object getField(final Object obj, final String field) {

nit: fieldName

@@ +41,5 @@
> +    }
> +
> +    private static Object getField(final Object obj, final String field) {
> +        try {
> +            final Field fld = getClassField(obj.getClass(), field);

nit: With field then unused, -> field

@@ +53,5 @@
> +               the caller is doing something wrong and should be fixed */
> +            fail(e.toString());
> +        } catch (final IllegalAccessException e) {
> +            /* We expect an accessible field; if it's not accessible,
> +               the caller is doing something wrong and should be fixed */

Isn't an IllegalAccessException the framework's fault for not using setAccesible correctly? Maybe we should throw an `IllegalStateException` and say it's our fault.

@@ +56,5 @@
> +            /* We expect an accessible field; if it's not accessible,
> +               the caller is doing something wrong and should be fixed */
> +            fail(e.toString());
> +        }
> +        throw new IllegalArgumentException("Invalid field " + field);

This should never be reached, right? This should probably be `IllegalStateException("Should never happen!")` or similar. I like the use of this over `return null;` though!

@@ +84,5 @@
> +    public static Context getViewContext(final View v) {
> +        return (Context) getField(v, "mContext");
> +    }
> +
> +    public static Context setViewContext(final View v, final Context c) {

`swapViewContext`? There's a better naming convention for returning the previous value, right?

Comment 24

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8378508 [details] [diff] [review]
Add GeckoViewComponent for UITests (v2)

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

(In reply to Jim Chen [:jchen :nchen] from comment #20)
> Mochitest assertion messages are supposed to be error messages [1], because
> people only read these messages when the assertions fail, and it's simpler
> to just tell them what the errors are.

From a different perspective, when I read through the logs, it is true I really only read through them when one fails. However, when I'm trying to debug what went wrong, I see many more assertions that pass than those that fail and I find it much easier to invert the logic of a single failing statement than invert the logic of the numerous others that passed. The page you linked says these messages are: "Description of the check", which passes under both interpretations.

> To avoid confusion and to keep consistency, I think we should treat
> assertion messages as error messages here as well.

UITest is written as success messages so it would be inconsistent to do otherwise. Also, jumping into a few arbitrary `extends BaseTest` ([1] [2] [3]), it seems to be the norm there as well. The notable exception is for thrown exceptions.

If you think it's a problem, file a followup.

r- on the `wait` code and assertion messages.

[1]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/tests/testNewTab.java#30
[2]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/tests/testInputUrlBar.java#124
[3]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/tests/testPrefsObserver.java#54

::: mobile/android/base/tests/components/GeckoViewComponent.java
@@ +25,5 @@
> + * A class representing any interactions that take place on GeckoView.
> + */
> +public class GeckoViewComponent extends BaseComponent {
> +
> +    public TextInput mTextInput;

I suppose it's fine to have the public field but then please make it final.

@@ +69,5 @@
> +            return this;
> +        }
> +
> +        public TextInput waitForActive() {
> +            WaitHelper.waitFor("active view", new Condition() {

nit: More descriptive: waitFor("the current text input to become active"

@@ +82,5 @@
> +        /**
> +         * Returns whether an InputConnection is avaiable.
> +         *  An InputConnection is available when text input is being directed to the
> +         *  GeckoView, and a text field (input, textarea, contentEditable, etc.) is
> +         *  currently focused inside the GeckoView.

nit: Indentation.

@@ +95,5 @@
> +            return this;
> +        }
> +
> +        public TextInput waitForInputConnection() {
> +            WaitHelper.waitFor("Active InputConnection", new Condition() {

nit: more descriptive.

@@ +121,5 @@
> +            assertNotNull("No test", test);
> +            assertInputConnection();
> +
> +            // GeckoInputConnection can run on another thread than the main thread,
> +            // so we need to be testing it on that same thread it's running on

But we assert it's not on the same thread below? I'm not sure I understand.

@@ +122,5 @@
> +            assertInputConnection();
> +
> +            // GeckoInputConnection can run on another thread than the main thread,
> +            // so we need to be testing it on that same thread it's running on
> +            final View v = getView();

nit: v -> geckoView or similar

@@ +123,5 @@
> +
> +            // GeckoInputConnection can run on another thread than the main thread,
> +            // so we need to be testing it on that same thread it's running on
> +            final View v = getView();
> +            final Handler h = v.getHandler();

nit: h -> uiThreadHandler

@@ +125,5 @@
> +            // so we need to be testing it on that same thread it's running on
> +            final View v = getView();
> +            final Handler h = v.getHandler();
> +
> +            assertNotSame("IC running on instrumentation thread?",

nit: "InputConnection..."

@@ +126,5 @@
> +            final View v = getView();
> +            final Handler h = v.getHandler();
> +
> +            assertNotSame("IC running on instrumentation thread?",
> +                Looper.myLooper(), h.getLooper());

You can store Looper.myLooper() in a temp var to make it more readable:

`final Looper currentThreadLooper = Looper.myLooper();`

Similarly:

`final Looper uiThreadLooper = h.getLooper();` 

nit: Can we throw this in a helper method (e.g. assertGeckoInputConnectionThread)? I think it'd help readability.

@@ +131,5 @@
> +
> +            final Context oldContext = FrameworkHelper.getViewContext(v);
> +
> +            // Switch to a no-InputMethodManager context to avoid interference
> +            mTestContext.getInstrumentation().runOnMainSync(new Runnable() {

nit: Helper method.

@@ +165,5 @@
> +            };
> +            synchronized (test) {
> +                h.post(runnableTest);
> +                // Wait for the test to finish
> +                test.wait();

via [1]:

A waiting thread can be sent interrupt() to cause it to prematurely stop waiting, so wait should be called in a loop to check that the condition that has been waited for has been met before continuing.

[1]: https://developer.android.com/reference/java/lang/Object.html#wait%28%29

@@ +169,5 @@
> +                test.wait();
> +            }
> +
> +            // Restore the old context
> +            mTestContext.getInstrumentation().runOnMainSync(new Runnable() {

nit: Helper method.

Comment 25

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8378614 [details] [diff] [review]
Run InputConnection tests on IC thread (v1)

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

Tangential: This loop looks like it can be expensive on large stacks - is there a way to top out the depth? Also, how often is getHandler() called?

::: mobile/android/base/GeckoInputConnection.java
@@ +542,5 @@
>                  "android.view.inputmethod.InputMethodManager".equals(frame.getClassName())) {
>                  // only return our own Handler to InputMethodManager
>                  return true;
>              }
> +            if ("testInputConnection".equals(frame.getMethodName()) &&

"testInputConnection" should be a constant for visibility, since we can't enforce this at compile time.

nit: newline above `if`.

@@ +544,5 @@
>                  return true;
>              }
> +            if ("testInputConnection".equals(frame.getMethodName()) &&
> +                ("org.mozilla.gecko.tests.components." +
> +                    "GeckoViewComponent$TextInput").equals(frame.getClassName())) {

I would prefer if you got the class name at compile time (e.g. `GeckoViewComponent.TextInput.getClassName()`).

Comment 26

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

Attachment: Add FrameworkHelper for UITests (v3)

Addressed the comments from comment 23

Comment 27

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

Attachment: Add GeckoViewComponent for UITests (v3)

Addressed the points from comment 24. In particular, fixed the assertion messages, added helper methods, and isolated "wait on InputConnection test" logic into its own inner class with proper condition checking during wait().

Comment 28

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

Attachment: Run InputConnection tests on IC thread (v1.1)

Made string literals into constants.

Comment 29

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

Attachment: Add testInputConnection with basic tests (v1.1)

Added some edge cases and fixed assertion messages.

Comment 30

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8383151 [details] [diff] [review]
Add FrameworkHelper for UITests (v3)

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

r+ w/ nits.

::: mobile/android/base/tests/helpers/FrameworkHelper.java
@@ +37,5 @@
> +        } while (cls != null);
> +        // We tried getDeclaredField before; now try getField instead.
> +        // getField behaves differently in that getField traverses the inheritance
> +        // list, but it only works on public fields. In any case, we want it to throw
> +        // NoSuchFieldException for us if it fails.

nit: Mention that this means it doesn't do anything new for us, but creates cleaner code.

@@ +57,5 @@
> +        } catch (final IllegalAccessException e) {
> +            // This should not happen. If it does, setAccessible above is not working.
> +            fail("Field should be accessible: " + e.toString());
> +        }
> +        throw new IllegalStateException("Cannot continue from previous failures");

nit: "Should not continue..."

Comment 31

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8383154 [details] [diff] [review]
Add GeckoViewComponent for UITests (v3)

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

r+ w/ nits.

::: mobile/android/base/tests/components/GeckoViewComponent.java
@@ +41,5 @@
> +     * Returns the GeckoView.
> +     */
> +    private View getView() {
> +        final View geckoView = mSolo.getView(R.id.layer_view);
> +        assertNotNull("Must have a GeckoView", geckoView);

nit: `mSolo.getView` actually makes this assertion. [1]

[1]: https://github.com/RobotiumTech/robotium/blob/master/robotium-solo/src/main/java/com/robotium/solo/Solo.java#L1972

@@ +152,5 @@
> +            setContext(oldGeckoViewContext);
> +            return this;
> +        }
> +
> +        private class InputConnectionTestRunner implements Runnable {

Nice encapsulation. :)

Comment 32

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

Addressed nits.

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/96c94ab2c0ba
https://hg.mozilla.org/integration/mozilla-inbound/rev/081f28069e48
https://hg.mozilla.org/integration/mozilla-inbound/rev/385d0c08dfe2
https://hg.mozilla.org/integration/mozilla-inbound/rev/38921bba8197
https://hg.mozilla.org/integration/mozilla-inbound/rev/ca6fe7cea127
https://hg.mozilla.org/integration/mozilla-inbound/rev/b35c78057352

Comment 33

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/96c94ab2c0ba
https://hg.mozilla.org/mozilla-central/rev/081f28069e48
https://hg.mozilla.org/mozilla-central/rev/385d0c08dfe2
https://hg.mozilla.org/mozilla-central/rev/38921bba8197
https://hg.mozilla.org/mozilla-central/rev/ca6fe7cea127
https://hg.mozilla.org/mozilla-central/rev/b35c78057352