725483 - Robocop: testBookmark is failing

Status: RESOLVED FIXED

(Testing :: Mochitest, defect)

Description

[Geoff Brown [:gbrown]]

The patch for bug 720930 just landed and seemed to make testBookmark reliable, but now it's broken again! It seems to hang when there are no bookmarks...perhaps because of the new distinction between Mobile and Desktop bookmarks.

Curiously, this test appears to still pass on tbpl / try.

Comment 1

[:Margaret Leibovic]

I'm also working on a patch for bug 725454 that will hide the headers if there are no bookmarks in a group, so we'll have to make sure our tests are robust enough to handle that. Ideally, we should have tests that make sure no headers show up when there are no bookmarks, then check that the correct headers show up when there are bookmarks.

Comment 2

[Geoff Brown [:gbrown]]

I am seeing things go wrong on the first call to selectFirstItemInList: We are trying to determine if there are any bookmarks available by trying to click on the first item in the bookmarks list. clickInList(1) returns a non-null list of TextView even when there are no bookmarks -- presumably the ExpandableListView header for "Mobile Bookmarks".

Robotium seems to make no allowance for ExpandableListView. We might need to inspect the TextViews or something like that.

Perhaps it would be best to wait for bug 725454 to be resolved before trying to fix this test.

Comment 3

[:Margaret Leibovic]

(In reply to Geoff Brown [:gbrown] from comment #2)

> Perhaps it would be best to wait for bug 725454 to be resolved before trying
> to fix this test.

Actually now we should just wait for bug 722020 to land. I can help out writing tests for that.

Comment 4

[:Margaret Leibovic]

I started working on modifying testBookmark to handle the new interface from bug 722020, so I can take this!

Comment 5

[:Margaret Leibovic]

Attachment: WIP

This basically has the same functionality as the old test, but it's cleaned up and works with the new UI from bug 722020.

I also want to add test coverage for pressing the back button to go up a folder level, long tapping for the context menu (and testing various context menu items).

Comment 6

[Geoff Brown [:gbrown]]

Comment on attachment 599387 [details] [diff] [review]
WIP

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

Looks good!

> I also want to add test coverage for pressing the back button to go up a folder
> level, long tapping for the context menu (and testing various context menu
> items).

Great. testWebContentContextMenu.java.in may be helpful.

::: mobile/android/base/tests/testBookmark.java.in
@@ -53,29 +66,34 @@
> > -        //If a bookmark was created by this test, remove it now.
> > +    private void setUpBookmark() {
> > -        if (created) {
> > +        // Bookmark a page for the test
NaN more ...
> > +        mAsserter.is(mSolo.waitForText("Bookmark removed"), true, "bookmark removed successfully");
> > -                if (bookmarkList != null) {
> > +
> > -                    ArrayList<android.widget.ListView> views = mSolo.getCurrentListViews();
NaN more ...

You may be able to leverage the waitForTest code being introduced in bug 725609: https://bugzilla.mozilla.org/attachment.cgi?id=599348

Comment 7

[:Margaret Leibovic]

I also just noticed that Brian directly added a bookmark to the DB in his test in bug 725609, and I could potentially do the same thing here, although I guess going through the menu is good for testing the bookmark menu item.

Comment 8

[Geoff Brown [:gbrown]]

Context for comment 6 should have been something like:

+        boolean listIsEmpty = false;
+        while (listIsEmpty == false) {            
+            if (adapter != null && adapter.isEmpty()) {
+                listIsEmpty = true;
+            } else {
+                // wait a little while before trying again
+                try { Thread.sleep(100); } catch(Exception e) {}

Comment 9

[:Margaret Leibovic]

Attachment: patch

I decided to address the context menu tests in a separate bug, since we should probably write a file that tests the context menu for items in all 3 different awesomescreen tabs.

I used waitForTest from bug 725609, which was handy. This test depends on that, as well as bug 722020 and friends. Those dependencies are all in my patch queue if you want to try this out: http://hg.mozilla.org/users/margaret.leibovic_gmail.com/patches/

Comment 10

[Geoff Brown [:gbrown]]

Comment on attachment 599752 [details] [diff] [review]
patch

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

This looks awesome!

Comment 11

[:Margaret Leibovic]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e57a42227b40

Comment 12

[Geoff Brown [:gbrown]]

(In reply to Margaret Leibovic [:margaret] from comment #11)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/e57a42227b40

I think this landed on m-c a while ago, didn't it?

Comment 13

[:Margaret Leibovic]

I guess the merge sheriff missed it:

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