Closed Bug 792394 Opened 9 years ago Closed 9 years ago

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

Categories

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

defect

Tracking

(firefox15 fixed, firefox16 fixed, firefox17 fixed, firefox18 fixed, firefox19 fixed, firefox-esr10 unaffected)

RESOLVED FIXED
Tracking Status
firefox15 --- fixed
firefox16 --- fixed
firefox17 --- fixed
firefox18 --- fixed
firefox19 --- fixed
firefox-esr10 --- unaffected

People

(Reporter: AndreeaMatei, Assigned: AndreeaMatei)

References

()

Details

(Whiteboard: s=121008 u=failure c=awesomebar p=1)

Attachments

(5 files, 9 obsolete files)

4.78 KB, patch
whimboo
: review+
whimboo
: checkin+
Details | Diff | Splinter Review
65.62 KB, image/png
Details
1.42 KB, patch
Details | Diff | Splinter Review
6.18 KB, patch
whimboo
: review+
whimboo
: checkin+
Details | Diff | Splinter Review
4.40 KB, patch
davehunt
: review+
davehunt
: checkin+
Details | Diff | Splinter Review
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.
Assignee: nobody → andreea.matei
Depends on: 782918
I wouldn't care of bug 782918 in this case because you will transform it to waitFor() now. Alex should leave this part alone.
No longer depends on: 782918
Andrea, please add bugs for the test failures as dependencies please.
Priority: -- → P2
Blocks: 786183, 790174
Attached patch patch v1 (obsolete) — Splinter Review
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.
Attachment #662844 - Flags: review?(hskupin)
Attachment #662844 - Flags: review?(dave.hunt)
Status: NEW → ASSIGNED
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.
Attachment #662844 - Flags: review?(hskupin)
Attachment #662844 - Flags: review?(dave.hunt)
Attachment #662844 - Flags: review-
Attached patch patch v2 (obsolete) — Splinter Review
Updated as requested.
Attachment #662844 - Attachment is obsolete: true
Attachment #663005 - Flags: review?(hskupin)
Attachment #663005 - Flags: review?(dave.hunt)
Attached patch patch v2 (obsolete) — Splinter Review
Sorry, missed to add a space.
Attachment #663005 - Attachment is obsolete: true
Attachment #663005 - Flags: review?(hskupin)
Attachment #663005 - Flags: review?(dave.hunt)
Attachment #663007 - Flags: review?(hskupin)
Attachment #663007 - Flags: review?(dave.hunt)
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.
Attachment #663007 - Flags: review?(hskupin)
Attachment #663007 - Flags: review?(dave.hunt)
Attachment #663007 - Flags: review-
Attached patch patch v2.1 (obsolete) — Splinter Review
Updated the message so we show the parameter's value.
Attachment #663007 - Attachment is obsolete: true
Attachment #663328 - Flags: review?(hskupin)
Attachment #663328 - Flags: review?(dave.hunt)
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."
Attachment #663328 - Flags: review?(hskupin)
Attachment #663328 - Flags: review?(dave.hunt)
Attachment #663328 - Flags: review-
Attached patch patch v3 (obsolete) — Splinter Review
Added index parameter which gives us the current iteration in the message.
Attachment #663328 - Attachment is obsolete: true
Attachment #663338 - Flags: review?(hskupin)
Attachment #663338 - Flags: review?(dave.hunt)
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.
Attachment #663338 - Flags: review?(hskupin)
Attachment #663338 - Flags: review?(dave.hunt)
Attachment #663338 - Flags: review-
Attached patch patch v3.1 (obsolete) — Splinter Review
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.
Attachment #663338 - Attachment is obsolete: true
Attachment #663405 - Flags: review?(hskupin)
Attachment #663405 - Flags: review?(dave.hunt)
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.
Attachment #663405 - Flags: review?(hskupin)
Attachment #663405 - Flags: review?(dave.hunt)
Attachment #663405 - Flags: review-
Attached patch patch v3.2Splinter Review
Updated the messages with (aIndex + 1).
Attachment #663405 - Attachment is obsolete: true
Attachment #665322 - Flags: review?(hskupin)
Attachment #665322 - Flags: review?(dave.hunt)
Attachment #665322 - Flags: review?(hskupin)
Attachment #665322 - Flags: review?(dave.hunt)
Attachment #665322 - Flags: review+
http://hg.mozilla.org/qa/mozmill-tests/rev/e6ccf87758f0 (default)

If all is fine we want to see that backported to all other branches.
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.
We have lesser failures as before but it's still not fixed. Lets have a look at next week.
Whiteboard: s=121208 u=failure c=awesomebar p=1
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.
Keep in mind that we still have the real OS X boxes in Mozmill CI which could be used sensitively for testing that.
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.
Uploaded the VM in question to: smb://fs2.office.mozilla.org/public/QA/Virtualization/WinXP-bug792394-Henrik.tgz
Whiteboard: s=121208 u=failure c=awesomebar p=1 → s=121008 u=failure c=awesomebar p=1
Attached image 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.
(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?
(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.
(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.
Attached patch patch v1 (obsolete) — Splinter Review
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.
Attachment #673226 - Flags: review?(hskupin)
Attachment #673226 - Flags: review?(dave.hunt)
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.
Attachment #673226 - Flags: review?(hskupin)
Attachment #673226 - Flags: review?(dave.hunt)
Attachment #673226 - Attachment is obsolete: true
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
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
(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?
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.
The patch it's already pushed to Aurora and it is also applying cleanly for beta, esr10 and release.
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.
Can we skip testSwitchToTab.js on OS X for now until we have this issue fully fixed?
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.
(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.
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.
Attachment #676107 - Flags: review?(hskupin)
Attachment #676107 - Flags: review?(dave.hunt)
Attached patch fix v4 (obsolete) — Splinter Review
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.
Attachment #676194 - Flags: review?(hskupin)
Attachment #676194 - Flags: review?(dave.hunt)
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.
Attachment #676194 - Flags: review?(hskupin)
Attachment #676194 - Flags: review?(dave.hunt)
Attachment #676194 - Flags: review-
Comment on attachment 676107 [details] [diff] [review]
skip switchToTab.js

Lets get it fixed and not skipped.
Attachment #676107 - Flags: review?(hskupin)
Attachment #676107 - Flags: review?(dave.hunt)
(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).
Attached patch fix v4.1 (obsolete) — Splinter Review
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.
Attachment #676194 - Attachment is obsolete: true
Attachment #676637 - Flags: review?(hskupin)
Attachment #676637 - Flags: review?(dave.hunt)
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");
Attachment #676637 - Flags: review?(hskupin)
Attachment #676637 - Flags: review?(dave.hunt)
Attachment #676637 - Flags: review-
Attached patch patch v4.2Splinter Review
As discussed on IRC, restored forEach and updated accordingly to the review.
Attachment #676637 - Attachment is obsolete: true
Attachment #678260 - Flags: review?(hskupin)
Attachment #678260 - Flags: review?(dave.hunt)
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.
Attachment #678260 - Flags: review?(hskupin)
Attachment #678260 - Flags: review?(dave.hunt)
Attachment #678260 - Flags: review+
I have re-triggered the failing testrun on 10.7 for default. Lets see if that proves the fix.
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
Backport for esr10, beta and release.
Attachment #679096 - Flags: review?(hskupin)
Attachment #679096 - Flags: review?(dave.hunt)
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)
Attachment #679096 - Flags: review?(hskupin)
Attachment #679096 - Flags: review?(dave.hunt)
Attachment #679096 - Flags: review+
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Product: Mozilla QA → Mozilla QA Graveyard
You need to log in before you can comment on or make changes to this bug.