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

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Comment 1

[Lucas Rocha (:lucasr)]

Attachment: (1/3) Tag all ListViews in the new about:home UI

Comment 2

[Lucas Rocha (:lucasr)]

Attachment: (2/3) Add findListViewWithTag() to BaseTest

Comment 3

[Lucas Rocha (:lucasr)]

Attachment: (3/3) Add testMostRecentPage for new about:home

Comment 4

[Lucas Rocha (:lucasr)]

FYI: this is a straight port of the original testHistoryTab.

Comment 5

[:Margaret Leibovic]

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.

Comment 6

[:Margaret Leibovic]

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.

Comment 7

[:Margaret Leibovic]

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.

Comment 8

[Lucas Rocha (:lucasr)]

(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.

Comment 9

[Lucas Rocha (:lucasr)]

(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.

Comment 10

[Lucas Rocha (:lucasr)]

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.

Comment 11

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/45ed1f18184f
https://hg.mozilla.org/mozilla-central/rev/fbbc87078c3c