1270078 - AutocompleteResults.get_matching_text() broken due to the new awesomebar design

Status: RESOLVED FIXED

(Testing :: Firefox UI Tests, defect)

Description

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

Via bug 1181078 we got a new design for the awesomebar. As result some of our tests are failing now:

https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&filter-searchStr=Firefox%20UI&filter-tier=2&filter-tier=3&selectedJob=27215679

TEST-UNEXPECTED-ERROR | test_toolbars.py TestAutoCompleteResults.test_matching_text | JavascriptException: JavascriptException: TypeError: arguments[0].boxObject.firstChild.childNodes[1] is undefined

TEST-UNEXPECTED-ERROR | test_suggest_bookmarks.py TestStarInAutocomplete.test_star_in_autocomplete | JavascriptException: JavascriptException: TypeError: arguments[0].boxObject.firstChild.childNodes[1] is undefined

Comment 1

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

Attachment: MozReview Request: Bug 1270078 - Fix AutocompleteResults.get_matching_text() for new awesomebar design. r?adw

Review commit: https://reviewboard.mozilla.org/r/50497/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/50497/

Comment 2

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

Interesting. The try build is still failing for one case:

TEST-UNEXPECTED-FAIL | test_toolbars.py TestAutoCompleteResults.test_matching_text | AssertionError: u'a' not found in 'moz'

I will investigate.

Comment 3

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

Given by Marco we are doing it right for waiting until the search is complete:

http://mxr.mozilla.org/mozilla-central/source/testing/puppeteer/firefox/firefox_puppeteer/ui/browser/toolbars.py#384

But we still seem to have a race condition here when you quickly search for different terms:

input: "about"
opening autocomplete...
done
waiting for search complete
2
4
done
all matches:  [u'About', u'about']
closing...
closed
input: "moz"
opening autocomplete...
done
waiting for search complete
2
4
done
all matches:  [u'moz']
all matches:  [u'moz']
all matches:  [u'moz']
all matches:  [u'moz']
all matches:  [u'moz']
closing...
closed
input: "bout"
opening autocomplete...
done
waiting for search complete
2
4
done
all matches:  [u'bout', u'bout']
all matches:  [u'moz']

For the last iteration the second result has the input from the last iteration emphasized. Drew, do you have an idea why that happens? Could this actually be a bug?

Comment 4

[Drew Willcoxon :adw]

It looks like the final iteration is seeing one item left over from the previous iteration.  Depending on how your test works, that's entirely possible because the popup collapses results from previous searches asynchronously, after the search has completed.

It looks like your problem is here: http://mxr.mozilla.org/mozilla-central/source/testing/puppeteer/firefox/firefox_puppeteer/ui/browser/toolbars.py#358

I bet that that code is running before the items from the previous iteration are collpased.  Instead of looking at uncollapsed items, you should be checking items up to index popup._matchCount, since _matchCount is updated immediately when a search finishes.

I can't say why you're hitting this now and weren't before, but the fact that you weren't before was probably just lucky timing.

I could be wrong about all this since I haven't tried to verify it, but that's what it looks like to me.

Comment 5

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

That was a great hint, Drew! That definitely fixed the problem! :) A new patch is coming up soon.

Comment 6

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

Comment on attachment 8748725 [details]
MozReview Request: Bug 1270078 - Fix AutocompleteResults.get_matching_text() for new awesomebar design. r?adw

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50497/diff/1-2/

Comment 7

[Drew Willcoxon :adw]

Comment on attachment 8748725 [details]
MozReview Request: Bug 1270078 - Fix AutocompleteResults.get_matching_text() for new awesomebar design. r?adw

https://reviewboard.mozilla.org/r/50497/#review47915

Glad that fixed it.  I don't know these tests at all, so take my r+ with a grain of salt, but as far as the autocomplete parts of this go, this looks OK to me.

::: testing/puppeteer/firefox/firefox_puppeteer/ui/browser/toolbars.py:362
(Diff revision 2)
>            let node = arguments[0];
> -          for (let i = 0; i < node.itemCount; ++i) {
> +          let count = arguments[1];
> +
> +          for (let i = 0; i < count; ++i) {
>              let item = node.getItemAtIndex(i);
>              if (!item.hasAttribute("collapsed")) {

I think this should always be true, so you should be able to remove this conditional and just unconditionally push the item, if you'd like.

Comment 8

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

https://reviewboard.mozilla.org/r/50497/#review47915

> I think this should always be true, so you should be able to remove this conditional and just unconditionally push the item, if you'd like.

Oh, right. Tested that and it works. I will get it removed then. Thanks for noticing that.

Comment 9

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

Comment on attachment 8748725 [details]
MozReview Request: Bug 1270078 - Fix AutocompleteResults.get_matching_text() for new awesomebar design. r?adw

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50497/diff/2-3/

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/999dad371641

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/999dad371641

Comment 12

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

Tests are fixed on inbound and mozilla-central. Lets backport it also to mozilla-aurora. Thanks.

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/6772bf149f18

Comment 14

[Intermittent Failures Robot]

48 automation job failures were associated with this bug in the last 7 days.

Repository breakdown:
* mozilla-aurora: 35
* mozilla-central: 13

Platform breakdown:
* linux64: 13
* windows8-64: 5
* osx-10-9: 5
* osx-10-11: 5
* osx-10-10: 5
* windows7-64: 4
* osx-10-6: 4
* linux32: 3
* windowsxp: 2
* windows8-32: 2

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1270078&startday=2016-05-10&endday=2016-05-16&tree=all