Closed Bug 899187 Opened 6 years ago Closed 6 years ago

[fig] Create new testBookmarksPage test to replace original testBookmarksTab.java.in

Categories

(Firefox for Android :: General, defect, P1)

All
Android
defect

Tracking

()

RESOLVED FIXED
Firefox 28
Tracking Status
fennec + ---

People

(Reporter: capella, Assigned: Margaret)

References

Details

Attachments

(1 file, 6 obsolete files)

Bug 896576 removed the getBookmarksList() logic from BaseTest.java.in and removed this test (which depends on it).

This needs to be re-implemented when the new Testing |utility class to get things from about:home| concept is finalized.
Assignee: nobody → adrian.tamas
Blocks: 895673
Attached patch Bookmarks Page test (obsolete) — Splinter Review
Recreated the test mirroring the old testBookmarks tab. In the first part I am testing the folder hierachy and in the second I am testing the bookmark context menu. For the Top Bookmarks section I would rather make a new test then to extend this any more.
Attachment #793383 - Flags: review?(lucasr.at.mozilla)
Depends on: 899182
tracking-fennec: --- → ?
Priority: -- → P1
Hardware: ARM → All
tracking-fennec: ? → 26+
Comment on attachment 793383 [details] [diff] [review]
Bookmarks Page test

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

The amount of reflection based-code here is kinda disturbing. But I'm willing to let this in as a temporary measure until we implement the new UI testing API.
Attachment #793383 - Flags: review?(lucasr.at.mozilla) → review+
Depends on: 916107
Adrian, just wondering: are you just waiting for someone to push your patch?
Flags: needinfo?(adrian.tamas)
Bug 919516 would cause this test not to work. Also I need to redo the patch to account for the changes in the new-new-about-home. I'll make the changes after bug 919516 is fixed. Also this needs bug 899182 to land since I am using a lot of the methods created there.
Flags: needinfo?(adrian.tamas)
Depends on: 919516
Attached patch bookmarksPage.patch (obsolete) — Splinter Review
Updated the patch to remove the GridView tests since it was moved. Also I updated tests to not use as many reflections.

Tryrun: https://tbpl.mozilla.org/?tree=Try&rev=d66b4d4ea30e
Attachment #793383 - Attachment is obsolete: true
Attachment #814102 - Flags: review?(lucasr.at.mozilla)
Comment on attachment 814102 [details] [diff] [review]
bookmarksPage.patch

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

Looks fine overall, just want the questions answered before giving r+

::: mobile/android/base/tests/testBookmarksPage.java.in
@@ +160,5 @@
> +            for (int i = 0; i < adapter.getCount(); i++ ) {
> +                View bookmarkView = bookmarksTabList.getChildAt(i);
> +                try {
> +                    ClassLoader classLoader = getActivity().getClassLoader();
> +                    Class bookmarkFolderView = classLoader.loadClass("org.mozilla.gecko.home.BookmarkFolderView");

Is this use of reflection actually needed here? I mean, BookmarkFolderView is just a TextView anyway.

@@ +186,5 @@
> +        try {
> +            ClassLoader classLoader = getActivity().getClassLoader();
> +            Class syncUtilityClass = classLoader.loadClass("org.mozilla.gecko.sync.Utils");
> +            Method generateGuid = syncUtilityClass.getMethod("generateGuid", (Class[]) null);
> +            generatedGuid = (String)generateGuid.invoke(null);

Move this to DatabaseHelper or something? Doesn't the content provider create a guid for us anyway?
Attachment #814102 - Flags: review?(lucasr.at.mozilla) → feedback+
(In reply to Lucas Rocha (:lucasr) from comment #6)
> Comment on attachment 814102 [details] [diff] [review]
> bookmarksPage.patch
> 
> Review of attachment 814102 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks fine overall, just want the questions answered before giving r+
> 
> ::: mobile/android/base/tests/testBookmarksPage.java.in
> @@ +160,5 @@
> > +            for (int i = 0; i < adapter.getCount(); i++ ) {
> > +                View bookmarkView = bookmarksTabList.getChildAt(i);
> > +                try {
> > +                    ClassLoader classLoader = getActivity().getClassLoader();
> > +                    Class bookmarkFolderView = classLoader.loadClass("org.mozilla.gecko.home.BookmarkFolderView");
> 
> Is this use of reflection actually needed here? I mean, BookmarkFolderView
> is just a TextView anyway.
> 

Just rechecked now, I don't remember why I thought the TwoLinewPageRow was a text view and I could not differentiate between them. I'll remove the reflection there.

> @@ +186,5 @@
> > +        try {
> > +            ClassLoader classLoader = getActivity().getClassLoader();
> > +            Class syncUtilityClass = classLoader.loadClass("org.mozilla.gecko.sync.Utils");
> > +            Method generateGuid = syncUtilityClass.getMethod("generateGuid", (Class[]) null);
> > +            generatedGuid = (String)generateGuid.invoke(null);
> 
> Move this to DatabaseHelper or something? Doesn't the content provider
> create a guid for us anyway?
The insert needs a guid and it was first just the url used as guid but Margaret suggested in https://bugzilla.mozilla.org/show_bug.cgi?id=901432#c9 to generate it. This is used just to add desktop bookmarks since the methods for those actions are not public. We can move it to the DatabaseHelper if it will be needed in other tests at some point but since this is used just here maybe keep it here?
Attached patch bookmarksPage.patch (obsolete) — Splinter Review
Removed the DesktopFolderView reflection
Attachment #814102 - Attachment is obsolete: true
Attachment #815851 - Flags: review?(lucasr.at.mozilla)
Comment on attachment 815851 [details] [diff] [review]
bookmarksPage.patch

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

Looks good, just needs nits fixed.

::: mobile/android/base/tests/testBookmarksPage.java.in
@@ +57,5 @@
> +            mAsserter.is(adapter.getCount(), 4, "Checking that the correct number of folders is displayed in the Desktop Bookmarks folder");
> +        }
> +
> +        clickOnBookmarkFolder(StringHelper.TOOLBAR_FOLDER_LABEL);
> +        

nit: remove trailing space here.

@@ +158,5 @@
> +        ListAdapter adapter = bookmarksTabList.getAdapter();
> +        if (adapter != null) {
> +            for (int i = 0; i < adapter.getCount(); i++ ) {
> +                View bookmarkView = bookmarksTabList.getChildAt(i);
> +                if (bookmarkView instanceof android.widget.TextView) {

nit: import android.widget.TextView and use it as simply 'TextView' here.

@@ +185,5 @@
> +        } catch (Exception e) {
> +            mAsserter.dumpLog("Exeption in setUpDesktopBookmarks" + e);
> +        }
> +        if (generatedGuid == null) {
> +            mAsserter.ok(false, "Generating a random Guid for the bookmark", "We could not generate a Guid for the bookmark");

It would be simpler to just assert (generatedGuid != null), no?
Attachment #815851 - Flags: review?(lucasr.at.mozilla) → review+
Attached patch bookmarksPage.patch (obsolete) — Splinter Review
Addressed the nit requests.
Attachment #815851 - Attachment is obsolete: true
Attachment #816517 - Flags: review?(lucasr.at.mozilla)
Comment on attachment 816517 [details] [diff] [review]
bookmarksPage.patch

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

Yep.
Attachment #816517 - Flags: review?(lucasr.at.mozilla) → review+
https://hg.mozilla.org/mozilla-central/rev/0b6f43cbb7dd
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 27
Caused bug 926655 and also now:
https://tbpl.mozilla.org/php/getParsedLog.php?id=29138880&tree=Mozilla-Inbound
{
7 INFO TEST-UNEXPECTED-FAIL | testBookmarksPage | Checking that default bookmark: about:firefox is displayed in the bookmarks list - about:firefox is displayed as a bookmark
junit.framework.AssertionFailedError: 7 INFO TEST-UNEXPECTED-FAIL | testBookmarksPage | Checking that default bookmark: about:firefox is displayed in the bookmarks list - about:firefox is displayed as a bookmark
8 INFO TEST-UNEXPECTED-FAIL | testBookmarksPage | Exception caught - junit.framework.AssertionFailedError: 7 INFO TEST-UNEXPECTED-FAIL | testBookmarksPage | Checking that default bookmark: about:firefox is displayed in the bookmarks list - about:firefox is displayed as a bookmark
10-15 08:49:00.255 I/Robocop ( 2455): 7 INFO TEST-UNEXPECTED-FAIL | testBookmarksPage | Checking that default bookmark: about:firefox is displayed in the bookmarks list - about:firefox is displayed as a bookmark
10-15 08:49:00.295 I/Robocop ( 2455): junit.framework.AssertionFailedError: 7 INFO TEST-UNEXPECTED-FAIL | testBookmarksPage | Checking that default bookmark: about:firefox is displayed in the bookmarks list - about:firefox is displayed as a bookmark
10-15 08:49:00.295 I/Robocop ( 2455): 8 INFO TEST-UNEXPECTED-FAIL | testBookmarksPage | Exception caught - junit.framework.AssertionFailedError: 7 INFO TEST-UNEXPECTED-FAIL | testBookmarksPage | Checking that default bookmark: about:firefox is displayed in the bookmarks list - about:firefox is displayed as a bookmark
10-15 08:49:01.725 I/TestRunner( 2455): junit.framework.AssertionFailedError: 8 INFO TEST-UNEXPECTED-FAIL | testBookmarksPage | Exception caught - junit.framework.AssertionFailedError: 7 INFO TEST-UNEXPECTED-FAIL | testBookmarksPage | Checking that default bookmark: about:firefox is displayed in the bookmarks list - about:firefox is displayed as a bookmark
}

Backed out for now:
remote:   https://hg.mozilla.org/integration/fx-team/rev/79360e1c33a5
Status: RESOLVED → REOPENED
Depends on: 926655
Resolution: FIXED → ---
No longer depends on: 926655
Duplicate of this bug: 926655
We've got most test ported to new about:home. No need to keep test-related bugs in Fx26 at this point.
tracking-fennec: 26+ → +
Attached patch bookmarksPage.patch (obsolete) — Splinter Review
Try run: https://tbpl.mozilla.org/?tree=Try&rev=99b79db13bd8

Splitting the test in 2 tests: testBookmarksPage and testBookmarkFolders to ensure the test runs better and the transition between the 2 tests does not create any issues. Also addressed the previous intermittent fails hopefully.
Attachment #816517 - Attachment is obsolete: true
Attachment #823360 - Flags: review?(lucasr.at.mozilla)
Comment on attachment 823360 [details] [diff] [review]
bookmarksPage.patch

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

Yep.
Attachment #823360 - Flags: review?(lucasr.at.mozilla) → review+
Attached patch bookmarksPage.patch (obsolete) — Splinter Review
New version since the last one had one fail.

tryrun: https://tbpl.mozilla.org/?tree=Try&rev=97ea5199e4a9
Attachment #823360 - Attachment is obsolete: true
Try run: https://tbpl.mozilla.org/?tree=Try&rev=7c2c40c34029

Added extra tests to make sure the views exist when trying to click them and that they are displayed.
Attachment #824040 - Attachment is obsolete: true
Attachment #825916 - Flags: review?(lucasr.at.mozilla)
Comment on attachment 825916 [details] [diff] [review]
bookmarksPage.patch

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

::: mobile/android/base/tests/StringHelper.java.in
@@ +44,5 @@
>          "Edit",
>          "Add to Home Screen"
>      };
>  
> +    public static final String[] BOOKMARK_CONTEXT_MENU_ITEMS = {"Open in New Tab", "Open in Private Tab", "Share", "Edit", "Remove", "Add to Home Screen"};

Drive-by: We're removing "Share" and "Add to Home Screen" in bug 933428.
Assignee: adrian.tamas → nobody
Comment on attachment 825916 [details] [diff] [review]
bookmarksPage.patch

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

This is a lot of code, kinda hard to review. But this looks good overall. Margaret, could you update the patch to account for the changes in bug 933428? I'm assuming Adrian won't be working on this patch anymore.

::: mobile/android/base/tests/AboutHomeTest.java.in
@@ +67,5 @@
> +            @Override
> +            public boolean test() {
> +                View bookmark = getDisplayedBookmark(url);
> +                if (bookmark != null) {
> +                    return mDatabaseHelper.isBookmark(url)?true:false;

nit: add space around the colon.

@@ +69,5 @@
> +                View bookmark = getDisplayedBookmark(url);
> +                if (bookmark != null) {
> +                    return mDatabaseHelper.isBookmark(url)?true:false;
> +                } else {
> +                    return mDatabaseHelper.isBookmark(url)?false:true;

Ditto.

@@ +73,5 @@
> +                    return mDatabaseHelper.isBookmark(url)?false:true;
> +                }
> +            }
> +        }, MAX_WAIT_MS);
> +        mAsserter.ok(isCorrect, "Checking that " + url + (mDatabaseHelper.isBookmark(url)?" is":" is not")  + " displayed as a bookmark", url + (mDatabaseHelper.isBookmark(url)?" is":" is not") + " displayed");

Using isBookmark here just to tweak the log message seems overkill, no?

@@ +105,5 @@
>      // @return the View associated with bookmark for the provided url or null if the link is not bookmarked
>      protected View getDisplayedBookmark(String url) {
>          openAboutHomeTab(AboutHomeTabs.BOOKMARKS);
> +        mSolo.hideSoftKeyboard();
> +        getInstrumentation().waitForIdleSync();

Why is this necessary now? A comment here would probably be helpful.

@@ +195,3 @@
>          mSolo.clickOnText("Edit");
>          waitForText("Edit Bookmark");
> +        for (String value:newValues) {

nit: add space around the colon.

::: mobile/android/base/tests/testBookmarksPage.java.in
@@ +3,5 @@
> +
> +import @ANDROID_PACKAGE_NAME@.*;
> +
> +public class testBookmarksPage extends AboutHomeTest {
> +    private static String BOOKMARK_URL;

No need to have this as a class member.

@@ +19,5 @@
> +
> +        openAboutHomeTab(AboutHomeTabs.BOOKMARKS);
> +
> +        // Check that the default bookmarks are displayed
> +        for (String url:StringHelper.DEFAULT_BOOKMARKS_URLS) {

nit: space around the colon.

@@ +27,5 @@
> +        // Open default bookmarks in a new tab and a new private tab since the url is substituted with "Switch to tab" after opening the link
> +        openBookmarkContextMenu(StringHelper.DEFAULT_BOOKMARKS_URLS[1]);
> +
> +        // Test that the options are all displayed
> +        for (String contextMenuOption:StringHelper.BOOKMARK_CONTEXT_MENU_ITEMS) {

nit: space around the colon.
Attachment #825916 - Flags: review?(lucasr.at.mozilla) → review+
Flags: needinfo?(margaret.leibovic)
Sure, I'll take this to finish it.
Assignee: nobody → margaret.leibovic
Flags: needinfo?(margaret.leibovic)
Updated and pushed to try:
https://tbpl.mozilla.org/?tree=Try&rev=d23c9f0370f1

However, running this locally, I'm finding that openAboutHomeTab(AboutHomeTabs.BOOKMARKS); is swiping too far and opening the reading list page instead of the bookmarks page.

Chenxia, do you have any idea what might be going wrong for me? I'm running these tests on a Nexus 4.
Flags: needinfo?(liuche)
Summarizing discussion on irc: when updating the new about:home test (AboutHomeTest.java.in) I updated the screen side-swipe distance to the entire width of the screen (instead of half, as it was before). This was due to some problems with underswiping with tablets.
Flags: needinfo?(liuche)
Looking green on try, so I pushed the updated patch:
https://hg.mozilla.org/integration/fx-team/rev/16b40185dd3f

I filed bug 935244 about the issue I was seeing running this locally on my phone.
https://hg.mozilla.org/mozilla-central/rev/16b40185dd3f
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Target Milestone: Firefox 27 → Firefox 28
You need to log in before you can comment on or make changes to this bug.