896576 - [fig] Remove getAllPagesList/getBookmarksList/getHistoryList from BaseTest

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[:Margaret Leibovic]

This methods don't do anything anymore (I disabled the tests that depend on them).

Instead of re-implementing them for the new about:home, I think it would be better to create a utility class or something for testing things related to about:home, instead of continuing to dump things in BaseTest.

Comment 1

[Mark Capella [:capella]]

Attachment: Remove [getAllPagesList]

This removes the [getAllPagesList] pieces ...

Let me  know if I'm confused and you just wanted to remove those 6 method stubs in BaseTest and leave everything else in place (for later?) ...

Comment 2

[:Margaret Leibovic]

Comment on attachment 779532 [details] [diff] [review]
Remove [getAllPagesList]

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

This is looking good, but I want to see a new version that doesn't delete so much stuff. I also want to make sure we file the follow-up bugs I mentioned.

This is a good first step, but we'll need to replace the places that getAllPagesList is used with some other call to get the visited list in the new about:home. I'd like to see if there's a way we can make a utility class to get things from about:home, rather than sticking those methods back in BaseTest.

::: mobile/android/base/tests/robocop_autophone.ini
@@ -4,5 @@
>  # fails on gs2, nexus one, lg revolution, droid pro, nexus s
>  
> -#[testAllPagesTab]
> -# fails on gs2, nexus one, lg revolution, droid pro, nexus s
> -

Huh, I didn't know this file existed, we didn't update it in bug 880060. But if you're killing this test, this is the right thing to do.

::: mobile/android/base/tests/testAllPagesTab.java.in
@@ -19,5 @@
> -/* Tests opening the all pages tab, that items look correct, clicking on an item
> -   and long tapping on an item
> -*/
> -
> -public class testAllPagesTab extends BaseTest {

Please file a follow-up bug about replacing this test with a new testVisitedPage test. We can probably salvage some of the test logic from here in a new test for the new visited page.

::: mobile/android/base/tests/testAwesomebarSwipes.java.in
@@ -21,5 @@
>  
>      public void testAwesomebarSwipes() {
>          blockForGeckoReady();
>  
> -        ListView list = getAllPagesList("about:firefox");

Let's just comment this line out for now, and add a comment saying that getAllPagesList was removed in this bug. This will make us less confused when someone goes to fix bug 896565.

::: mobile/android/base/tests/testShareLink.java.in
@@ -82,5 @@
> -        } else {
> -            // The view should not be null but sometimes getChildAt fails
> -            // TODO: Investigate why this fails
> -            mAsserter.todo_isnot(allpages, null, "View should not be null but sometimes it is");
> -        }

This is all commented out anyway, so we don't need to remove this right now. You can add a comment above the getAllPagesList call to say it was removed in this bug.

Could you also file a bug to fix this part of this test (and have it block 895673)? We'll want to fix this, rather than just remove it.

Comment 3

[Mark Capella [:capella]]

Bug 897481 - [fig] Update testShareLink.java.in - re-implement [Test share popup in Top Sites]

Comment 4

[Mark Capella [:capella]]

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

Comment 5

[Mark Capella [:capella]]

Attachment: Remove [getAllPagesList]

If this gets review+, I plan to move ahead to the next piece for [getBookmarksList] ... same approach as this one ...

Comment 6

[:Margaret Leibovic]

Comment on attachment 780418 [details] [diff] [review]
Remove [getAllPagesList]

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

Looks good to me. Thanks for filing those follow-ups.

Comment 7

[Mark Capella [:capella]]

first patch [getAllPagesList] ... 
https://hg.mozilla.org/projects/fig/rev/aab022ea5da6

Comment 8

[Mark Capella [:capella]]

Attachment: Remove [getBookmarksList]

for [getBookmarksList] I'm thinking you'll want to:

   Remove the method stubs in BaseTest
   Remove testBookmarksTab and file a bug to create a new testBookmarksPage
   Comment and file a bug to update |share popup in the Bookmarks tab| in testShareLink.java.in
   Comment and file a bug to update |runAwesomeScreenTest()| in testBookmark
   Comment and file a bug to update |bookmarklets clicked in awesomescreen| in testBookmarklets

Comment 9

[:Margaret Leibovic]

Comment on attachment 781321 [details] [diff] [review]
Remove [getBookmarksList]

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

Looks good, but make sure you get those follow-up bugs filed.

::: mobile/android/base/tests/robocop_autophone.ini
@@ +9,5 @@
>  
>  #[testAxisLocking]
>  # fails on gs2, nexus one, lg revolution, droid pro, nexus s
>  
> +# [testBookmark]

I don't think we should disable these on fig, since they're not posing a problem for us right now on TBPL, and I don't want us to forget about re-enabling them.

::: mobile/android/base/tests/testBookmark.java.in
@@ +38,5 @@
>              mAsserter.is(true, false, "Unable to find method");
>          }
>  
> +        // Removed by Bug 896576 - [fig] Remove [getBookmarksList] from BaseTest
> +        // runAwesomeScreenTest();

With this disabled, does this test pass? Maybe we could re-enable testBookmark if that's the case, then fix this part in a follow-up.

::: mobile/android/base/tests/testBookmarklets.java.in
@@ +59,5 @@
>          }
>          if (!found) {
>              mAsserter.is(found, true, "Found the bookmark: " + js + " and clicked on it");
>          }
> +        */

Same question here, I wonder if we could just comment out enough broken code to get the test to pass, re-enable it, and fix this part in a follow-up. If it's non-trivial to try to fix it though, we can just deal with the re-enabling part in the follow-up.

::: mobile/android/base/tests/testShareLink.java.in
@@ +90,2 @@
>          // Test the share popup in the Bookmarks tab
>          ListView blist = getBookmarksList("about:firefox");

At first I was confused, then realized that this is in the middle of a commented-out chunk :)

Comment 10

[Mark Capella [:capella]]

Quick question for later ...

why the diff between robocop.ini and robocop_autophone.ini ? Should any given robocop test be defined in both robocop.ini and robocop_autophone.ini, or is it an either/or situation? (I see active 7 tests run from both places)

(8 counting testMigration but we removed that in m-c)

[testAboutPage], [testAwesomebar], [testBrowserProvider], [testFormHistory], [testNewTab], [testPasswordProvider], [testSystemPages]

Comment 11

[Mark Capella [:capella]]

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

Comment 12

[Mark Capella [:capella]]

Bug 899183 - [fig] Re-implement robocop testBookmarklets.java.in

Comment 13

[Mark Capella [:capella]]

Bug 899185 - [fig] Update testShareLink.java.in - re-implement [Test the share popup in the Bookmarks tab]

Comment 14

[Mark Capella [:capella]]

Bug 899187 - [fig] Create new testBookmarksPage test to replace original testBookmarksTab.java.in

Comment 15

[Mark Capella [:capella]]

Attachment: Remove [getBookmarksList]

Without getBookmarksList() there's really nothing of value in partially enabling the remaining tests, so I've left then disabled as before.

Comment 16

[:Margaret Leibovic]

Comment on attachment 782735 [details] [diff] [review]
Remove [getBookmarksList]

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

Once again, thanks for filing all those follow-up bugs.

Comment 17

[Mark Capella [:capella]]

Comment on attachment 782735 [details] [diff] [review]
Remove [getBookmarksList]

then it's on to the last piece |getHistoryList|

https://hg.mozilla.org/projects/fig/rev/9b7ae4a52863

Comment 18

[Mark Capella [:capella]]

Followup bugs after getHistoryList() removal...

Bug 900105 - [fig] Create new testHistoryPage test to replace original testHistoryTab.java.in

Bug 900107 - [fig] Update testShareLink.java.in - re-implement [Test the share popup in the History tab]

Bug 900109 - [fig] Re-implement robocop testClearPrivateData.java.in

Bug 900110 - [fig] Re-implement robocop testHistory.java.in

Comment 19

[Mark Capella [:capella]]

Attachment: Remove [getHistoryList]

Getting close to finished here :-)

Comment 20

[:Margaret Leibovic]

Comment on attachment 783863 [details] [diff] [review]
Remove [getHistoryList]

Looks good. With this, we can close out this bug.

And now we'll have to actually think about what kind of utility we'd want to make for getting these pages! There are a handful of fig test patches in my review queue, so maybe some of them start to address this.

Comment 21

[Mark Capella [:capella]]

Comment on attachment 783863 [details] [diff] [review]
Remove [getHistoryList]

Corrected attachment name
https://hg.mozilla.org/projects/fig/rev/b0e8ef08abae

Comment 22

[:Margaret Leibovic]

See bug 901432, it looks like that is going to re-implement the shared methods we need (or at least provide a place for us to put those methods).

Comment 23

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/aab022ea5da6
https://hg.mozilla.org/mozilla-central/rev/9b7ae4a52863
https://hg.mozilla.org/mozilla-central/rev/b0e8ef08abae