Closed Bug 1372205 Opened 3 years ago Closed 2 years ago

Intermittent test_toolbars.py TestAutoCompleteResults.test_popup_elements | AssertionError: 9 != 10

Categories

(Testing :: Marionette, defect)

55 Branch
defect
Not set

Tracking

(firefox54 unaffected, firefox55 fixed, firefox56 fixed)

RESOLVED FIXED
mozilla56
Tracking Status
firefox54 --- unaffected
firefox55 --- fixed
firefox56 --- fixed

People

(Reporter: whimboo, Assigned: whimboo)

References

Details

(Keywords: intermittent-failure, pi-marionette-firefox-puppeteer)

Attachments

(2 files)

As seen on bug 1371923 we seem to fail to retrieve the correct amount of visible autocomplete entries, when the merge to beta will happen today.

The proposal from Marco is:

(In reply to Marco Bonardo [::mak] from bug 1371923 comment #7)
> > If that is a proper solution, we could definitely do this here:
> > https://dxr.mozilla.org/mozilla-central/source/testing/marionette/puppeteer/
> > firefox/firefox_puppeteer/ui/browser/toolbars.py#348
> 
> if you can loop through .children and do a manual count for uncollapsed
> items, instead of using itemCount, it may work.
Marco, can you please have a look at the patch and let me know if that is what you preferred to see? Thanks.
Flags: needinfo?(mak77)
I also pushed a try build on beta:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2f42e7829e8111ef30afc2358c9669707d5e2718
Assignee: nobody → hskupin
Status: NEW → ASSIGNED
Comment on attachment 8876764 [details]
Bug 1372205 - Fix AutocompleteResults.visible_results() to only return visible items.

https://reviewboard.mozilla.org/r/148096/#review152438

::: testing/firefox-ui/tests/puppeteer/test_toolbars.py:164
(Diff revision 1)
>          self.browser.navbar.locationbar.urlbar.send_keys('a')
>          results = self.autocomplete_results.results
>          Wait(self.marionette).until(lambda _: self.autocomplete_results.is_complete)
>          visible_result_count = len(self.autocomplete_results.visible_results)
> -        self.assertTrue(visible_result_count > 0)
> -        self.assertEqual(visible_result_count,
> +        self.assertGreater(visible_result_count, 0)
> +        self.assertLessEqual(visible_result_count,

potentially with the collapsed count you may keep the assertEqual and would be a stricter test.
Would it work on beta? I this so, but it would be interesting to know for sure.

If the test only cares about "there's some results and not too many", using less sounds ok too.
Flags: needinfo?(mak77)
Attachment #8876764 - Flags: review?(mjzffr)
Ok I had misread the patch. visible_results was right already, the problem is itemCount that counts also invisible results.

With this patch, I expect visible_results to basically be identical to before, and it works because you changed Equal to LessOrEqual.

What I would do instead, is changing itemCount to a count of noncollapsed children and keep the Equal.
So I talked with Marco on IRC and what we will do is to keep the modified version of `visible_results`, and then take the old `_matchCount` implementation as reference for the assertion. The reason is that with iterating the children and checking the collapsed property, we definitely only get visible results.
Comment on attachment 8876764 [details]
Bug 1372205 - Fix AutocompleteResults.visible_results() to only return visible items.

https://reviewboard.mozilla.org/r/148096/#review152438

> potentially with the collapsed count you may keep the assertEqual and would be a stricter test.
> Would it work on beta? I this so, but it would be interesting to know for sure.
> 
> If the test only cares about "there's some results and not too many", using less sounds ok too.

As discussed on IRC and mentioned on Bugzilla, I'm going to close this issue.
Comment on attachment 8876764 [details]
Bug 1372205 - Fix AutocompleteResults.visible_results() to only return visible items.

https://reviewboard.mozilla.org/r/148096/#review152530

The logic looks good and the code looks correct off-hand. Try will tell us how good we have been :)
Attachment #8876764 - Flags: review+
So it's the collapsed count that returns 10?
I wonder if it's possible this is due to the shrinking delay we have before collapsing items...
http://searchfox.org/mozilla-central/rev/d840ebd5858a61dbc1622487c1fab74ecf235e03/toolkit/content/widgets/autocomplete.xml#1295
Even setting gURLBar.shrinkDelay to 0, you should still wait a tick.
Either we use a timeout, or just give up and use lessOrEqual
First lets update the summery to make it easier to star by sheriffs.

Also there are some things I don't understand:

1) Why is this only happening on Beta? What is different for release compared to nightly builds in the logic how we show autocomplete results? 

2) Here is the current code:

>        self.assertEqual(len(self.autocomplete_results.visible_results),
>                         self.autocomplete_results.element.get_property('_matchCount'))

Given the current message of `10 != 8` it means that `_matchCount` has the correct value right after opening the autocomplete popup. But it also means that iterating through the children and checking the collapsed state is not going to work without any other delays or pref changes.

What I want is definitely a perfectly fine working solution for `visible_results`. With the modifications I did for this property it doesn't work. So I would like to revert it again. But before doing it I would like to know that we can definitely be sure that all results up to index `_matchCount - 1` are those which will be displayed? Or could there also be some which will be collapsed?

3) Are there any further events we could listen for?


For now I would say we should disable the test on beta until a proper solution is in place. I will attach a patch in a moment.
Summary: Fix AutocompleteResults.visible_results() to only return visible items → Intermittent test_toolbars.py TestAutoCompleteResults.test_popup_elements | AssertionError: 9 != 10
Carsten, we need this skip patch for beta to stop the perma failure of the toolbar test. Can you please review and checkin? Thanks.
Attachment #8877037 - Flags: review?(cbook)
Marco, please see comment 14. Thanks.
Flags: needinfo?(mak77)
(In reply to Henrik Skupin (:whimboo) from comment #14)
> What I want is definitely a perfectly fine working solution for
> `visible_results`. With the modifications I did for this property it doesn't
> work. So I would like to revert it again. But before doing it I would like
> to know that we can definitely be sure that all results up to index
> `_matchCount - 1` are those which will be displayed? Or could there also be
> some which will be collapsed?

_matchCount is fine, because we use _matchCount to collapse results.

> 3) Are there any further events we could listen for?

nope, unfortunately the collapsing happens on a timer and there's no event.

The best you could do atm, without adding delays, is probably to just check that _matchCount is > 0 and <= .maxResults.

I'm sorry for all the churn, there are lots of small details in the urlbar that are particular.
Flags: needinfo?(mak77)
Attachment #8877037 - Flags: review?(cbook) → review+
(In reply to Marco Bonardo [::mak] from comment #17)
> _matchCount is fine, because we use _matchCount to collapse results.

Ok, so I will revert the code of `visible_results` to what we had before.

> The best you could do atm, without adding delays, is probably to just check
> that _matchCount is > 0 and <= .maxResults.

Is _matchCount getting reset when the autocomplete closes? How does it behave for multiple openings of the popup? Will it directly change its value eg. 9 => 6? If yes, I don't think the above check makes sense, cause it would return immediately.

To fix the test we can use a `Wait().until()` with a maximum timeout of 5s, which waits for both numbers being equal.
Flags: needinfo?(mak77)
(In reply to Henrik Skupin (:whimboo) from comment #18)
> Ok, so I will revert the code of `visible_results` to what we had before.

yeah, sorry.

> Is _matchCount getting reset when the autocomplete closes?

It gets reset to 0 when the location bar loses focus, in case of tab switching, when a new search starts and in other cases like pressing Escape or backspacing.

> How does it
> behave for multiple openings of the popup? Will it directly change its value
> eg. 9 => 6? If yes, I don't think the above check makes sense, cause it
> would return immediately.

I'm not sure what you mean by multiple openings, the rules are the ones above (at this time), so if multiple openings means doing different searches, yes, it will be set to 0 when a search starts, and will have the final value when the search is complete. If multiple openings is just open/close the popup it should stay the same until the location bar has focus since the search and the results stay the same.
Flags: needinfo?(mak77)
So I can reproduce this failure locally now and the problem is not in the test `test_popup_elements`, but in `test_matching_text`. Something may modify the a state which is not getting reset. And as such we have a different number of autocomplete results.
I think that there is clearly a bug in Firefox here. The test is doing the following:

1) Entering 'about' in the location bar and there are 10 visible results
2) Clearing locationbar and entering 'zilla', which also gives 10 visible results
3) Clearing locationbar and entering 'a', which only gives 7 visible results

For step 3 I can clearly see that we are shrinking the maximum visible results from 10 to 7. But why is that happening? Searching only for 'a' should give at least the same amount of results as searching for one of the others above.
Flags: needinfo?(mak77)
The failure is gone when I use at least two letters for the entered string into the locationbar. Is one char something special now?
(In reply to Henrik Skupin (:whimboo) from comment #24)
> The failure is gone when I use at least two letters for the entered string
> into the locationbar. Is one char something special now?

are search suggestions enabled? It sounds plausible.
we provide search suggestions if the string length is >= 2.
So what you see is that strings from 2 chars on get search suggestions filling the remaining space, while 1 char strings only get local results.
Flags: needinfo?(mak77)
We do not modify the preference "browser.urlbar.suggest.searches" in our tests. As such its value is the default one.

So as Marco mentioned on IRC the problem here is the default bookmarks which differ between those two versions. As such the test works fine in Nightly but now started to fail in Beta. I assume that there was an update for Firefox 55?

Disabling search suggestions for all of our tests make it actually work, because `itemCount` as what we use is getting updated to the value of _matchCount. But with search suggestions enabled, `itemCount` is not getting updated and stays at 10.
Flags: needinfo?(mak77)
(In reply to Henrik Skupin (:whimboo) from comment #26)
> Disabling search suggestions for all of our tests make it actually work,
> because `itemCount` as what we use is getting updated to the value of
> _matchCount. But with search suggestions enabled, `itemCount` is not getting
> updated and stays at 10.

Well, it is updated, but search suggestions fill up any missing space left by local results up to maxResults. I expect itemCount to reach 10 at the first search with more than 1 char, then staying at 10.
As I said itemCount is not a good measure since it's just the number of elements in the richlistbox, independent on whether they are visible or not, once all 10 elements are created itemCount will stick to 10 forever.
Flags: needinfo?(mak77)
Marco, can you please have a look at the patch again? It's not that strict anymore but as we have seen there might be no other way around. And exact tests for _matchCount are most likely available via mochitests.
Flags: needinfo?(mak77)
(In reply to Henrik Skupin (:whimboo) from comment #30)
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=b3bd5b794c49fba02319690a57765e9d4d0b0149

That try build is actually busted because I only removed the skip part but didn't patch the file with the corrections. Here the updated try build:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9ac14882824549962f68fcd41be2082d8aed4a79
Comment on attachment 8876764 [details]
Bug 1372205 - Fix AutocompleteResults.visible_results() to only return visible items.

https://reviewboard.mozilla.org/r/148096/#review153994

Yeah ok, this covers basic functionality, doesn't need to get too fancy.
It should work :)
Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/042dc35d6fb5
Fix AutocompleteResults.visible_results() to only return visible items. r=mak
Flags: needinfo?(mak77)
https://hg.mozilla.org/mozilla-central/rev/042dc35d6fb5
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
We need this test-only patch uplifted to beta. Please make sure to backout the skip patch first, which landed as:

https://hg.mozilla.org/releases/mozilla-beta/rev/e1ebaa962af1
Whiteboard: [checkin-needed-beta][backout skip patch first]
https://hg.mozilla.org/releases/mozilla-beta/rev/aaa0c9df9efe
Flags: in-testsuite+
Whiteboard: [checkin-needed-beta][backout skip patch first]
You need to log in before you can comment on or make changes to this bug.