897483 - [fig] Create new testVisitedPage test to replace original testAllPagesTab.java.in

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Mark Capella [:capella]]

Bug 896576 removed getAllPagesList from BaseTest obviating testAllPagesTab.java.in

This needs to be re-implemented when the new Testing |utility class to get things from about:home| concept is finalized.

Comment 1

[Lucas Rocha (:lucasr)]

Attachment: (1/2) Add waitForListToLoad() to BaseTest

Comment 2

[Lucas Rocha (:lucasr)]

Attachment: (2/2) Add testMostVisitedPage for new about:home

Comment 3

[Lucas Rocha (:lucasr)]

FYI: this is a straight port of testAllPagesTab to new about:home.

Comment 4

[:Margaret Leibovic]

Comment on attachment 789097 [details] [diff] [review]
(1/2) Add waitForListToLoad() to BaseTest

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

::: mobile/android/base/tests/BaseTest.java.in
@@ +453,5 @@
>          }
>          return rc;
>      }
>  
> +    public boolean waitForListToLoad(final ListView listView) {

Please add some documentation about what this does.

@@ +471,5 @@
> +            }
> +        }, MAX_WAIT_MS);
> +
> +        return result;
> +    }

Same comment I had in bug 903478... if this is going to just be used to test about:home, perhaps this belongs in AboutHomeTest.

Comment 5

[:Margaret Leibovic]

Comment on attachment 789098 [details] [diff] [review]
(2/2) Add testMostVisitedPage for new about:home

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

Similarly to my comment in bug 903478, we should make sure there's a follow-up filed to clean this up more.

::: mobile/android/base/tests/testMostVisitedPage.java.in
@@ +20,5 @@
> +   and long tapping on an item
> +*/
> +
> +public class testMostVisitedPage extends AboutHomeTest {
> +    private static ListView listview = null;

Now that we just get the ListView once in testMostVisitedPage and pass it around as a parameter, we could get rid of this.

@@ +31,5 @@
> +    public void testMostVisitedPage() {
> +        blockForGeckoReady();
> +
> +        // load one page so there is something in our history
> +        String url = "http://www.arstechnica.com/";//getAbsoluteUrl("/robocop/robocop_big_link.html");

Was there something wrong with the robocop pages? I don't think we'd want to leave this in our test suite.

@@ +87,5 @@
> +            mAsserter.is(visible, expectedImages, "Correct number of ImageViews visible");
> +        }
> +    }
> +
> +    private void testContextMenu(ListView list, String url) {

I feel like a good follow-up might be to move all the context menu related testing into its own test, since this looks very similar to the code in your new testMostRecentPage.

Comment 6

[Lucas Rocha (:lucasr)]

(In reply to :Margaret Leibovic from comment #4)
> Comment on attachment 789097 [details] [diff] [review]
> (1/2) Add waitForListToLoad() to BaseTest
> 
> Review of attachment 789097 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/tests/BaseTest.java.in
> @@ +453,5 @@
> >          }
> >          return rc;
> >      }
> >  
> > +    public boolean waitForListToLoad(final ListView listView) {
> 
> Please add some documentation about what this does.

Done.

> @@ +471,5 @@
> > +            }
> > +        }, MAX_WAIT_MS);
> > +
> > +        return result;
> > +    }
> 
> Same comment I had in bug 903478... if this is going to just be used to test
> about:home, perhaps this belongs in AboutHomeTest.

Done.

Comment 7

[Lucas Rocha (:lucasr)]

Pushed the first patch in order to land the fix for bug 903448:

http://hg.mozilla.org/projects/fig/rev/c67df6233bd1

I'll push the second patch once the test goes green on try.

Comment 8

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/c67df6233bd1