901432 - [FIG] Cleanup BaseTest and create a new Utility class to organize the methods in BaseTest

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Adrian Tamas (:AdrianT)]

Continuing the discussion from https://bugzilla.mozilla.org/show_bug.cgi?id=896566#c2 about cleaning up BaseTest and possibly creating a new class to keep methods from BaseTest in order to logically structure the classes more.

My point of view was to keep in BaseTests methods that have to do with navigation, adding tabs, menus and context menus and create a new subclass to keep all the methods that have to do with the new about:home including database operations, getting list views, editing bookmarks etc. This will not create the ambiguous Utility class Geoff was talking about in https://bugzilla.mozilla.org/show_bug.cgi?id=896566#c4 and will logically structure the classes.

Comment 1

[Adrian Tamas (:AdrianT)]

Attachment: BaseTest cleanup and creation of the new subclass with methods that work with the new about:home

Created the new subclass and moved all the methods that have to do with the new about:home and it's content to it. Changed the tests the used PixelTest since we can't have both methods from PixelTest and AboutHomeTest. 4 of the tests need rewriting anyway and the 5th (ImportFromAndroid) works without any issues

Also added the method for clearPrivateData in BaseTest, the fix for focusUrl, changed the getFirefoxData to getBrowserDBUrls and changed the parameter to Enum as requested in https://bugzilla.mozilla.org/show_bug.cgi?id=896566#c2

Comment 2

[Adrian Tamas (:AdrianT)]

Attachment: BaseTest cleanup and creation of the new subclass with methods that work with the new about:home

A small change in clearPrivateData to make it work. I used the wrong method to access the settings menu the first time.

Comment 3

[Adrian Tamas (:AdrianT)]

I kept the database access using cursors since it has been used also in the same way in addOrUpdateBookmark and deleteBookmark. I can look into using the BrowserContract here but if this works at the moment we might want to to consider looking into getting all the tests working and having this be an improvement.

Comment 4

[:Margaret Leibovic]

Comment on attachment 785730 [details] [diff] [review]
BaseTest cleanup and creation of the new subclass with methods that work with the new about:home

Thanks for this patch, Adrian!

I haven't had the chance to look through it yet, but I want to also add capella for feedback, since he's been looking into testing the new about:home as well.

Comment 5

[Geoff Brown [:gbrown]]

Comment on attachment 785730 [details] [diff] [review]
BaseTest cleanup and creation of the new subclass with methods that work with the new about:home

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

Great start - thanks!

I think I like the AboutHomeTest class.

::: mobile/android/base/tests/AboutHomeTest.java.in
@@ +12,5 @@
> +import android.widget.ListView;
> +
> +import java.util.ArrayList;
> +
> +abstract class AboutHomeTest extends BaseTest {

Put a little comment here to remind us of what this class is for and how it would be used.

@@ +26,5 @@
> +
> +    /**
> +     * This method can be used to check if an URL is pressent in the bookmarks database
> +     */
> +    public boolean isBookmark(String url) {

I expect all of the members of this class can be protected -- they would only be accessed from the test class, wouldn't they?

@@ +132,5 @@
> +                    if (cursor.getString(cursor.getColumnIndex("url")) != null) {
> +                        browserData.add(cursor.getString(cursor.getColumnIndex("url")));
> +                    }
> +                    if(!cursor.isLast()) {
> +                            cursor.moveToNext();

Careful with indentation please!

@@ +175,5 @@
> +        waitForText("Bookmark updated");
> +    }
> +
> +    public boolean checkBookmarkEdit(int bookmarkIndex, String addedText, ListView list) {
> +        Device mDevice = new Device();

I do not see mDevice used anywhere.

::: mobile/android/base/tests/BaseTest.java.in
@@ +621,5 @@
>              }
>          }
>      }
>  
> +    public void clearData() {

"clearData" seems too generic to me. Maybe "clearPrivateData" instead?

@@ +622,5 @@
>          }
>      }
>  
> +    public void clearData() {
> +        selectSettingsItem(clearPrivateData[0], clearPrivateData[1]);

I prefer selectSettingsItems("Privacy", "Clear private data") -- I don't think the variables help.

::: mobile/android/base/tests/testBookmarklets.java.in
@@ +21,5 @@
>  
>          blockForGeckoReady();
>  
>          // load a standard page so bookmarklets work
> +        loadUrl(url);

We were (generally) using loadAndPaint in preference to loadUrl to ensure that the page was actually rendered before we proceeded with test assertions -- that seemed to avoid some races and improve test results. I do not know of any particular case where that is necessary, so I do not object, as long as the test runs reliably. Just a caution...

Comment 6

[Adrian Tamas (:AdrianT)]

(In reply to Geoff Brown [:gbrown] from comment #5)
> @@ +26,5 @@
> > +
> > +    /**
> > +     * This method can be used to check if an URL is pressent in the bookmarks database
> > +     */
> > +    public boolean isBookmark(String url) {
> 
> I expect all of the members of this class can be protected -- they would
> only be accessed from the test class, wouldn't they?
They can be protected. I'll make the change

> @@ +132,5 @@
> > +                    if (cursor.getString(cursor.getColumnIndex("url")) != null) {
> > +                        browserData.add(cursor.getString(cursor.getColumnIndex("url")));
> > +                    }
> > +                    if(!cursor.isLast()) {
> > +                            cursor.moveToNext();
> 

Margaret pointed that out before but I missed it (again). I'll make the changes

> @@ +175,5 @@
> > +        waitForText("Bookmark updated");
> > +    }
> > +
> > +    public boolean checkBookmarkEdit(int bookmarkIndex, String addedText, ListView list) {
> > +        Device mDevice = new Device();
> 
> I do not see mDevice used anywhere.

The keyboard used to be opened when in pop-ups but only for pandas (ICS+). I removed the dependency with Bug 877659, then Bug 880650 fixed the issue and the keyboard was not shown anymore but I missed this. We can remove it.

> @@ +622,5 @@
> >          }
> >      }
> >  
> > +    public void clearData() {
> > +        selectSettingsItem(clearPrivateData[0], clearPrivateData[1]);
> 
> I prefer selectSettingsItems("Privacy", "Clear private data") -- I don't
> think the variables help.

I would prefer without also but Margaret asked in https://bugzilla.mozilla.org/show_bug.cgi?id=896566#c2 to take the strings out of the code and add them as variables.

> ::: mobile/android/base/tests/testBookmarklets.java.in
> @@ +21,5 @@
> >  
> >          blockForGeckoReady();
> >  
> >          // load a standard page so bookmarklets work
> > +        loadUrl(url);
> 
> We were (generally) using loadAndPaint in preference to loadUrl to ensure
> that the page was actually rendered before we proceeded with test assertions
> -- that seemed to avoid some races and improve test results. I do not know
> of any particular case where that is necessary, so I do not object, as long
> as the test runs reliably. Just a caution...
The only way we could keep access here is by extending PixelTest not BaseTest in AboutHomeTest. There was a time when loadUrl was really unreliable and loadAndPaint would be necessary everywhere. Lately I have not seen this. We could see how the rewrite of the 4 tests that are disabled goes and maybe then think of extending BaseTest?

Comment 7

[:Margaret Leibovic]

(In reply to Adrian Tamas (:AdrianT) from comment #6)

> > @@ +622,5 @@
> > >          }
> > >      }
> > >  
> > > +    public void clearData() {
> > > +        selectSettingsItem(clearPrivateData[0], clearPrivateData[1]);
> > 
> > I prefer selectSettingsItems("Privacy", "Clear private data") -- I don't
> > think the variables help.
> 
> I would prefer without also but Margaret asked in
> https://bugzilla.mozilla.org/show_bug.cgi?id=896566#c2 to take the strings
> out of the code and add them as variables.

I prefer factoring strings out into constants so that the tests are less tightly coupled with the UI (makes it easier to update when the UI changes), but I agree with gbrown that these variables are harder to read. Instead of an array like this, I was thinking more of constants like:

private final String PRIVACY_LABEL = "Privacy";
private final String CLEAR_PRIVATE_DATA_LABEL = "Clear Private Data";

I don't feel too strongly about us doing this in this bug, but it would be nice to work on reducing these hard-coded strings throughout our robocop tests.

Maybe a good follow-up would be to make a Settings class that has these settings item labels declared in them (and methods like selectSettingsItem), so that individual tests could be ignorant of the strings we're using in the UI.

Comment 8

[Geoff Brown [:gbrown]]

(In reply to :Margaret Leibovic from comment #7)
> private final String PRIVACY_LABEL = "Privacy";
> private final String CLEAR_PRIVATE_DATA_LABEL = "Clear Private Data";

That works for me.
 
> Maybe a good follow-up would be to make a Settings class that has these
> settings item labels declared in them (and methods like selectSettingsItem),
> so that individual tests could be ignorant of the strings we're using in the
> UI.

Sounds good too!

Comment 9

[:Margaret Leibovic]

Comment on attachment 785730 [details] [diff] [review]
BaseTest cleanup and creation of the new subclass with methods that work with the new about:home

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

I definitely like where this is heading! I agree with everything gbrown had to say, and I have some comments of my own I think we should address before landing this.

::: mobile/android/base/tests/AboutHomeTest.java.in
@@ +13,5 @@
> +
> +import java.util.ArrayList;
> +
> +abstract class AboutHomeTest extends BaseTest {
> +    public enum BROWSER_DATA_TYPE {BOOKMARKS, HISTORY};

Java style nit: enums are usually declared in CamelCase, so this should be BrowserDataType.

@@ +15,5 @@
> +
> +abstract class AboutHomeTest extends BaseTest {
> +    public enum BROWSER_DATA_TYPE {BOOKMARKS, HISTORY};
> +
> +    protected boolean isBookmark(String[] bookmarks, String aValue) {

I don't see this being used anywhere, can we get rid of this? It looks like the isBookmark() method down below is more useful.

@@ +49,5 @@
> +
> +    public long addOrUpdateBookmark(String title, String url, boolean bookmarklet) {
> +        ContentResolver resolver = getActivity().getContentResolver();
> +        Uri bookmarksUri = Uri.parse("content://@ANDROID_PACKAGE_NAME@.db.browser/bookmarks");
> +        bookmarksUri = bookmarksUri.buildUpon().appendQueryParameter("profile", "default").build();

It would be nice to move this Uri-building logic to a helper method, since that's repeated in a bunch of places (and it could take a BrowserDataType as a paramter).

@@ +54,5 @@
> +        long folderId = -1;
> +        Cursor c = null;
> +        // NOTE: this is hardcoded to toolbar/mobile and triggered from bookmarklet because the existing
> +        //       testcases were written this way
> +        String location = "toolbar";

Name this variable guid, not location.

@@ +65,5 @@
> +                               "guid = ?",
> +                               new String[] { location },
> +                               null);
> +            if (c.moveToFirst()) {
> +                folderId = c.getLong(c.getColumnIndexOrThrow("_id"));

If we don't get the folderId, I think we should fail, rather than continue on with a folderId of -1.

@@ +80,5 @@
> +        long now = System.currentTimeMillis();
> +        values.put("modified", now);
> +        if (!bookmarklet) {
> +            values.put("type", 1);
> +            values.put("guid", url);

Setting guid to url? I suppose this might be okay for a test, but it's probably safer to generate a random guid using generateGuid:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/sync/Utils.java#46

@@ +92,5 @@
> +        if (updated == 0) {
> +            Uri uri = resolver.insert(bookmarksUri, values);
> +            mAsserter.ok(true, "Inserted at: ", uri.toString());
> +            return ContentUris.parseId(uri);
> +        }

All this manual DB interaction makes me nervous... can you use reflection with LocalBrowserDB to do this instead? I see you're just moving existing logic, let's file a follow-up bug about making this more robust.

@@ +96,5 @@
> +        }
> +        return ContentUris.parseId(bookmarksUri);
> +    }
> +
> +    public void deleteBookmark(String title) {

Wouldn't it be better to delete a bookmark based on its URL, not title?

@@ +118,5 @@
> +            uri = Uri.parse("content://@ANDROID_PACKAGE_NAME@.db.browser/bookmarks");
> +        } else if (dataType == BROWSER_DATA_TYPE.HISTORY) {
> +            uri = Uri.parse("content://@ANDROID_PACKAGE_NAME@.db.browser/history");
> +        } else {
> +            mAsserter.ok(false, "The wrong data type has been provided = " + dataType.toString(), "Please provide a BROWSER_DATA_TYPE value");            

Nit: trailing whitespace. Also, nice use of a failure check.

@@ +124,5 @@
> +        uri = uri.buildUpon().appendQueryParameter("profile", "default")
> +                             .appendQueryParameter("sync", "true").build();
> +        try {
> +            cursor = resolver.query(uri, null, null, null, null);
> +            if (cursor != null) {

Do we ever expect the cursor to be null? If not, we should fail if it is null.

@@ +155,5 @@
> +    public void editBookmark(int bookmarkIndex, int fieldIndex, String addedText, ListView list) {
> +
> +        // Open the Edit Bookmark context menu
> +        View child;
> +        mSolo.clickOnText("Bookmarks");

Does this need to be updated for the new about:home?

@@ +185,5 @@
> +        waitForText("Switch to tab");
> +        mSolo.clickLongOnView(child);
> +        waitForText("Share");
> +        mSolo.clickOnText("Edit");
> +        waitForText("Edit Bookmark");

This is exactly repeated in the method above. In general, if more than 2 lines are exact copies, I think they should be factored out into a separate method.

@@ +190,5 @@
> +
> +        // Check if the new text was added
> +        if (mSolo.searchText(addedText)) {
> +            clickOnButton("Cancel");
> +            waitForText("about:home");

Is this still valid for the new about:home?

::: mobile/android/base/tests/BaseTest.java.in
@@ +68,5 @@
> +    // Settings menu paths
> +    private static final String[] clearPrivateData = new String[] {
> +        "Privacy",
> +        "Clear private data"
> +    };

We should come up with a better way to declare UI strings. As I mentioned in my previous comment, maybe this should even be a class like SettingsStrings or something:

class SettingsStrings {
    public static final PRIVACY = "Privacy";
    ...
}

I wonder what you and gbrown think of something like that.

Comment 10

[Adrian Tamas (:AdrianT)]

Most of the changes asked here are in methods that were just copy/pasted most from BaseTest and yes most of them should be rewritten for the new about:home. I am not sure which would be better to go through all and make them work or remove the ones that are from tests that are disabled and leave them to be implemented with the test.

Regarding the SettingsStrings class I don't have a problem with that but I would not move Robocop methods there. In my opinion it would create a lot of issues with accessing them similar to what happens here where if we extend AboutHomeTest we can't use loadAndPaint(), but I may be wrong on this.

Comment 11

[Adrian Tamas (:AdrianT)]

Attachment: BaseTest cleanup and AboutHomeTest creation

I believe I have addressed all the comments. Please let me know if I missed anything.

editBookmark and checkBookmarkEdit have not been rewritten but I moved them to AboutHomeTest. I would like to rewrite them with the test for the Bookmarks tab to be able to test them correctly.

The isBookmark(String[] bookmarks, String aValue) method was removed. It was used in the removed test testBookmarksTab.

I created a new method openAboutHomeTab(aboutHomeTabs tab) for about:home tab navigation and also a new buildUri(browserDataType dataType) method for the URI build.

I didn't change the manual database inserts, updates and removes. I'll log a separate bug for that since that seems it will need some work. Also I didn't create a new settingsStrings class yet since we need to figure out how that will be used and can also make the focus of a new bug.

Comment 12

[Adrian Tamas (:AdrianT)]

Attachment: BaseTest cleanup and AboutHomeTest creation v2.1

Sorry while updating testClearPrivateData I noticed that CLEAR_PRIVATE_DATA_LABEL was declared wrong and the clearPrivaterData method was failing to open the clear data pop-up.

Comment 13

[Adrian Tamas (:AdrianT)]

Attachment: BaseTest cleanup and AboutHomeTest creation v3

Changed all the database access to use reflections and the methods from BrowserDB. The only manual query left is for getFolderId to get the id of the mobile folder in order to get the bookmarks using getBookmarksInFolder. I tried with -1 for the root folder but that doesn't work and the getFolderId method from LocalBrowserDB is private.

Comment 14

[Lucas Rocha (:lucasr)]

Just realized that I was implementing a lot of equivalent code that is in AboutHomeTest here :-/

I'll wait base my changes on it then.

Comment 15

[Geoff Brown [:gbrown]]

Comment on attachment 786931 [details] [diff] [review]
BaseTest cleanup and AboutHomeTest creation v3

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

Looking good -- just some final nits to work through.

::: mobile/android/base/tests/AboutHomeTest.java.in
@@ +20,5 @@
> + * The purpose of this class is to collect all the logically connected methods that deal with about:home
> + * To use any of these methods in your test make sure it extends AboutHomeTest instead of BaseTest
> + */
> +abstract class AboutHomeTest extends BaseTest {
> +    protected enum browserDataType {BOOKMARKS, HISTORY};

s/browserDataType/BrowserDataType/

@@ +21,5 @@
> + * To use any of these methods in your test make sure it extends AboutHomeTest instead of BaseTest
> + */
> +abstract class AboutHomeTest extends BaseTest {
> +    protected enum browserDataType {BOOKMARKS, HISTORY};
> +    protected enum aboutHomeTabs {MOST_VISITED, MOST_RECENT, TABS_FROM_LAST_TIME, BOOKMARKS, READING_LIST};

s/aboutHomeTabs/AboutHomeTabs/

@@ +86,5 @@
> +            Method addBookmark = browserDB.getMethod("addBookmark", ContentResolver.class, String.class, String.class);
> +            addBookmark.invoke(null, resolver, title, url);
> +            mAsserter.ok(true, "Inserting a new bookmark", "Inserter the bookmark with the title = " + title + " and the url = " + url);
> +        } catch (Exception e) {
> +            mAsserter.dumpLog("Exception adding bookmark: " + e);

Unless you have a good reason not to, let's make this mAsserter.ok(false, ...) -- I would hate to have this type of failure go unnoticed.

@@ +99,5 @@
> +            Method updateBookmark = browserDB.getMethod("updateBookmark", ContentResolver.class, String.class, String.class);
> +            updateBookmark.invoke(null, resolver, title, url, keyword);
> +            mAsserter.ok(true, "Updating existing bookmark", "Setting the values to title = " + title + ", url = " + url + " and keyword = " + keyword);
> +        } catch (Exception e) {
> +            mAsserter.dumpLog("Exception updating bookmark: " + e);

mAsserter.ok again.

@@ +111,5 @@
> +            Class browserDB = classLoader.loadClass("org.mozilla.gecko.db.BrowserDB");
> +            Method removeBookmark = browserDB.getMethod("removeBookmarksWithURL", ContentResolver.class, String.class);
> +            removeBookmark.invoke(null, resolver, url);
> +        } catch (Exception e) {
> +            mAsserter.dumpLog("Exception deleting bookmark: " + e);

...again!

@@ +123,5 @@
> +            Class browserDB = classLoader.loadClass("org.mozilla.gecko.db.BrowserDB");
> +            Method removeHistory = browserDB.getMethod("removeHistoryEntry", ContentResolver.class, String.class);
> +            removeHistory.invoke(null, resolver, url);
> +        } catch (Exception e) {
> +            mAsserter.dumpLog("Exception deleting history item: " + e);

...you get the idea.

@@ +168,5 @@
> +                Class browserDBClass = classLoader.loadClass("org.mozilla.gecko.db.BrowserDB");
> +                Method getAllVisitedHistory = browserDBClass.getMethod("getAllVisitedHistory", ContentResolver.class);
> +                cursor = (Cursor)getAllVisitedHistory.invoke(null, resolver);
> +            } catch (Exception e) {
> +                mAsserter.dumpLog("Exception while getting history" + e);

mAsserter.ok

@@ +176,5 @@
> +                Class browserDBClass = classLoader.loadClass("org.mozilla.gecko.db.BrowserDB");
> +                Method getBookmarks = browserDBClass.getMethod("getBookmarksInFolder", ContentResolver.class, Long.TYPE);
> +                cursor = (Cursor)getBookmarks.invoke(null, resolver, getFolderId("mobile"));
> +            } catch (Exception e) {
> +                mAsserter.dumpLog("Exception while getting bookmarks " + e);

mAsserter.ok

@@ +181,5 @@
> +            }
> +        }
> +        if (cursor != null) {
> +            cursor.moveToFirst();
> +                for (int i = 0; i < cursor.getCount(); i++ ) {

spacing/indentation is off here, and following

@@ +230,5 @@
> +        mSolo.clickOnText("OK");
> +        waitForText("Bookmark updated");
> +    }
> +
> +    // FIXME: rewrite this to work with fig

Is there a follow-up bug to fix the FIXMEs?

@@ +241,5 @@
> +        waitForText("Switch to tab");
> +        mSolo.clickLongOnView(child);
> +        waitForText("Share");
> +        mSolo.clickOnText("Edit");
> +        waitForText("Edit Bookmark");

I think margaret suggested earlier that code here and in editBookmark are very similar -- consider moving some into a common function.

@@ +261,5 @@
> +     * @param aboutHomeTabs enum item {MOST_VISITED, MOST_RECENT, TABS_FROM_LAST_TIME, BOOKMARKS, READING_LIST}
> +     */
> +    protected void openAboutHomeTab(aboutHomeTabs tab) {
> +        int width = mDriver.getGeckoWidth() / 2;
> +        int height = mDriver.getGeckoHeight() / 2;

I think there is a risk of confusion here, perhaps in the future. "width" here really means "half the width" or the horizontal midpoint, so why not call it "xMidpoint" or "halfWidth"?

@@ +262,5 @@
> +     */
> +    protected void openAboutHomeTab(aboutHomeTabs tab) {
> +        int width = mDriver.getGeckoWidth() / 2;
> +        int height = mDriver.getGeckoHeight() / 2;
> +        int buttonHeight = mDriver.getGeckoTop() + mDriver.getGeckoHeight() - 15; // Getting Gecko height here since the vkb opening makes this get the visible height

Why 15? Is that going to work on all screens? Could we get the size of a view and use that instead?

@@ +277,5 @@
> +                mSolo.clickOnText(BOOKMAKRS_LABEL);
> +                mActions.drag(0, width, height, height);
> +                waitForText(HISTORY_LABEL);
> +                int buttonXPosition = mDriver.getGeckoWidth() / 2; // This should be the middle of the Most Recent button
> +                mSolo.clickOnScreen(buttonXPosition, buttonHeight);

This seems fragile -- couldn't we click on text, or a view, instead of a position?

@@ +278,5 @@
> +                mActions.drag(0, width, height, height);
> +                waitForText(HISTORY_LABEL);
> +                int buttonXPosition = mDriver.getGeckoWidth() / 2; // This should be the middle of the Most Recent button
> +                mSolo.clickOnScreen(buttonXPosition, buttonHeight);
> +                mAsserter.ok(waitForText(MOST_RECENT_LABEL), "Checking that we are in the history tab of about:home", "We are in the history tab");

"history" or "most recent"?

Comment 16

[Adrian Tamas (:AdrianT)]

(In reply to Geoff Brown [:gbrown] from comment #15)
> Comment on attachment 786931 [details] [diff] [review]

> @@ +230,5 @@
> > +        mSolo.clickOnText("OK");
> > +        waitForText("Bookmark updated");
> > +    }
> > +
> > +    // FIXME: rewrite this to work with fig
> 
> Is there a follow-up bug to fix the FIXMEs?
There is a bug for a rewrite of the BookmarksTab test. I thought it best to rewrite these then since they will need to be changed now to account for the "top bookmarks section" - pinned tabs - in the new about:home
> @@ +241,5 @@
> > +        waitForText("Switch to tab");
> > +        mSolo.clickLongOnView(child);
> > +        waitForText("Share");
> > +        mSolo.clickOnText("Edit");
> > +        waitForText("Edit Bookmark");
> 
> I think margaret suggested earlier that code here and in editBookmark are
> very similar -- consider moving some into a common function.
They are similar but there were some issues when I first wrote this with making this work reliably with all this code as one method. I don't remember exactly why but I can look into this and the re-write of the bookmarks tab test in the separate bug. Maybe comment this out or leave it as it is for now since it has the FIXME?
> @@ +261,5 @@
> > +     * @param aboutHomeTabs enum item {MOST_VISITED, MOST_RECENT, TABS_FROM_LAST_TIME, BOOKMARKS, READING_LIST}
> > +     */
> > +    protected void openAboutHomeTab(aboutHomeTabs tab) {
> > +        int width = mDriver.getGeckoWidth() / 2;
> > +        int height = mDriver.getGeckoHeight() / 2;
> 
> I think there is a risk of confusion here, perhaps in the future. "width"
> here really means "half the width" or the horizontal midpoint, so why not
> call it "xMidpoint" or "halfWidth"?
i'll rename
> @@ +262,5 @@
> > +     */
> > +    protected void openAboutHomeTab(aboutHomeTabs tab) {
> > +        int width = mDriver.getGeckoWidth() / 2;
> > +        int height = mDriver.getGeckoHeight() / 2;
> > +        int buttonHeight = mDriver.getGeckoTop() + mDriver.getGeckoHeight() - 15; // Getting Gecko height here since the vkb opening makes this get the visible height

Actually at this point the vkb is down and geckoHeight would be correct. Getting the values later while in the about:home could be tricky with the transitions of opening and closing the vkb

> Why 15? Is that going to work on all screens? Could we get the size of a
> view and use that instead?
This will work on all screens since the icons are around 20 to 30 pixels heigh. I can look into making it half the view but that would just complicate things. Anyway, your call here.
> @@ +277,5 @@
> > +                mSolo.clickOnText(BOOKMAKRS_LABEL);
> > +                mActions.drag(0, width, height, height);
> > +                waitForText(HISTORY_LABEL);
> > +                int buttonXPosition = mDriver.getGeckoWidth() / 2; // This should be the middle of the Most Recent button
> > +                mSolo.clickOnScreen(buttonXPosition, buttonHeight);
> 
> This seems fragile -- couldn't we click on text, or a view, instead of a
> position?
The buttons on the History tab are created as a view with id for the bar and 3 icons that are drawable resources. I could look into using clickOnImageButton but i'm not sure it will work with this setup. Being 3 buttons on the bar and clicking at 1/4 the width, 1/2 the width and 3/4 the width should work every time
> @@ +278,5 @@
> > +                mActions.drag(0, width, height, height);
> > +                waitForText(HISTORY_LABEL);
> > +                int buttonXPosition = mDriver.getGeckoWidth() / 2; // This should be the middle of the Most Recent button
> > +                mSolo.clickOnScreen(buttonXPosition, buttonHeight);
> > +                mAsserter.ok(waitForText(MOST_RECENT_LABEL), "Checking that we are in the history tab of about:home", "We are in the history tab");
> 
> "history" or "most recent"?
I'll change to most recent...I had it set up as history first but then saw that the view for the History tab had the title as "Most recent" but I forgot to change the text

Comment 17

[Lucas Rocha (:lucasr)]

(In reply to Adrian Tamas (:AdrianT) from comment #16)
> The buttons on the History tab are created as a view with id for the bar and
> 3 icons that are drawable resources. I could look into using
> clickOnImageButton but i'm not sure it will work with this setup. Being 3
> buttons on the bar and clicking at 1/4 the width, 1/2 the width and 3/4 the
> width should work every time

You can fetch the TabWidget view i.e. mSolo.getView(TabWidget.class, 0).

Then you click on specific children inside it.

Comment 18

[Geoff Brown [:gbrown]]

(In reply to Adrian Tamas (:AdrianT) from comment #16)
> (In reply to Geoff Brown [:gbrown] from comment #15)
> > Comment on attachment 786931 [details] [diff] [review]
> > > +    // FIXME: rewrite this to work with fig
> > 
> > Is there a follow-up bug to fix the FIXMEs?
> There is a bug for a rewrite of the BookmarksTab test. I thought it best to
> rewrite these then since they will need to be changed now to account for the
> "top bookmarks section" - pinned tabs - in the new about:home

That's great -- as long as there is a plan for getting these addressed, I'm happy!

> > @@ +241,5 @@
> > > +        waitForText("Switch to tab");
> > > +        mSolo.clickLongOnView(child);
> > > +        waitForText("Share");
> > > +        mSolo.clickOnText("Edit");
> > > +        waitForText("Edit Bookmark");
> > 
> > I think margaret suggested earlier that code here and in editBookmark are
> > very similar -- consider moving some into a common function.
> They are similar but there were some issues when I first wrote this with
> making this work reliably with all this code as one method. I don't remember
> exactly why but I can look into this and the re-write of the bookmarks tab
> test in the separate bug. Maybe comment this out or leave it as it is for
> now since it has the FIXME?

Sure - leave it as is for now.

Comment 19

[Geoff Brown [:gbrown]]

(In reply to Lucas Rocha (:lucasr) from comment #17)
> (In reply to Adrian Tamas (:AdrianT) from comment #16)
> You can fetch the TabWidget view i.e. mSolo.getView(TabWidget.class, 0).
> 
> Then you click on specific children inside it.

That sounds like the right way to go.

Comment 20

[Adrian Tamas (:AdrianT)]

Attachment: BaseTest cleanup and AboutHomeTest creation v4

I think I addressed all the issues you pointed out. Adding Lucas for feedback since he worked on something similar to this.

The only issue is that when using openAboutHome I can't really check if Most Visited works since there is no more label to wait for - recent change in fig, probably with the last waves of check-ins. I do check that we are in the History tab but we will just have to do further checks in the tests.

Comment 21

[Mark Capella [:capella]]

Noone asked me but this is annoying |BOOKMAKRS_LABEL = "BOOKMARKS";|

:-D

Comment 22

[Adrian Tamas (:AdrianT)]

Attachment: BaseTest cleanup and AboutHomeTest creation v4.1

Cleaned-up the misspelling pointed out by Mark

Comment 23

[Lucas Rocha (:lucasr)]

Comment on attachment 788844 [details] [diff] [review]
BaseTest cleanup and AboutHomeTest creation v4.1

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

Nice.

::: mobile/android/base/tests/AboutHomeTest.java.in
@@ +269,5 @@
> +    protected void openAboutHomeTab(AboutHomeTabs tab) {
> +        int halfWidth = mDriver.getGeckoWidth() / 2;
> +        int halfHeight = mDriver.getGeckoHeight() / 2;
> +        ViewPager pager = (ViewPager)mSolo.getView(ViewPager.class, 0);
> +        TabWidget tabwidget = (TabWidget)mSolo.getView(TabWidget.class, 0);

It would probably be safe to wait until the History page is selected before fetching it from the view tree. The only reason why this works right now is because we select the History page by default when the URL bar gets focus.

Comment 24

[Adrian Tamas (:AdrianT)]

Attachment: BaseTest cleanup and AboutHomeTest creation v4.2

(In reply to Lucas Rocha (:lucasr) from comment #23)
> Comment on attachment 788844 [details] [diff] [review]
> BaseTest cleanup and AboutHomeTest creation v4.1
> 
> Review of attachment 788844 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Nice.
> 
> ::: mobile/android/base/tests/AboutHomeTest.java.in
> @@ +269,5 @@
> > +    protected void openAboutHomeTab(AboutHomeTabs tab) {
> > +        int halfWidth = mDriver.getGeckoWidth() / 2;
> > +        int halfHeight = mDriver.getGeckoHeight() / 2;
> > +        ViewPager pager = (ViewPager)mSolo.getView(ViewPager.class, 0);
> > +        TabWidget tabwidget = (TabWidget)mSolo.getView(TabWidget.class, 0);
> 
> It would probably be safe to wait until the History page is selected before
> fetching it from the view tree. The only reason why this works right now is
> because we select the History page by default when the URL bar gets focus.
Moved the pager creation after focusing the URL and the tabwidget creation after going to History.
Just a small correction here when focusing the URL bar we are in the Bookmarks tab not the History tab but the previous version of the code still worked. But to be sure everything works this change is better.

Comment 25

[Lucas Rocha (:lucasr)]

Comment on attachment 788887 [details] [diff] [review]
BaseTest cleanup and AboutHomeTest creation v4.2

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

::: mobile/android/base/tests/testBookmarklets.java.in
@@ +6,5 @@
>  
>  import android.widget.ListView;
>  
>  
> +public class testBookmarklets extends AboutHomeTest {

Is this necessary? Does this fix bug 899183?

::: mobile/android/base/tests/testClearPrivateData.java.in
@@ +3,5 @@
>  
>  import @ANDROID_PACKAGE_NAME@.*;
>  import android.widget.ListView;
>  
> +public class testClearPrivateData extends AboutHomeTest  {

Is this necessary for this patch? Does it fix bug 896566?

Comment 26

[Adrian Tamas (:AdrianT)]

(In reply to Lucas Rocha (:lucasr) from comment #25)
> Comment on attachment 788887 [details] [diff] [review]
> BaseTest cleanup and AboutHomeTest creation v4.2
> 
> Review of attachment 788887 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/tests/testBookmarklets.java.in
> @@ +6,5 @@
> >  
> >  import android.widget.ListView;
> >  
> >  
> > +public class testBookmarklets extends AboutHomeTest {
> 
> Is this necessary? Does this fix bug 899183?
> 
> ::: mobile/android/base/tests/testClearPrivateData.java.in
> @@ +3,5 @@
> >  
> >  import @ANDROID_PACKAGE_NAME@.*;
> >  import android.widget.ListView;
> >  
> > +public class testClearPrivateData extends AboutHomeTest  {
> 
> Is this necessary for this patch? Does it fix bug 896566?
The changes, although they do not fix the tests, are needed because methods that are still active in the tests - not commented out - are moved to AboutHomeTest. This is purely in order for the code to be compiled without errors at this time. For bug 896566 there is a patch added with a re-write that works on fig.

Comment 27

[Geoff Brown [:gbrown]]

Comment on attachment 788887 [details] [diff] [review]
BaseTest cleanup and AboutHomeTest creation v4.2

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

::: mobile/android/base/tests/AboutHomeTest.java.in
@@ +4,5 @@
> +import @ANDROID_PACKAGE_NAME@.*;
> +
> +import android.content.ContentResolver;
> +import android.content.ContentValues;
> +import android.content.ContentUris;

nit - some of these are no longer needed. Review/cleanup imports.

Comment 28

[Adrian Tamas (:AdrianT)]

Attachment: BaseTest cleanup and AboutHomeTest creation v4.3

Removed the unnecessary imports

Comment 29

[Adrian Tamas (:AdrianT)]

Attachment: BaseTest cleanup and AboutHomeTest creation v4.4

Added a waitForEnabledText when selecting bookmarks before swiping left or right to make sure the bookmarks tab is loaded before dragging left or right - this is needed because at the moment when a new tab is opened about:home is loaded at bookmarks tab but when the url bar is focused about:home is loaded at the history tab.

Comment 30

[Lucas Rocha (:lucasr)]

Pushed: http://hg.mozilla.org/projects/fig/rev/0db3b6642ad0

Comment 31

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/0db3b6642ad0