Closed Bug 1270078 Opened 4 years ago Closed 4 years ago

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

Categories

(Testing :: Firefox UI Tests, defect)

48 Branch
defect
Not set

Tracking

(firefox48 fixed, firefox49 fixed)

RESOLVED FIXED
mozilla49
Tracking Status
firefox48 --- fixed
firefox49 --- fixed

People

(Reporter: whimboo, Assigned: whimboo)

References

Details

(Keywords: regression)

Attachments

(1 file)

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
Assignee: nobody → hskupin
Status: NEW → ASSIGNED
Summary: Auto-complete tests are broken due to new awesome design → AutocompleteResults.get_matching_text() broken due to the new awesomebar design
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.
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?
Flags: needinfo?(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.
Flags: needinfo?(adw)
That was a great hint, Drew! That definitely fixed the problem! :) A new patch is coming up soon.
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/
Attachment #8748725 - Attachment description: MozReview Request: Bug 1270078 - Fix AutocompleteResults.get_matching_text() for new awesomebar design. r?mak → MozReview Request: Bug 1270078 - Fix AutocompleteResults.get_matching_text() for new awesomebar design. r?adw
Attachment #8748725 - Flags: review?(mak77) → review?(adw)
Attachment #8748725 - Flags: review?(adw) → review+
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.
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 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/
https://hg.mozilla.org/mozilla-central/rev/999dad371641
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Tests are fixed on inbound and mozilla-central. Lets backport it also to mozilla-aurora. Thanks.
Whiteboard: [checkin-needed-aurora]
You need to log in before you can comment on or make changes to this bug.