Here's the plan to get things started: - New BaseTest subclass: UITest - Basic API to instantiate and access components - Hierarchy (e.g., /subsystem, /helpers) - Placeholders for testing components: Toolbar, AboutHome, AppMenu, and CommonUtils.

(Re-posting my message to the mailing list here for better context) If you ever worked on tests based on our robocop harness, you probably noticed how unpleasant it is to work on them. Our current UI testing code suffers from two major problems: - Inconsistency/verbosity: The tests and their underlying API are very ad-hoc. We're duplicating a lot of code, the code is very verbose, and the current structure with BaseTest subclasses doesn't scale well and almost inevitably leads to bloated classes with tons of unrelated APIs. - Unreliability: The tests themselves are unreliable in many ways. Each test pokes the UI in a different way to check the same things. I've been doing some explorations around a new API for UI testing in Fennec. Last week, some of us (from the front-end team) met to do some brainstorming and outline how we want the new API to look like. The goals for the new API are pretty obvious. We want UI tests to be: 1. Simple to write and extend 2. Consistently written 3. Reliable I believe the very first step towards these goals is to add structure to our existing UI testing code. I'd like to avoid big rewrites from day one. So our plan is to build the new API *on top* of the current robocop harness and *in parallel* to existing UI tests. This way can gradually build the new API and port existing UI tests without causing any big disruptions. The new structure won't necessarily help in terms of reliability, but the new API should indirectly help us in this front by providing a more consistent code base to work on. The general principles of the new API are: - It should be split into self-contained components covering different parts of the UI. - It should provide clear ways to extend the framework to cover new UI components and/or behaviours. - Each component should contain API to interact with and assert expected behaviour on specific parts of the UI. - Components should be modular enough that we can easily use multiple components in a single test. With that mind, here's the patch that bootstraps the new API. A few implementation details: - Note how I haven't changed any of the existing tests or harness utils classes. Remember, we're building the new API in parallel. We can probably replace or rewrite the harness code later if we need. But that's not necessary at this point. - Tests using the new API should inherit from UITest, which is completely independent from the BaseTest-based stuff. - All assertions are done using FEST-Assert and FEST-Android. They provide a fluent API for assertions and a well defined framework to create our custom assertions when necessary. - For now, I only bootstrapped enough of the TOOLBAR and ABOUTHOME components to give a more concrete idea of what the API should look like. - UI testing components have a fluent/chainable API and implement their own FEST assertions. - Reusable testing bits were consolidated into helper classes. For now, I've only added ViewHelper, GestureHelper, WaitHelper, and UITestCommon. We'll probably create more as we go. - I wrote two sample tests (testNewTabHistory and testAboutHomeVisibility) just to give a clearer sense of how the API would be used in tests. They don't need to be pushed to repo just yet. - Reflection code is now done using FEST-Reflect. It provides a fluent API for reflection. Much cleaner. Knows issues and open questions: - Using FEST means we won't be doing assertions using FennecMochitestAssert or FennecTalosAssert. I noticed that these classes do a whole bunch of logging (i.e. one log message per assertion). Not sure how important this is. I'd like to understand the logging requirements a bit better here to adapt the new API accordingly. - A whole bunch of things that I haven't considered yet based on your feedback :-) The patch in bug 910859 only implements the bare minimum API to get things started. We'll have to extend it as we try to port existing tests to the new structure. Once this patch lands in m-c, we can start working in different components and tests in parallel (see meta bug 910791). The patch is not yet ready for review but definitely ready for feedback. Drive-by reviews are very welcome.

I think this is a good time to drop the preprocessing on these tests, similar to what bug 856163 did.

Comment on attachment 798840 [details] [diff] [review] Bootstrap new UI testing API Review of attachment 798840 [details] [diff] [review]: ----------------------------------------------------------------- I'm nervous about bringing the FEST stuff in here. It seems unrelated to the goals of this project, and having more frameworks in place seems really likely to cause confusion and errors. The reflection bits sound nice, but again, I don't think they save you a whole lot, and the docs are going to be harder to find? Do we need two ways to do everything? i.e. the isTablet reflection looks like: final ClassLoader cl = activity.getClassLoader(); Class cls = classLoader.loadClass("org.mozilla.gecko.GeckoAppShell"); Method method = cls.getMethod("isTablet", null); boolean ret = ((Boolean) method.invoke(null)).booleanValue(); vs. final ClassLoader cl = activity.getClassLoader(); final Class appShellClass = type(APP_SHELL_CLASS).withClassLoader(cl).load(); boolean isTablet = method("isTablet").withReturnType(Boolean.class) .in(appShellClass) .invoke(); Making our current asserts fluent seems neat and pretty trivial if you wanted to. I don't think anyone is going to complain if you fix them, but again, its also seems unrelated to this? ::: mobile/android/base/tests/AboutHomeComponent.java.in @@ +45,5 @@ > + > + public AboutHomeComponent(UITestContext testContext) { > + super(testContext); > + > + mHomePagerView = (ViewPager) ViewHelper.findViewById(HOME_PAGER_ID); I like this id thing. Seems a lot less likely to be flaky than looking for text. ::: mobile/android/base/tests/ToolbarComponent.java.in @@ +143,5 @@ > + > + return this; > + } > + > + public ToolbarComponent pressBack() { Be careful with naming things like this. Its pretty similar to what's below, and seems kinda ambiguous.... ::: mobile/android/base/tests/UITestCommon.java.in @@ +95,5 @@ > + domContentLoaded.blockForEvent(); > + domContentLoaded.unregisterListener(); > + } > + > + public static void goBack() { Similarly, this is a vague method name that does two different things. I imagine this current implementation would fail in cases where hitting the toolbar back button doesn't do the same thing as hitting the hardware one.

(In reply to Wesley Johnston (:wesj) from comment #3) > Comment on attachment 798840 [details] [diff] [review] > Bootstrap new UI testing API > > Review of attachment 798840 [details] [diff] [review]: > ----------------------------------------------------------------- > > I'm nervous about bringing the FEST stuff in here. It seems unrelated to the > goals of this project, and having more frameworks in place seems really > likely to cause confusion and errors. I disagree. FEST is a core part of this proposal. The goal here is to streamline the way we write tests. FEST provides exactly that for assertions. It covers most of built-in types in Java and it's way more readable than our current (cryptic) assert bits. In theory, we could come up with a similar custom framework but that's a non-trivial amount of code that I don't think we should have to maintain. So, I actually think using FEST will make our UI testing code *less* confusing and error-prone. > The reflection bits sound nice, but again, I don't think they save you a > whole lot, and the docs are going to be harder to find? Do we need two ways > to do everything? > > i.e. the isTablet reflection looks like: > final ClassLoader cl = activity.getClassLoader(); > Class cls = classLoader.loadClass("org.mozilla.gecko.GeckoAppShell"); > Method method = cls.getMethod("isTablet", null); > boolean ret = ((Boolean) method.invoke(null)).booleanValue(); > > vs. > final ClassLoader cl = activity.getClassLoader(); > final Class appShellClass = type(APP_SHELL_CLASS).withClassLoader(cl).load(); > boolean isTablet = method("isTablet").withReturnType(Boolean.class) > .in(appShellClass) > .invoke(); This is about streamlining our API. Maybe this is not the most verbose case of reflection code (which is where FEST-Reflect can make a bigger difference?). I don't feel as strongly about it as I feel about FEST-Assert though. > Making our current asserts fluent seems neat and pretty trivial if you > wanted to. I don't think anyone is going to complain if you fix them, but > again, its also seems unrelated to this? The FEST stuff doesn't only apply to our components. It applies to anything (Android type, java types, etc). In summary: using FEST here means that we'll always assert stuff in the same way through a much more readable API. > ::: mobile/android/base/tests/AboutHomeComponent.java.in > @@ +45,5 @@ > > + > > + public AboutHomeComponent(UITestContext testContext) { > > + super(testContext); > > + > > + mHomePagerView = (ViewPager) ViewHelper.findViewById(HOME_PAGER_ID); > > I like this id thing. Seems a lot less likely to be flaky than looking for > text. > > ::: mobile/android/base/tests/ToolbarComponent.java.in > @@ +143,5 @@ > > + > > + return this; > > + } > > + > > + public ToolbarComponent pressBack() { > > Be careful with naming things like this. Its pretty similar to what's below, > and seems kinda ambiguous.... Yeah, I should probably rename this to pressBackButton() to be more explicit. > ::: mobile/android/base/tests/UITestCommon.java.in > @@ +95,5 @@ > > + domContentLoaded.blockForEvent(); > > + domContentLoaded.unregisterListener(); > > + } > > + > > + public static void goBack() { > > Similarly, this is a vague method name that does two different things. I > imagine this current implementation would fail in cases where hitting the > toolbar back button doesn't do the same thing as hitting the hardware one. The idea here is abstract the different ways to "go back" on phones and tablets. You don't want tests to handle that locally. Besides, the UI to go back on phones is actually the system back button. If pressing back doesn't actually go back is something that tests themselves have to properly handle case by case, no?

Comment on attachment 798840 [details] [diff] [review] Bootstrap new UI testing API Review of attachment 798840 [details] [diff] [review]: ----------------------------------------------------------------- Overall I think this is a HUGE improvement over what we have now. The test code is very straightforward and easy to read, which is the most important thing. ::: mobile/android/base/tests/AboutHomeComponent.java.in @@ +53,5 @@ > + return WaitHelper.wait(new WaitHelper.WaitTest() { > + @Override > + public boolean test() { > + ANDROID.assertThat(mHomePagerView).hasCurrentItem(expectedItem); > + return true; This technically doesn't fit the contract of a WaitTest, does it? In this case it should return true iff the home pager view has the expected item, but AFAIK we don't have a way to test that without asserting it. Which might be a more general problem (being able to get state without necessarily asserting it). @@ +114,5 @@ > + > + public AboutHomeComponentAssert isVisible() { > + isNotNull(); > + > + ANDROID.assertThat(mActual.mHomePagerView).overridingErrorMessage("Home pager should be visible") The overridingErrorMessage syntax seems a bit clunky although I guess we inherit that from FEST?

(In reply to Lucas Rocha (:lucasr) from comment #4) > In theory, we could come up with a similar custom framework but that's a non-trivial amount of code that > I don't think we should have to maintain. Yeah, I don't think I agree here. Very very very very few (none?) of the issues with our current tests are driven by the fact that our asserts are 'cryptic'. Hence, this seems orthogonal to the real goal here. i.e. you're taking something that would read: mAsserter.is(Toolbar.getTitle(), UITestCommon.BLANK01_TITLE, "Title is correct"); mAsserter.is(Toolbar.getURL(), UITestCommon.BLANK01_TITLE, "Url is correct"); and writing: assertThat(TOOLBAR).hasTitle(UITestCommon.BLANK01_TITLE) .hasUrl(UITestCommon.BLANK01_URL); and in return, we're giving up a lot of control over asserts, and introducing two competing frameworks for the same thing in our test harness. I don't hate the FEST bit, but it seems like it should 1.) Be a separate bug and 2.) Be done unilaterally to avoid the "why do we do this here and that there?" questions that led to a lot of this test fragmentation to begin with. i.e. people are going to copy/paste code. We should make it so doing so doesn't result in badness. > The idea here is abstract the different ways to "go back" on phones and > tablets. You don't want tests to handle that locally. Besides, the UI to go > back on phones is actually the system back button. If pressing back doesn't > actually go back is something that tests themselves have to properly handle > case by case, no? "go back" is the vague term here, right? It can mean two different things. I don't think our toolbar back button should always do the same thing as hitting the android back button (it doesn't in other apps that have a "back" button in the actionbar either). Abstracting them as if they are the same seems likely to fail?

Has anyone discussed these changes with QA and Softvision? I appreciate that the feedback list is quite long already, but many of the existing tests were written/re-written/fixed by QA/Softvision folks.

Comment on attachment 798840 [details] [diff] [review] Bootstrap new UI testing API Review of attachment 798840 [details] [diff] [review]: ----------------------------------------------------------------- I think a helpers package would be useful here to prevent unreasonable bloating. Specifically, the "components" and "helpers" packages seem useful. Overall, looks pretty good! I think there are a few formatting changes that need to be made to discourage accidental bloating though (i.e. miscellaneous helper class, test names - see the comments) Also, I might be nitting too much for f? so feel free to ignore those that you feel are irrelevant. ::: mobile/android/base/tests/AboutHomeComponent.java.in @@ +23,5 @@ > + private static final String HOME_PAGER_ID = "home_pager"; > + > + private static AboutHomeComponentAssert sAssert; > + > + public enum Page { You should probably comment that this needs to stay in sync with org.mozilla.gecko.home.HomePager.Page since we can't import it. @@ +132,5 @@ > + isNotNull(); > + isVisible(); > + > + ANDROID.assertThat(mActual.mHomePagerView).overridingErrorMessage("Current page is not <%s>", page) > + .hasCurrentItem(page.getIndex()); Can we use page.ordinal() here and remove the mIndex code? http://developer.android.com/reference/java/lang/Enum.html#ordinal%28%29 ::: mobile/android/base/tests/ToolbarComponent.java.in @@ +24,5 @@ > + private static final String URL_EDIT_TEXT_ID = "url_edit_text"; > + private static final String URL_TITLE_TEXT_ID = "url_bar_title"; > + private static final String GO_BUTTON_ID = "go"; > + private static final String BACK_BUTTON_ID = "back"; > + private static final String FORWARD_BUTTON_ID = "forward"; You should comment that these need to stay in sync with the main codebase. @@ +49,5 @@ > + > + // Back button > + private final ImageButton mBackButton; > + > + // Forward button These comments largely seem redundant with the variable names. ::: mobile/android/base/tests/UITestCommon.java.in @@ +15,5 @@ > + > +import static org.fest.reflect.core.Reflection.method; > +import static org.fest.reflect.core.Reflection.type; > + > +final class UITestCommon { I don't think the distinction between the methods included here and in UITest is clear. This class should probably be called *Helper. Perhaps it can be split up with a NavigationHelper (goBack, goPrevious, etc.) and a few other separate helpers (blockForGeckoReady())? Having a miscellaneous helper is dangerous because it encourages dumping functions into one massive file rather than creating a new helper file if you find that your function does not fit anywhere else (since there's no miscellaneous file to put it in). ::: mobile/android/base/tests/UITestDevice.java.in @@ +14,5 @@ > + > +import static org.fest.reflect.core.Reflection.method; > +import static org.fest.reflect.core.Reflection.type; > + > +class UITestDevice { This seems like it should be named UITestDeviceHelper. ::: mobile/android/base/tests/ViewHelper.java.in @@ +24,5 @@ > + sTestContext = testContext; > + } > + > + public static View findViewById(String id) { > + assertThat(sTestContext).isNotNull(); This call (and similar ones in other base classes should be encapsulated in a "assertCorrectlyInitialized()" function. Though it's just one call now, if the constructor is ever expanded, it'll become a copy-and-paste nightmare. Additionally, it makes a better call stack if it fails: assertThat(sTestContext).isNotNull(); at ... assertCorrectlyInitialized(); at ... @@ +27,5 @@ > + public static View findViewById(String id) { > + assertThat(sTestContext).isNotNull(); > + > + final Activity activity = sTestContext.getActivity(); > + final Driver driver = sTestContext.getDriver(); Can these calls be extracted to the init() function and stored in instance vars? It's redundant to write them for every helper function. ::: mobile/android/base/tests/WaitHelper.java.in @@ +10,5 @@ > +import com.jayway.android.robotium.solo.Solo; > + > +import android.os.SystemClock; > + > +final class WaitHelper { What's the reason for making this final? ::: mobile/android/base/tests/testAboutHomeVisibility.java.in @@ +6,5 @@ > +import @ANDROID_PACKAGE_NAME@.tests.AboutHomeComponent.Page; > + > +/* Test correct state for URL bar after loading pages. > + */ > +public class testAboutHomeVisibility extends UITest { In my opinion, the testing class should not have the same name as the tests inside - it should be an overarching category ("TestAboutHome" contains "testAboutHomeVisibility" for example). @@ +7,5 @@ > + > +/* Test correct state for URL bar after loading pages. > + */ > +public class testAboutHomeVisibility extends UITest { > + public void testAboutHomeVisibility() { This test name is inaccurate because more than visibility is being tested. In my opinion, the tests should be have a smaller scope than types of tests we wrote with the previous framework. If a test is called testAboutHomeVisibility, it should ONLY make assertions about what is or is not visible in about:home. In this case, enterAndLoadUrl should assert enough state internally that we know that the appropriate page was loaded correctly. Thus, assuming we don't need the .hasTitle assertions to assert state, this test could be refactored into at least two tests: testAboutHomeVisibility and testPageNavigationTitles/Urls/something. They can have the same copy and pasted interaction pattern, but I feel it makes debugging easier ("oh, about:home visibility failed and is broken!" vs. "about:home visibility failed because the page titles are broken"). More importantly, it better helps to show what has already been tested. If I make an update to page titles, I shouldn't have to know to update testAboutHomeVisibility - I only have to update testPageNavigationTitles. Additionally, when adding features and the associated tests, it's easier to see "Oh! Page titles have never been tested! I should add a thorough test larger than the feature I added!". @@ +41,5 @@ > + // Loading about:home should show about:home again > + UITestCommon.enterAndLoadUrl(UITestCommon.ABOUT_HOME_URL); > + assertThat(TOOLBAR).hasTitle(UITestCommon.ABOUT_HOME_TITLE) > + .hasUrl(UITestCommon.ABOUT_HOME_URL); > + assertThat(ABOUTHOME).isVisible() Another example of something that can be more clearly tested individually (e.g. "testAboutHomeUrlFromWebsite"). ::: mobile/android/base/tests/testNewAboutHomeSwipes.java.in @@ +7,5 @@ > + > +/* Test correct state for URL bar after loading pages. > + */ > +public class testNewAboutHomeSwipes extends UITest { > + public void testNewAboutHomeSwipes() { I would also swipeToNext when you're at READING_LIST to ensure it remains READING_LIST and vice versa. Edge cases are fun. :) ::: mobile/android/base/tests/testNewTabHistory.java.in @@ +5,5 @@ > + > +/* Test correct state for URL bar after loading pages. > + */ > +public class testNewTabHistory extends UITest { > + public void testNewTabHistory() { "History" is ambiguous because of the browser history (previously visited links not necessarily in this tab). I would use something like "testTabBackStack".

(In reply to Wesley Johnston (:wesj) from comment #6) > (In reply to Lucas Rocha (:lucasr) from comment #4) > > In theory, we could come up with a similar custom framework but that's a non-trivial amount of code that > > I don't think we should have to maintain. > > Yeah, I don't think I agree here. Very very very very few (none?) of the > issues with our current tests are driven by the fact that our asserts are > 'cryptic'. Hence, this seems orthogonal to the real goal here. > > i.e. you're taking something that would read: > mAsserter.is(Toolbar.getTitle(), UITestCommon.BLANK01_TITLE, "Title is > correct"); > mAsserter.is(Toolbar.getURL(), UITestCommon.BLANK01_TITLE, "Url is > correct"); > > and writing: > assertThat(TOOLBAR).hasTitle(UITestCommon.BLANK01_TITLE) > .hasUrl(UITestCommon.BLANK01_URL); > > and in return, we're giving up a lot of control over asserts, and > introducing two competing frameworks for the same thing in our test harness. What kind of control would we want to keep? AFAIK, the only reason we might want control over the asserts is harness logging, which I'm not yet sure is something actually relevant for us (still waiting for feedback on that from gbrown & co). Even so, we can probably find a FEST-based solution for it. As for your point about competing frameworks, keep in mind that we'll have to keep the old and new API in parallel for some time. This is the only reason why I haven't completely removed the assert APIs from the Assert interface. Note how the old assert APIs are not used at all in the new API code. > I don't hate the FEST bit, but it seems like it should > > 1.) Be a separate bug and > 2.) Be done unilaterally to avoid the "why do we do this here and that > there?" questions that led to a lot of this test fragmentation to begin > with. i.e. people are going to copy/paste code. We should make it so doing > so doesn't result in badness. What would doing this in a follow-up bug give us? My goal with this patch is to set the general 'tone' for the new API. And this includes the way we do assertions, which is a core part of any testing API i.e. they are definitely the most frequent thing we do in any test code. If we do this in a follow-up bug, it will get more and more annoying to port the existing assertions to FEST later as the code based on the new API will only grow with time. Also, it won't necessarily be a simple replacement. There are some API design decisions based on this. For instance, we don't necessarily need getters like getTitle()/getUrl() in the components if we take the FEST-based route. So, I would be great if we could decide on this sooner rather than later. Just to be clear, the reason I really like FEST is: 1. It's more much more expressive. FEST assertions almost read like plain English. 2. It streamlines the way we assert in our tests, and provides a nice base for new types of assertions. 3. It's concise, as opposed to the current asserts that look mostly like boilerplate. 4. It's fun to use! The main drawback I see here is that it's a new API we'll have to learn. The fact that none of us use an IDE (with some magic autocompletion) will make it slightly less convenient to explore the available assertions. But we'll get better at it as we go. As for your point 2, see my comment above. The only reason why I kept the existing assertion API is because we have to keep the old API around for the existing tests. If you take the FEST route, all assertions will be FEST-based. If you're thinking about copy/pasting happening from old API to new, I would be surprised if that happens, but who knows? ;-) > > The idea here is abstract the different ways to "go back" on phones and > > tablets. You don't want tests to handle that locally. Besides, the UI to go > > back on phones is actually the system back button. If pressing back doesn't > > actually go back is something that tests themselves have to properly handle > > case by case, no? > > "go back" is the vague term here, right? It can mean two different things. I > don't think our toolbar back button should always do the same thing as > hitting the android back button (it doesn't in other apps that have a "back" > button in the actionbar either). Abstracting them as if they are the same > seems likely to fail? I understand your point about using system back button to mean "go back" might lead to failure in some tests but what would be the alternative to that? This is *the* phone UI to "go back" after all. This is just code I imported from BaseTest.Navigation by the way. It's not new code. Whatever we decide here, it should probably be fixed in BaseTest too.

Comment on attachment 798840 [details] [diff] [review] Bootstrap new UI testing API Request feedback from Adrian Tamas too, as he wrote a lot of code in our current tests.

Comment on attachment 798840 [details] [diff] [review] Bootstrap new UI testing API Review of attachment 798840 [details] [diff] [review]: ----------------------------------------------------------------- I appreciate that you have taken this on and hopeful that this effort will improve the reliability of the test suite. From my perspective, reliability (few intermittent failures *and* a high probability of failure when the function under test fails) is the most important and most elusive goal. I like the basic structure. I have slight misgivings about the number of getXxx() functions that seem to be required to do anything. > The goals for the new API are pretty obvious. We want UI tests to be: > 2. Consistently written Keep in mind that Robocop is used by different people/groups for various purposes and some flexibility will be necessary. You may need to settle for groups of consistent tests! > - Note how I haven't changed any of the existing tests or harness utils > classes. Remember, we're building the new API in parallel. We can probably > replace or rewrite the harness code later if we need. But that's not > necessary at this point. That makes me nervous. Part of the confusion in today's Robocop is that it contains several attempts at frameworks for tests: BaseTest (or something) is extended to provide some useful function, and then that function is used in a few tests, but older tests continue doing something similar in a completely different way. If you create a new way to write tests, but keep all the old code, you will only make this situation worse! It will be much better if all the existing tests are ported to UITests. > - All assertions are done using FEST-Assert and FEST-Android I don't see much value in FEST myself, but it doesn't bother me. > - Using FEST means we won't be doing assertions using FennecMochitestAssert > or FennecTalosAssert. I noticed that these classes do a whole bunch of > logging (i.e. one log message per assertion). Not sure how important this > is. I'd like to understand the logging requirements a bit better here to > adapt the new API accordingly. One function of the logging is to write to a file. The test harness pulls that file and dumps in along with other info like device info and the logcat. Keep in mind that the logcat picked up and dumped in tbpl logs is rarely complete. The log format (especially bits like TEST-PASS/UNEXPECTED-FAIL) was influenced by existing test log formats (especially mochitest). There are test harness and tbpl scripts that rely on these formats to identify test pass/fail and report the assertion pass/fail counts. Those are generally regexes that could be modified to suit a new format, but that's another complication with little value. Finally, don't forget that some Talos tests use Robocop and those have different logging requirement. :jmaher is your best source of knowledge here. Good luck! The log format is somewhat tied to ::: mobile/android/base/tests/ToolbarComponent.java.in @@ +63,5 @@ > + mUrlEditText = (TextView) ViewHelper.findViewById(mToolbarView, URL_EDIT_TEXT_ID); > + mUrlTitleText = (TextView) ViewHelper.findViewById(mToolbarView, URL_TITLE_TEXT_ID); > + mGoButton = (ImageButton) ViewHelper.findViewById(mToolbarView, GO_BUTTON_ID); > + mBackButton = (ImageButton) ViewHelper.findViewById(mToolbarView, BACK_BUTTON_ID); > + mForwardButton = (ImageButton) ViewHelper.findViewById(mToolbarView, FORWARD_BUTTON_ID); I am a little concerned about keeping copies of all these views around. If the ToolbarComponent's lifetime is out of sync with the actual toolbar view, or one of it's views... AFAIK, this is safe today, but it seems like a risk over time. ::: mobile/android/base/tests/UITestDevice.java.in @@ +34,5 @@ > + // UI test context > + private final UITestContext mTestContext; > + > + // Type of device (phone or tablet) > + public final Type type; I'm sure these were just copy/pasted, but I would like to see our basic naming conventions in use. s/type/mType/, s/width/mScreenWidth/, etc. I expect they need not be public. ::: mobile/android/base/tests/ViewHelper.java.in @@ +29,5 @@ > + > + final Activity activity = sTestContext.getActivity(); > + final Driver driver = sTestContext.getDriver(); > + > + final Element element = driver.findElement(activity, id); fwiw, I have never liked the Element class, and would prefer to see it folded in to something like findViewById. ::: mobile/android/base/tests/testAboutHomeVisibility.java.in @@ +7,5 @@ > + > +/* Test correct state for URL bar after loading pages. > + */ > +public class testAboutHomeVisibility extends UITest { > + public void testAboutHomeVisibility() { :mcomella makes good points, but on the other hand, there are drawbacks to having more/smaller tests. Each test has significant overhead to set up a fresh profile, launch the browser, wait for activity completion, collect logs, etc. Also, you end up exercising the same startup and page load and wait-for-gecko-ready code over and over again. On the whole, I would prefer to see fewer, larger tests. @@ +26,5 @@ > + // Go to blank 02 > + UITestCommon.enterAndLoadUrl(UITestCommon.BLANK02_URL); > + assertThat(TOOLBAR).hasTitle(UITestCommon.BLANK02_TITLE) > + .hasUrl(UITestCommon.BLANK02_URL); > + assertThat(ABOUTHOME).isNotVisible(); A common theme in intermittent robocop failures has been code that looks just like this: 1. execute an action: click, drag, or send keys 2. assert: check that a certain state exists There's always a need to wait for an event or wait for a change in view state between the action and the assertion: 1. execute: click, drag, or send keys 2. wait: wait for something to happen, with a timeout - on timeout, fail the test 3. assert: check that a certain state exists A poor wait, or even a null wait, will produce a passing test in a surprising number of circumstances...most of the time! I don't think that we should hide away the important "wait" step in the name of tidy code or ease of writing new tests. It is essential and it should be explicit in every UI test we write.

Comment on attachment 798840 [details] [diff] [review] Bootstrap new UI testing API Review of attachment 798840 [details] [diff] [review]: ----------------------------------------------------------------- Looks good! I think one thing to watch out for is making sure that the testing API handles waiting properly, so we don't have callers having to call things like "waitForText" to verify that an action has been successfully completed. We should take advantage of the new Condition interface and waitForCondition added in Robotium 4.2 - callers can always pass in always-true conditions if they don't need to wait. ::: mobile/android/base/tests/AboutHomeComponent.java.in @@ +49,5 @@ > + mHomePagerView = (ViewPager) ViewHelper.findViewById(HOME_PAGER_ID); > + } > + > + private boolean waitForCurrentItem(final int expectedItem) { > + return WaitHelper.wait(new WaitHelper.WaitTest() { We can use mSolo.waitForCondition here, per new Robotium 4.2 API. @@ +61,5 @@ > + > + private void swipe(int direction) { > + assertThat(this).isVisible(); > + > + final int currentItem = mHomePagerView.getCurrentItem(); Can we rename this (and following places) to something like pageIndex? ::: mobile/android/base/tests/ToolbarComponent.java.in @@ +40,5 @@ > + > + // Url edit text view > + private final TextView mUrlEditText; > + > + // Url edit text view Comment doesn't match object here. @@ +147,5 @@ > + public ToolbarComponent pressBack() { > + ANDROID.assertThat(mBackButton).isNotNull(); > + ANDROID.assertThat(mBackButton).isVisible(); > + > + mSolo.clickOnView(mBackButton); Since we return immediately here and don't wait here for anything to load/happen, you might want to add a comment to that effect. Or possibly accept an argument (probably a Condition) to wait on before returning. Same for pressForward. ::: mobile/android/base/tests/UITestDevice.java.in @@ +49,5 @@ > + // Determine device version > + int sdk = Build.VERSION.SDK_INT; > + if (sdk < Build.VERSION_CODES.HONEYCOMB) { > + version = VERSION_2x; > + } else { Why not use an "else if...else" here? ::: mobile/android/base/tests/ViewHelper.java.in @@ +23,5 @@ > + public static void init(UITestContext testContext) { > + sTestContext = testContext; > + } > + > + public static View findViewById(String id) { This should include a comment saying that for non-unique views (like layouts that are reused in HomeFragment), the first view with a matching id will be returned, and may not be the one that's expected; or it could be renamed to findViewByUniqueId. ::: mobile/android/base/tests/WaitHelper.java.in @@ +16,5 @@ > + private static UITestContext sTestContext; > + > + private static final int DEFAULT_MAX_WAIT_MS = 5000; > + > + public interface WaitTest { In Robotium 4.2, there is a new interface called Condition, and an associated mSolo.waitForCondition(condition, timeout). We should use that instead; we could optionally wrap that in this class so that we can use DEFAULT_MAX_WAIT_MS.

re :gbrown: > Each test has significant overhead to set up a fresh profile, launch > the browser, wait for activity completion, collect logs, etc. Point noted. It might be worth benchmarking to see exactly how long (and how detrimental) this might be. > Also, you end up exercising the same startup and page load and > wait-for-gecko-ready code over and over again. Hopefully taken care of in a "setUp()" method or equivalent. > On the whole, I would prefer to see fewer, larger tests. From my perception of how long it takes some devices to start Gecko, this seems reasonable. If we do go this route, we should still try to exercise "smaller test cases" in code even if we don't actually create new JUnit tests (and thus start with a clean profile). For example... public class TestAboutHome { public void testAboutHome() throws Exception { testAboutHomeSwipe(); // not a JUnit test testAboutHomePagesClick(); // not a JUnit test ... } ... } To mitigate these helper functions test dependencies, we can also reset the state to something agreed upon for each test class, i.e. testAboutHomeSwipe(); resetAfterTestAboutHomeSwipe(); testAboutHomePagesClick(); resetAfterTestAboutHomePagesClick();

Comment on attachment 798840 [details] [diff] [review] Bootstrap new UI testing API Review of attachment 798840 [details] [diff] [review]: ----------------------------------------------------------------- This seems really scalable and may work great. My issues with this would be: * As Geoff pointed out there aren't a lot of waits between assertions and I think this will not be very reliable. The original Robocop classes, to me, seemed to mimic Selenium behavior: click on something, wait for a result, check the result. * The FEST assertions seem easy enough to use but the I'm not sure what work needs to be done to make these work with the current setup for running tests. The FennecNativeAsserter logging was important because in a lot of situations you could, just from looking at what step failed, to debug and understand what went wrong. * I have seen comments pro and against rewriting this with and without the current Robocop tests active. Personally, unless this is done in a separate branch, I would not remove all tests and start again from scratch with writing the tests. There are still a lot of tests disabled but at least we have some running. People creating can create tests in the old Robocop framework for now and we could just rewrite them later if they don't know about the new structure * Regarding the size of the tests. Usually we would make 1 test per feature trying to cover as many testcases from a MozTrap BFT suite as possible. This has in the past created problems with tests running to long but we should not fragment the tests a lot and end up with a lot of small tests in my opinion. * Timing a run locally on a tablet via adb I see consistently between 18 and 19 seconds from test start to Gecko:Ready plus an additional 10-15 seconds for the app to finish background activities after Gecko:Ready unblock. I assume this time will increase a bit over LAN in the labs. I have talked to :gbrown about this previously, and maybe with the Front-end dev team helping we could improve on this, but after unblocking for Gecko:Ready and returning from the blockForGeckoReady method, because of other background activities, the debug builds are really sluggish. Although the Settings menu item is enabled with the handling of Gecko:Ready on debug builds this is not instant, it happens much later and coincides with the app returning to normal operation with no more sluggishness. Because of this sluggishness usually the first page load in a test is not very reliable. ::: mobile/android/base/tests/AboutHomeComponent.java.in @@ +58,5 @@ > + } > + }); > + } > + > + private void swipe(int direction) { Instead of the different swipes can't we get something similar to the openAboutHomeTab method from the current AboutHomeTest? It seems better to me just to have a way of opening a specific about:home tab directly without having to always make sure you are in the right place when swiping left/right

Comment on attachment 798840 [details] [diff] [review] Bootstrap new UI testing API Review of attachment 798840 [details] [diff] [review]: ----------------------------------------------------------------- This looks nice! I agree with everything everyone else has had to say so far, and added a few more comments. As a general comment, I think we should include some links to FEST documentation either in our code or testing wiki page (or both!) because it can be hard to follow how external APIs are used (e.g. what's the proper way to extend AbstractAssert?). I think it would also be nice for someone (not necessarily you) to make a diagram of how our testing architecture works, including the relationship with the code in build/robocop. I'm still not entirely clear about the relationships, and I think that would be a good step towards making it easier for people to feel comfortable writing tests. ::: mobile/android/base/tests/AboutHomeComponent.java.in @@ +53,5 @@ > + return WaitHelper.wait(new WaitHelper.WaitTest() { > + @Override > + public boolean test() { > + ANDROID.assertThat(mHomePagerView).hasCurrentItem(expectedItem); > + return true; I agree with kats here. In addition to the fact that we may incorrectly return true here when the condition we're testing isn't met, it looks like in that case, the test will just fail, rather than actually waiting until the condition we're trying to test is true. We should figure out a good way to implement this pattern, since it's used often. @@ +64,5 @@ > + > + final int currentItem = mHomePagerView.getCurrentItem(); > + > + if (direction == Solo.LEFT) { > + GestureHelper.swipeLeft(); I like the way these static helper classes are used. ::: mobile/android/base/tests/ToolbarComponent.java.in @@ +133,5 @@ > + > + return this; > + } > + > + public ToolbarComponent enterUrl(String url) { Add a comment that this should only be called after enterEditingMode (it looks like that's the case). ::: mobile/android/base/tests/UITestAssertions.java.in @@ +12,5 @@ > + > +import android.app.Activity; > + > +class UITestAssertions extends Assertions { > + public static AboutHomeComponentAssert assertThat(AboutHomeComponent actual) { Nit: Tabs! ::: mobile/android/base/tests/UITestCommon.java.in @@ +95,5 @@ > + domContentLoaded.blockForEvent(); > + domContentLoaded.unregisterListener(); > + } > + > + public static void goBack() { If we want this to just be a utility to help us navigate session history (and add a separate test for testing the back button itself), we could just send a "Session:Back" message to gecko. Same thing for forward/reload. ::: mobile/android/base/tests/UITestDevice.java.in @@ +14,5 @@ > + > +import static org.fest.reflect.core.Reflection.method; > +import static org.fest.reflect.core.Reflection.type; > + > +class UITestDevice { Along the lines of renaming this to UITestDeviceHelper, could it be made into a final class with static methods, similar to the other FooHelper classes? ::: mobile/android/base/tests/ViewHelper.java.in @@ +17,5 @@ > + // UI test context > + private static UITestContext sTestContext; > + > + private ViewHelper() { > + } Does a class need a constructor if it's never called? Also, could these helper classes be declared as static? ::: mobile/android/base/tests/WaitHelper.java.in @@ +44,5 @@ > + result = t.test(); > + } catch (AssertionError ae) { > + // We might want to assert inside in WaitTest in > + // order to check expected state. > + result = false; Ah, this answers my concern from up above. It's not intuitive that test() can either return false or throw an assertion error to indicate that we should keep waiting. Is there a way for us to just do one or the other? Maybe just always throw an AssertionError?

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #5) > This technically doesn't fit the contract of a WaitTest, does it? In this > case it should return true iff the home pager view has the expected item, > but AFAIK we don't have a way to test that without asserting it. Which might > be a more general problem (being able to get state without necessarily > asserting it). Fair enough. I'll get rid of it and replace our waiting API with the equivalent one in Robotium i.e. waitForCondition() > @@ +114,5 @@ > > + > > + public AboutHomeComponentAssert isVisible() { > > + isNotNull(); > > + > > + ANDROID.assertThat(mActual.mHomePagerView).overridingErrorMessage("Home pager should be visible") > > The overridingErrorMessage syntax seems a bit clunky although I guess we > inherit that from FEST? Yeah, I'm still investigating better ways to do logging/error messages with FEST. More on that later.

mcomella will be take this bug from where I left off. We have posted some notes on what needs to change in my original patch here: https://mobile.etherpad.mozilla.org/21

I made the changes that Lucas and I discussed in [1], most notably removing FEST and swapping its Assertion API with something more like JUnit, to match the unit test suite we are planning to add to m-c (bug 903528). I wasn't sure to do with the mAsserter names here, however - please comment on that if you have an idea. One major change I would still like to do is move *Component to a components/ package and *Helper to a helpers/ package. nalexander says this can be done after the robocop preprocessing is removed (bug 905703). I also need to improve the reliability of some methods, mostly by adding additional wait calls. Additionally, being able to import R and other classes from the main code base can be useful at times. I left in a some TODOs about things I'm unsure of or things that I still have todo. Some of the more notable things I'm unsure of are: 1) Should *Helper be made into instance vars? There's a benefit to adding a BaseHelper class that has an init() method, so we can freely assertInitialized in each of the methods in the Helpers with rewriting that code. Also, each Helper can share common instance variables (e.g. mSolo, mActions). 2) Some Components/Helpers take in UITest, rather than UITestContext. This seems wrong, and I'm not sure what to do about it without making UITestContext pretty beefy. Most notably, getting references to the Components (e.g. TOOLBAR) is difficult. Some larger, more abstract topics discussed in this bug that I'd like to address at some point are: 1) Few large tests vs. many small tests 2) Take some action, wait, and assert style of coding - comment 11 (I think the waiting can be masked by the framework, but we have to be diligent about ensuring all framework code is written like this) 3) Documentation for extending the framework and for writing tests (I've taken some notes on what I've learned and what I'd want to write about) I'll continue to work on the TODOs that are not design-oriented (such as improving test reliability) while awaiting feedback. I've been making my changes diffing off Lucas' initial patch [2] in a github repo [3], if micro-commits are easier to read/review - be warned that they're a bit haphazard. [1]: https://mobile.etherpad.mozilla.org/21 [2]: https://bugzilla.mozilla.org/attachment.cgi?id=798840&action=edit [3]: https://github.com/mcomella/new-ui-testing/compare/mcomella

Comment on attachment 826058 [details] [diff] [review] Bootstrap new API for UI testing in Fennec. Just Lucas for now, to avoid making everyone else feel obligated to review. I'll send out a wider request for feedback after his feedback and the subsequent changes.

Comment on attachment 826058 [details] [diff] [review] Bootstrap new API for UI testing in Fennec. Review of attachment 826058 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/tests/AssertionHelper.java.in @@ +7,5 @@ > + > +import @ANDROID_PACKAGE_NAME@.Assert; > + > +// TODO: Should we use mAsserter names? It would make the API inconsistent with JUnit. > +final class AssertionHelper { This hole class doesn't seem to bring anything more then the existing Robocop Asserter. Plus please take into account the ispixel assertions which are needed in the Robocop talos tests and in some existing Robocop tests(for e.g. testFindInPAge,testLoad,testOverscroll,testScrollLock) ::: mobile/android/base/tests/DeviceHelper.java.in @@ +111,5 @@ > + solo.setActivityOrientation(Solo.PORTRAIT); > + } else { > + solo.setActivityOrientation(Solo.LANDSCAPE); > + } > + } While we found out that this works for devices the panda and tegra boards have the orientation not set (-1 I believe) so this will always go to landscape. Maybe just do a setLandscape and a setPortrait method ::: mobile/android/base/tests/NavigationHelper.java.in @@ +22,5 @@ > + static final String BLANK02_URL = "/robocop/robocop_blank_02.html"; > + static final String BLANK02_TITLE = "Browser Blank Page 02"; > + > + static final String BLANK03_URL = "/robocop/robocop_blank_03.html"; > + static final String BLANK03_TITLE = "Browser Blank Page 03"; At the moment there is a StringHelper class defined that also defines resources like pages and titles. Maybe it would be good to keep that since the purpose of it was to make the tests more independent of string changes. @@ +68,5 @@ > + if (DeviceHelper.isTablet()) { > + assertNotNull(sTest.TOOLBAR); > + sTest.TOOLBAR.pressBack(); > + } else { > + // TODO: Lower soft keyboard first? You can use here mSolo.hideSoftKeyboard() here. It's a new robotium 4.3 method that works now. ::: mobile/android/base/tests/ToolbarComponent.java.in @@ +22,5 @@ > + private static final String URL_DISPLAY_CONTAINER_ID = "url_display_container"; > + private static final String URL_TITLE_TEXT_ID = "url_bar_title"; > + private static final String GO_BUTTON_ID = "go"; > + private static final String BACK_BUTTON_ID = "back"; > + private static final String FORWARD_BUTTON_ID = "forward"; Candidates for a section in StringHelper called ID's or a separate IDs class? @@ +60,5 @@ > + } > + > + ImageButton getForwardButton() { > + return (ImageButton) ViewHelper.findViewById(getToolbarView(), FORWARD_BUTTON_ID); > + } There is a getView(String id) method in robotium that may be used here

Regarding the size of the test and the time spent for setup before the test run please see from Comment 14: > * Timing a run locally on a tablet via adb I see consistently between 18 > and 19 seconds from test start to Gecko:Ready plus an additional 10-15 > seconds for the app to finish background activities after Gecko:Ready > unblock. I assume this time will increase a bit over LAN in the labs.

Comment on attachment 826058 [details] [diff] [review] Bootstrap new API for UI testing in Fennec. Review of attachment 826058 [details] [diff] [review]: ----------------------------------------------------------------- This looking pretty good. ::: mobile/android/base/tests/AboutHomeComponent.java.in @@ +32,5 @@ > + public AboutHomeComponent(UITestContext testContext) { > + super(testContext); > + } > + > + ViewPager getHomePagerView() { Should this be private? @@ +41,5 @@ > + return getHomePagerView().getCurrentItem() == page.ordinal(); > + } > + > + public boolean isVisible() { > + return getHomePagerView().getVisibility() == View.VISIBLE; nit: enclose expression with parens. @@ +46,5 @@ > + } > + > + // TODO: Take specific page as parameter rather than swipe in a direction? > + // TODO: What is next/prev? > + public AboutHomeComponent swipeToNext() { swipeToNextPage() for clarity? @@ +51,5 @@ > + swipe(Solo.LEFT); > + return this; > + } > + > + public AboutHomeComponent swipeToPrevious() { swipeToPreviousPage()? @@ +71,5 @@ > + assertNotNull(adapter); > + > + // Swiping left goes to next, swiping right goes to previous > + final int unboundedPageIndex = pageIndex + (direction == Solo.LEFT ? 1 : -1); > + final int expectedPageIndex = Math.min(Math.max(0, unboundedPageIndex), adapter.getCount() - 1); nit: empty line here. @@ +81,5 @@ > + private void waitForPageIndex(final int expectedIndex) { > + WaitHelper.wait(new Condition() { > + @Override > + public boolean isSatisfied() { > + return getHomePagerView().getCurrentItem() == expectedIndex; nit: enclose with parens. ::: mobile/android/base/tests/AssertionHelper.java.in @@ +17,5 @@ > + public static void init(final UITestContext testContext) { > + mAsserter = testContext.getAsserter(); > + } > + > + public static void assertEquals(final Object expected, final Object actual) { You should probably add a version of each of these methods with an extra 'errorMessage' argument. @@ +26,5 @@ > + mAsserter.is(actual, expected, ""); > + } > + > + public static void assertFalse(final boolean actual) { > + // TODO: What is diag? (Also assertTrue) diag? ::: mobile/android/base/tests/CommonUseHelper.java.in @@ +30,5 @@ > + private static UITest sTest; > + > + private CommonUseHelper() { /* To disallow instantiation. */ } > + > + // TODO: Should be UITestContext. Indeed. But you still need a way to access the components, no? ::: mobile/android/base/tests/DeviceHelper.java.in @@ +22,5 @@ > + PHONE, > + TABLET > + } > + > + public enum AndroidPlatformVersion { PlatformVersion should be clear enough, no? @@ +60,5 @@ > + > + private static void setScreenDimensions() { > + final Activity activity = sTestContext.getActivity(); > + final DisplayMetrics dm = new DisplayMetrics(); > + activity.getWindowManager().getDefaultDisplay().getMetrics(dm); nit: empty line here. @@ +66,5 @@ > + sScreenWidth = dm.widthPixels; > + } > + > + private static void setDeviceType() { > + final Activity activity = sTestContext.getActivity(); nit: empty line here. @@ +106,5 @@ > + public static void rotate() { > + final Solo solo = sTestContext.getSolo(); > + final Activity activity = sTestContext.getActivity(); > + > + if (activity.getRequestedOrientation () == ActivityInfo.SCREEN_ORIENTATION_LANDSCAPE) { nit: remove space before () ::: mobile/android/base/tests/GestureHelper.java.in @@ +31,5 @@ > + final int halfHeight = driver.getGeckoHeight() / 2; > + > + actions.drag(direction == Solo.LEFT ? halfWidth : 0, > + direction == Solo.LEFT ? 0 : halfWidth, > + halfHeight, halfHeight); Robotium now has a swipe() method. Use it instead of actions.drag(). ::: mobile/android/base/tests/NavigationHelper.java.in @@ +22,5 @@ > + static final String BLANK02_URL = "/robocop/robocop_blank_02.html"; > + static final String BLANK02_TITLE = "Browser Blank Page 02"; > + > + static final String BLANK03_URL = "/robocop/robocop_blank_03.html"; > + static final String BLANK03_TITLE = "Browser Blank Page 03"; I'm not a fan of the StringHelper (too many unrelated things in the same class) but let's keep strings there for now so that this can land sooner. @@ +47,5 @@ > + domContentLoaded.blockForEvent(); > + domContentLoaded.unregisterListener(); > + } > + > + // TODO: Doc? How is it adjusted? This simply prefixes the URLs with the docshell HTTP server host for the tests. @@ +59,5 @@ > + return url; > + } > + > + // TODO: Ambiguous name? Is CommonUseHelper.goBack() enough? Perhaps w/ doc? > + // TODO: Can send "Session:Back" to Gecko - but defeats benefit of UI testing. Yeah, this is about UI testing. @@ +70,5 @@ > + sTest.TOOLBAR.pressBack(); > + } else { > + // TODO: Lower soft keyboard first? > + // TODO: Solo.goBack > + sTest.getActions().sendSpecialKey(Actions.SpecialKey.BACK); Just wondering if there's a Robotium API for that? @@ +77,5 @@ > + // TODO: wait for new page to be fully loaded > + } > + > + public static void goForward() { > + // TODO: Necessary? Strictly speaking, no. ::: mobile/android/base/tests/ToolbarComponent.java.in @@ +22,5 @@ > + private static final String URL_DISPLAY_CONTAINER_ID = "url_display_container"; > + private static final String URL_TITLE_TEXT_ID = "url_bar_title"; > + private static final String GO_BUTTON_ID = "go"; > + private static final String BACK_BUTTON_ID = "back"; > + private static final String FORWARD_BUTTON_ID = "forward"; Ideally, IDs should be private and owned by their respective components. So, no need for a separate IDs class. @@ +31,5 @@ > + > + /** > + * Returns the root View for the browser toolbar. > + */ > + View getToolbarView() { Shouldn't this be private? @@ +50,5 @@ > + > + /** > + * Returns the View for the go button in the browser toolbar. > + */ > + ImageButton getGoButton() { Shouldn't all these getters be private? @@ +74,5 @@ > + return getUrlEditText().getText().equals(expected); > + } > + > + public boolean isEditing() { > + // TODO: DisplayContainer vs. TitleText. What is the difference? Display container contains title+favicon+lock icon+stop. If it's not visible, we're editing. @@ +83,5 @@ > + > + public ToolbarComponent enterEditingMode() { > + assertFalse(isEditing()); > + > + mSolo.clickOnView(getUrlTitleText(), true); nit: empty line here. @@ +92,5 @@ > + return isEditing(); > + } > + }); > + > + // TODO: Previous comment: "Failed to get input focus on editing mode" Do you want to extend the wait() method to include an error message? @@ +122,5 @@ > + > + public ToolbarComponent dismissEditingMode() { > + assertTrue(isEditing()); > + > + if (getUrlEditText().hasFocus()) { I wonder if this should be isInputMethodTarget() instead? For consistency with enterEditingMode() @@ +126,5 @@ > + if (getUrlEditText().hasFocus()) { > + // Drop the soft keyboard. > + // TODO: Solo.hideSoftKeyboard? Seemed to work strangely. > + mActions.sendSpecialKey(Actions.SpecialKey.BACK); > + } nit: empty line here. @@ +142,5 @@ > + return this; > + } > + > + public ToolbarComponent enterUrl(final String url) { > + assertTrue(isEditing()); Add a custom message to these assertions. So that it's easier to debug from the logs. @@ +147,5 @@ > + assertTrue(getUrlEditText().isInputMethodTarget()); > + > + // TODO: Robotium's methods caused NPE. Note that Robotium's methods are less > + // user-like, however, and does not clear text if the text is highlighted > + // and a key is pressed. May need to add an extra test for this behavior. Not sure I follow. Is the NPE on the EditText? @@ +157,5 @@ > + return this; > + } > + > + // TODO: Ambiguous name? pressBackButton? Add doc or is Component.pressBack descriptive enough? > + public ToolbarComponent pressBack() { pressBackButton is probably better. @@ +170,5 @@ > + > + return this; > + } > + > + public ToolbarComponent pressForward() { Ditto. pressForwardButton. ::: mobile/android/base/tests/UITest.java.in @@ +45,5 @@ > + private String mBaseUrl; > + // Base to build raw absolute URLs > + private String mRawBaseUrl; > + > + // TODO: These aren't constants. Keep names capitalized for visibility? Yeah, I capitalized them for visibility. But I'm fine with AboutHome and Toolbar too. @@ +110,5 @@ > + > + super.tearDown(); > + } > + > + private void initComponents() throws Exception { Why the 'throws Exception'? @@ +116,5 @@ > + TOOLBAR = new ToolbarComponent(this); > + } > + > + // TODO: Should the helpers be instance variables? > + private void initHelpers() { I made them static so that they are accessible from anywhere in the test/components/helpers. If you make them instance variables, you might end up needing to add more API to the UITestContext interface. Not a big deal though. ::: mobile/android/base/tests/ViewHelper.java.in @@ +35,5 @@ > + > + final Activity activity = sTestContext.getActivity(); > + final Driver driver = sTestContext.getDriver(); > + > + final Element element = driver.findElement(activity, id); I wonder if we can replicate the same voodoo that Robotium does with String ids here so that we can get rid of this findElement API altogether? ::: mobile/android/base/tests/WaitHelper.java.in @@ +32,5 @@ > + /** > + * Waits for the given {@link Solo.Condition} using the given wait duration; will throw an > + * AssertionError if the duration is elapsed and the condition is not satisfied. > + */ > + public static void wait(final Condition condition, final int waitMillis) { Maybe add a 'message' argument for the assertion here? ::: mobile/android/base/tests/testAboutHomeVisibility.java.in @@ +12,5 @@ > + public void testAboutHomeVisibility() { > + CommonUseHelper.blockForGeckoReady(); > + > + // Check initial state with about:home > + assertTrue(TOOLBAR.hasTitle(NavigationHelper.ABOUT_HOME_TITLE)); We should probably add custom messages to these assertions. With that in mind, maybe it's the case to have things like assertTitle() in TOOLBAR instead? This way you can write the message only once and reuse it everywhere. It will also look a bit more concise: From: assertTrue(TOOLBAR.hasTitle(NavigationHelper.ABOUT_HOME_TITLE)) To: TOOLBAR.assertTitle(NavigationHelper.ABOUT_HOME_TITLE) I remember we talked about it but I can't remember what we've decided :-) Also, making the assert* methods return a reference to the given component will allow you chain the calls: TOOLBAR.assertTitle(NavigationHelper.ABOUT_HOME_TITLE) .assertCurrentPage(Page.TOP_SITES) .assertVisible(); @@ +13,5 @@ > + CommonUseHelper.blockForGeckoReady(); > + > + // Check initial state with about:home > + assertTrue(TOOLBAR.hasTitle(NavigationHelper.ABOUT_HOME_TITLE)); > + assertTrue(ABOUTHOME.isVisible()); Same here. Maybe do ABOUTHOME.assertVisible(). @@ +14,5 @@ > + > + // Check initial state with about:home > + assertTrue(TOOLBAR.hasTitle(NavigationHelper.ABOUT_HOME_TITLE)); > + assertTrue(ABOUTHOME.isVisible()); > + assertTrue(ABOUTHOME.hasCurrentPage(Page.TOP_SITES)); assertCurrentPage()?

(In reply to Adrian Tamas (:AdrianT) from comment #20) > This whole class doesn't seem to bring anything more then the existing > Robocop Asserter. I agree, but I was going for consistency between the eventual JUnit test suite that will be added (bug 903528). The Robocop Asserter is hard to use at times because its interface is hidden away in a separate file tree (build/mobile/robocop/ vs. mobile/android/base/tests/; this is more relevant to new developers though) and it's not clear how to use it correctly (e.g. bug 928566). Having the API consistent with JUnit will help alleviate these concerns. As lucas mentioned in comment 22, I'm leaning towards these assertions taking a name, like the Robocop Asserter, to be more descriptive in the logs. > There is a getView(String id) method in robotium that may be used here getView does not allow you to specify a root View so this method is still relevant. In the javadoc, I recommend using Solo.getView if you do not have a particular root view. If we can import R (and thus ids) directly, we can remove ViewHelper.findViewById in favor of using `View.findViewById(int id)` directly.

(In reply to Adrian Tamas (:AdrianT) from comment #20) > Candidates for a section in StringHelper called ID's or a separate IDs class? Ideally, we replace the ID Strings with R.id. Otherwise, I like the ids at private access levels within the components they should be used for, considering no other test class should be accessing those ids (and perhaps even the views they represent) directly (though I stand open to corrections). If we did take the put-everything-into-a-helper-class approach, I'd opt for an "IdHelper" class for to keep the line count down for usability.

(In reply to Lucas Rocha (:lucasr) from comment #22) > You should probably add a version of each of these methods with an extra > 'errorMessage' argument. I'm going to change this so we *only* have "errorMessage" versions since I feel it's more correct (and easier to maintain!). > diag? It's an argument to mAsserter.ok (https://mxr.mozilla.org/mozilla-central/source/build/mobile/robocop/Assert.java.in#15). It seems largely redundant with "name" though so I don't know the purpose. > Indeed. But you still need a way to access the components, no? Right. Should I extend UITestContext to include `get*Component()`? > Just wondering if there's a Robotium API for that? Yes, for both pressing back and lowering the soft keyboard. However, they did not seem to work quite like I expected (e.g. I don't think lower soft keyboard waits until after the keyboard is lowered) so I haven't inserted them yet (hence the TODOs). > Not sure I follow. Is the NPE on the EditText? I'll report back when I debug some more. > I wonder if we can replicate the same voodoo that Robotium does with String > ids here so that we can get rid of this findElement API altogether? According to nalexander, we should be able to use import R directly once the preprocessing is removed (bug 905703), but if that bug gets held up, maybe we can grab it from Robotium. > We should probably add custom messages to these assertions. With that in > mind, maybe it's the case to have things like assertTitle() in TOOLBAR > instead? This way you can write the message only once and reuse it > everywhere. It will also look a bit more concise: Sounds reasonable. > I remember we talked about it but I can't remember what we've decided :-) We decided against single line component assertions, but that was without the context of repeating a message, so now I'm for it. I'm still concerned about having a lot of duplication though (e.g. getTitle(), hasTitle(), assertTitle() for each element, though we can probably drop the hasTitle() version).

(In reply to Michael Comella (:mcomella) from comment #25) > (In reply to Lucas Rocha (:lucasr) from comment #22) > > You should probably add a version of each of these methods with an extra > > 'errorMessage' argument. > > I'm going to change this so we *only* have "errorMessage" versions since I > feel it's more correct (and easier to maintain!). Makes sense. > > Indeed. But you still need a way to access the components, no? > > Right. Should I extend UITestContext to include `get*Component()`? I'd go with a more generic API: getComponent(Component c). With Component being an Enum. > > Just wondering if there's a Robotium API for that? > > Yes, for both pressing back and lowering the soft keyboard. However, they > did not seem to work quite like I expected (e.g. I don't think lower soft > keyboard waits until after the keyboard is lowered) so I haven't inserted > them yet (hence the TODOs). Yeah, it seems Robotium's hideSoftKeyboard() waits a bit *before* hiding the keyboard, not after. See: https://github.com/jayway/robotium/blob/master/robotium-solo/src/main/java/com/jayway/android/robotium/solo/Solo.java#L2274 https://github.com/jayway/robotium/blob/master/robotium-solo/src/main/java/com/jayway/android/robotium/solo/DialogUtils.java#L119 > > I wonder if we can replicate the same voodoo that Robotium does with String > > ids here so that we can get rid of this findElement API altogether? > > According to nalexander, we should be able to use import R directly once the > preprocessing is removed (bug 905703), but if that bug gets held up, maybe > we can grab it from Robotium. Awesome. > > I remember we talked about it but I can't remember what we've decided :-) > > We decided against single line component assertions, but that was without > the context of repeating a message, so now I'm for it. Ok. > I'm still concerned about having a lot of duplication though (e.g. > getTitle(), hasTitle(), assertTitle() for each element, though we can > probably drop the hasTitle() version). I'd start with having the assert* methods (e.g. assertTitle, assertVisible, ...) public. At first sight, I don't expect UITest subclasses to need things like getTitle(). Right?

> I made them static so that they are accessible from anywhere in the > test/components/helpers. If you make them instance variables, you might > end up needing to add more API to the UITestContext interface. Not a > big deal though. (quote refers to making Helpers static) I opted to keep them static because they reference each other, meaning there would need to be some awkward dependency management (e.g. construct the helpers, then pass those references into all of those helpers - possible, particularly with a superclass, but messy). I did create a BaseHelper with static references to the UITestContext elements though. > Yeah, it seems Robotium's hideSoftKeyboard() waits a bit *before* hiding the > keyboard, not after. See: It seems the issue is that Solo.hideSoftKeyboard does not actually clear focus, meaning pressing back again (such as to dismiss editing mode) will cause focus to be removed, rather than causing the UI to go back a page. My workaround is to press back twice instead of using the soft keyboard specific method.

I updated the tests for the review comments by either fixing the issues directly, or adding TODOs for things that can be done later (including after this initial patch lands - e.g. assertIsPixel). If something is missing that you commented on, it's likely forgotten - feel free to mention it again. For an incremental view of the changes between the previous revision and the current one, see [1]. In addition to handling the comments, the major other changes I made are: * Made testNewTabHistory reliable by implementing WaitHelper.StateChangeVerifier * Added a BaseHelper class that all Helpers extend to allow access to various instances (e.g. Solo, *Component) without getters. * Used Robotium's back button method. * Used Robotium's text input methods. The one major issue to hit before this lands is to implement goForward (as testNewTabHistory currently fails on it). Other major things that should happen at some point: * Packages * Remove preprocessing and use R directly for IDs I will continue to investigate the TODOs while awaiting feedback. [1]: https://github.com/mcomella/new-ui-testing/compare/mcomella...02

Comment on attachment 828808 [details] [diff] [review] Bootstrap new API for UI testing in Fennec. Will f? a wider audience once this feedback comes in.

Comment on attachment 828808 [details] [diff] [review] Bootstrap new API for UI testing in Fennec. Review of attachment 828808 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/tests/testNewAboutHomeSwipes.java.in @@ +7,5 @@ > + > +// TODO: Elaborate on test desc and use /**. > +/* Test correct state for URL bar after loading pages. > + */ > +public class testNewAboutHomeSwipes extends UITest { Does this work for both phones and tablets? The order of the about:home tabs was different on phones and tablets and http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/tests/AboutHomeTest.java.in#41 suggests that the 2 were treated differently

> Does this work for both phones and tablets? Good call, I actually have not tested on tablets yet - I will add that to my list of things to do before landing.

Comment on attachment 828808 [details] [diff] [review] Bootstrap new API for UI testing in Fennec. Review of attachment 828808 [details] [diff] [review]: ----------------------------------------------------------------- Nice. Just not entirely convinced about the ChangeVerifier bits in WaitHelper. ::: mobile/android/base/tests/AboutHomeComponent.java.in @@ +16,5 @@ > +import android.support.v4.view.ViewPager; > +import android.view.View; > + > +class AboutHomeComponent extends UITestComponent { > + // TODO: Can we just import the enum directly? Will this ever be possible from our Robocop code? ::: mobile/android/base/tests/BaseHelper.java.in @@ +38,5 @@ > + sDriver = sContext.getDriver(); > + sActions = sContext.getActions(); > + sAsserter = sContext.getAsserter(); > + > + ABOUTHOME = (AboutHomeComponent) sContext.getComponent(Component.ABOUTHOME); Just a quick note for the record: I originally tried to lazy load UI components so that the tests would only create instances for the components they actually use. The init bits of UI components seems light enough that it won't make a difference though. It should be fine I guess. ::: mobile/android/base/tests/CommonUseHelper.java.in @@ +11,5 @@ > + * > + * To prevent code crufting, this should not be a miscellaneous "dump everything!" class - new > + * Components should be freely created, even for lone methods. > + */ > +final class CommonUseHelper extends BaseHelper { Unused. Dump this class? ::: mobile/android/base/tests/DeviceHelper.java.in @@ +87,5 @@ > + public static int getScreenWidth() { > + return sScreenWidth; > + } > + > + public static PlatformVersion getAndroidVersion() { Either rename PlatformVersion to AndroidVersion or rename getAndroidVersion() to getPlatformVersion(). ::: mobile/android/base/tests/GeckoHelper.java.in @@ +23,5 @@ > + > + // TODO: Bug 709230: Remove reflection? > + final ClassLoader cl = sActivity.getClassLoader(); > + final Class geckoThreadClass = cl.loadClass(GECKO_THREAD_CLASS); > + final Class launchStateClass = cl.loadClass(LAUNCH_STATE_CLASS); nit: empty line here. ::: mobile/android/base/tests/GestureHelper.java.in @@ +18,5 @@ > + final int halfWidth = sDriver.getGeckoWidth() / 2; > + final int halfHeight = sDriver.getGeckoHeight() / 2; > + > + sSolo.drag(direction == Solo.LEFT ? halfWidth : 0, > + direction == Solo.LEFT ? 0 : halfWidth, nit: fix indentation? ::: mobile/android/base/tests/NavigationHelper.java.in @@ +19,5 @@ > + StringHelper.ROBOCOP_BLANK_PAGE_03_URL > + }; > + > + public static void enterAndLoadUrl(String url) { > + assertNotNull("Asserting url is not null", url); The 'Asserting...' in all assertion messages seems a bit redundant. I'd prefer to simply write 'URL is not null' here. Same applies to all other assertion messages. You can force the 'Asserting' prefix under the hood in AssertionHelper if you feel strongly about having this in the logs. @@ +22,5 @@ > + public static void enterAndLoadUrl(String url) { > + assertNotNull("Asserting url is not null", url); > + > + url = adjustIfPredefined(url); > + TOOLBAR.enterEditingMode().enterUrl(url).commitEditingMode(); Maybe this style is easier to read? TOOLBAR.enterEditing() .enterUrl(url) .commitEditingMode(); @@ +68,5 @@ > + > + WaitHelper.waitForPageLoad(new InitiatingAction() { > + @Override > + public void doAction() { > + // TODO: Press forward with APPMENU component The same applies to the tablet UI, right? ::: mobile/android/base/tests/ToolbarComponent.java.in @@ +85,5 @@ > + private ImageButton getForwardButton() { > + return (ImageButton) ViewHelper.findViewById(getToolbarView(), FORWARD_BUTTON_ID); > + } > + > + public CharSequence getTitle() { Doesn't need to be public, does it? @@ +90,5 @@ > + // TODO: Assert editing state? May break ToolbarTitleTextChangeVerifier. > + return getUrlTitleText().getText(); > + } > + > + public boolean isEditing() { Ditto. Doesn't need to be public. ::: mobile/android/base/tests/UITest.java.in @@ +93,5 @@ > + > + mBaseUrl = ((String) config.get("host")).replaceAll("(/$)", ""); > + mRawBaseUrl = ((String) config.get("rawhost")).replaceAll("(/$)", ""); > + > + initComponents(); // Helpers depend on components so initialize them first. // Helpers depend on components so initialize them first. initComponents(); initHelpers(); ;-) ::: mobile/android/base/tests/UITestContext.java.in @@ +29,5 @@ > + public String getAbsoluteRawUrl(final String url); > + > + public UITestComponent getComponent(final Component component); > + > + public static enum Component { Declare the enum before the getComponent() method? ::: mobile/android/base/tests/WaitHelper.java.in @@ +16,5 @@ > + * Performs the given action to start an event to be waited for. Implementations of this > + * interface are used in methods that need to check for changes in state from before the > + * given initiating action to after it. > + */ > + public static interface InitiatingAction { Wouldn't it be simpler to just stick with the Runnable interface instead? @@ +103,5 @@ > + > + @Override > + public boolean hasStateChanged() { > + // TODO: Efficiency? Should we naively cache the View? > + return !oldTitleText.equals(TOOLBAR.getTitle()); Not sure I follow. What's the value in checking the title simply 'changed'? What we usually care about is that the title changed to a specific state, no? What about the cases where the title is not expected to change?

(In reply to Lucas Rocha (:lucasr) from comment #32) > @@ +68,5 @@ > > + > > + WaitHelper.waitForPageLoad(new InitiatingAction() { > > + @Override > > + public void doAction() { > > + // TODO: Press forward with APPMENU component > > The same applies to the tablet UI, right? I don't believe so - the tablet UI can press the forward button directly. > > + public CharSequence getTitle() { > > Doesn't need to be public, does it? It does: it's used directly in WaitHelper.ToolbarTitleTextChangeVerifier. > Not sure I follow. What's the value in checking the title simply 'changed'? > What we usually care about is that the title changed to a specific state, > no? What about the cases where the title is not expected to change? The page load event, which we wait for in waitForPageLoad, is the same event that the browser waits for to change the title (and we later assert). Since this change happens asynchronously, we can't guarantee the title will have changed by the time we make our assertion. Thus, we either wait until the title changed (i.e. it's received and acted upon the page load event), in which case we can continue on to our assertion or there's a timeout. If there's a timeout, the title may never change (e.g. the same page has been loaded) and we're fine, or the title will eventually change, which means the assertion will regrettably fail, but I'm not sure how to better handle this. I think we can assume that for performance reasons, all the user facing post-page load events should take less than the default wait time (currently 2 seconds), but this isn't very reliable (e.g. when a machine is under heavy load). If this logic makes sense and we keep the code, I tried explain this behavior in waitForPageLoad so perhaps I need to better clarify the comments.

Updated for review comments. Major changes since the previous revision: * Removed pre-processing * Created components/ and helpers/ packages For a micro-commit diff, see [1]. Still TODO before this lands: * Test and fix changes for tablets * Fix potential race condition TODO * Either comment out the unimplemented sections of testNewTabHistory or implement them (I vote the former so we have more hands working on this). Concerns: * I have had testAboutHomeVisibility fail very infrequently on go to blank 01's assertTitle line (may have been an old build though) * If you don't import AssertionHelper.*, JUnit assertions take over instead without an error. Perhaps we should namespace our assertion methods (e.g. rcAssertTrue)? * lucas' concerns on the StateChangeVerifier

Comment on attachment 831240 [details] [diff] [review] Bootstrap new API for UI testing in Fennec. Review of attachment 831240 [details] [diff] [review]: ----------------------------------------------------------------- I think a lot of this is sensible and am thrilled to see it get love. I'm not as happy with some of the inheritance, and the line between Fennec and Robocop seems wrong to me, but this is a big step forward. Happy to review the build system pieces. ::: build/mobile/robocop/Makefile.in @@ +33,5 @@ > java-harness := $(addprefix $(srcdir)/,$(_JAVA_HARNESS)) > +java-tests := \ > + $(wildcard $(TESTPATH)/*.java) \ > + $(wildcard $(TESTPATH)/components/*.java) \ > + $(wildcard $(TESTPATH)/helpers/*.java) Urgh. This has my blessing, but still... urgh. ::: mobile/android/base/tests/StringHelper.java @@ +16,5 @@ > }; > public static final int DEFAULT_BOOKMARKS_COUNT = DEFAULT_BOOKMARKS_TITLES.length; > > // About pages > public static final String ABOUT_BLANK_URL = "about:blank"; Can we fish these out of Fennec's strings? This all seems a little fragile. @@ +30,3 @@ > // Context Menu menu items > public static final String[] CONTEXT_MENU_ITEMS_IN_PRIVATE_TAB = new String[] { > "Open Link in Private Tab", nit: kill trailing whitespace. ::: mobile/android/base/tests/UITest.java @@ +24,5 @@ > +import android.text.TextUtils; > + > +import java.util.HashMap; > + > +abstract class UITest extends ActivityInstrumentationTestCase2<Activity> Please include a nice comment explaining when this is appropriate and what it does for you. @@ +47,5 @@ > + private String mBaseUrl; > + // Base to build raw absolute URLs > + private String mRawBaseUrl; > + > + AboutHomeComponent ABOUTHOME; Uh, what? Why is this not scoped (private?) and why on earth are the names all caps? Is this somehow intended to fit with an enum somewhere? ::: mobile/android/base/tests/UITestContext.java @@ +32,5 @@ > + ABOUTHOME, > + TOOLBAR > + } > + > + public BaseComponent getComponent(final Component component); The fact that Component is an enum but BaseComponent is a class is confusing. ComponentType? ::: mobile/android/base/tests/components/AboutHomeComponent.java @@ +18,5 @@ > +import android.view.View; > + > +public class AboutHomeComponent extends BaseComponent { > + // TODO: Can we just import the enum directly? > + // This enum should be kept in sync with ...home.HomePager.Page. +1000 for this. This shouldn't land as is. @@ +26,5 @@ > + BOOKMARKS, > + READING_LIST > + } > + > + // TODO: Import R directly (nalexander said it can be done after preprocessing is rm). Yes. ::: mobile/android/base/tests/components/ToolbarComponent.java @@ +15,5 @@ > +import android.widget.EditText; > +import android.widget.ImageButton; > +import android.widget.TextView; > + > +public class ToolbarComponent extends BaseComponent { I am coming with little context, but I find the whole "Component" abstraction strange. Fennec has components, but the Robocop tests shouldn't. We're driving Fennec's components, right? Calling this thing a component seems wrong. It's a helper layer on top of whatever Fennec has. ::: mobile/android/base/tests/helpers/AssertionHelper.java @@ +6,5 @@ > + > +import org.mozilla.gecko.Assert; > + > +// TODO: Add ispixel assertions. > +public final class AssertionHelper extends BaseHelper { The Robocop framework includes an interface that does this. You're just passing through. What's the point? ::: mobile/android/base/tests/helpers/BaseHelper.java @@ +21,5 @@ > + protected static Activity sActivity; > + protected static Solo sSolo; > + protected static Driver sDriver; > + protected static Actions sActions; > + protected static Assert sAsserter; So, this BaseHelper class is really "test state". I have a strong preference for composition over inheritance; I'd much rather add a final state variable to all the helpers than have them all inherit from this class.

(In reply to Michael Comella (:mcomella) from comment #34) > Created attachment 831240 [details] [diff] [review] > Bootstrap new API for UI testing in Fennec. > > Updated for review comments. > > Major changes since the previous revision: > * Removed pre-processing > * Created components/ and helpers/ packages > > For a micro-commit diff, see [1]. > > Still TODO before this lands: > * Test and fix changes for tablets > * Fix potential race condition TODO > * Either comment out the unimplemented sections of testNewTabHistory or > implement them (I vote the former so we have more hands working on this). I'd simply remove any major pending bits before landing this. TODO comments are fine though. File follow-ups. > Concerns: > * I have had testAboutHomeVisibility fail very infrequently on go to blank > 01's assertTitle line (may have been an old build though) File a follow-up. > * If you don't import AssertionHelper.*, JUnit assertions take over instead > without an error. Perhaps we should namespace our assertion methods (e.g. > rcAssertTrue)? Namespacing would be unfortunately because the assert* calls wouldn't be as concise. If the signatures are exactly the same, maybe simply override the methods and redirect the calls to AssertionHelper.*? That's a bit prone to confusion though. > * lucas' concerns on the StateChangeVerifier The comment seems a bit clearer. We might want to add a StateChangeVerifier for the throbber visibility too.

bug 916507 adds some changes that allow us to import R and fennec apk classes directly.

(In reply to Nick Alexander :nalexander from comment #35) > Can we fish these out of Fennec's strings? This all seems a little fragile. bug 938845. Followup because StringHelper is used on more than just UITest and I'd like to avoid bit rot. > Uh, what? Why is this not scoped (private?) and why on earth are the names > all caps? Is this somehow intended to fit with an enum somewhere? The intent was to improve the visibility of these variables, but you're right that it's confusing and breaks convention. And yes, they should also be private so they're not assigned by a malicious helper! I'll change the naming convention and add getters. > I am coming with little context, but I find the whole "Component" > abstraction strange. Fennec has components, but the Robocop tests > shouldn't. We're driving Fennec's components, right? Calling this thing a > component seems wrong. It's a helper layer on top of whatever Fennec has. (Discussed via IRC). I'm leaning toward calling it "*ComponentHelper" but to avoid refactoring twice, I'll bring it up as an issue for the wider audience f?. (referencing AssertionHelper) > The Robocop framework includes an interface that does this. You're just > passing through. What's the point? Consistency with our eventual junit test suite (though as per comment 34, causes its own issues). Do you think it's not worthwhile? Side note that we should probably fix bug 928566 if we don't keep AssertionHelper.

(In reply to Lucas Rocha (:lucasr) from comment #36) > > Concerns: > > * I have had testAboutHomeVisibility fail very infrequently on go to blank > > 01's assertTitle line (may have been an old build though) > > File a follow-up. bug 938969. > Namespacing would be unfortunately because the assert* calls wouldn't be as > concise. If the signatures are exactly the same, maybe simply override the > methods and redirect the calls to AssertionHelper.*? That's a bit prone to > confusion though. Override the methods directly in UITest, you mean?

(Referencing component vars in UITest) > Uh, what? Why is this not scoped (private?) In testing, I found private access level is not ideal. If we set the vars private... 1) The tests have to be littered with "getAboutHomeComponent()" (or worse, currently (Cast) getComponent(ComponentType.TYPE)) 2) Every test has to set a local variable for each component 3) Move just the tests to a new package, subclass UITest in that package with a class that creates a protected variable for each component, and then have each test extend that subclass. What do you think? 3 doesn't seem too bad and we might want to put the UITests into a separate package anyway so it's obvious they're not the same.

> so it's obvious they're not the same. as BaseTests.

(In reply to Michael Comella (:mcomella) from comment #40) > (Referencing component vars in UITest) > > Uh, what? Why is this not scoped (private?) > > In testing, I found private access level is not ideal. If we set the vars > private... > 1) The tests have to be littered with "getAboutHomeComponent()" (or worse, > currently (Cast) getComponent(ComponentType.TYPE)) > 2) Every test has to set a local variable for each component > 3) Move just the tests to a new package, subclass UITest in that package > with a class that creates a protected variable for each component, and then > have each test extend that subclass. > > What do you think? 3 doesn't seem too bad and we might want to put the > UITests into a separate package anyway so it's obvious they're not the same. Wouldn't it be simpler to just have a protected variable in UITest itself?

> Wouldn't it be simpler to just have a protected variable in UITest itself? I was thinking the different components and helpers would be able to change this reference, but I just realized that since we pass the UITestContext interface into their constructors and init methods (rather than UITest directly), that access is not possible. Sure, I'm fine setting it protected then.

(In reply to Michael Comella (:mcomella) from comment #38) > (In reply to Nick Alexander :nalexander from comment #35) > > Can we fish these out of Fennec's strings? This all seems a little fragile. > > bug 938845. Followup because StringHelper is used on more than just UITest > and I'd like to avoid bit rot. Good. > > Uh, what? Why is this not scoped (private?) and why on earth are the names > > all caps? Is this somehow intended to fit with an enum somewhere? > > The intent was to improve the visibility of these variables, but you're > right that it's confusing and breaks convention. And yes, they should also > be private so they're not assigned by a malicious helper! I'll change the > naming convention and add getters. No need to add getters if they're static or final, and always prefer protected to private. (We're not worried about malicious anything.) By scoping I mean explicitly declaring the scope -- I can't recall if Java's default is private or public. > > I am coming with little context, but I find the whole "Component" > > abstraction strange. Fennec has components, but the Robocop tests > > shouldn't. We're driving Fennec's components, right? Calling this thing a > > component seems wrong. It's a helper layer on top of whatever Fennec has. > > (Discussed via IRC). I'm leaning toward calling it "*ComponentHelper" but to > avoid refactoring twice, I'll bring it up as an issue for the wider audience > f?. wfm. > (referencing AssertionHelper) > > The Robocop framework includes an interface that does this. You're just > > passing through. What's the point? > > Consistency with our eventual junit test suite (though as per comment 34, > causes its own issues). Do you think it's not worthwhile? Side note that we > should probably fix bug 928566 if we don't keep AssertionHelper. I'm 50/50 on this. The reason behind the is() stuff is that's the Mochitest precedent, and this stuff grew out of Mochitest. I'd say kill this code and just use the existing Assert implementations; if we then fix bug 928566, we'll have less layers to update.

Major changes in this revision: * Tested on tablets * Commented out test functionality that is not yet implemented * Import R and other fennec code directly * Remove reflection (where applicable) This is revision will get a wide feedback request. For those just tuning in, small things that still need to be completed or fixed are marked as TODOs, and larger issues are filed as dependent file-up bugs. If you are providing feedback, two major points of discussion that I'd like additional input on are: 1) Renaming *Component to be less ambiguous. nalexander, in comment 35 and via IRC, has mentioned that since Fennec does not have "Components", this is confusing (I partially agree). He suggested a better name might be *Helper, but this conflicts with our static helpers (currently in the helpers/ package). *Driver was also proposed, but that conflicts with the FennecNativeDriver (and I feel the "Driver" term is ambiguous anyway). Thus *ComponentHelper and *ComponentDriver were also proposed but these still use "Component" and don't add much over using it in isolation. 2) Removing AssertionHelper in favor of using mAsserter directly. AssertionHelper currently wraps the mAsserter to create a JUnit-like API (to allow consistency with our eventual JUnit test suite) and allows us to access the assertions statically. However, it doesn't add much else over using mAsserter directly, requires an additional maintenance cost, and can cause some confusion if not imported as the actual JUnit methods, which have the same method signatures, are used instead (see comment 34). Provided the two issues above are resolved and there is no major pushback after feedback, this is ready to land. I'll be waiting at least a week for feedback before requesting the final review (again, provided no major pushback). Drive-bys welcome!

Comment on attachment 833095 [details] [diff] [review] Bootstrap new API for UI testing in Fennec. Requesting feedback from the original requestees and anyone who has previously given feedback. See comment 45 for a rundown on the outstanding issues and the state of the patch. For a micro-commit diff from lucas' original patch, see https://github.com/mcomella/new-ui-testing/compare/03 Be wary that I didn't try to keep the commits in a reasonable state for reviews - it was more for my purposes.

Comment on attachment 833095 [details] [diff] [review] Bootstrap new API for UI testing in Fennec. Review of attachment 833095 [details] [diff] [review]: ----------------------------------------------------------------- I gave this a cursory once-over. It looks pretty nice! Some random comments below. ::: mobile/android/base/tests/helpers/AssertionHelper.java @@ +25,5 @@ > + sAsserter.is(actual, expected, message); > + } > + > + public static void assertFalse(final String message, final boolean actual) { > + // TODO: What is diag? (Also assertTrue) AFAIK "diag" stands for "diagnostic" but doesn't really have any useful purpose. ::: mobile/android/base/tests/helpers/DeviceHelper.java @@ +16,5 @@ > +import android.os.Build; > +import android.util.DisplayMetrics; > + > +public final class DeviceHelper { > + private static final String APP_SHELL_CLASS = "org.mozilla.gecko.GeckoAppShell"; unused? ::: mobile/android/base/tests/robocop.ini @@ +67,5 @@ > #[testCheck2] > #[testBrowserProviderPerf] > +[testAboutHomeVisibility] > +[testNewAboutHomeSwipes] > +[testNewTabHistory] Add a comment above these new tests to indicate that they are using the new testing framework. Also probably leave a blank line between them and the talos ones. ::: mobile/android/base/tests/testNewAboutHomeSwipes.java @@ +7,5 @@ > + > +// TODO: Elaborate on test desc and use /**. > +/* Test correct state for URL bar after loading pages. > + */ > +public class testNewAboutHomeSwipes extends UITest { I don't think we'll be calling this "new" about:home forever. You should probably rename this to just "testAboutHomeSwipes"

Comment on attachment 833095 [details] [diff] [review] Bootstrap new API for UI testing in Fennec. Review of attachment 833095 [details] [diff] [review]: ----------------------------------------------------------------- So this is looking really good, and bikeshedding is inevitable, but... why are the Helper classes static? It seems like you could follow the existing model (with Assert and Driver, etc) and just have a DeviceHelper instance owned by, say, UITest or UITestContext. But I'm not going to block on this. If it passes try, r=nalexander if you want it and lucasr agrees. ::: mobile/android/base/tests/helpers/WaitHelper.java @@ +14,5 @@ > + > +import com.jayway.android.robotium.solo.Condition; > +import com.jayway.android.robotium.solo.Solo; > + > +public final class WaitHelper { class comments -- even just a line or two -- really help. ::: mobile/android/base/tests/testNewAboutHomeSwipes.java @@ +43,5 @@ > + mAboutHome.assertCurrentPage(Page.HISTORY); > + > + mAboutHome.swipeToPageOnRight(); > + mAboutHome.assertCurrentPage(Page.TOP_SITES); > + } else { nit: we're allowed multiple test* functions, right? Can we split this into two tests, one tablet, one not? And then just early return if the is* test fails?

Comment on attachment 833095 [details] [diff] [review] Bootstrap new API for UI testing in Fennec. Review of attachment 833095 [details] [diff] [review]: ----------------------------------------------------------------- This framework allows you to write clean, simple looking tests: NavigationHelper.enterAndLoadUrl(StringHelper.ROBOCOP_BLANK_PAGE_02_URL); mToolbar.assertTitle(StringHelper.ROBOCOP_BLANK_PAGE_02_TITLE); mAboutHome.assertNotVisible(); I love that...and I am troubled by it. The biggest problem with the existing robocop tests is that we forget to wait, or we wait ineffectively, between action and result. It is natural to think this way: I click here, I expect this to happen. But really, you click here, time passes, and then a new UI state is achieved. Forget to wait, or wait ineffectively, and yet another intermittent failure is born. The "wait" is built into enterAndLoadUrl and I assume it is effective. But writing tests this way reinforces that natural tendency to forget about the middle step. As I said in Comment 11, I would prefer to see explicit waits, perhaps: NavigationHelper.enterAndLoadUrl(StringHelper.ROBOCOP_BLANK_PAGE_02_URL); NavigationHelper.waitForPageToLoad(); mToolbar.assertTitle(StringHelper.ROBOCOP_BLANK_PAGE_02_TITLE); mAboutHome.assertNotVisible(); That's more code. And there's a risk that the waitForPageToLoad call will be accidentally dropped in some future use. But it reminds us of what is really happening and the way we need to be thinking when we write UI tests. Are there try runs for these new tests? ::: mobile/android/base/tests/StringHelper.java @@ +1,5 @@ > package org.mozilla.gecko.tests; > > import org.mozilla.gecko.*; > > +public class StringHelper { Can we state a rule here for when to include a string in StringHelper? All test strings are defined here? All strings shared between 2 or more tests? ...? ::: mobile/android/base/tests/UITest.java @@ +28,5 @@ > +/** > + * A base test class for Robocop (UI-centric) tests. This and the related classes attempt to > + * provide a framework to improve upon the issues discovered with the previous BaseTest > + * implementation by providing simple test authorship and framework extension, consistency, > + * and reliability. Are there changes here that improve reliability? ::: mobile/android/base/tests/helpers/NavigationHelper.java @@ +50,5 @@ > + private static String adjustIfPredefined(final String url) { > + assertNotNull("url is not null", url); > + > + for (final String predefinedUrl : PREDEFINED_URLS) { > + if (TextUtils.equals(url, predefinedUrl)) { As more tests use more test pages, we'll need to keep updating PREDEFINED_URLS. When would we not use getAbsoluteUrl? @@ +87,5 @@ > + WaitHelper.waitForPageLoad(new Runnable() { > + @Override > + public void run() { > + // TODO: Press forward with APPMENU component > + throw new UnsupportedOperationException("Not yet implemented."); This seems like basic functionality that should be available in the initial version. ::: mobile/android/base/tests/helpers/WaitHelper.java @@ +46,5 @@ > + * AssertionError if the duration is elapsed and the condition is not satisfied. > + */ > + public static void waitFor(String message, final Condition condition) { > + message = "Waiting for " + message + "."; > + assertTrue(message, sSolo.waitForCondition(condition, DEFAULT_MAX_WAIT_MS)); BaseTest.waitForCondition does not assert on timeout. Callers of BaseTest.waitForCondition frequently assert on the waitForCondition return code, but not always. Is this asserting a best practice, or will it prove overly restrictive, I wonder? @@ +94,5 @@ > + }, CHANGE_WAIT_MS); > + // TODO: Make wait time an aggregated countdown? > + > + if (hasTimedOut) { > + sContext.dumpLog(verifier.getClass().getName() + " timed out."); It looks like here is a case where we cannot use the waitFor() helper because assertion is not desired. ::: mobile/android/base/tests/robocop.ini @@ +67,5 @@ > #[testCheck2] > #[testBrowserProviderPerf] > +[testAboutHomeVisibility] > +[testNewAboutHomeSwipes] > +[testNewTabHistory] My preference would be to mix them in with the list of other tests, now ordered alphabetically. ::: mobile/android/base/tests/testNewAboutHomeSwipes.java @@ +7,5 @@ > + > +// TODO: Elaborate on test desc and use /**. > +/* Test correct state for URL bar after loading pages. > + */ > +public class testNewAboutHomeSwipes extends UITest { +1 @@ +11,5 @@ > +public class testNewAboutHomeSwipes extends UITest { > + // TODO: Define this test dynamically by creating dynamic representations of the Page > + // enum for both phone and tablet, then swiping through the pages. This will also > + // benefit having a HomePager with custom pages. > + public void testNewAboutHomeSwipes() { This test seems similar to testAwesomebarSwipes. Could we remove testAwesomebarSwipes? Each test takes time to execute, and I hate to see the same functionality tested twice. testAwesomebarSwipes is currently disabled, but even so, we should delete it so that no one wastes their time debugging and re-enabling a test that would just be redundant. ::: mobile/android/base/tests/testNewTabHistory.java @@ +9,5 @@ > +/* Test correct state for URL bar after loading pages. > + */ > +public class testNewTabHistory extends UITest { > + public void testNewTabHistory() { > + GeckoHelper.blockForReady(); Again, this is really the same as testTabHistory (also disabled) -- we should never run both.

(In reply to Geoff Brown [:gbrown] from comment #49) > > +public class testNewAboutHomeSwipes extends UITest { > > +1 Sorry, context was lost here. I was agreeing with kats -- let's call this testAboutHomeSwipes.

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #47) > I don't think we'll be calling this "new" about:home forever. You should > probably rename this to just "testAboutHomeSwipes" To avoid confusion with the BaseTest testAwesomebarSwipes, I'll leave it as testNewAboutHomeSwipes for now, and add the following TODO: // TODO: Rename to testAboutHomeSwipes (or more inclusive name), add changes from testAwesomebarSwipes, and rm it.

(In reply to Geoff Brown [:gbrown] from comment #49) > testAwesomebarSwipes is currently disabled, > but even so, we should delete it so that no one wastes their time debugging > and re-enabling a test that would just be redundant. Ah, I did not realize this! I will add the commented out code to testNewAboutHomeSwipes and rename it instead of what I said in comment 51.

Comment on attachment 833095 [details] [diff] [review] Bootstrap new API for UI testing in Fennec. Review of attachment 833095 [details] [diff] [review]: ----------------------------------------------------------------- I didn't go through everything in detail, but overall I'm really liking the look of this! I really like how this breaks common routines out into helper classes/methods. ::: mobile/android/base/tests/StringHelper.java @@ +1,5 @@ > package org.mozilla.gecko.tests; > > import org.mozilla.gecko.*; > > +public class StringHelper { I like the idea of making a rule to just include all strings in here. It would be consistent, and it would also reduce the chance of accidentally declaring the same string in 2 different tests. Also, when changing a string in the UI, we only need to update this file, rather than a random set of tests. ::: mobile/android/base/tests/UITest.java @@ +103,5 @@ > + mRawBaseUrl = ((String) config.get("rawhost")).replaceAll("(/$)", ""); > + > + // Helpers depend on components so initialize them first. > + initComponents(); > + initHelpers(); Right now it doesn't seem like there's much overhead to initializing all these things, but in the future should we consider some sort of lazy initialization? Especially since a lot of tests will extend UITest, but they're not all going to use all of these components/helpers. @@ +187,5 @@ > + return null; // Should not reach this statement but required by javac. > + } > + } > + > + protected Type getTestType() { Since this just confused me, could you add a comment explaining that MOCHITEST is the default, but tests can override this to create TALOS tests? @@ +197,5 @@ > + return mBaseUrl + "/" + url.replaceAll("(^/)", ""); > + } > + > + @Override > + public String getAbsoluteRawUrl(final String url) { Can you add comments about the different between this an getAbsoluteUrl? Looking at mBaseUrl and mRawBaseUrl, it's not clear what the difference is. ::: mobile/android/base/tests/components/AboutHomeComponent.java @@ +35,5 @@ > + if (DeviceHelper.isTablet()) { > + // Left circular shift Page enum since the History tab is moved to the rightmost. > + expectedPageIndex -= 1; > + expectedPageIndex = > + (expectedPageIndex >= 0) ? expectedPageIndex : Page.values().length - 1; I don't love this dependency on the Page enum. I may be biased because my WIP in bug 862805 adds a type to the Page enum which isn't necessarily always present, but perhaps it would be better to be explicit about the order of pages we expect in phone vs. tablet by creating some new data structure in this test. @@ +58,5 @@ > + return this; > + } > + > + // TODO: Take specific page as parameter rather than swipe in a direction? > + public AboutHomeComponent swipeToPageOnRight() { Even though this actually swipes to the left, I like this method name because it saves me one brain cycle to figure out what's going on. ::: mobile/android/base/tests/helpers/AssertionHelper.java @@ +7,5 @@ > +import org.mozilla.gecko.Assert; > +import org.mozilla.gecko.tests.UITestContext; > + > +// TODO: Add ispixel assertions. > +public final class AssertionHelper { I love this class. @@ +25,5 @@ > + sAsserter.is(actual, expected, message); > + } > + > + public static void assertFalse(final String message, final boolean actual) { > + // TODO: What is diag? (Also assertTrue) This has also confused me when writing assertions. I love the idea of just having one message. ::: mobile/android/base/tests/helpers/GeckoHelper.java @@ +11,5 @@ > +import org.mozilla.gecko.tests.UITestContext; > + > +import android.app.Activity; > + > +public final class GeckoHelper { Add some documentation about what this class is for. The class name doesn't make it 100% clear. ::: mobile/android/base/tests/helpers/NavigationHelper.java @@ +96,5 @@ > + public static void reload() { > + // TODO: On tablets, press reload in TOOLBAR. Note that this is technically > + // an app menu item so tread carefully. > + // On phones, press reload in APPMENU. > + throw new UnsupportedOperationException("Not yet implemented."); Maybe it breaks our black-box testing model, but I wonder if we should just send the appropriate "Session:Reload"/etc messages to gecko, rather than try clicking on buttons/menu items.

(In reply to Nick Alexander :nalexander from comment #48) > why are the Helper classes static? It allows the classes to be more easily accessed from everywhere with less verbose code (e.g. no getters, no additional variables, etc.). Since we're not running these tests in parallel (and not likely to do so soon because they're UI-based), there does not appear to be any negatives associated with this approach (besides maybe initialization, which is already noted with a comment). Would you agree?

(In reply to Michael Comella (:mcomella) from comment #54) > (In reply to Nick Alexander :nalexander from comment #48) > > why are the Helper classes static? > > It allows the classes to be more easily accessed from everywhere with less > verbose code (e.g. no getters, no additional variables, etc.). Since we're > not running these tests in parallel (and not likely to do so soon because > they're UI-based), there does not appear to be any negatives associated with > this approach (besides maybe initialization, which is already noted with a > comment). Would you agree? The thing is that every Robocop test case has to inherit from IForgetTheRealNameInstrumentationTestCase2 or similar. So there's this strong inheritance requirement, which means that all this stuff can be kept in scope easily. Roll on, but I bet we end up making at least one of these helpers configurable.

(In reply to Geoff Brown [:gbrown] from comment #49) > The biggest problem with the existing robocop tests is that we forget to > wait, or we wait ineffectively, between action and result. It is natural to > think this way: I click here, I expect this to happen. But really, you click > here, time passes, and then a new UI state is achieved. Forget to wait, or > wait ineffectively, and yet another intermittent failure is born. With this rewrite, I intend for the framework to take care of the necessary waiting so it is centralized. If an intermittent occurs, there's one place to fix it. Yes, it's harder to remember to insert these waits if they're written less frequently or don't follow a convention (like you mentioned), but at the same time, if you're updating framework code, you're probably going to be a little more careful. Since I know that's an issue, I'm planning on accompanying this framework with wiki documentation with a big note that will say, "Don't forget to wait!". > The "wait" is built into enterAndLoadUrl and I assume it is effective. But > writing tests this way reinforces that natural tendency to forget about the > middle step. I would argue that's fine - developers writing tests shouldn't need to worry about this. Developers writing the framework do, but the test writers shouldn't have to suffer so the framework developers can better remember to wait. I think there's a possibility for greater error where developers writing tests also need to be concerned with *what* to wait for (e.g. "Is it *really* only page load I should be waiting for here? Do I need to wait for the toolbar to hide independently?"). > Are there try runs for these new tests? Good call! With some of the most recent review changes: https://tbpl.mozilla.org/?tree=Try&rev=0941975ec24d > Are there changes here that improve reliability? A lot of the functionality that was often copy and pasted between tests previously is encapsulated in the framework which will (hopefully) improve reliability. For example, see testAwesomebarSwipes vs testNewAboutHomeSwipes (testAwesomescreenPages in the next patch revision). > This seems like basic functionality that should be available in the initial > version. To be fair, we could say that for a lot of the functionality that the framework could provide in the future - I think it's more important to land what we have so I'm not the only one working on it. > BaseTest.waitForCondition does not assert on timeout. Callers of > BaseTest.waitForCondition frequently assert on the waitForCondition return > code, but not always. Is this asserting a best practice, or will it prove > overly restrictive, I wonder? I didn't fully consider that before. I moved the assertions over to WaitHelper to avoid copious amounts of "assertTrue(nearlyIdenticalMessage, ...)". However, since the functionality is easily accessible directly with sSolo.waitForCondition, I don't think this is a big issue. Perhaps I should make `WaitHelper.DEFAULT_MAX_WAIT_MS` public though? > My preference would be to mix them in with the list of other tests, now > ordered alphabetically. I agree - I'll change it.

(In reply to Nick Alexander :nalexander from comment #55) > The thing is that every Robocop test case has to inherit from > IForgetTheRealNameInstrumentationTestCase2 or similar. So there's this > strong inheritance requirement, which means that all this stuff can be kept > in scope easily. Roll on, but I bet we end up making at least one of these > helpers configurable. This is true for `... extends UITest` but what about when helpers need to access each other? Since you cannot do this reference passing at construction time (since they rely on other, not-yet constructed helpers), the code would need quite a few sContext.get*; the static approach seemed cleaner (if we can get away with it ;).

(In reply to Michael Comella (:mcomella) from comment #57) > (In reply to Nick Alexander :nalexander from comment #55) > > The thing is that every Robocop test case has to inherit from > > IForgetTheRealNameInstrumentationTestCase2 or similar. So there's this > > strong inheritance requirement, which means that all this stuff can be kept > > in scope easily. Roll on, but I bet we end up making at least one of these > > helpers configurable. > > This is true for `... extends UITest` but what about when helpers need to > access each other? Since you cannot do this reference passing at > construction time (since they rely on other, not-yet constructed helpers), > the code would need quite a few sContext.get*; the static approach seemed > cleaner (if we can get away with it ;). Good point. Roll on.

(In reply to :Margaret Leibovic from comment #53) > I don't love this dependency on the Page enum. I may be biased because my > WIP in bug 862805 adds a type to the Page enum which isn't necessarily > always present, but perhaps it would be better to be explicit about the > order of pages we expect in phone vs. tablet by creating some new data > structure in this test. It seems we've interpreted the use of the Page enum differently - I have been using it as an explicit representation of the order of the pages in the awesomescreen whereas I think you're using it as page type. I agree with your usage - perhaps it should be renamed `PageType` in bug 862805 (or a followup to this)? In this case, I suppose it's okay to make an explicit recreation of the Page enum for each configuration (imo, it would otherwise be bad to duplicate such info). I'll change it. > Maybe it breaks our black-box testing model, but I wonder if we should just > send the appropriate "Session:Reload"/etc messages to gecko, rather than try > clicking on buttons/menu items. What benefit do you see with using the Gecko messages? I think it's generally better to press buttons - we're both testing that the UI works as expected and that the underlying functionality displays the result correctly.

(In reply to Michael Comella (:mcomella) from comment #59) > > Maybe it breaks our black-box testing model, but I wonder if we should just > > send the appropriate "Session:Reload"/etc messages to gecko, rather than try > > clicking on buttons/menu items. > > What benefit do you see with using the Gecko messages? I think it's > generally better to press buttons - we're both testing that the UI works as > expected and that the underlying functionality displays the result correctly. You woudln't have to worry about actually getting the views to click, which can be different in phones/tablets. It seems like it's a non-trivial amount of effort, since you left some of these unimplemented ;)

> You woudln't have to worry about actually getting the views to click, True. If we need this functionality quickly, it sounds like it can be a temporary solution until we can implement the tapping methodology.

(In reply to Geoff Brown [:gbrown] from comment #49) > As more tests use more test pages, we'll need to keep updating > PREDEFINED_URLS. When would we not use getAbsoluteUrl? Good point - I'll replace adjustIfPredefined with adjustUrl that will blindly change the url. We can later add a boolean flag to disable this functionality if we need it.

(In reply to :Margaret Leibovic from comment #53) > Can you add comments about the different between this an getAbsoluteUrl? > Looking at mBaseUrl and mRawBaseUrl, it's not clear what the difference is. Honestly, I'm not sure what the difference is; I think it was originally carried over from BaseTest. Lucas, do you know?

(In reply to Geoff Brown [:gbrown] from comment #49) > Can we state a rule here for when to include a string in StringHelper? All > test strings are defined here? All strings shared between 2 or more tests? I think anything that could potentially be used among multiple files (e.g. urls, page titles, button labels, etc.) should be put here, but anything else should be put into their most appropriate file - it's more or less a judgement call. For example, `LAUNCHER_ACTIVITY = TestConstants.ANDROID_PACKAGE_NAME + ".App" won't be used anywhere besides UITest (and maybe the subclasses) so it's most appropriately defined in UITest. I dislike the idea of putting all strings in StringHelper because, while it's easy to use when the file is small, it becomes difficult when the size grows. It's better to keep the Strings sectioned on where they're used (which is roughly the same reason all of the code isn't contained in one file).

(In reply to Michael Comella (:mcomella) from comment #64) > (In reply to Geoff Brown [:gbrown] from comment #49) > > Can we state a rule here for when to include a string in StringHelper? All > > test strings are defined here? All strings shared between 2 or more tests? > > I think anything that could potentially be used among multiple files (e.g. > urls, page titles, button labels, etc.) should be put here, but anything > else should be put into their most appropriate file - it's more or less a > judgement call. > > For example, `LAUNCHER_ACTIVITY = TestConstants.ANDROID_PACKAGE_NAME + > ".App" won't be used anywhere besides UITest (and maybe the subclasses) so > it's most appropriately defined in UITest. To clarify, I thought StringHelper was about strings that are exposed in the UI, so I think it's fine to exclude things like the app name. > I dislike the idea of putting all strings in StringHelper because, while > it's easy to use when the file is small, it becomes difficult when the size > grows. It's better to keep the Strings sectioned on where they're used > (which is roughly the same reason all of the code isn't contained in one > file). It sounds like bug 938845 will fix this problem for us anyway, right?

(In reply to Michael Comella (:mcomella) from comment #63) > (In reply to :Margaret Leibovic from comment #53) > > Can you add comments about the different between this an getAbsoluteUrl? > > Looking at mBaseUrl and mRawBaseUrl, it's not clear what the difference is. > > Honestly, I'm not sure what the difference is; I think it was originally > carried over from BaseTest. Lucas, do you know? I simply moved this over from the old code. Geoff? :-)

Comment on attachment 833095 [details] [diff] [review] Bootstrap new API for UI testing in Fennec. Review of attachment 833095 [details] [diff] [review]: ----------------------------------------------------------------- You've got a lot of good feedback already ;-) Looks good to me. ::: mobile/android/base/tests/components/ToolbarComponent.java @@ +92,5 @@ > + public CharSequence getPotentiallyInconsistentTitle() { > + return getTitleHelper(false); > + } > + > + private CharSequence getTitleHelper(final boolean shouldAssertNotEditing) { The boolean argument is kinda hard to read. And the 'getTitleHelper' method name is a bit vague. I'd go with getTitleText() that just does: getTitleText() { return getUrlTitleText().getText(); } And then: getTitle() { assertIsNotEditing(); return getTitleText(); } getPotentiallyInconsistentTitle() { return getTitleText(); } Or something along these lines.

(In reply to Lucas Rocha (:lucasr) from comment #66) > (In reply to Michael Comella (:mcomella) from comment #63) > > (In reply to :Margaret Leibovic from comment #53) > > > Can you add comments about the different between this an getAbsoluteUrl? > > > Looking at mBaseUrl and mRawBaseUrl, it's not clear what the difference is. > > > > Honestly, I'm not sure what the difference is; I think it was originally > > carried over from BaseTest. Lucas, do you know? > > I simply moved this over from the old code. Geoff? :-) bnicholson added this for testSearchSuggestions, in bug 586885. I believe the difference is the difference between "host" and "rawhost" here: https://hg.mozilla.org/mozilla-central/annotate/597287004ff5/testing/mochitest/runtestsremote.py#l503, which should be using the IP address in the rawhost vs the proxy name "mochi.test" in host.

try from comment 56 has a bug where the title is updated to the url (e.g. "http://...") before it gets the page title from the server, fooling the ToolbarTitleTextChangeVerifier into thinking the page title changed. I added a check to prevent return true on hasStateChanged for urls with protocol prefixes. Here is my new push: https://tbpl.mozilla.org/?tree=Try&rev=ec526271cfdf

The issue in comment 69 may be the same as in bug 915350 so if this works, we might want to port this solution over or move those tests to UITest when possible.

Comment on attachment 833095 [details] [diff] [review] Bootstrap new API for UI testing in Fennec. Review of attachment 833095 [details] [diff] [review]: ----------------------------------------------------------------- Clearing review flag because I'm going on PTO.

Comment on attachment 833095 [details] [diff] [review] Bootstrap new API for UI testing in Fennec. Review of attachment 833095 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/tests/StringHelper.java @@ +1,5 @@ > package org.mozilla.gecko.tests; > > import org.mozilla.gecko.*; > > +public class StringHelper { Maybe this needs more discussion, possibly in a separate bug. There are people that like this class and there are people that consider this class not to be ok because strings without logical links are all grouped here. I think everyone should be on the same page with this class if it needs to be here or not and if not how things like common strings between tests should be addressed in order to have consistency over tests, allow access to this strings from everywhere and make it easy to change something if a string is changed ::: mobile/android/base/tests/robocop.ini @@ +67,5 @@ > #[testCheck2] > #[testBrowserProviderPerf] > +[testAboutHomeVisibility] > +[testNewAboutHomeSwipes] > +[testNewTabHistory] If these get mixed between the old ones at some point as tests are being re-written in the new framework it will get hard to track which test has been changed and which has not ::: mobile/android/base/tests/testAboutHomeVisibility.java @@ +18,5 @@ > + .assertCurrentPage(Page.TOP_SITES); > + > + // Go to blank 01. > + NavigationHelper.enterAndLoadUrl(StringHelper.ROBOCOP_BLANK_PAGE_01_URL); > + mToolbar.assertTitle(StringHelper.ROBOCOP_BLANK_PAGE_01_TITLE); Since more test pages have been added people usually named them Robocop <what the page is for or a page description> (for e.g Robocop Big Link, Robocop Mailto) these 3 bank pages remain named "Browser Blank 0x". Could this as a convention be renamed to "Robocop Blank 0x"? I think it would be nice to have a naming convention for the page titles.

(In reply to Adrian Tamas (:AdrianT) from comment #72) > ::: mobile/android/base/tests/robocop.ini > @@ +67,5 @@ > > #[testCheck2] > > #[testBrowserProviderPerf] > > +[testAboutHomeVisibility] > > +[testNewAboutHomeSwipes] > > +[testNewTabHistory] > > If these get mixed between the old ones at some point as tests are being > re-written in the new framework it will get hard to track which test has > been changed and which has not FWIW, I agree with Adrian here.

(In reply to Adrian Tamas (:AdrianT) from comment #72) referencing StringHelper: > Maybe this needs more discussion, possibly in a separate bug. bug 943062 > ::: mobile/android/base/tests/robocop.ini > @@ +67,5 @@ > > #[testCheck2] > > #[testBrowserProviderPerf] > > +[testAboutHomeVisibility] > > +[testNewAboutHomeSwipes] > > +[testNewTabHistory] > > If these get mixed between the old ones at some point as tests are being > re-written in the new framework it will get hard to track which test has > been changed and which has not I agree - I've moved them to a separate section at the bottom. > I think it would be > nice to have a naming convention for the page titles. bug 943063 (I didn't add these constants so I think it's better to handle in a standalone bug).

Review revision. Made changes for feedback comments, removed some unneeded TODOs, fixed a bug (hackishly) in the WaitHelper.ToolbarTextChangeVerifier class, removed testAwesomebarSwipes and testTabHistory, and renamed the tests (testAboutHomeVisibility, testAwesomescreenPages, and testTabHistory). For incremental changes, please see [1]. Drive-bys welcome. [1]: https://github.com/mcomella/new-ui-testing/compare/03...04

Note that I occasionally get intermittents in testAwesomescreenPages when my device lags and the swipe is not registered - I assume this is because my device is often inconsistent and crappy, however. I have had no problems when testing on a Nexus 4 or a TF101.

Comment on attachment 8338070 [details] [diff] [review] Bootstrap new API for UI testing in Fennec. Review of attachment 8338070 [details] [diff] [review]: ----------------------------------------------------------------- Awesome. Let's land this and go from there. ps: follow-up bugs are more important than the TODO comments. Make sure you file a follow-up bug for every TODO comment that definitely needs fixing in the short-term. ::: mobile/android/base/tests/StringHelper.java @@ +30,5 @@ > // Context Menu menu items > public static final String[] CONTEXT_MENU_ITEMS_IN_PRIVATE_TAB = new String[] { > + "Open Link in Private Tab", > + "Copy Link", > + "Share Link", For future patches, I'd remove trailing spaces in a separate changeset to avoid adding noise to your patch. ::: mobile/android/base/tests/UITest.java @@ +31,5 @@ > + * implementation by providing simple test authorship and framework extension, consistency, > + * and reliability. > + */ > +abstract class UITest extends ActivityInstrumentationTestCase2<Activity> > + implements UITestContext { nit: add empty line here. @@ +58,5 @@ > + protected ToolbarComponent mToolbar; > + > + static { > + try { > + sLauncherActivityClass = (Class<Activity>) Class.forName(LAUNCHER_ACTIVITY); Just wondering: does this actually need to be static? @@ +79,5 @@ > + setActivityIntent(intent); > + > + // Start the activity > + // TODO: Fix. getActivity() returns null so I call launchActivityWithIntent explicitly. > + // Perhaps this is because the class' type parameter is "Activity"? It returns null because we have overwritten the getActivity() method to simply return mActivity, which is null at this point. The getActivity() from the parent class calls launchActivityWithIntent() under the hood. Either just let the parent class handle that through its getActivity() method (if possible) or remove the TODO (as this seems to be working as intended). Up to you. @@ +101,5 @@ > + > + mBaseHostnameUrl = ((String) config.get("host")).replaceAll("(/$)", ""); > + mBaseIpUrl = ((String) config.get("rawhost")).replaceAll("(/$)", ""); > + > + // TODO: Consider lazy initialization. I'd remove this TODO comment. File a follow-up to track it. IMO, we should only care about this if it makes tests run a lot faster. @@ +125,5 @@ > + mToolbar = new ToolbarComponent(this); > + } > + > + private void initHelpers() { > + // TODO: Classes import AssertionHelper.init statically. If we keep AssertionHelper, Which classes import AssertionHelper.init statically? @@ +176,5 @@ > + } > + > + @Override > + public BaseComponent getComponent(final ComponentType type) { > + switch (type) { nit: I *think* we indent switch blocks differently in our code. @@ +178,5 @@ > + @Override > + public BaseComponent getComponent(final ComponentType type) { > + switch (type) { > + case ABOUTHOME: > + return mAboutHome; nit: add empty line here. @@ +197,5 @@ > + return Type.MOCHITEST; > + } > + > + @Override > + public String getAbsoluteHostnameUrl(final String url) { url doesn't need to be final. @@ +202,5 @@ > + return getAbsoluteUrl(mBaseHostnameUrl, url); > + } > + > + @Override > + public String getAbsoluteIpUrl(final String url) { url doesn't need to be final. @@ +206,5 @@ > + public String getAbsoluteIpUrl(final String url) { > + return getAbsoluteUrl(mBaseIpUrl, url); > + } > + > + private String getAbsoluteUrl(final String baseUrl, final String url) { baseUrl and url don't need to be final. ::: mobile/android/base/tests/UITestContext.java @@ +30,5 @@ > + > + /** > + * Returns the absolute version of the given URL using the host's hostname. > + */ > + public String getAbsoluteHostnameUrl(final String url); nit: empty line here. @@ +36,5 @@ > + * Returns the absolute version of the given URL using the host's IP address. > + */ > + public String getAbsoluteIpUrl(final String url); > + > + public static enum ComponentType { nit: I tend to prefer being consistent and always declare public types at the top of the class/interface. Not a big deal though. ::: mobile/android/base/tests/components/AboutHomeComponent.java @@ +35,5 @@ > + assertVisible(); > + > + // TODO: A "PhonePage" and "TabletPage" enum should be set explicitly or decided > + // dynamically, likely with the work done in bug 940565. The current solution kind of > + // sucks. I'd avoid the "kinda of sucks" part. "This is a temporary solution" would probably do it :-) @@ +45,5 @@ > + (expectedPageIndex >= 0) ? expectedPageIndex : Page.values().length - 1; > + } > + > + assertEquals("The current HomePager page is " + Page.values()[expectedPageIndex], > + expectedPageIndex, getHomePagerView().getCurrentItem()); nit: empty line here. @@ +51,5 @@ > + } > + > + public AboutHomeComponent assertNotVisible() { > + assertFalse("The HomePager is not visible", > + getHomePagerView().getVisibility() == View.VISIBLE); nit: not sure this is just on the review tool but I'd try to align the getHomePagerView() line with the first argument above. Same for all other calls. Add empty line here. @@ +57,5 @@ > + } > + > + public AboutHomeComponent assertVisible() { > + assertEquals("The HomePager is visible", > + View.VISIBLE, getHomePagerView().getVisibility()); nit: add empty line here. ::: mobile/android/base/tests/components/ToolbarComponent.java @@ +34,5 @@ > + assertFalse("The toolbar is not in the editing state", isEditing()); > + return this; > + } > + > + public ToolbarComponent assertTitle(final String expected) { No need to be final. @@ +39,5 @@ > + assertEquals("The Toolbar title is " + expected, expected, getTitle()); > + return this; > + } > + > + public ToolbarComponent assertUrl(final String expected) { No need to be final. @@ +94,5 @@ > + public CharSequence getPotentiallyInconsistentTitle() { > + return getTitleHelper(false); > + } > + > + private CharSequence getTitleHelper(final boolean shouldAssertNotEditing) { Noticed you didn't cover my previous comment on this method. Do you feel strongly about it? I don't, so feel free to keep it. Just checking if you kept it intentionally or just because you missed my comment before. @@ +97,5 @@ > + > + private CharSequence getTitleHelper(final boolean shouldAssertNotEditing) { > + if (shouldAssertNotEditing) { > + assertIsNotEditing(); > + } nit: empty line here. @@ +153,5 @@ > + > + return this; > + } > + > + public ToolbarComponent enterUrl(final String url) { Ok, it seems the 'final' in the arguments is very intentional :-) It's ok to keep it. My concern with this is that we're not used to do it anywhere else in our code and it won't matter 99% of the time. So, be prepared for a lot inconsistencies in this regard. ::: mobile/android/base/tests/helpers/NavigationHelper.java @@ +21,5 @@ > +final public class NavigationHelper { > + private static UITestContext sContext; > + private static Solo sSolo; > + > + private static ToolbarComponent sToolbarComponent; I'd go with sToolbar to be more concise. Up to you. ::: mobile/android/base/tests/helpers/WaitHelper.java @@ +74,5 @@ > + // Some changes to the UI occur in response to the same event we listen to for when > + // the page has finished loading (e.g. a page title update). As such, we ensure this > + // UI state has changed before returning from this method; here we store the initial > + // state. > + final ChangeVerifier[] pageLoadVerifiers = PAGE_LOAD_VERIFIERS; Is this pageLoadVerifiers variable actually necessary? @@ +96,5 @@ > + public boolean isSatisfied() { > + return verifier.hasStateChanged(); > + } > + }, CHANGE_WAIT_MS); > + // TODO: Make wait time an aggregated countdown? File a follow-up. ::: mobile/android/base/tests/testAwesomescreenPages.java @@ +7,5 @@ > + > +// TODO: Elaborate on test desc and use /**. > +/* Test correct state for URL bar after loading pages. > + */ > +public class testAwesomescreenPages extends UITest { There's no 'Awesomescreen' in our code. Maybe 'testAboutHomePages'? @@ +78,5 @@ > + } > + > + // TODO: The following is code from the old testAwesomebarSwipes which contains > + // additional functionality we don't cover here. We should reimplement it in the new > + // framework. File a follow-up. ::: mobile/android/base/tests/testTabHistory.java @@ +6,3 @@ > > +// TODO: Class name confusing with history list. > +// TODO: Elaborate on test desc and use /**. Remove those TODOs or just fix them now. @@ +7,5 @@ > +// TODO: Class name confusing with history list. > +// TODO: Elaborate on test desc and use /**. > +/* Test correct state for URL bar after loading pages. > + */ > +public class testTabHistory extends UITest { testSessionHistory? @@ +29,2 @@ > > + // TODO: Implement this functionality and uncomment. File a follow-up.

(In reply to Michael Comella (:mcomella) from comment #77) > https://tbpl.mozilla.org/?tree=Try&rev=b2df1a5f92f1 This try job is missing "robocop-4" for Android 4.0 opt, so does not show any of the new tests running -- oops!

Comment on attachment 8338070 [details] [diff] [review] Bootstrap new API for UI testing in Fennec. Review of attachment 8338070 [details] [diff] [review]: ----------------------------------------------------------------- There's been lots of discussion here - what's left to say? I recommend re-running try with robocop-4 before landing.

(In reply to Geoff Brown [:gbrown] from comment #80) > I recommend re-running try with robocop-4 before landing. https://tbpl.mozilla.org/?tree=Try&rev=c7e28bf324f9

And after changes for lucas' comments: https://tbpl.mozilla.org/?tree=Try&rev=61bf9b7e8bf0

Updated for lucas' comments. I will reply to the comments I didn't directly fix in this patch later.

Comment on attachment 8338994 [details] [diff] [review] Bootstrap new API for UI testing in Fennec. Move r+.

(In reply to Lucas Rocha (:lucasr) from comment #78) > ps: follow-up bugs are more important than the TODO comments. Make sure you > file a follow-up bug for every TODO comment that definitely needs fixing in > the short-term. I have filed most of the ones you've commented on as dependent and I'll continue doing this tomorrow, including ones you may not have mentioned. > @@ +58,5 @@ > > + protected ToolbarComponent mToolbar; > > + > > + static { > > + try { > > + sLauncherActivityClass = (Class<Activity>) Class.forName(LAUNCHER_ACTIVITY); > > Just wondering: does this actually need to be static? It can't be set in the constructor (since super(...) needs it) so it needs to be defined for its first assignment. However, it needs to be preprocessed (and then some) so we can't do that - it was tried in bug 916507. bug 938820 is the followup bug to eventually fix this here and in BaseTest. > It returns null because we have overwritten the getActivity() method to > simply return mActivity, which is null at this point. The getActivity() from > the parent class calls launchActivityWithIntent() under the hood. Either > just let the parent class handle that through its getActivity() method (if > possible) or remove the TODO (as this seems to be working as intended). Up > to you. Removed the overridden method (oops!). > Which classes import AssertionHelper.init statically? For ease of use, I import "AssertionHelper.*". So any class that uses AssertionHelper, including the tests, imports it. I filed bug 943703 to fix this. > @@ +57,5 @@ > > + } > > + > > + public AboutHomeComponent assertVisible() { > > + assertEquals("The HomePager is visible", > > + View.VISIBLE, getHomePagerView().getVisibility()); > > nit: add empty line here. I didn't add some of these empty lines for two line methods because it keeps the code less double-spaced. > Noticed you didn't cover my previous comment on this method. Do you feel > strongly about it? I don't, so feel free to keep it. Just checking if you > kept it intentionally or just because you missed my comment before. Missed it the first time - thanks for checking! I dislike those particular names you suggested because it's hard to tell the difference without looking at the code - e.g. what's the difference between getTitle and getTitleText? *Helper isn't very helpful because it's so generic, however, when you look at a method and see it call out directly to the private *Helper, you can assume it does the functionality that this particular method is supposed to do and this happens for both getTitle methods. > Ok, it seems the 'final' in the arguments is very intentional :-) It's ok to > keep it. My concern with this is that we're not used to do it anywhere else > in our code and it won't matter 99% of the time. So, be prepared for a lot > inconsistencies in this regard. That's fine - some finals are better than none! :) > ::: mobile/android/base/tests/helpers/WaitHelper.java > @@ +74,5 @@ > > + // Some changes to the UI occur in response to the same event we listen to for when > > + // the page has finished loading (e.g. a page title update). As such, we ensure this > > + // UI state has changed before returning from this method; here we store the initial > > + // state. > > + final ChangeVerifier[] pageLoadVerifiers = PAGE_LOAD_VERIFIERS; > > Is this pageLoadVerifiers variable actually necessary? No, but doing it this way means we only have to change it in one spot if we assign it to another value (rather than 2). > There's no 'Awesomescreen' in our code. Maybe 'testAboutHomePages'? -> testAboutHomePageNavigation > ::: mobile/android/base/tests/testTabHistory.java > @@ +6,3 @@ > > > > +// TODO: Class name confusing with history list. > > +// TODO: Elaborate on test desc and use /**. > > Remove those TODOs or just fix them now. Done. Ask questions if this is unclear - this was written quickly!

Intermittent in try from comment 82 - I'm holding landing until I can figure this out. It occurred on 2.2 armv6 so it's possible our timeout is too short (though the hasTimedOut message is not in the output, so perhaps not. Here's a try push with a longer timeout (2000->5000): https://tbpl.mozilla.org/?tree=Try&rev=7c9f65e32fa0

Further discussion about the intermittent (see comment 86) will be in bug 938969.

All relevant next step TODO bugs are filed. Additionally next steps are to start writing new tests and how-to test writing documentation.

I fixed the intermittent from bug 938969 (which I will explain in a comment there). I am going to file a follow up to aggregate the page load time so waiting for both events is encompassed by PAGE_LOAD_WAIT_MS. There is one more patch I will upload as a workaround to bug 945521.

Final patch, workaround for bug 945521 which would cause the tests to fail on some devices.

Comment on attachment 8342503 [details] [diff] [review] Part 3: Add missing go button work around. Review of attachment 8342503 [details] [diff] [review]: ----------------------------------------------------------------- Go! ::: mobile/android/base/tests/components/ToolbarComponent.java @@ +136,5 @@ > + // Bug 945521 workaround: Some keyboards do not > + // display the go button so we hit enter instead. > + mSolo.sendKey(Solo.ENTER); > + } else { > + mSolo.clickOnView(getGoButton()); I do wonder if we should simply sendKey(ENTER) in all cases? Up to you.

Comment on attachment 8342495 [details] [diff] [review] Part 2: Wait for DOMTitleChanged event in waitForPageLoad. Review of attachment 8342495 [details] [diff] [review]: ----------------------------------------------------------------- Ok. ::: mobile/android/base/tests/helpers/WaitHelper.java @@ +84,2 @@ > final EventExpecter contentEventExpecter = sActions.expectGeckoEvent("DOMContentLoaded"); > + final EventExpecter titleEventExpecter = sActions.expectGeckoEvent("DOMTitleChanged"); I assume we always get a DOMTitleChanged event, even on pages without a <title> tag?

Comment change as per discussion on IRC.

(In reply to Lucas Rocha (:lucasr) from comment #95) > I do wonder if we should simply sendKey(ENTER) in all cases? Up to you. I would say no - the enter button is less likely to break (and would be more noticeable by user testing) whereas clicking the go button finds bugs like 945521.

Comment on attachment 8342506 [details] [diff] [review] Part 3: Add missing go button work around. v2 Move r+.

(In reply to Lucas Rocha (:lucasr) from comment #96) > I assume we always get a DOMTitleChanged event, even on pages without a > <title> tag? bug 946349.

bug 946366 is the aggregation bug mentioned in comment 93.

Luckily I kept my changes in a git repo so these are all the files that were touched with "import org.mozilla.gecko" in them, as per the command to get this output. g ls | xargs ack "import org.mozilla.gecko" --no-color

Do not annotate TestConstants as per discussion via IRC.

Comment on attachment 8342805 [details] [diff] [review] Part 4: Add Proguard annotations. v2 Review of attachment 8342805 [details] [diff] [review]: ----------------------------------------------------------------- All looks fine now you've gotten rid of the redundant annotation from V1 ::: mobile/android/base/GeckoAppShell.java @@ +2420,5 @@ > } > > /* Called by JNI from AndroidBridge, and by reflection from tests/BaseTest.java.in */ > @WrapElementForJNI > + @RobocopTarget The WrapElementForJNI annotation already protects elements from Proguard. Up to you if you want to keep the Robocop annotation for explictness or such. ::: mobile/android/base/home/SuggestClient.java @@ -12,5 @@ > import android.net.ConnectivityManager; > import android.net.NetworkInfo; > import android.text.TextUtils; > import android.util.Log; > -import org.mozilla.gecko.mozglue.RobocopTarget; I see what you did there.

remote: https://hg.mozilla.org/integration/fx-team/rev/b3096c58816b remote: https://hg.mozilla.org/integration/fx-team/rev/db9eae6662a0 remote: https://hg.mozilla.org/integration/fx-team/rev/cedb942aa081 remote: https://hg.mozilla.org/integration/fx-team/rev/a7c9340f5a34

https://hg.mozilla.org/mozilla-central/rev/b3096c58816b https://hg.mozilla.org/mozilla-central/rev/db9eae6662a0 https://hg.mozilla.org/mozilla-central/rev/cedb942aa081 https://hg.mozilla.org/mozilla-central/rev/a7c9340f5a34