Last Comment Bug 777719 - Batch of Robocop tests
: Batch of Robocop tests
Status: RESOLVED FIXED
:
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: Trunk
: ARM Android
: -- minor (vote)
: Firefox 18
Assigned To: Adrian Tamas (:AdrianT)
:
Mentors:
Depends on: 798816
Blocks: 744859
  Show dependency treegraph
 
Reported: 2012-07-26 07:25 PDT by Xiao Meng Wei :xwei
Modified: 2012-10-07 08:48 PDT (History)
5 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testBookmarkStar (2.29 KB, patch)
2012-08-02 10:39 PDT, Xiao Meng Wei :xwei
no flags Details | Diff | Review
testHistory (1.73 KB, patch)
2012-08-02 10:39 PDT, Xiao Meng Wei :xwei
no flags Details | Diff | Review
testBrowsing (486 bytes, patch)
2012-08-02 10:40 PDT, Xiao Meng Wei :xwei
no flags Details | Diff | Review
testVerifyBookmark (7.86 KB, patch)
2012-08-02 10:44 PDT, Xiao Meng Wei :xwei
no flags Details | Diff | Review
patch (222 bytes, patch)
2012-08-09 12:01 PDT, Xiao Meng Wei :xwei
jmaher: review-
Details | Diff | Review
Patch 2 (14.09 KB, patch)
2012-08-09 13:42 PDT, Xiao Meng Wei :xwei
jmaher: review-
Details | Diff | Review
patch 3 (14.20 KB, patch)
2012-08-13 16:53 PDT, Xiao Meng Wei :xwei
jmaher: review-
Details | Diff | Review
Patch 4 (14.36 KB, patch)
2012-08-15 11:37 PDT, Xiao Meng Wei :xwei
jmaher: review+
Details | Diff | Review
Patch 5 (15.31 KB, patch)
2012-08-24 10:50 PDT, Xiao Meng Wei :xwei
jmaher: review+
Details | Diff | Review
Patch 6 (15.18 KB, patch)
2012-08-24 12:10 PDT, Xiao Meng Wei :xwei
no flags Details | Diff | Review
test opening the Addons Manager (3.32 KB, patch)
2012-10-01 02:21 PDT, Adrian Tamas (:AdrianT)
no flags Details | Diff | Review
Opening the AMO page from the Addons Manager (3.64 KB, patch)
2012-10-01 02:22 PDT, Adrian Tamas (:AdrianT)
no flags Details | Diff | Review
addon_manager_tests (8.39 KB, patch)
2012-10-01 07:33 PDT, Adrian Tamas (:AdrianT)
jmaher: review-
Details | Diff | Review
addon_manager_tests_v2 (5.93 KB, patch)
2012-10-02 07:06 PDT, Adrian Tamas (:AdrianT)
jmaher: review+
Details | Diff | Review
addon_manager_tests_v3 (5.91 KB, patch)
2012-10-02 08:00 PDT, Adrian Tamas (:AdrianT)
no flags Details | Diff | Review
addon_manager_tests_v4 (5.85 KB, patch)
2012-10-02 08:28 PDT, Adrian Tamas (:AdrianT)
no flags Details | Diff | Review
addon_manager_tests_v4.1 (5.84 KB, patch)
2012-10-02 08:32 PDT, Adrian Tamas (:AdrianT)
jmaher: review+
Details | Diff | Review
bookmarkButton_history_browsing_patch_v7 (6.07 KB, patch)
2012-10-03 06:52 PDT, Adrian Tamas (:AdrianT)
no flags Details | Diff | Review
bookmarkButton_history_browsing_patch_v7.1 (7.70 KB, patch)
2012-10-03 07:12 PDT, Adrian Tamas (:AdrianT)
jmaher: review-
Details | Diff | Review
history_test_patch_v8 (2.89 KB, patch)
2012-10-04 09:22 PDT, Adrian Tamas (:AdrianT)
jmaher: review+
Details | Diff | Review

Description Xiao Meng Wei :xwei 2012-07-26 07:25:10 PDT
This is the meta bug for committing valid robocop test cases.
Comment 1 Xiao Meng Wei :xwei 2012-08-02 10:39:24 PDT
Created attachment 648385 [details] [diff] [review]
testBookmarkStar
Comment 2 Xiao Meng Wei :xwei 2012-08-02 10:39:48 PDT
Created attachment 648386 [details] [diff] [review]
testHistory
Comment 3 Xiao Meng Wei :xwei 2012-08-02 10:40:22 PDT
Created attachment 648388 [details] [diff] [review]
testBrowsing
Comment 4 Xiao Meng Wei :xwei 2012-08-02 10:44:56 PDT
Created attachment 648389 [details] [diff] [review]
testVerifyBookmark
Comment 5 Joel Maher (:jmaher) 2012-08-06 12:44:14 PDT
Comment on attachment 648385 [details] [diff] [review]
testBookmarkStar

>#filter substitution
>package @ANDROID_PACKAGE_NAME@.tests;
>
>import @ANDROID_PACKAGE_NAME@.*;
>import android.app.Activity;
>import android.util.Log;
>
>public class testBookmarkStar extends BaseTest {
>    public void testBookmarkStar() {
>        setTestType("mochitest");
>        mActions.expectGeckoEvent("Gecko:Ready").blockForEvent();
>
>        // Load the about: page
>        String url = "about:";
>        loadUrl(url);
>
>        Element awesomebar = mDriver.findElement(getActivity(), "awesome_bar");
>        mAsserter.isnot(awesomebar, null, "Got the awesomebar");
>        assertMatches(awesomebar.getText(), "About (Fennec|Nightly|Aurora|Firefox|Firefox Beta)", "page title match");
>
>        // Open a new page to remove the about: page from the current tab
>        url = getAbsoluteUrl("/robocop/robocop_blank_01.html");
>        loadUrl(url);
>
>        // Use the menu to open the Settings
>        mActions.sendSpecialKey(Actions.SpecialKey.MENU);
>
>        // Look for the 'More' menu if this device/OS uses it
>        if (mSolo.waitForText("^Bookmark$")) {
>            mSolo.clickOnText("^Bookmark$");
>        }
the comment doesn't match up with what you are doing here>

>
>        mActions.sendSpecialKey(Actions.SpecialKey.MENU);
>
>        // Look for the 'More' menu if this device/OS uses it
>        if (mSolo.waitForText("^More$")) {
>            mSolo.clickOnText("^More$");
>        }
>
>        mSolo.waitForText("^Settings$");
>        mSolo.clickOnText("^Settings$");
why is this not in an if clause like the previous ones?

>
>        // Set up listeners to catch the page load we're about to do
>        Actions.EventExpecter tabEventExpecter = mActions.expectGeckoEvent("Tab:Added");
>        Actions.EventExpecter contentEventExpecter = mActions.expectGeckoEvent("DOMContentLoaded");
>
>        // Tap on the "About Xxxx" setting
>        mSolo.waitForText("About (Fennec|Nightly|Aurora|Firefox|Firefox Beta)");
>        mSolo.clickOnText("About (Fennec|Nightly|Aurora|Firefox|Firefox Beta)");
>
>        // Wait for the new tab and page to load
>        tabEventExpecter.blockForEvent();
>        contentEventExpecter.blockForEvent();
>
>        // Grab the title to make sure the about: page was loaded
>        awesomebar = mDriver.findElement(getActivity(), "awesome_bar");
>        mAsserter.isnot(awesomebar, null, "Got the awesomebar");
>        assertMatches(awesomebar.getText(), "About (Fennec|Nightly|Aurora|Firefox|Firefox Beta)", "page title match");
>    }
>}

I don't see how this is different than testAboutPage.java.in
Comment 6 Joel Maher (:jmaher) 2012-08-06 12:48:48 PDT
Comment on attachment 648386 [details] [diff] [review]
testHistory

>#filter substitution
>package @ANDROID_PACKAGE_NAME@.tests;
>
>import @ANDROID_PACKAGE_NAME@.*;
>
>public class testHistory extends BaseTest {
>    public void testHistory() {
>        setTestType("mochitest");
>        
>        ListView hList = openHistoryList();
>        mSolo.waitForText(url);
>        mAsserter.ok(hList != null, "checking history exists", "history exists");
why do you waitForText between getting hList and asserting on it?

>
>        // Click on the history item and wait for the page to load
>        Actions.EventExpecter contentEventExpecter = mActions.expectGeckoEvent("DOMContentLoaded");
>        mSolo.clickInList(1);
this will fail if we didn't actually open the history list

>        verifyUrl(url3);
>        contentEventExpecter.blockForEvent();
>    
>    }
>    
>    private ListView openHistoryList() {
>        Activity awesomeBarActivity = clickOnAwesomeBar();
>		String expected = "http://www.cnn.com";
>        // Click the "History" tab to switch to history list
>        mSolo.clickOnText("History");
>		verifyUrl(expected);
>        Element historyList = mDriver.findElement(awesomeBarActivity, "history_list");
>        if (historyList != null) {
>            ArrayList<ListView> lists = mSolo.getCurrentListViews();
>            for (ListView list : lists) {
>                if (list.getId() == historyList.getId())
>                    return list;
you are returning list, but you don't seem to do anything with it?  this function implies that you are opening something.

>            }
>        }
>
>        // Just return null if we can't find the history list view
>        return null;
>    }
>}
>
Comment 7 Joel Maher (:jmaher) 2012-08-06 12:50:25 PDT
Comment on attachment 648388 [details] [diff] [review]
testBrowsing

>#filter substitution
>package @ANDROID_PACKAGE_NAME@.tests;
>
>import @ANDROID_PACKAGE_NAME@.*;
>
>public class testAwesomebar extends BaseTest {
>    public void testAwesomebar() {
We already have a class testAwesomebar, you should rename it.


>        setTestType("mochitest");
>        mActions.expectGeckoEvent("Gecko:Ready").blockForEvent();
>        String url = getAbsoluteUrl("/robocop/robocop_blank_01.html");
>        loadUrl(url);
>        verifyUrl(url);

it seems like we do this in many other tests already, it would be nice to browse a few urls and test other browser functionality.

>    }
>    @Override
>    protected int getTestType() {
>        return TEST_MOCHITEST;
>    }
nothing uses this function.

>}
Comment 8 Joel Maher (:jmaher) 2012-08-06 12:57:45 PDT
Comment on attachment 648389 [details] [diff] [review]
testVerifyBookmark

>
>public class testVerifyBookmark extends BaseTest  {
>    private static final int MAX_WAIT_MS = 3000; 
>    private static final String ABOUT_HOME_URL = "about:home";
>    private static String BOOKMARK_URL = "/robocop/robocop_blank_01.html";
>    private static String BOOKMARK_TITLE = "Browser Blank Page 01";
this seems overkill to define these constants, I would rather pass these values around in the test.

>
>    private ClassLoader mClassLoader;
>    private Method mAddBookmark;
>    private Method mRemoveBookmark;
>    private Method mIsBookmarked;
>    private String[] defaultBookmarks = new String[] {
>        "about:firefox",
>        "about:home",
>        "http://support.mozilla.org/en-US/mobile",
>        "https://addons.mozilla.org/en-US/android/"
>    };
If the default bookmarks change this could fail.  I don't think it is a bug problem, but I would prefer we query the bookmarks during initialization and use that as the starting point for when we insert and verify.

>
>    public void testVerifyBookmark() {
>        setTestType("mochitest");
>        BOOKMARK_URL = getAbsoluteUrl(BOOKMARK_URL);
>
>        mClassLoader = getActivity().getApplicationContext().getClassLoader();
>        try {
>            Class browserDB = mClassLoader.loadClass("org.mozilla.gecko.db.BrowserDB");
>            mAddBookmark = browserDB.getMethod("addBookmark", ContentResolver.class, String.class, String.class);
>            mRemoveBookmark = browserDB.getMethod("removeBookmarksWithURL", ContentResolver.class, String.class);
>            mIsBookmarked = browserDB.getMethod("isBookmark", ContentResolver.class, String.class);
>        } catch (java.lang.ClassNotFoundException ex) {
>            mAsserter.is(true, false, "Unable to find class");
>        } catch (java.lang.NoSuchMethodException ex) {
>            mAsserter.is(true, false, "Unable to find method");
>        }
>
>        runAwesomeScreenTest();
>        runMenuTest();
>    }
>
>    public void runMenuTest() {
>        try {
>            boolean isbookmark = (Boolean)mIsBookmarked.invoke(null, getActivity().getContentResolver(), BOOKMARK_URL);
>            mAsserter.is(isbookmark, false, "Page is not bookmarked initially");
>            setUpBookmark(); // loads the page, taps the star button, and waits for the "Bookmark Added" message
>            mAsserter.is(waitForBookmarked(true), true, "Tapping star button bookmarked page");
>
>            cleanUpBookmark(); // loads the page, taps the star button, and waits for the "Bookmark Removed" message
>            mAsserter.is(waitForBookmarked(false), false, "Tapping star button bookmarked page");
>        } catch(java.lang.IllegalAccessException ex) {
>            mAsserter.is(true, false, "Can not call addBookmark");
>        } catch(java.lang.reflect.InvocationTargetException ex) {
>            mAsserter.is(true, false, "Error calling addBookmark");
>        }
>    }
>
>    public void runAwesomeScreenTest() {
>        mActions.expectGeckoEvent("Gecko:Ready").blockForEvent();
>
>        // Open the bookmark list and check the root folder view
>        ListView bookmarksList = openBookmarksList();
what if bookmarkList is null?

>
>        // Wait for bookmark to appear in list
>        mSolo.waitForText(ABOUT_HOME_URL);
>
>        mAsserter.ok(bookmarksList != null, "checking that bookmarks list exists", "bookmarks list exists");
>
>        // No folders should be visible if no desktop bookmarks exist
this comment doesn't make sense?  desktop bookmarks and folders?

>        mAsserter.is(bookmarksList.getChildCount(), 4,
>            "bookmarks list has 4 children (the default bookmarks)");
>
>        for (int i = 0; i < bookmarksList.getChildCount(); i++) {
>            Cursor c = (Cursor)bookmarksList.getItemAtPosition(i);
>            String url = c.getString(c.getColumnIndexOrThrow("url"));
>            mAsserter.ok(Arrays.binarySearch(defaultBookmarks, url) > -1,
>                         "Find default bookmark", "Default bookmark for " + url + " found");
it seems that before the for loop we verify the 4 default bookmarks are in the list, then we verify each one again?
>        }
>
>        insertOneBookmark();
>        mSolo.waitForText(BOOKMARK_TITLE);
>
>        mAsserter.is(bookmarksList.getChildCount(), 5,
>            "bookmarks list has 5 children (the default bookmarks and the new one)");
> 
>        // Click on the bookmark we created and wait for the bookmarked page to load
>        Actions.EventExpecter contentEventExpecter = mActions.expectGeckoEvent("DOMContentLoaded");
>        mSolo.clickInList(1);
>        contentEventExpecter.blockForEvent();
>
>        // Clean up the bookmark we created
>        deleteBookmark();
>    }
>
>    private boolean waitForBookmarked(final boolean isBookmarked) {
>        waitForTest(new BooleanTest() {
>            public boolean test() {
>                try {
>                   return isBookmarked == (Boolean)mIsBookmarked.invoke(null, getActivity().getContentResolver(), BOOKMARK_URL);
>                } catch(java.lang.IllegalAccessException ex) {
>                    mAsserter.is(true, false, "Can not call addBookmark");
>                } catch(java.lang.reflect.InvocationTargetException ex) {
>                    mAsserter.is(true, false, "Error calling addBookmark");
>                }
>                return false;
>            }
>        }, MAX_WAIT_MS);
>        try {
>            Boolean res = (Boolean)mIsBookmarked.invoke(null, getActivity().getContentResolver(), BOOKMARK_URL);
>            return res.booleanValue();
>        } catch(java.lang.IllegalAccessException ex) {
>            mAsserter.is(true, false, "Can not call addBookmark");
>        } catch(java.lang.reflect.InvocationTargetException ex) {
>            mAsserter.is(true, false, "Error calling addBookmark");
>        }
>        return !isBookmarked;
>    }
>
>    private void insertOneBookmark() {
>        try {
>            mAddBookmark.invoke(null, getActivity().getContentResolver(), BOOKMARK_TITLE, BOOKMARK_URL);
>        } catch(java.lang.IllegalAccessException ex) {
>            mAsserter.is(true, false, "Can not call addBookmark");
>        } catch(java.lang.reflect.InvocationTargetException ex) {
>            mAsserter.is(true, false, "Error calling addBookmark");
>        }
>    }
>    
>    private void deleteBookmark() {
>        try {
>            mRemoveBookmark.invoke(null, getActivity().getContentResolver(), BOOKMARK_URL);
>        } catch(java.lang.IllegalAccessException ex) {
>            mAsserter.is(true, false, "Can not call removeBookmark");
>        } catch(java.lang.reflect.InvocationTargetException ex) {
>            mAsserter.is(true, false, "Error calling removeBookmark");
>        }
>    }
>
>    private ListView openBookmarksList() {
>        Activity awesomeBarActivity = clickOnAwesomeBar();
>
>        // Click the "Bookmarks" tab to switch to bookmarks list
>        mSolo.clickOnText("Bookmarks");
>
>        Element bookmarkList = mDriver.findElement(awesomeBarActivity, "bookmarks_list");
>        if (bookmarkList != null) {
>            ArrayList<ListView> lists = mSolo.getCurrentListViews();
>            for (ListView list : lists) {
>                if (list.getId() == bookmarkList.getId())
>                    return list;
>            }
>        }
>
>        // Just return null if we can't find the bookmarks list view
>        return null;
>    }
>
>    // This method opens the menu and selects the "Bookmark" menu item
>    private void toggleBookmark() {
>        getInstrumentation().waitForIdleSync();
>        mActions.sendSpecialKey(Actions.SpecialKey.MENU);
>        mSolo.waitForText("Bookmark");
>        mSolo.clickOnText("Bookmark");
>    }
>
>    private void setUpBookmark() {
>        // Bookmark a page for the test
>        loadUrl(BOOKMARK_URL);
>        toggleBookmark();
>        mAsserter.is(mSolo.waitForText("Bookmark added"), true, "bookmark added sucessfully");
>
>        // Navigate back to about:home for the test
>        loadUrl(ABOUT_HOME_URL);
>    }
>
>    private void cleanUpBookmark() {
>        // Go back to the page we bookmarked
>        loadUrl(BOOKMARK_URL);
>        toggleBookmark();
>        mAsserter.is(mSolo.waitForText("Bookmark removed"), true, "bookmark removed successfully");
>    }
>}
Comment 9 Xiao Meng Wei :xwei 2012-08-09 12:01:07 PDT
Created attachment 650636 [details] [diff] [review]
patch
Comment 10 Joel Maher (:jmaher) 2012-08-09 12:06:39 PDT
I think you forgot to 'hg add <filename>' to your patch.
Comment 11 Xiao Meng Wei :xwei 2012-08-09 13:42:56 PDT
Created attachment 650666 [details] [diff] [review]
Patch 2
Comment 12 Joel Maher (:jmaher) 2012-08-13 10:38:45 PDT
Comment on attachment 650636 [details] [diff] [review]
patch

I still need to have data in this file.  Just cleaning up my review queue.
Comment 13 Xiao Meng Wei :xwei 2012-08-13 10:39:40 PDT
(In reply to Joel Maher (:jmaher) from comment #12)
> Comment on attachment 650636 [details] [diff] [review]
> patch
> 
> I still need to have data in this file.  Just cleaning up my review queue.

Hi Joel, I uploaded a patch 2 but forgot to add you to the review flag.
Comment 14 Joel Maher (:jmaher) 2012-08-13 11:27:03 PDT
Comment on attachment 650666 [details] [diff] [review]
Patch 2

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

I might have missed a few things, but overall there are a lot of nits and a few big things.

::: mobile/android/base/tests/testBookmarkStar.java.in
@@ +14,5 @@
> +        String url = "about:";
> +        loadUrl(url);
> +
> +        Element awesomebar = mDriver.findElement(getActivity(), "awesome_bar");
> +        mAsserter.isnot(awesomebar, null, "Got the awesomebar");

this seems redundant to assert if awesomebar is null or not.  Shouldn't we check for null and return if it is?

@@ +37,5 @@
> +            mSolo.clickOnText("^More$");
> +        }
> +
> +        if (mSolo.waitForText("^Settings$")) {
> +        	mSolo.clickOnText("^Settings$");

fix the spacing here.

@@ +39,5 @@
> +
> +        if (mSolo.waitForText("^Settings$")) {
> +        	mSolo.clickOnText("^Settings$");
> +        }
> +

so we tap the menu button, more, settings?  If more doesn't exist, will settings exist and will it click?  This isn't a blocker, just something that could be problematic on different devices.

@@ +54,5 @@
> +        contentEventExpecter.blockForEvent();
> +
> +        // Grab the title to make sure the about: page was loaded
> +        awesomebar = mDriver.findElement(getActivity(), "awesome_bar");
> +        mAsserter.isnot(awesomebar, null, "Got the awesomebar");

another case where you assert on awesomebar, but don't really check for it.

::: mobile/android/base/tests/testBrowsing.java.in
@@ +9,5 @@
> +        mActions.expectGeckoEvent("Gecko:Ready").blockForEvent();
> +        String url1 = getAbsoluteUrl("/robocop/robocop_blank_01.html");
> +        String url2 = getAbsoluteUrl("ftp://ftp.mozilla.org/pub/mozilla.org/mobile/nightly/");
> +        String url3 = getAbsoluteUrl("http://www.google.com");
> +        //repeat visiting local page, ftp page, and normal website 

nit: extra space at the end here.

@@ +15,5 @@
> +        verifyUrl(url1);
> +        loadUrl(url2);
> +        verifyUrl(url2);
> +        loadUrl(url3);
> +        verifyUrl(url3);

we don't have access to external websites in automation, please keep the websites to something relative inside the test harness/robocop apk.

::: mobile/android/base/tests/testHistory.java.in
@@ +18,5 @@
> +        loadUrl(url2);
> +        verifyUrl(url2);
> +        loadUrl(url3);
> +        verifyUrl(url3);
> +        

extra white space in a few lines, also lets keep the URLs to something internal.  Our automation does not have external web access.

@@ +31,5 @@
> +    }
> +    
> +    private ListView openHistoryList() {
> +        Activity awesomeBarActivity = clickOnAwesomeBar();
> +		String expected = "http://www.cnn.com";

fix spacing here

@@ +34,5 @@
> +        Activity awesomeBarActivity = clickOnAwesomeBar();
> +		String expected = "http://www.cnn.com";
> +        // Click the "History" tab to switch to history list
> +        mSolo.clickOnText("History");
> +		verifyUrl(expected);

fix spacing here.

::: mobile/android/base/tests/testVerifyBookmark.java.in
@@ +13,5 @@
> +import java.lang.reflect.Method;
> +import android.content.ContentResolver;
> +
> +public class testVerifyBookmark extends BaseTest  {
> +    private static final int MAX_WAIT_MS = 3000; 

nit: extra space at the end here.

@@ +70,5 @@
> +        mActions.expectGeckoEvent("Gecko:Ready").blockForEvent();
> +
> +        // Open the bookmark list and check the root folder view
> +        ListView bookmarksList = openBookmarksList();
> +		mAsserter.ok(bookmarksList != null, "checking default bookmarks exist", "bookmarks exist");

keep the same spacing here.

@@ +135,5 @@
> +        } catch(java.lang.reflect.InvocationTargetException ex) {
> +            mAsserter.is(true, false, "Error calling addBookmark");
> +        }
> +    }
> +    

nit, extra spaces
Comment 15 Xiao Meng Wei :xwei 2012-08-13 16:53:40 PDT
Created attachment 651570 [details] [diff] [review]
patch 3

More changes.
Comment 16 Joel Maher (:jmaher) 2012-08-14 15:11:21 PDT
Comment on attachment 651570 [details] [diff] [review]
patch 3

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

I would like to review testVerifyBookmark.java again, but I think we are down to the little details.

::: mobile/android/base/tests/testBookmarkStar.java.in
@@ +27,5 @@
> +        mActions.sendSpecialKey(Actions.SpecialKey.MENU);
> +
> +        // Tapping on the bookmark option
> +        if (mSolo.waitForText("^Bookmark$")) {
> +			mSolo.clickOnText("^Bookmark$");

NIT: fix spacing here, 4 space indent per code level

@@ +34,5 @@
> +        mActions.sendSpecialKey(Actions.SpecialKey.MENU);
> +
> +        // Look for the 'More' menu if this device/OS uses it, then go to "settings"
> +        if (mSolo.waitForText("^More$")) {
> +			mSolo.clickOnText("^More$");

nit: 4 space indent

@@ +36,5 @@
> +        // Look for the 'More' menu if this device/OS uses it, then go to "settings"
> +        if (mSolo.waitForText("^More$")) {
> +			mSolo.clickOnText("^More$");
> +        	if (mSolo.waitForText("^Settings$")) {
> +				mSolo.clickOnText("^Settings$");

nit: 4 space indent

@@ +59,5 @@
> +
> +        // Grab the title to make sure the about: page was loaded
> +        awesomebar = mDriver.findElement(getActivity(), "awesome_bar");
> +        //adding a short delay here to ensure next step won't be false alarm
> +        mSolo.sleep(1000);

I really don't like sleep commands, if there was a waitfor that is better to use.  At the very least, please put a comment that we should redo this as in the future.

::: mobile/android/base/tests/testHistory.java.in
@@ +11,5 @@
> +
> +        String url = getAbsoluteUrl("/robocop/robocop_blank_01.html");
> +        string url2 = getAbsoluteUrl("/robocop/robocop_blank_02.html");
> +        string url3 = getAbsoluteUrl("/robocop/robocop_blank_03.html");
> +        

nit: removed extra whitespace/tab here

@@ +28,5 @@
> +        mSolo.clickInList(1);
> +        verifyUrl(url3);
> +        contentEventExpecter.blockForEvent();
> +    }
> +    

nit: remove extra whitespace/tab here

@@ +31,5 @@
> +    }
> +    
> +    private ListView openHistoryList() {
> +		Activity awesomeBarActivity = clickOnAwesomeBar();
> +		String expected = "http://www.cnn.com";

nit: 4 space indent
we are expecting cnn.com?

@@ +34,5 @@
> +		Activity awesomeBarActivity = clickOnAwesomeBar();
> +		String expected = "http://www.cnn.com";
> +        // Click the "History" tab to switch to history list
> +        mSolo.clickOnText("History");
> +		verifyUrl(expected);

nit: 4 space indent

@@ +48,5 @@
> +        // Return null if we can't find the history list view
> +        return null;
> +    }
> +}
> +	
\ No newline at end of file

nit: remove extra whitespace/tab here

::: mobile/android/base/tests/testVerifyBookmark.java.in
@@ +70,5 @@
> +        mActions.expectGeckoEvent("Gecko:Ready").blockForEvent();
> +
> +        // Open the bookmark list and check the root folder view
> +        ListView bookmarksList = openBookmarksList();
> +		mAsserter.ok(bookmarksList != null, "checking default bookmarks exist", "bookmarks exist");

nit: 4 space indent

@@ +75,5 @@
> +
> +        // Wait for bookmark to appear in list
> +        mSolo.waitForText(ABOUT_HOME_URL);
> +
> +        mAsserter.ok(bookmarksList != null, "checking that bookmarks list exists", "bookmarks list exists");

we have this assert on line 74

@@ +77,5 @@
> +        mSolo.waitForText(ABOUT_HOME_URL);
> +
> +        mAsserter.ok(bookmarksList != null, "checking that bookmarks list exists", "bookmarks list exists");
> +        mAsserter.is(bookmarksList.getChildCount(), 4,
> +            "bookmarks list has 4 children (the default bookmarks)");

if bookmarkList == null, calling getChildCount will fail.  put this in:
if bookmarksList == null) {
    //assert a failure message
    return
} else {
  mAsserter.is(bookmarksList.getChildCount()....);

  for (int i = 0, i < bookmakrsList.getChildCount....)
  .
  .
}

@@ +106,5 @@
> +    private boolean waitForBookmarked(final boolean isBookmarked) {
> +        waitForTest(new BooleanTest() {
> +            public boolean test() {
> +                try {
> +					return isBookmarked == (Boolean)mIsBookmarked.invoke(null, getActivity().getContentResolver(), BOOKMARK_URL);

nit: 4 space indent

@@ +108,5 @@
> +            public boolean test() {
> +                try {
> +					return isBookmarked == (Boolean)mIsBookmarked.invoke(null, getActivity().getContentResolver(), BOOKMARK_URL);
> +                } catch(java.lang.IllegalAccessException ex) {
> +					mAsserter.is(true, false, "Can not call addBookmark");

nit: 4 space indent

@@ +110,5 @@
> +					return isBookmarked == (Boolean)mIsBookmarked.invoke(null, getActivity().getContentResolver(), BOOKMARK_URL);
> +                } catch(java.lang.IllegalAccessException ex) {
> +					mAsserter.is(true, false, "Can not call addBookmark");
> +                } catch(java.lang.reflect.InvocationTargetException ex) {
> +					mAsserter.is(true, false, "Error calling addBookmark");

nit: 4 space indent
Comment 17 Xiao Meng Wei :xwei 2012-08-15 11:37:59 PDT
Created attachment 652158 [details] [diff] [review]
Patch 4

What tool do you use to check to incorrect whitespaces?
Comment 18 Joel Maher (:jmaher) 2012-08-16 04:10:48 PDT
just look at this in splinter which is the 'splinter review' link next to the attachment on this bug.
Comment 19 Xiao Meng Wei :xwei 2012-08-20 13:11:59 PDT
(In reply to Joel Maher (:jmaher) from comment #18)
> just look at this in splinter which is the 'splinter review' link next to
> the attachment on this bug.

Hi Joel,

Can you help me review the patch in comment 17 some time this week? I want to get this done before I leave :)
Comment 20 Joel Maher (:jmaher) 2012-08-23 06:47:51 PDT
Comment on attachment 652158 [details] [diff] [review]
Patch 4

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

I just have little spacing nits below.  The only thing is that we have found on some devices the loadURL is not sufficient enough, so we have been switching to loadandpaint from PixelTest.  Anyway, this is great stuff.

::: mobile/android/base/tests/testBookmarkStar.java.in
@@ +11,5 @@
> +        mActions.expectGeckoEvent("Gecko:Ready").blockForEvent();
> +
> +        // Load the about: page
> +        String url = "about:";
> +        loadUrl(url);

we found this week that loading >1 url in a test case is problematic and using loadandpaint from the PixelTest is ideal.  We can do that in a followup.

@@ +37,5 @@
> +        if (mSolo.waitForText("^More$")) {
> +            mSolo.clickOnText("^More$");
> +        	if (mSolo.waitForText("^Settings$")) {
> +                mSolo.clickOnText("^Settings$");
> +        	}

4 space indent please.

@@ +41,5 @@
> +        	}
> +        } else {
> +        	if (mSolo.waitForText("^Settings$")) {
> +                mSolo.clickOnText("^Settings$");
> +        	}

4 space indent please.

::: mobile/android/base/tests/testVerifyBookmark.java.in
@@ +77,5 @@
> +            mAsserter.ok(bookmarksList != null, "checking default bookmarks exist", "bookmarks exist");
> +            return;
> +        } else {
> +            mAsserter.is(bookmarksList.getChildCount(), 4, "bookmarks list has 4 children (the default bookmarks)");
> +		    //verify the four bookmarks are in fact the default ones

4 space indent please.
Comment 21 Xiao Meng Wei :xwei 2012-08-23 14:15:48 PDT
Thanks Joel, I really appreciate your help. Can you help me check in code since I do not have committer access.
Comment 22 Joel Maher (:jmaher) 2012-08-24 06:44:08 PDT
added entries to robocop.ini and pushed to try:
https://tbpl.mozilla.org/?tree=Try&rev=08728375d9c2
Comment 23 Joel Maher (:jmaher) 2012-08-24 06:59:14 PDT
hmm, this failed to build.  :xwei, can you fix up the tests so they build in a mozilla-central based tree?
Comment 24 Xiao Meng Wei :xwei 2012-08-24 10:50:14 PDT
Created attachment 655057 [details] [diff] [review]
Patch 5

Turns out I've been editing in a different tree. I've updated with the changes needed. Right now it shouldn't generate any build time errors.
Comment 25 Joel Maher (:jmaher) 2012-08-24 11:58:00 PDT
Comment on attachment 655057 [details] [diff] [review]
Patch 5

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

please remove the setTestType comments in all of the test case files as mentioned below, also a few spacing nits below.  The rest of this looks pretty good.

::: mobile/android/base/tests/testBookmarkStar.java.in
@@ +12,5 @@
> +        return TEST_MOCHITEST;
> +    }
> +
> +    public void testBookmarkStar() {
> +        //setTestType("mochitest");

please remove this commented out line.

@@ +48,5 @@
> +        } else {
> +        	if (mSolo.waitForText("^Settings$")) {
> +                   mSolo.clickOnText("^Settings$");
> +        	}
> +        }

these second level if statement are over  indented, 4 space indent only!

::: mobile/android/base/tests/testVerifyBookmark.java.in
@@ +82,5 @@
> +            mAsserter.ok(bookmarksList != null, "checking default bookmarks exist", "bookmarks exist");
> +            return;
> +        } else {
> +            mAsserter.is(bookmarksList.getChildCount(), 4, "bookmarks list has 4 children (the default bookmarks)");
> +		    //verify the four bookmarks are in fact the default ones

this comment needs to be 4 space indent.
Comment 26 Xiao Meng Wei :xwei 2012-08-24 12:10:57 PDT
Created attachment 655087 [details] [diff] [review]
Patch 6
Comment 27 Joel Maher (:jmaher) 2012-08-24 12:15:51 PDT
posted to try server:
https://tbpl.mozilla.org/?tree=Try&rev=bd43d16b9eab
Comment 28 Joel Maher (:jmaher) 2012-08-24 17:19:39 PDT
running on try server, we have a few errors in testHistory and testVerifyBookmark:
https://tbpl.mozilla.org/?tree=Try&rev=bd43d16b9eab

8 INFO TEST-UNEXPECTED-FAIL | testHistory | Awesomebar URL stayed the same - got http://mochi.test:8888/tests/robocop/robocop_blank_03.html, expected /robocop/robocop_blank_01.html
Bug 770456 - Intermittent Robocop | testHistoryTab | Exception caught - junit.framework.AssertionFailedError: View is null and can therefore not be clicked!
Bug 770461 - Intermittent Robocop | testHistoryTab | Context menu has New Tab option (x2) junit.framework.AssertionFailedError: 8 INFO TEST-UNEXPECTED-FAIL | testHistory | Awesomebar URL stayed the same - got http://mochi.test:8888/tests/robocop/robocop_blank_03.html, expected /robocop/robocop_blank_01.html
9 INFO TEST-UNEXPECTED-FAIL | testHistory | Exception caught - junit.framework.AssertionFailedError: 8 INFO TEST-UNEXPECTED-FAIL | testHistory | Awesomebar URL stayed the same - got http://mochi.test:8888/tests/robocop/robocop_blank_03.html, expected /robocop/robocop_blank_01.html
2 INFO TEST-UNEXPECTED-FAIL | testVerifyBookmark | checking default bookmarks exist - bookmarks exist
junit.framework.AssertionFailedError: 2 INFO TEST-UNEXPECTED-FAIL | testVerifyBookmark | checking default bookmarks exist - bookmarks exist
3 INFO TEST-UNEXPECTED-FAIL | testVerifyBookmark | Exception caught - junit.framework.AssertionFailedError: 2 INFO TEST-UNEXPECTED-FAIL | testVerifyBookmark | checking default bookmarks exist - bookmarks exist
4 INFO TEST-UNEXPECTED-FAIL | testAllPagesTab | all pages list has 5 children (the default bookmarks) - got 8, expected 5
junit.framework.AssertionFailedError: 4 INFO TEST-UNEXPECTED-FAIL | testAllPagesTab | all pages list has 5 children (the default bookmarks) - got 8, expected 5
5 INFO TEST-UNEXPECTED-FAIL | testAllPagesTab | Exception caught - junit.framework.AssertionFailedError: 4 INFO TEST-UNEXPECTED-FAIL | testAllPagesTab | all pages list has 5 children (the default bookmarks) - got 8, expected 5
4 INFO TEST-UNEXPECTED-FAIL | testHistoryTab | bookmark added sucessfully - got false, expected true
Bug 770456 - Intermittent Robocop | testHistoryTab | Exception caught - junit.framework.AssertionFailedError: View is null and can therefore not be clicked!
Bug 770461 - Intermittent Robocop | testHistoryTab | Context menu has New Tab option (x2) junit.framework.AssertionFailedError: 4 INFO TEST-UNEXPECTED-FAIL | testHistoryTab | bookmark added sucessfully - got false, expected true
5 INFO TEST-UNEXPECTED-FAIL | testHistoryTab | Exception caught - junit.framework.AssertionFailedError: 4 INFO TEST-UNEXPECTED-FAIL | testHistoryTab | bookmark added sucessfully - got false, expected true


I suspect the other errors in the existing tests are due to changes we make to the profile in the new tests.
Comment 29 Xiao Meng Wei :xwei 2012-08-24 18:52:42 PDT
It seems like the whole testing process is stuck at this page http://mochi.test:8888/tests/robocop/robocop_blank_03.html, which caused most of the above error.
Comment 30 Joel Maher (:jmaher) 2012-08-27 14:05:27 PDT
any thoughts on how why that might be happening?
Comment 31 Adrian Tamas (:AdrianT) 2012-10-01 02:21:57 PDT
Created attachment 666476 [details] [diff] [review]
test opening the Addons Manager

Adding 2 robocop tests to the metabug.

I created these tests before knowing about the tests added here and the debuging needed on them. I will start trying to debug the other tests also.

These are the first tests I wrote using Robocop. If I should create a separate bug or add them as a patch file please let me know. Also any feedback on the tests would be appreciated.
Comment 32 Adrian Tamas (:AdrianT) 2012-10-01 02:22:42 PDT
Created attachment 666477 [details] [diff] [review]
Opening the AMO page from the Addons Manager
Comment 33 Aaron Train [:aaronmt] 2012-10-01 07:00:55 PDT
The above two attachments need to be submitted as patches: https://developer.mozilla.org/en-US/docs/Creating_a_patch
Comment 34 Adrian Tamas (:AdrianT) 2012-10-01 07:33:13 PDT
Created attachment 666544 [details] [diff] [review]
addon_manager_tests

Created a patch from the 2 tests
Comment 35 Joel Maher (:jmaher) 2012-10-01 08:09:37 PDT
Comment on attachment 666544 [details] [diff] [review]
addon_manager_tests

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

The basics here are that we are just testing to see what tabs are opened (or not opened) which clicking AMO related buttons/links in the fennec UI?  Is there anyway we could merge both tests into the same test case?  

Please remove all extra whitespaces, it really litters up the review :)

I am doing a r- due to code that could be refactored better between the two tests.  A lot of duplication.

::: mobile/android/base/tests/testAMOPage.java.in
@@ +23,5 @@
> +    }
> +     
> +    public void testAMOPage() {
> +        mActions.expectGeckoEvent("Gecko:Ready").blockForEvent();
> +          

nit: please remove unnecessary whitespace.

@@ +24,5 @@
> +     
> +    public void testAMOPage() {
> +        mActions.expectGeckoEvent("Gecko:Ready").blockForEvent();
> +          
> +     openAddonManager();

this looks at a different indentation level than the other lines in this method.

::: mobile/android/base/tests/testAddonManager.java.in
@@ +30,5 @@
> +
> +        // Grab the title to make sure the about:addons page was loaded
> +        Element awesomebar = mDriver.findElement(getActivity(), "awesome_bar_title");
> +        mAsserter.isnot(awesomebar, null, "Got the awesomebar");
> +        assertMatches(awesomebar.getText(), "Add-ons", "page title match");

This code and openFromMenu is already included in the method defined in testAMOPage.java.in.  We should put that code in a central library and we have plenty of opportunites in PixelTest or BaseTest to do that.

@@ +41,5 @@
> +        //Test that opening the page from the menu does not open a new tab
> +        openFromMenu();
> +
> +        //TODO: Find a better way to wait and see if any unwanted tab is opened 
> +        pause(2000);        

I don't think factoring sleep out into a pause function is worthwhile or a good idea here.
Comment 36 Geoff Brown [:gbrown] (pto May 28-June 13) 2012-10-01 08:39:44 PDT
(In reply to Joel Maher (:jmaher) from comment #35)
> > +        //TODO: Find a better way to wait and see if any unwanted tab is opened 
> > +        pause(2000);        
> 
> I don't think factoring sleep out into a pause function is worthwhile or a
> good idea here.

mSolo.sleep() works just the same as Thread.sleep(), but does not throw an exception.

Better still, have a look at waitForTest(), which would allow you to poll for the new tab and exit as soon as one is found.

Or perhaps find an event that you can wait for that is guaranteed to come after tab creation?
Comment 37 Adrian Tamas (:AdrianT) 2012-10-02 07:06:21 PDT
Created attachment 666978 [details] [diff] [review]
addon_manager_tests_v2

(In reply to Joel Maher (:jmaher) from comment #35)
> Comment on attachment 666544 [details] [diff] [review]
> addon_manager_tests
> 
> Review of attachment 666544 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The basics here are that we are just testing to see what tabs are opened (or
> not opened) which clicking AMO related buttons/links in the fennec UI?  Is
> there anyway we could merge both tests into the same test case?  

Added the AMO test as part of the Addon Manager test

> ::: mobile/android/base/tests/testAddonManager.java.in
> @@ +30,5 @@
> > +
> > +        // Grab the title to make sure the about:addons page was loaded
> > +        Element awesomebar = mDriver.findElement(getActivity(), "awesome_bar_title");
> > +        mAsserter.isnot(awesomebar, null, "Got the awesomebar");
> > +        assertMatches(awesomebar.getText(), "Add-ons", "page title match");
> 
> This code and openFromMenu is already included in the method defined in
> testAMOPage.java.in.  We should put that code in a central library and we
> have plenty of opportunites in PixelTest or BaseTest to do that.

I created 3 new methods in BaseTest: 
 selectMenuItem - to open the menu and select the item with an intermediary check for a "More" options for Android 2.3
 verifyPageTitle - check the title of the page in the URL bar without opening the Awesomescreen
 verifyTabCount - check if the number of tabs is what is expected - this method was not suggested in the review but there are a lot of tests that check for tab count so this may help in the long run

> @@ +41,5 @@
> > +        //Test that opening the page from the menu does not open a new tab
> > +        openFromMenu();
> > +
> > +        //TODO: Find a better way to wait and see if any unwanted tab is opened 
> > +        pause(2000);        
> 
> I don't think factoring sleep out into a pause function is worthwhile or a
> good idea here.

Bypassed using sleep by doing the AMO test first and then just opening the page from menu. This would make the Addon Manager tab the current tab if it is opened in an inactive tab instead of opening a new tab. Waiting here for an event - the tab switch - solves the issue.
Comment 38 Joel Maher (:jmaher) 2012-10-02 07:43:52 PDT
Comment on attachment 666978 [details] [diff] [review]
addon_manager_tests_v2

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

overall this is a great second pass.  I just have style issues and those don't need a followup review, just an updated patch.

::: mobile/android/base/tests/BaseTest.java.in
@@ +310,5 @@
> +        //build the item name ready to be used
> +        String itemName = "^" + menuItemName + "$";
> +        
> +        mActions.sendSpecialKey(Actions.SpecialKey.MENU);
> +        

please remove the extra whitespace added in this file.

::: mobile/android/base/tests/testAddonManager.java.in
@@ +27,5 @@
> +    With the Addons Manager open the test will verify that when it is opened again from the menu no new tab will be opened*/
> +    
> +    public void testAddonManager() {
> +        mActions.expectGeckoEvent("Gecko:Ready").blockForEvent();          
> +        

please remove the extra whitespace in this file.  No extra spaces or tabs on blank lines or after a line

@@ +28,5 @@
> +    
> +    public void testAddonManager() {
> +        mActions.expectGeckoEvent("Gecko:Ready").blockForEvent();          
> +        
> +        /*Use the menu to open the Addon Manger*/ 

style nit, use // for comment.

@@ +45,5 @@
> +
> +        // Close the Add-on Manager
> +        mActions.sendSpecialKey(Actions.SpecialKey.BACK);
> +
> +        /* Load the about:addons page and verify it was loaded*/

style nit: use // for comment.

@@ +70,5 @@
> +        // Wait for the new tab and page to load
> +        tabEventExpecter.blockForEvent();
> +        contentEventExpecter.blockForEvent();
> +
> +        //Verify tab count has increased

style nit: add a space after the comment tag "// Verify..."
Comment 39 Adrian Tamas (:AdrianT) 2012-10-02 08:00:10 PDT
Created attachment 666998 [details] [diff] [review]
addon_manager_tests_v3

Corrected the styling issues indicated in the review
Comment 40 Joel Maher (:jmaher) 2012-10-02 08:07:29 PDT
if you could view the patch you uploaded in 'splinter review' on this page, you will see some whitespace issues still remain.
Comment 41 Adrian Tamas (:AdrianT) 2012-10-02 08:28:33 PDT
Created attachment 667007 [details] [diff] [review]
addon_manager_tests_v4

Sorry didn't know about the split diff feature. Thanks for all help.
Comment 42 Adrian Tamas (:AdrianT) 2012-10-02 08:32:29 PDT
Created attachment 667008 [details] [diff] [review]
addon_manager_tests_v4.1

Sorry about this but I missed one tab
Comment 43 Joel Maher (:jmaher) 2012-10-02 10:21:42 PDT
Comment on attachment 667008 [details] [diff] [review]
addon_manager_tests_v4.1

green on try
Comment 44 Adrian Tamas (:AdrianT) 2012-10-03 06:52:17 PDT
Created attachment 667464 [details] [diff] [review]
bookmarkButton_history_browsing_patch_v7

I have redone the original tests added in this bug by Eric. 

I rewrote the testBrowsing test to use loadAndPaint instead of loadUrl so it won't fail to load "robocop_blank_03.html". The same approach was used in testHistory along with changing the way the test got the History list which for me failed.

For the testBookmarkStar text I focused on testing adding a bookmark using the Bookmark button, as the original implementation of the test didn't seem to differ from the existing Robocop tests testBookmark or testBookmarkTab.

I have eliminated the test testVerifyBookmark because the implementation is more or less the exact match of the existing test testBookmark.
Comment 45 Adrian Tamas (:AdrianT) 2012-10-03 07:12:38 PDT
Created attachment 667468 [details] [diff] [review]
bookmarkButton_history_browsing_patch_v7.1

Cleaned up 2 tabs from the testBookmarkStar test
Comment 46 Joel Maher (:jmaher) 2012-10-03 08:11:32 PDT
addon test inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f6d4ab10d670
Comment 47 Ryan VanderMeulen [:RyanVM] 2012-10-03 19:01:33 PDT
https://hg.mozilla.org/mozilla-central/rev/f6d4ab10d670
Comment 48 Adrian Tamas (:AdrianT) 2012-10-04 00:20:48 PDT
I found that there actually is an older meta bug for tracking robocop tests - bug 744859. I think that this bug may be closed after the review/integration of the original patch and any new patches added in separate specific bugs blocking bug 744859.
Comment 49 Joel Maher (:jmaher) 2012-10-04 06:35:08 PDT
Comment on attachment 667468 [details] [diff] [review]
bookmarkButton_history_browsing_patch_v7.1

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

some general comments that might be addressable without a new patch.

::: mobile/android/base/tests/BaseTest.java.in
@@ +331,5 @@
> +        Element tabCount = mDriver.findElement(activity, "tabs_count");
> +        String tabCountText = tabCount.getText();
> +        int tabCountInt = Integer.parseInt(tabCountText);
> +        mAsserter.is(tabCountInt, expectedTabCount, "The correct number of tabs are opened");
> +    }

isn't all this stuff already checked in from the addons patch?

::: mobile/android/base/tests/testBookmarkStar.java.in
@@ +16,5 @@
> +    protected int getTestType() {
> +        return TEST_MOCHITEST;
> +    }
> +
> +    public void testBookmarkStar() {

Not sure I understand why this is called testBookmarkStar?  I don't see us using the star at all.

@@ +40,5 @@
> +
> +        // Click on the bookmark item and wait for the page to load
> +        mFirstChild = null;
> +        Actions.EventExpecter contentEventExpecter = mActions.expectGeckoEvent("DOMContentLoaded");
> +        mFirstChild = list.getChildAt(0);

can we guarantee that the first item in the list is our new bookmark?

::: mobile/android/base/tests/testHistory.java.in
@@ +29,5 @@
> +        verifyPageTitle("Browser Blank Page 01");
> +        loadAndPaint(url2);
> +        verifyPageTitle("Browser Blank Page 02");
> +        loadAndPaint(url3);
> +        verifyPageTitle("Browser Blank Page 03");

this is not really any different than testBrowsing.java, maybe we should just get rid of testBrowsing since we are doing the same steps here.

@@ +41,5 @@
> +        mFirstChild = hList.getChildAt(1);
> +        mAsserter.isnot(mFirstChild, null, "Got history item");
> +        mSolo.clickOnView(mFirstChild);
> +        contentEventExpecter.blockForEvent();
> +        verifyUrl(url3);

would it make sense to verify the hList.getChildAt(3) == url ?
Comment 50 Adrian Tamas (:AdrianT) 2012-10-04 07:22:53 PDT
(In reply to Joel Maher (:jmaher) from comment #49)
> Comment on attachment 667468 [details] [diff] [review]
> bookmarkButton_history_browsing_patch_v7.1
> 
> Review of attachment 667468 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> some general comments that might be addressable without a new patch.
> 
> ::: mobile/android/base/tests/BaseTest.java.in
> @@ +331,5 @@
> > +        Element tabCount = mDriver.findElement(activity, "tabs_count");
> > +        String tabCountText = tabCount.getText();
> > +        int tabCountInt = Integer.parseInt(tabCountText);
> > +        mAsserter.is(tabCountInt, expectedTabCount, "The correct number of tabs are opened");
> > +    }
> 
> isn't all this stuff already checked in from the addons patch?

This was added but at the time the addons patch was not integrated and the verifyPageTitle was needed. I can remove those now.

> ::: mobile/android/base/tests/testBookmarkStar.java.in
> @@ +16,5 @@
> > +    protected int getTestType() {
> > +        return TEST_MOCHITEST;
> > +    }
> > +
> > +    public void testBookmarkStar() {
> 
> Not sure I understand why this is called testBookmarkStar?  I don't see us
> using the star at all.

This was the original name of the test. In theory the test uses the bookmark star button. I think we can change the name. I kept it to keep the test the same as in the original patches. 
Should I add this the the testBookmark.java.in test or still keep it separate but rename it?

> @@ +40,5 @@
> > +
> > +        // Click on the bookmark item and wait for the page to load
> > +        mFirstChild = null;
> > +        Actions.EventExpecter contentEventExpecter = mActions.expectGeckoEvent("DOMContentLoaded");
> > +        mFirstChild = list.getChildAt(0);
> 
> can we guarantee that the first item in the list is our new bookmark?
Unless there is sync set up a new bookmark will be the first. I can try and search for the URL and launch it like that. I'll give it a try.

> ::: mobile/android/base/tests/testHistory.java.in
> @@ +29,5 @@
> > +        verifyPageTitle("Browser Blank Page 01");
> > +        loadAndPaint(url2);
> > +        verifyPageTitle("Browser Blank Page 02");
> > +        loadAndPaint(url3);
> > +        verifyPageTitle("Browser Blank Page 03");
> 
> this is not really any different than testBrowsing.java, maybe we should
> just get rid of testBrowsing since we are doing the same steps here.

This was just a rewrite of the original test. I am fine with removing this.

> @@ +41,5 @@
> > +        mFirstChild = hList.getChildAt(1);
> > +        mAsserter.isnot(mFirstChild, null, "Got history item");
> > +        mSolo.clickOnView(mFirstChild);
> > +        contentEventExpecter.blockForEvent();
> > +        verifyUrl(url3);
> 
> would it make sense to verify the hList.getChildAt(3) == url ?

hList is a ListView and I think this may be a bit more complex to do then just loading the History. Also this would test a simple click on a History item - https://litmus.mozilla.org/show_test.cgi?id=53467 - but I can take a look and see if I can check the item without opening it.

I'll work on changing this and i'll upload a new patch soon.
Comment 51 Adrian Tamas (:AdrianT) 2012-10-04 09:03:53 PDT
(In reply to adrian tamas from comment #50)
> (In reply to Joel Maher (:jmaher) from comment #49)
> > ::: mobile/android/base/tests/testBookmarkStar.java.in
> > @@ +16,5 @@
> > > +    protected int getTestType() {
> > > +        return TEST_MOCHITEST;
> > > +    }
> > > +
> > > +    public void testBookmarkStar() {
> > 
> > Not sure I understand why this is called testBookmarkStar?  I don't see us
> > using the star at all.
> 
> This was the original name of the test. In theory the test uses the bookmark
> star button. I think we can change the name. I kept it to keep the test the
> same as in the original patches. 
> Should I add this the the testBookmark.java.in test or still keep it
> separate but rename it?

Actually I just looked over the exiting tests again and adding bookmarks from the menu is tested in testBookmark.java.in. I believe we can also remove this test also in order not to duplicate tests.

Also testHistory is covered, although implemented in a different way in testHistoryTab so I think I should create a patch just for testBrowsing. Any thoughts on the best approach on this and what tests should I keep? Should we just consider all these tests covered?
Comment 52 Joel Maher (:jmaher) 2012-10-04 09:07:41 PDT
I would vote for keeping testHistory since it is different, but getting rid of the bookmark test. 

This is great cleanup work!
Comment 53 Adrian Tamas (:AdrianT) 2012-10-04 09:22:06 PDT
Created attachment 668017 [details] [diff] [review]
history_test_patch_v8

Leaving only the History test from the original tests.

Leaving the testing of the History item to loading the item and verifying the page as this also covers the BFT test https://litmus.mozilla.org/show_test.cgi?id=53467
Comment 54 Joel Maher (:jmaher) 2012-10-04 09:40:00 PDT
Comment on attachment 668017 [details] [diff] [review]
history_test_patch_v8

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

looks good, lets stage this on try server and then land it when that is green.
Comment 55 Mozilla RelEng Bot 2012-10-04 11:15:35 PDT
Try run for 51f85ab9ca66 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=51f85ab9ca66
Results (out of 2 total builds):
    exception: 1
    success: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/jmaher@mozilla.com-51f85ab9ca66
Comment 56 Mozilla RelEng Bot 2012-10-04 12:15:33 PDT
Try run for 51f85ab9ca66 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=51f85ab9ca66
Results (out of 3 total builds):
    exception: 1
    success: 2
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/jmaher@mozilla.com-51f85ab9ca66
Comment 58 Ed Morley [:emorley] 2012-10-06 12:52:20 PDT
https://hg.mozilla.org/mozilla-central/rev/0c62119a72d5

Note You need to log in before you can comment on or make changes to this bug.