Closed Bug 899182 Opened 7 years ago Closed 7 years ago

[fig] Re-implement robocop testBookmark.java.in

Categories

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

All
Android
defect

Tracking

()

RESOLVED FIXED
Firefox 27
Tracking Status
firefox26 --- fixed
firefox27 --- fixed
fennec 26+ ---

People

(Reporter: capella, Assigned: AdrianT)

References

Details

Attachments

(1 file, 5 obsolete files)

Bug 896576 removed the getBookmarksList() logic from BaseTest.java.in and disabled 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.
Blocks: 895673
QA Contact: adrian.tamas
Attached patch testBookmark rewrite (obsolete) — Splinter Review
Added a few new methods in AboutHomeTest to get bookmarks/top bookmarks and to check if bookmarks/top bookmarks are displayed. Not all are used in testBookmarks, but they will be needed in testBookmarksTab. The only issue I had was that getting the view for the bookmark in getDisplayedBookmark(String url) with getView(i,null, bookmarksTabList) caused the clickOnView and longClickOnView not to work for some reason. I get the view with getChildAt but I make sure the view is scrolled to the bottom first so this should not create issues.

Added the new waitForAboutHomeTab method to wait for the tab to be rendered when changing tabs. waitForEnabledText no longer works reliably for me locally on the tablets with the recent changes in about:home

BaseTest.toggleBookmark does not work for me on tablets. Also there were some changes to the Navigation.Bookmark() method at some point which led to it not working anymore. Corrected the device/os verification and now it works.

I re-wrote the testBookmark test, covering the original tests and also adding tests for the Top Bookmarks section.
Attachment #791274 - Flags: review?(lucasr.at.mozilla)
Attached patch testBookmark fix v2 (obsolete) — Splinter Review
removed the openAboutHomeTab fix which was moved to bug 906662
Attachment #791274 - Attachment is obsolete: true
Attachment #791274 - Flags: review?(lucasr.at.mozilla)
Attachment #792200 - Flags: review?(lucasr.at.mozilla)
Blocks: 899187
Assignee: nobody → adrian.tamas
tracking-fennec: --- → ?
Priority: -- → P1
Hardware: ARM → All
tracking-fennec: ? → 26+
Comment on attachment 792200 [details] [diff] [review]
testBookmark fix v2

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

Needs a rebase to fetch lists by tag instead of using reflection.
Attachment #792200 - Flags: review?(lucasr.at.mozilla) → review-
Depends on: 916107
Attached patch testBookmark.patch v3 (obsolete) — Splinter Review
Added tags to views in bug 916107, rewrote all methods to work with the tags and only left the getTopBookmark to use reflections since I need the getURl method. Also changed the Navigation.Bookmark method which did not seem to work on pandas because it has the 7"tablet UI which has the bookmark button in the menu and not on the UI.
Attachment #792200 - Attachment is obsolete: true
Attachment #804460 - Flags: review?(lucasr.at.mozilla)
Attached patch testBookmark.patch v4 (obsolete) — Splinter Review
Changes the patch to not rely on a tagged Top Bookmarks view and also to work with the recently integrated DatabaseHelper and StringHelper classes
Attachment #804460 - Attachment is obsolete: true
Attachment #804460 - Flags: review?(lucasr.at.mozilla)
Attachment #805258 - Flags: review?(lucasr.at.mozilla)
Attached patch testBookmark.patch v4.1 (obsolete) — Splinter Review
The try run for the last version had some intermittent fails because the bookmarks page content was not rendered in time. Added a wait until the bookmark page content is displayed and pushed a new tryrun: https://tbpl.mozilla.org/?tree=Try&rev=3ba54df36ad2
Attachment #805258 - Attachment is obsolete: true
Attachment #805258 - Flags: review?(lucasr.at.mozilla)
Attachment #805345 - Flags: review?(lucasr.at.mozilla)
Comment on attachment 805345 [details] [diff] [review]
testBookmark.patch v4.1

removing the review flag per the chat on irc since the patch will be modified to remove the tests for Top Bookmarks
Attachment #805345 - Flags: review?(lucasr.at.mozilla)
Depends on: 919516
Removed the top bookmarks part of the test because of the about:home changes. This works locally but a tryrun is pointless now until bug 919516 is fixed
Attachment #805345 - Attachment is obsolete: true
Attachment #808579 - Flags: review?(lucasr.at.mozilla)
Comment on attachment 808579 [details] [diff] [review]
testBookmark.patch

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

Looks good.

::: mobile/android/base/tests/AboutHomeTest.java.in
@@ +102,5 @@
> +        ListAdapter adapter = bookmarksTabList.getAdapter();
> +        if (adapter != null) {
> +            for (int i = 0; i < adapter.getCount(); i++ ) {
> +                // I am unable to click the view taken with getView for some reason so getting the child at i
> +                bookmarksTabList.smoothScrollToPosition(i);

This is likely to cause intermittent due to timing issues from the smooth scrolling, no? Not against it, just wondering.
Attachment #808579 - Flags: review?(lucasr.at.mozilla) → review+
(In reply to Lucas Rocha (:lucasr) from comment #9)
> Comment on attachment 808579 [details] [diff] [review]
> testBookmark.patch
> 
> Review of attachment 808579 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good.
> 
> ::: mobile/android/base/tests/AboutHomeTest.java.in
> @@ +102,5 @@
> > +        ListAdapter adapter = bookmarksTabList.getAdapter();
> > +        if (adapter != null) {
> > +            for (int i = 0; i < adapter.getCount(); i++ ) {
> > +                // I am unable to click the view taken with getView for some reason so getting the child at i
> > +                bookmarksTabList.smoothScrollToPosition(i);
> 
> This is likely to cause intermittent due to timing issues from the smooth
> scrolling, no? Not against it, just wondering.

I haven't had any issues with it. Usually the scroll is very short for since we don't have long bookmark lists tests.
Adrian, are you just waiting for someone to push your patch?
Flags: needinfo?(adrian.tamas)
Until bug 919516 is fixed this will be permaorange on pandas. This is why I didn't even bother to do a tryserver run with this. Once bug 919516 is fixed I can do a run with the patch and if everything is ok we can push it.
Flags: needinfo?(adrian.tamas)
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/35b839744801
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/35b839744801
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 27
Needs bug 919516 first, duh.
https://hg.mozilla.org/releases/mozilla-aurora/rev/6c1ea36a42e6
Whiteboard: [don't uplift until bug 919516 has approval]
You need to log in before you can comment on or make changes to this bug.