Closed Bug 903478 Opened 11 years ago Closed 11 years ago

[fig] Update testHistoryTab (now called testMostRecentPage) for new about:home

Categories

(Firefox for Android Graveyard :: General, defect)

All
Android
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 26

People

(Reporter: lucasr, Assigned: lucasr)

References

Details

Attachments

(3 files)

      No description provided.
Attachment #788978 - Flags: review?(margaret.leibovic)
Attachment #788979 - Flags: review?(margaret.leibovic)
Attachment #788980 - Flags: review?(margaret.leibovic)
FYI: this is a straight port of the original testHistoryTab.
Comment on attachment 788978 [details] [diff] [review]
(1/3) Tag all ListViews in the new about:home UI

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

::: mobile/android/base/home/HomePager.java
@@ +41,5 @@
> +    static final String LIST_TAG_BOOKMARKS = "bookmarks";
> +    static final String LIST_TAG_READING_LIST = "reading_list";
> +    static final String LIST_TAG_MOST_VISITED = "most_visited";
> +    static final String LIST_TAG_MOST_RECENT = "most_recent";
> +    static final String LIST_TAG_LAST_TABS = "last_tabs";

Add a comment documenting what these are used for. Hopefully that will help us remember to add new tags for any new fragments we create in the future.
Attachment #788978 - Flags: review?(margaret.leibovic) → review+
Comment on attachment 788979 [details] [diff] [review]
(2/3) Add findListViewWithTag() to BaseTest

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

::: mobile/android/base/tests/BaseTest.java.in
@@ +282,5 @@
>          }
>          mAsserter.is(urlBarText, url, "Browser toolbar URL stayed the same");
>      }
>  
> +    public final ListView findListViewWithTag(String tag) {

Nit: add some documentation on top of this method.

@@ +295,5 @@
> +            }
> +        }
> +
> +        return null;
> +    }

This does seem generic enough to belong in BaseTest, but if it's only going to be used for about:home views, I wonder if it would be better in AboutHomeTest.
Attachment #788979 - Flags: review?(margaret.leibovic) → review+
Comment on attachment 788980 [details] [diff] [review]
(3/3) Add testMostRecentPage for new about:home

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

I know you're just porting over testHistoryTab, but it's still kinda painful to see some of this code being added back. Please be sure to file a follow-up on tidying this up, and perhaps add some comments in places where you know we want something to change.

::: mobile/android/base/tests/testMostRecentPage.java.in
@@ +174,5 @@
> +        }
> +    }
> +
> +    private void testClick(final ListView listview, String url) {
> +        // waitForText(url);

Can this just be removed?

@@ +222,5 @@
> +        uri = uri.buildUpon().appendQueryParameter("profile", "default")
> +                             .appendQueryParameter("sync", "true").build();
> +        resolver.delete(uri, "url = ?", new String[] {
> +            "http://mochi.test:8888/tests/robocop/robocop_blank_01.html"
> +        });

We should udpate this use the new deleteBookmark method in AboutHomeTest. It's okay to do that in a follow-up.
Attachment #788980 - Flags: review?(margaret.leibovic) → review+
(In reply to :Margaret Leibovic from comment #5)
> Comment on attachment 788978 [details] [diff] [review]
> (1/3) Tag all ListViews in the new about:home UI
> 
> Review of attachment 788978 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/home/HomePager.java
> @@ +41,5 @@
> > +    static final String LIST_TAG_BOOKMARKS = "bookmarks";
> > +    static final String LIST_TAG_READING_LIST = "reading_list";
> > +    static final String LIST_TAG_MOST_VISITED = "most_visited";
> > +    static final String LIST_TAG_MOST_RECENT = "most_recent";
> > +    static final String LIST_TAG_LAST_TABS = "last_tabs";
> 
> Add a comment documenting what these are used for. Hopefully that will help
> us remember to add new tags for any new fragments we create in the future.

Done.
(In reply to :Margaret Leibovic from comment #6)
> Comment on attachment 788979 [details] [diff] [review]
> (2/3) Add findListViewWithTag() to BaseTest
> 
> Review of attachment 788979 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/tests/BaseTest.java.in
> @@ +282,5 @@
> >          }
> >          mAsserter.is(urlBarText, url, "Browser toolbar URL stayed the same");
> >      }
> >  
> > +    public final ListView findListViewWithTag(String tag) {
> 
> Nit: add some documentation on top of this method.

Done.

> @@ +295,5 @@
> > +            }
> > +        }
> > +
> > +        return null;
> > +    }
> 
> This does seem generic enough to belong in BaseTest, but if it's only going
> to be used for about:home views, I wonder if it would be better in
> AboutHomeTest.

Fair enough. Moved to AboutHomeTest.
Pushed the first two patches in order to land the fix for bug 903448:

http://hg.mozilla.org/projects/fig/rev/45ed1f18184f
http://hg.mozilla.org/projects/fig/rev/fbbc87078c3c

I'll push the last patch once the test goes green on try.
https://hg.mozilla.org/mozilla-central/rev/45ed1f18184f
https://hg.mozilla.org/mozilla-central/rev/fbbc87078c3c
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 26
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: