792394 - Replace expect with waitFor to ensure the page title matches the underlined text in /testAwesomeBar tests

Status: RESOLVED FIXED

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

Description

[Andreea Matei [:AndreeaMatei]]

We had a lot of failures in AwesomeBar tests, at the expect that is checking for the page title to match the underlined text. It is necessary an waitFor there, instead of expect. 
It seems the test types very quickly the string into the location bar, but the underlining in the auto-complete list of results falls behind and misses to underline the last letter.

Comment 1

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

I wouldn't care of bug 782918 in this case because you will transform it to waitFor() now. Alex should leave this part alone.

Comment 2

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

Andrea, please add bugs for the test failures as dependencies please.

Comment 3

[Andreea Matei [:AndreeaMatei]]

Attachment: patch v1

Replaced expect.equal() with expect.waitFor() and in testCheckItemHighlight.js changed also the controller.assert() so it won't be in conflict with Alex's bug (he won't be doing any changes there anymore). 
Also renamed element parameter into aElement.

Comment 4

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Comment on attachment 662844 [details] [diff] [review]
patch v1

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

::: tests/functional/testAwesomeBar/testCheckItemHighlight.js
@@ +98,1 @@
>      }, "Underlined " + aType + " matches entered " + aType + " - got '" +

We should remove got/expected from the message. Use a similar message to other tests: "The page " + aType+ " matches the underlined text"

::: tests/functional/testAwesomeBar/testSuggestBookmarks.js
@@ +74,5 @@
>    var richlistItem = locationBar.autoCompleteResults.getResult(0);
>  
>    var entries = locationBar.autoCompleteResults.getUnderlinedText(richlistItem, "title");
>    entries.forEach(function (aEntry) {
> +    expect.waitFor(function() {

nit: Space before ()

@@ +76,5 @@
>    var entries = locationBar.autoCompleteResults.getUnderlinedText(richlistItem, "title");
>    entries.forEach(function (aEntry) {
> +    expect.waitFor(function() {
> +      return aEntry.toLowerCase() === LOCAL_TEST_PAGE.string;
> +    },"The page title matches the underlined text");

nit: Space after the comma please.

Comment 5

[Andreea Matei [:AndreeaMatei]]

Attachment: patch v2

Updated as requested.

Comment 6

[Andreea Matei [:AndreeaMatei]]

Attachment: patch v2

Sorry, missed to add a space.

Comment 7

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

Comment on attachment 663007 [details] [diff] [review]
patch v2

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

::: tests/functional/testAwesomeBar/testCheckItemHighlight.js
@@ +94,5 @@
>    // Check that the underlined URL matches the entered URL
> +  underlined.forEach(function (aElement) {
> +    expect.waitFor(function () {
> +      return aElement.toLowerCase() === LOCAL_TEST_PAGES[0].name;
> +    }, "The page " + aType + " matches the underlined text");

We should mention the value of aElement in the message. Otherwise it will be hard to figure out which iteration failed.

The same applies to the other two files.

Comment 8

[Andreea Matei [:AndreeaMatei]]

Attachment: patch v2.1

Updated the message so we show the parameter's value.

Comment 9

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

Comment on attachment 663328 [details] [diff] [review]
patch v2.1

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

::: tests/functional/testAwesomeBar/testCheckItemHighlight.js
@@ +95,5 @@
> +  underlined.forEach(function (aElement) {
> +    expect.waitFor(function () {
> +      return aElement.toLowerCase() === LOCAL_TEST_PAGES[0].name;
> +    }, "The page " + aType + " matches the underlined text - got '" +
> +       aElement.toLowerCase() + "'");

As mentioned a couple of times before we cannot use 'got'.

But I see the problem now. So we have to use the index of the iteration instead which will be the second parameter to the callback for forEach: "The page " + aType + " matches the underlined text for iteration 1."

Comment 10

[Andreea Matei [:AndreeaMatei]]

Attachment: patch v3

Added index parameter which gives us the current iteration in the message.

Comment 11

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

Comment on attachment 663338 [details] [diff] [review]
patch v3

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

::: tests/functional/testAwesomeBar/testCheckItemHighlight.js
@@ +94,5 @@
>    // Check that the underlined URL matches the entered URL
> +  underlined.forEach(function (aElement, index) {
> +    expect.waitFor(function () {
> +      return aElement.toLowerCase() === LOCAL_TEST_PAGES[0].name;
> +    }, "The page " + aType + " matches the underlined text for iteration " + index);

The variable has to be named aIndex. Also if we mention iteration in the message you would have to increase it by 1.

Comment 12

[Andreea Matei [:AndreeaMatei]]

Attachment: patch v3.1

Renamed the parameter and added a variable called iteration that takes the value of incremented index. 
If used incrementation directly in the message it will have to be ++aIndex, that would change the value of aIndex for the next iteration and it's not ok.
Passed internal review.

Comment 13

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Comment on attachment 663405 [details] [diff] [review]
patch v3.1

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

(In reply to Andreea Matei [:AndreeaMatei] from comment #12)
> Created attachment 663405 [details] [diff] [review]
> patch v3.1
> 
> Renamed the parameter and added a variable called iteration that takes the
> value of incremented index. 
> If used incrementation directly in the message it will have to be ++aIndex,
> that would change the value of aIndex for the next iteration and it's not ok.
> Passed internal review.

It is my understanding that using aIndex++ will also change the value of aIndex. If you want to avoid this you could use (aIndex + 1) directly in the message.

Comment 14

[Andreea Matei [:AndreeaMatei]]

Attachment: patch v3.2

Updated the messages with (aIndex + 1).

Comment 15

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

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

If all is fine we want to see that backported to all other branches.

Comment 16

[Andreea Matei [:AndreeaMatei]]

Unfortunately I see reports failing on Mac OS X for testCheckItemHighlight.js only:
http://mozmill-ci.blargon7.com/#/functional/report/d11b1de413a0179d904e7372304e1699
http://mozmill-ci.blargon7.com/#/functional/report/d11b1de413a0179d904e7372304a6e19

Will check on Mac to see what is different, each time it fails at the "title" callback which is the first one.

Comment 17

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

We have lesser failures as before but it's still not fixed. Lets have a look at next week.

Comment 18

[Andreea Matei [:AndreeaMatei]]

I've been trying to reproduce this on Mac OS X 10.7.5 and 10.8 where we had most reports and also on Ubuntu (to ckeck), but it's not happening for me on none of them.
I will also try with Vlad's Mac when available, since Alex is working on the video files on his Mac.
I'm wondering if is not a waitFor issue, so I will create a personalized waitFor which will dump the string and the time, checking every 100ms until it founds the match. That way we'll know how much time is needed to underline the whole string.

Comment 19

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

Keep in mind that we still have the real OS X boxes in Mozmill CI which could be used sensitively for testing that.

Comment 20

[juan becerra [:juanb]]

I checked this on Windows XP and I could not see the highlights in the awesomebar suggestions. The machine where I can see this is a VM running on VMWare Fusion. On a hardware machine running XP I can see the highlighting.

Comment 21

[juan becerra [:juanb]]

Uploaded the VM in question to: smb://fs2.office.mozilla.org/public/QA/Virtualization/WinXP-bug792394-Henrik.tgz

Comment 22

[Andreea Matei [:AndreeaMatei]]

Attachment: screenshot

I believe we have some updates here.
With Henrik's help on IRC this morning, I ran testCheckItemHighlight.js on a 100 times bases, with PLACES_DB_TIMEOUT = 16000 and added a short sleep if underlined.length == 0 in order to catch the screenshot.

The error we get is "Only one autocompleted result is underlined - '0' should equal '1'".
At first we thought we fail at saving the test page in the history, but the screenshot shows we have both the history and the bookmark entry, fully underlined, at least in this case. 
The thing is we get Bookmark entry first into the list and it has no "Community" in it's title. Rest of the time, the history entry is first. Somehow it gets switched, probably the database is still lazy even with 16s.

Then, tried another page (Mozilla Grants) which is not bookmarked and got it to fail on Ubuntu at iteration 71. The error was:
"Autocomplete popup has been opened - got 'false', expected 'true'"
which means we might not have gotten a history entry at all so there are no results to pop up.

It's clear we have a database time issue and to avoid using pages that are bookmarked as well won't fix the missing history entries.

Comment 23

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

(In reply to Andreea Matei [:AndreeaMatei] from comment #22)
> The thing is we get Bookmark entry first into the list and it has no
> "Community" in it's title. Rest of the time, the history entry is first.
> Somehow it gets switched, probably the database is still lazy even with 16s.

It doesn't matter if we have a bookmarked item or not. Important is that we do not make use of a web page which collides with any of the default bookmarks set. So we should probably go away from the mozilla_community.html test page and use another one.

> Then, tried another page (Mozilla Grants) which is not bookmarked and got it
> to fail on Ubuntu at iteration 71. The error was:
> "Autocomplete popup has been opened - got 'false', expected 'true'"
> which means we might not have gotten a history entry at all so there are no
> results to pop up.

That's a different issue then. I would say we file a new bug for the checkItemHighlight failure from above and replace the page. Afterward we can follow-up with the new issue you have discovered. Thing is we are checking title and url for each entry, so that has to be fulfilled first. Question through, do the two entries as shown in the screenshot appear in each iteration or what do we see in those cases?

Comment 24

[Andreea Matei [:AndreeaMatei]]

(In reply to Henrik Skupin (:whimboo) from comment #23)
> Thing is we are checking
> title and url for each entry, so that has to be fulfilled first. Question
> through, do the two entries as shown in the screenshot appear in each
> iteration or what do we see in those cases?

In normal cases, the history entry is first and then the bookmarked one. You can check this manually, but it the page to be visited recently like in the test (because with older history entries the bookmark is shown first). 
The case where it fails, the entries appear like in the screenshot and I believe that's why we fail, since richlistItem = locationBar.autocompleteResults.getResult(0) not getResult(1) as we'd need in such case.

If we're changing the community page on checkItemHighlight test, we should do the same in switchToTab test, which was also failing sometimes.

Comment 25

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

(In reply to Andreea Matei [:AndreeaMatei] from comment #24)
> If we're changing the community page on checkItemHighlight test, we should
> do the same in switchToTab test, which was also failing sometimes.

Exactly. We should also check all the other awesomebar tests so we can ensure that no other instance will remain. So please file a new bug and lets get this fixed there. Once landed we will see what's remaining here.

Comment 26

[Andreea Matei [:AndreeaMatei]]

Attachment: patch v1

Adding timeout sleep in testSwitchToTab.js in order to help adding the history items to the database.
Looks like the fix in bug 797389 helped and we don't see failures on testCheckItemHighlight.js anymore. The reports from testSwitchToTab.js must have been caused by the community page and the lack of this timeout.

We still have testSuggestBookmark.js where we already use grants page, but we are bookmarking it. 
So I believe we're facing that other failure which we could follow up in a new bug.

Comment 27

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

Comment on attachment 673226 [details] [diff] [review]
patch v1

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

Wrong bug. Please attach it to the right one.

Comment 28

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

The interim fix for the switchToTab test didn't help completely. It's still failing intermittently. 

http://hg.mozilla.org/qa/mozmill-tests/file/2e61e9030e3d/tests/functional/testAwesomeBar/testSwitchToTab.js

Comment 29

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

Another failure:

http://mozmill-ci.blargon7.com/#/functional/report/2e7486a8c08b5541b9074117e0a58149

> The page title matches the underlined text - 'g' should equal 'grants' 

which happens in testSuggestBookmarks.js::testStarInAutocomplete

Comment 30

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

(In reply to Henrik Skupin (:whimboo) from comment #15)
> http://hg.mozilla.org/qa/mozmill-tests/rev/e6ccf87758f0 (default)
> 
> If all is fine we want to see that backported to all other branches.

Andrea, so what's the state for this backport. Is it something which improves our tests at least a bit?

Comment 31

[Andreea Matei [:AndreeaMatei]]

It definitely improves the tests a bit, before all these changes there were failures more often. Not sure if the iteration in the message was very helpful since we always fail at the first iteration, but I think that's more related to the fact that we check in the title first.

I'll come up with a patch for the other branches.

Comment 32

[Andreea Matei [:AndreeaMatei]]

The patch it's already pushed to Aurora and it is also applying cleanly for beta, esr10 and release.

Comment 33

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

Landed the patch on the other branches:

http://hg.mozilla.org/qa/mozmill-tests/rev/05276b4209d1 (beta)
http://hg.mozilla.org/qa/mozmill-tests/rev/a501301e991c (release)
http://hg.mozilla.org/qa/mozmill-tests/rev/07c3e0f71bce (esr10)

So now we have to figure out the last remaining bit here to get the last remaining failure killed.

Comment 34

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

Didn't intended to change the flags. Reverting now.

Comment 35

[Andreea Matei [:AndreeaMatei]]

Yep. From all tests, testSwitchToTab.js still fails sometimes, only on OS X. I'm investigating that further, looking to reproduce like we did with testCheckItemHighlight.js, step by step.

Comment 36

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

Can we skip testSwitchToTab.js on OS X for now until we have this issue fully fixed?

Comment 37

[Andreea Matei [:AndreeaMatei]]

Yep, I'll have a skip patch for OS X, probably in bug 779127 right?

I was able to sometimes reproduce this on OS X, really heavy loaded only, and I've notice the followings:
- the string in location bar is correct all the time and the autocomplete results length is always 1
- we have "contribute", "governance", then "grants". The failure comes at the second check, with "governance".
- We don't need a longer timeout in waitFor since the underlined text gets to "governanc" from the beginning.

I'm checking now if a waitFor in the getUnderlinedText function would help, comparing with the locationBar value.
And still looking for differences between Linux and Mac, since it's only failing on OS X.

Comment 38

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

(In reply to Andreea Matei [:AndreeaMatei] from comment #37)
> Yep, I'll have a skip patch for OS X, probably in bug 779127 right?

No, it should end-up here because it affects multiple tests.

> - we have "contribute", "governance", then "grants". The failure comes at
> the second check, with "governance".
> - We don't need a longer timeout in waitFor since the underlined text gets
> to "governanc" from the beginning.

So that means the ending 'e' is not underlined? 

> I'm checking now if a waitFor in the getUnderlinedText function would help,
> comparing with the locationBar value.

I would say we should still compare against our pre-defined strings and not what we have entered in the location bar. That could cause hidden failures.

> And still looking for differences between Linux and Mac, since it's only
> failing on OS X.

There can be a lot. I don't think it's worth the time.

Comment 39

[Andreea Matei [:AndreeaMatei]]

Attachment: skip switchToTab.js

Skipping testSwitchToTab.js which fails the most, on OS X.

Regarding the question if last letter is not underlined, I will get back as soon as I visibly catch the error to see how it's underlined.

Comment 40

[Andreea Matei [:AndreeaMatei]]

Attachment: fix v4

In the waitFor's we fail, the underlined item/entry it's not changing no matter how long the interval is. Because we declare it outside and it's not getting updated. 
So I've put it in the waitFor function so that if for some reason it fails to take the whole underlined text, at the next 100ms iteration it will check.
Have been running testruns and single runs on OS X all day and it didn't fail for me.

Comment 41

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

Comment on attachment 676194 [details] [diff] [review]
fix v4

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

::: tests/functional/testAwesomeBar/testCheckItemHighlight.js
@@ -91,5 @@
>    assert.equal(underlined.length, 1,
>                 "Only one autocompleted result is underlined");
>  
>    // Check that the underlined URL matches the entered URL
> -  underlined.forEach(function (aElement, aIndex) {

Why have you removed the forEach loop? This applies to all the tests in the patch.

@@ +92,5 @@
>                 "Only one autocompleted result is underlined");
>  
>    // Check that the underlined URL matches the entered URL
> +  expect.waitFor(function () {
> +    var underlined = locationBar.autoCompleteResults.getUnderlinedText(aResult, aType);

Please don't redeclare the variable with 'var' here.

Comment 42

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

Comment on attachment 676107 [details] [diff] [review]
skip switchToTab.js

Lets get it fixed and not skipped.

Comment 43

[Andreea Matei [:AndreeaMatei]]

(In reply to Henrik Skupin (:whimboo) from comment #41)
> Why have you removed the forEach loop? This applies to all the tests in the
> patch.

Because there is only one underlined text in each title and url, I've checked the array length in getUnderlinedText() for each test. In checkItemHighlight.js we also assert this right before (underlined.length to equal 1).

Comment 44

[Andreea Matei [:AndreeaMatei]]

Attachment: fix v4.1

Removing the "var" in testCheckItemHighlight.js and adding a blank line at the end of the test as the style guide requires.
Did testruns again today and still no failures with my OS X.

Comment 45

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Comment on attachment 676637 [details] [diff] [review]
fix v4.1

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

::: tests/functional/testAwesomeBar/testCheckItemHighlight.js
@@ +91,5 @@
>    assert.equal(underlined.length, 1,
>                 "Only one autocompleted result is underlined");
>  
>    // Check that the underlined URL matches the entered URL
> +  expect.waitFor(function () {

I think we should keep the forEach loop, and use aIndex to re-fetch the 'current' underlined element. It would look something like:

underlined.forEach(function (aElement, aIndex) {
  underlinedText = locationBar.autoCompleteResults.getUnderlinedText(aResult, aType)[aIndex];
  return underlinedText.toLowerCase() === LOCAL_TEST_PAGES[0].name;
}, "The page " + aType + " matches the underlined text");

Alternatively, without the forEach we should at least retrieve just the first item as so:

expect.waitFor(function () {
  underlinedText = locationBar.autoCompleteResults.getUnderlinedText(aResult, aType)[0];
  return underlinedText.toLowerCase() === LOCAL_TEST_PAGES[0].name;
}, "The page " + aType + " matches the underlined text");

Comment 46

[Andreea Matei [:AndreeaMatei]]

Attachment: patch v4.2

As discussed on IRC, restored forEach and updated accordingly to the review.

Comment 47

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

Comment on attachment 678260 [details] [diff] [review]
patch v4.2

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

Looks good to me. Lets get it in right now.

Comment 48

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

Comment on attachment 678260 [details] [diff] [review]
patch v4.2

http://hg.mozilla.org/qa/mozmill-tests/rev/59fd596a9872 (default)

Comment 49

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

I have re-triggered the failing testrun on 10.7 for default. Lets see if that proves the fix.

Comment 50

[Andreea Matei [:AndreeaMatei]]

No failures so far, yey.

This patch applies to aurora too and for beta, release and esr10 I have another one, since there we don't have testSwitchToTab.js

Comment 51

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

http://hg.mozilla.org/qa/mozmill-tests/rev/8a2c5b7f1934 (aurora)

Comment 52

[Andreea Matei [:AndreeaMatei]]

Attachment: [beta, esr10, release] patch v4.2

Backport for esr10, beta and release.

Comment 53

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Comment on attachment 679096 [details] [diff] [review]
[beta, esr10, release] patch v4.2

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

Landed as:
http://hg.mozilla.org/qa/mozmill-tests/rev/79d33b70152f (mozilla-beta)
http://hg.mozilla.org/qa/mozmill-tests/rev/d7190390cfc9 (mozilla-release)
http://hg.mozilla.org/qa/mozmill-tests/rev/4e5b724fcdbe (mozilla-esr10)