Closed
Bug 899187
Opened 11 years ago
Closed 11 years ago
[fig] Create new testBookmarksPage test to replace original testBookmarksTab.java.in
Categories
(Firefox for Android Graveyard :: General, defect, P1)
Tracking
(fennec+)
RESOLVED
FIXED
Firefox 28
Tracking | Status | |
---|---|---|
fennec | + | --- |
People
(Reporter: capella, Assigned: Margaret)
References
Details
Attachments
(1 file, 6 obsolete files)
(deleted),
patch
|
lucasr
:
review+
|
Details | Diff | Splinter Review |
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.
Comment 1•11 years ago
|
||
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)
Updated•11 years ago
|
tracking-fennec: --- → ?
Priority: -- → P1
Hardware: ARM → All
Updated•11 years ago
|
tracking-fennec: ? → 26+
Comment 2•11 years ago
|
||
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+
Comment 3•11 years ago
|
||
Adrian, just wondering: are you just waiting for someone to push your patch?
Flags: needinfo?(adrian.tamas)
Comment 4•11 years ago
|
||
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)
Comment 5•11 years ago
|
||
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 6•11 years ago
|
||
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+
Comment 7•11 years ago
|
||
(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?
Comment 8•11 years ago
|
||
Removed the DesktopFolderView reflection
Attachment #814102 -
Attachment is obsolete: true
Attachment #815851 -
Flags: review?(lucasr.at.mozilla)
Comment 9•11 years ago
|
||
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+
Comment 10•11 years ago
|
||
Addressed the nit requests.
Attachment #815851 -
Attachment is obsolete: true
Attachment #816517 -
Flags: review?(lucasr.at.mozilla)
Comment 11•11 years ago
|
||
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+
Updated•11 years ago
|
Keywords: checkin-needed
Comment 12•11 years ago
|
||
Flags: in-testsuite+
Keywords: checkin-needed
Comment 13•11 years ago
|
||
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 27
Comment 14•11 years ago
|
||
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
Comment 16•11 years ago
|
||
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+ → +
Comment 17•11 years ago
|
||
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 18•11 years ago
|
||
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+
Comment 19•11 years ago
|
||
New version since the last one had one fail.
tryrun: https://tbpl.mozilla.org/?tree=Try&rev=97ea5199e4a9
Attachment #823360 -
Attachment is obsolete: true
Comment 20•11 years ago
|
||
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)
Assignee | ||
Comment 21•11 years ago
|
||
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.
Updated•11 years ago
|
Assignee: adrian.tamas → nobody
Comment 22•11 years ago
|
||
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+
Updated•11 years ago
|
Flags: needinfo?(margaret.leibovic)
Assignee | ||
Comment 23•11 years ago
|
||
Sure, I'll take this to finish it.
Assignee: nobody → margaret.leibovic
Flags: needinfo?(margaret.leibovic)
Assignee | ||
Comment 24•11 years ago
|
||
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)
Comment 25•11 years ago
|
||
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)
Assignee | ||
Comment 26•11 years ago
|
||
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.
Status: REOPENED → RESOLVED
Closed: 11 years ago → 11 years ago
Resolution: --- → FIXED
Target Milestone: Firefox 27 → Firefox 28
Updated•4 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•