1085328 - Test failure "The auto-complete result is a bookmark - 'favicon' should contain 'bookmark'" in /testAwesomeBar/testSuggestBookmarks.js

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect, P3)

Description

[Andrei Eftimie]

Module:    testStarInAutocomplete
Test:      /testAwesomeBar/testSuggestBookmarks.js    
Failure:   The auto-complete result is a bookmark - 'favicon' should contain 'bookmark'
Branches:  36, 35, 34
Platforms: Linux
Report:    http://mozmill-daily.blargon7.com/#/functional/report/447e3274a76247e6567e1c2aa6034725

Seems to have started on 2014-10-13

Comment 1

[Andrei Eftimie]

Looking more into this, it might be fallout from bug 1067888

Comment 2

[Teodor Druta]

Can't reproduce, watching this bug to see if it will reproduce in the future.

Comment 3

[Teodor Druta]

Attachment: fixtestsuggestbookmarks.patch

There are 3 bugs including this one, with failures related to this test (Bug 1106736, Bug 978708). They are actually all related to the same problem.
Investigating those issues locally, I went into what it seemed another problem "locationBar.autoCompleteResults.getResult(1);" returned null, that was because there wasn't really an item with that index. This test actually worked wrong, it wasn't doing what it supposed to do, by getting the autocomplete result at index 1, it was taking a hidden favicon result leftover from testFaviconInAutocomplete, sometimes this was actually the mozilla grant page that's why it didn't failed, sometimes it was other page. This led me to think that there is another issue with testFaviconInAutocomplete, which will require further investigation on that part. What we need here is to wait for the visible results to load, which should contain only one visible item in this case, then get the autocomplete result at index 0, instead of 1.
So, I attached to this bug a patch of my proposal fix.

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 8535685 [details] [diff] [review]
fixtestsuggestbookmarks.patch

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

::: firefox/tests/functional/testAwesomeBar/testSuggestBookmarks.js
@@ +49,5 @@
>    // Bookmark the test page via bookmarks menu
>    var bookmarksPanel = editBookmarksPanel.getElement({type: "bookmarkPanel"});
>    toolbars.waitForNotificationPanel(() => {
> +    var bookmarkPage = findElement.ID(controller.window.document, "menu_bookmarkThisPage")
> +    bookmarkPage.waitThenClick();

Please do not replace the mainMenu handling code. Can you please explain why you did that?

@@ +79,5 @@
>    }, "Autocomplete list has been opened");
>  
> +  // Wait for visible autocomplete results to load
> +  assert.waitFor(() => locationBar.autoCompleteResults.visibleResults.length > 0,
> +                 "Visible autocomplete results have loaded");

In your last comment you said that we have to wait for exactly 1 entry. This is not what the patch is doing.

Comment 5

[Teodor Druta]

(In reply to Henrik Skupin (:whimboo) from comment #4)
> Comment on attachment 8535685 [details] [diff] [review]
> fixtestsuggestbookmarks.patch
> 
> Review of attachment 8535685 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: firefox/tests/functional/testAwesomeBar/testSuggestBookmarks.js
> @@ +49,5 @@
> >    // Bookmark the test page via bookmarks menu
> >    var bookmarksPanel = editBookmarksPanel.getElement({type: "bookmarkPanel"});
> >    toolbars.waitForNotificationPanel(() => {
> > +    var bookmarkPage = findElement.ID(controller.window.document, "menu_bookmarkThisPage")
> > +    bookmarkPage.waitThenClick();
> 
> Please do not replace the mainMenu handling code. Can you please explain why
> you did that?
> 

Actually in my local investigations I sometimes, very rarely encountered the "Notification popup state has been opened" failure, the waitThenClick seemed to help with this, but I admit I might be wrong that it helps in this particular case.

> @@ +79,5 @@
> >    }, "Autocomplete list has been opened");
> >  
> > +  // Wait for visible autocomplete results to load
> > +  assert.waitFor(() => locationBar.autoCompleteResults.visibleResults.length > 0,
> > +                 "Visible autocomplete results have loaded");
> 
> In your last comment you said that we have to wait for exactly 1 entry. This
> is not what the patch is doing.

For this particular test we require just one visible autocomplete entry, but imo we shouldn't exclude the possibility of there being more than one visible suggestion.

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to Teodor Druta from comment #5)
> > > +    var bookmarkPage = findElement.ID(controller.window.document, "menu_bookmarkThisPage")
> > > +    bookmarkPage.waitThenClick();
> > 
> > Please do not replace the mainMenu handling code. Can you please explain why
> > you did that?
> 
> Actually in my local investigations I sometimes, very rarely encountered the
> "Notification popup state has been opened" failure, the waitThenClick seemed
> to help with this, but I admit I might be wrong that it helps in this
> particular case.

If that turns out to be a problem with the MenuAPI implementation it should be filed as a bug against mozmill. That way we could include it in the last upcoming release.

> For this particular test we require just one visible autocomplete entry, but
> imo we shouldn't exclude the possibility of there being more than one
> visible suggestion.

Tests are very specific and have pre-defined requisites. With a clean setup the test should not see more than two entries, right? This particular check could mask a particular failure.

Comment 7

[Teodor Druta]

Attachment: fixtestsuggestbookmark.patch

(In reply to Henrik Skupin (:whimboo) from comment #6)
> If that turns out to be a problem with the MenuAPI implementation it should
> be filed as a bug against mozmill. That way we could include it in the last
> upcoming release.
> 

I'll try to look more into this, in order to find out if this is really an issue.

> Tests are very specific and have pre-defined requisites. With a clean setup
> the test should not see more than two entries, right? This particular check
> could mask a particular failure.

Verifying against a single result should really show if there is a problem, and will be an easier way to find eventual future changes in the UI of the autocomplete feature.

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 8536590 [details] [diff] [review]
fixtestsuggestbookmark.patch

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

::: firefox/tests/functional/testAwesomeBar/testSuggestBookmarks.js
@@ +53,5 @@
>    }, {type: "bookmark", panel: bookmarksPanel});
>  
>    toolbars.waitForNotificationPanel(() => {
>      var doneButton = editBookmarksPanel.getElement({type: "doneButton"});
> +    doneButton.waitThenClick();

Why is that change needed? The done button should always exist.

@@ +78,5 @@
>    }, "Autocomplete list has been opened");
>  
> +  // Wait for visible autocomplete results to load
> +  assert.waitFor(() => locationBar.autoCompleteResults.visibleResults.length === 1,
> +                 "Visible autocomplete results have loaded");

Please be very explicit in what we are waiting for. This message does not tell anything, and we will ask ourselves again when changes to the behavior are made.

Also I do not see any reason for this comment, which plainly a duplication of the message.

@@ +84,2 @@
>    // Define the path to the second auto-complete result
> +  var richlistItem = locationBar.autoCompleteResults.getResult(0);

The comment above does no longer match the code.

Comment 9

[Teodor Druta]

Attachment: fixsuggestbookmarks.patch

Thanks for the feedback, Henrik !

Comment 10

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8537126 [details] [diff] [review]
fixsuggestbookmarks.patch

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

::: firefox/tests/functional/testAwesomeBar/testSuggestBookmarks.js
@@ +76,5 @@
>    assert.waitFor(function () {
>      return locationBar.autoCompleteResults.isOpened;
>    }, "Autocomplete list has been opened");
>  
> +  // Wait for one visible autocomplete result to be visible

What Henrik mentioned before was "Also I do not see any reason for this comment, which plainly a duplication of the message." related to this comment. It can be removed.
Otherwise we're good to land with the new patch.

Comment 11

[Teodor Druta]

Attachment: fixtestsuggestbookmarks.patch

Sorry, I think I missed that out, removed the comment.

Comment 12

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8538449 [details] [diff] [review]
fixtestsuggestbookmarks.patch

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

http://hg.mozilla.org/qa/mozmill-tests/rev/6f0285e476a9 (default)

Comment 13

[Teodor Druta]

Comment on attachment 8538449 [details] [diff] [review]
fixtestsuggestbookmarks.patch

I think that we should push this patch to mozilla-beta, it should fix the issues reported today in Bug 1106736

Comment 14

[Andreea Matei [:AndreeaMatei]]

Merge pushed the fix on aurora too, this is beta:
http://hg.mozilla.org/qa/mozmill-tests/rev/53044b799975 (beta)

Comment 15

[Teodor Druta]

Attachment: fixsgstbkmrksrls.patch

Here is the backported patch for mozilla-release branch.

Comment 16

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8548779 [details] [diff] [review]
fixsgstbkmrksrls.patch

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

http://hg.mozilla.org/qa/mozmill-tests/rev/60707164cf71 (release)