Last Comment Bug 770117 - Test failure "Number of visible rows should equal max rows - '1' should equal '6'" in /testAwesomeBar/testVisibleItemsMax.js
: Test failure "Number of visible rows should equal max rows - '1' should equal...
Status: RESOLVED FIXED
[mozmill-test-failure]
: regression
Product: Mozilla QA
Classification: Other
Component: Mozmill Tests (show other bugs)
: unspecified
: All All
: P2 normal (vote)
: ---
Assigned To: Andreea Matei [:AndreeaMatei]
:
Mentors:
http://mozmill-ci.blargon7.com/#/func...
Depends on:
Blocks: 583683
  Show dependency treegraph
 
Reported: 2012-07-02 03:51 PDT by Henrik Skupin (:whimboo)
Modified: 2012-09-05 03:20 PDT (History)
4 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---
fixed
fixed
fixed
unaffected
fixed


Attachments
patch v1(default) [checked-in] (1.35 KB, patch)
2012-07-02 05:34 PDT, Remus Pop (:RemusPop)
hskupin: review+
Details | Diff | Review
patch v2 (default) (2.86 KB, patch)
2012-07-16 07:10 PDT, Remus Pop (:RemusPop)
hskupin: review+
Details | Diff | Review
combinedPatch v1.0 [beta, release] (2.88 KB, patch)
2012-07-18 02:58 PDT, Alex Lakatos[:AlexLakatos]
hskupin: review+
Details | Diff | Review
combinedPatch v1.0 [esr] (4.46 KB, patch)
2012-07-18 02:59 PDT, Alex Lakatos[:AlexLakatos]
hskupin: review-
Details | Diff | Review
combinedPatch v1.1 [esr] (2.99 KB, patch)
2012-07-18 05:59 PDT, Alex Lakatos[:AlexLakatos]
hskupin: review+
Details | Diff | Review
patch v3 (1.73 KB, patch)
2012-08-16 06:13 PDT, Andreea Matei [:AndreeaMatei]
dave.hunt: review-
Details | Diff | Review
patch v3.1 (1.82 KB, patch)
2012-08-17 01:02 PDT, Andreea Matei [:AndreeaMatei]
hskupin: review-
Details | Diff | Review
patch v3.2 (1.91 KB, patch)
2012-08-17 08:14 PDT, Andreea Matei [:AndreeaMatei]
dave.hunt: review-
Details | Diff | Review
patch v4 (1.80 KB, patch)
2012-08-20 01:39 PDT, Andreea Matei [:AndreeaMatei]
hskupin: review-
Details | Diff | Review
patch v4.1 (1.77 KB, patch)
2012-08-21 01:04 PDT, Andreea Matei [:AndreeaMatei]
hskupin: review-
Details | Diff | Review
patch v4.2 [checked-in] (1.76 KB, patch)
2012-08-21 01:55 PDT, Andreea Matei [:AndreeaMatei]
hskupin: review+
Details | Diff | Review
[beta, release] patch v1 [checked-in] (1.78 KB, patch)
2012-09-04 05:47 PDT, Andreea Matei [:AndreeaMatei]
hskupin: review+
Details | Diff | Review
[ESR10] patch v1 (1.78 KB, patch)
2012-09-04 05:51 PDT, Andreea Matei [:AndreeaMatei]
hskupin: review-
Details | Diff | Review
[ESR10] patch v1.1 (1.78 KB, patch)
2012-09-05 00:52 PDT, Andreea Matei [:AndreeaMatei]
hskupin: review+
Details | Diff | Review

Description Henrik Skupin (:whimboo) 2012-07-02 03:51:37 PDT
This is a fairly recent regression which has been started on July 1st across platforms.

http://mozmill-ci.blargon7.com/#/functional/failure?branch=16.0&platform=Windows%20NT&from=2012-06-06&test=%2FtestAwesomeBar%2FtestVisibleItemsMax.js&func=testVisibleItemsMax.js%3A%3AtestVisibleItemsMax

Remus, I think we simply have to wait for open and assert for the amount of entries afterward. Also can you please check if something has been changed in Firefox itself? Thanks.
Comment 1 Remus Pop (:RemusPop) 2012-07-02 05:34:44 PDT
Created attachment 638320 [details] [diff] [review]
patch v1(default) [checked-in]

I've found that in the 1st July Nightly, the number of visible items have been changed to 12 (bug 583683).
Submited a fix with those changes.
Comment 2 Henrik Skupin (:whimboo) 2012-07-02 06:03:07 PDT
Comment on attachment 638320 [details] [diff] [review]
patch v1(default) [checked-in]

Looks fine but given the open question on bug 583683 we should wait with closing this bug until the final solution has been found.
Comment 3 Henrik Skupin (:whimboo) 2012-07-02 06:04:55 PDT
Comment on attachment 638320 [details] [diff] [review]
patch v1(default) [checked-in]

Pushed to default:
http://hg.mozilla.org/qa/mozmill-tests/rev/2394b81e8318
Comment 4 Henrik Skupin (:whimboo) 2012-07-04 02:20:43 PDT
Looks like the setting has been changed again. We get test failures:

http://mozmill-ci.blargon7.com/#/functional/report/4c461f9adf1253771fc6455667081761

Remus please come up with a fix ASAP. Thanks.
Comment 5 Remus Pop (:RemusPop) 2012-07-04 02:34:10 PDT
Henrik, this report comes from an old Firefox build:
Application: 	Firefox 16.0a1 (16.0a1, en-US, 20120630030532)

I think something went wrong on the test machine and Firefox did not update.
This is the latest build id available in this moment: 20120703110846
Comment 6 Henrik Skupin (:whimboo) 2012-07-04 03:11:21 PDT
Strange. That's the second request which is for an older build from yesterday. Not sure what happened here. Thanks Remus.
Comment 7 Henrik Skupin (:whimboo) 2012-07-05 07:08:23 PDT
Looks good so far. Lets close this bug until something will change again.
Comment 8 Remus Pop (:RemusPop) 2012-07-16 00:29:37 PDT
We should back out this as per the back out in bug 586683 comment 28.
Comment 9 Remus Pop (:RemusPop) 2012-07-16 00:30:44 PDT
Sorry, this meant to be bug 583683 comment 28.
Comment 10 Henrik Skupin (:whimboo) 2012-07-16 03:06:05 PDT
What we should do here is not only to revert the change but also to call waitFor for the popup to open and after that doing the actual check for the numbers. That way we will see the current and expected value in the dashboard.

Remus, can you work on that right now? I would appreciate that.
Comment 11 Remus Pop (:RemusPop) 2012-07-16 07:10:43 PDT
Created attachment 642575 [details] [diff] [review]
patch v2 (default)

We now wait for the autocomplete to open, and then compare the maxrows with the visible results. Maxrows is taken from browser.xul.
Comment 12 Henrik Skupin (:whimboo) 2012-07-16 11:38:56 PDT
Comment on attachment 642575 [details] [diff] [review]
patch v2 (default)

Looks good. Will land on default and we might considering to even update older branches. It just makes us more robust.
Comment 13 Henrik Skupin (:whimboo) 2012-07-16 11:41:52 PDT
Pushed to default:
http://hg.mozilla.org/qa/mozmill-tests/rev/23e03fc16f87

Remus, please check if we can backport the patch to older branches.
Comment 14 Henrik Skupin (:whimboo) 2012-07-17 08:06:06 PDT
(In reply to Henrik Skupin (:whimboo) from comment #13)
> Remus, please check if we can backport the patch to older branches.

Given that Remus is away, Vlad please come up with your results.
Comment 15 Alex Lakatos[:AlexLakatos] 2012-07-18 02:58:23 PDT
Created attachment 643304 [details] [diff] [review]
combinedPatch v1.0 [beta, release]

Taking this.
Patch for beta and release branch. The patch seems to already be applied on the aurora branch.
Comment 16 Alex Lakatos[:AlexLakatos] 2012-07-18 02:59:05 PDT
Created attachment 643305 [details] [diff] [review]
combinedPatch v1.0 [esr]

combined patch for the esr branch
Comment 17 Henrik Skupin (:whimboo) 2012-07-18 05:01:58 PDT
Comment on attachment 643305 [details] [diff] [review]
combinedPatch v1.0 [esr]

>+const PREF_LOCATION_BAR_SUGGEST = "browser.urlbar.default.behavior";
[..]
>+
>+  // Ensure Location bar suggests "History and Bookmarks"
>+  prefs.preferences.setPref(PREF_LOCATION_BAR_SUGGEST, 0);

All that code is part of the preferences patch but not this one. Please update the patch and remove all the preferences code changes.
Comment 19 Alex Lakatos[:AlexLakatos] 2012-07-18 05:59:25 PDT
Created attachment 643343 [details] [diff] [review]
combinedPatch v1.1 [esr]

removed pref code
Comment 20 Henrik Skupin (:whimboo) 2012-07-18 06:54:33 PDT
Fixed on esr10 branch:
http://hg.mozilla.org/qa/mozmill-tests/rev/0e2cfac9262b
Comment 21 Henrik Skupin (:whimboo) 2012-07-18 09:51:40 PDT
So we failed on Aurora today:
http://mozmill-ci.blargon7.com/#/functional/report/89726f6b98208a209e7ce2df1020b1b9

Number of visible rows should equal max rows - '1' should equal '6' 

So, instead of 6 results only 1 is visible. Could it be that we really have to use a waitFor twice? Looks like the results are getting slowly populated.
Comment 22 Alex Lakatos[:AlexLakatos] 2012-07-19 05:03:38 PDT
(In reply to Henrik Skupin (:whimboo) from comment #21)
> So we failed on Aurora today:
> http://mozmill-ci.blargon7.com/#/functional/report/
> 89726f6b98208a209e7ce2df1020b1b9
> 
> Number of visible rows should equal max rows - '1' should equal '6' 
> 
> So, instead of 6 results only 1 is visible. Could it be that we really have
> to use a waitFor twice? Looks like the results are getting slowly populated.
Given the error message, I think we are not waiting enough for the results to be populated.The results getting populated slow have nothing to do with us waiting twice. 
The results are slowly populated when there is high cpu usage. Given that we run in VM's, that is likely to happen. I think we should increase the timeout for the second waitFor, the one that waits for the results to be populated.
Comment 23 Henrik Skupin (:whimboo) 2012-08-08 10:11:00 PDT
Andreea, you wanted to take this failure? Have you made any progress on it today? Otherwise can Alex help out? If this is reproducible for you please find a fix and don't wait for an answer from one of us.
Comment 24 Andreea Matei [:AndreeaMatei] 2012-08-08 11:48:16 PDT
I have ran this about 50 times on a loaded MAC OS today, it reproduces, failing at the last expect where maxrows returns not a number - NaN. I'll have a fix for tomorrow.
Comment 25 Andreea Matei [:AndreeaMatei] 2012-08-09 05:49:49 PDT
I believe yesterday it reproduced with that "NaN" failure because mozmill was not upgraded on that Mac machine. Today from all the runs (single test and functional), it didn't failed once, we have reports here:
Default:
* http://mozmill-crowd.blargon7.com/#/functional/report/564ef78d736576a0c04ee6c7f00078e1
* http://mozmill-crowd.blargon7.com/#/functional/report/564ef78d736576a0c04ee6c7f00067af

Aurora:
* http://mozmill-crowd.blargon7.com/#/functional/report/564ef78d736576a0c04ee6c7f001fcb9

I will continue to check this and already looking for properties or attributes to use if another waitFor is needed.
Comment 26 Alex Lakatos[:AlexLakatos] 2012-08-09 07:38:42 PDT
This sounds like a trial and error type of fix. I was thinking, can't we start runs on the VM's with try repos? Kind of like try builds are. We use default in that manner, as a try branch, but sometimes we need to try something new on different branches. Is that possible, Henrik/Dave?
Comment 27 Henrik Skupin (:whimboo) 2012-08-09 23:04:04 PDT
We do not support try-server like testruns with Mozmill yet. I don't think we ever created a bug for it. Please check, and if there is none feel free to file one. But this is separate.
Comment 28 Henrik Skupin (:whimboo) 2012-08-15 03:11:16 PDT
Andreea, do you have an update for us? 6 days have been passed.
Comment 29 Andreea Matei [:AndreeaMatei] 2012-08-16 06:13:35 PDT
Created attachment 652419 [details] [diff] [review]
patch v3

I still wasn't able to reproduce the failure, but since we have that report, I've added a second wait for the autocomplete list to load, just to be sure.
I say we give this a try and if still failing we could increase the timeout on the waitFor.
Comment 30 Dave Hunt (:davehunt) 2012-08-16 10:04:50 PDT
Comment on attachment 652419 [details] [diff] [review]
patch v3

>Bug 770117 - Test failure 'Number of visible rows returned should equal 6' in /testAwesomeBar/testVisibleItemsMax.js. r=hskupin, r=dhunt

This should be updated to reflect the current failure message.

>+  controller.waitFor(function () {
>+    return locationBar.autoCompleteResults.length != 0;
>+  }, "Wait for autocomplete list to load");

I doubt this will be enough. The failure is due to there being less visible items than we're expecting, and therefore we should wait for the number of visible items to equal our expectation.
Comment 31 Andreea Matei [:AndreeaMatei] 2012-08-17 01:02:04 PDT
Created attachment 652694 [details] [diff] [review]
patch v3.1

Replaced expect with waitFor the visible rows to be equal with max rows.
Comment 32 Henrik Skupin (:whimboo) 2012-08-17 06:48:44 PDT
Comment on attachment 652694 [details] [diff] [review]
patch v3.1

>   var visibleRows = autoCompleteResultsList.getNode().getNumberOfVisibleRows();
>   var maxRows = locationBar.urlbar.getNode().getAttribute("maxrows");
[..]
>+  controller.waitFor(function () {
>+    return visibleRows === parseInt(maxRows);
>+  }, "Number of visible results from the autocomplete list - got: " +
>+    visibleRows + ", expected " + parseInt(maxRows));

This will not work because visibleRows is a plain number and not reference to an object. It will never get updated. You have to get rid of visibleRows and put the whole line into the waitFor callback. Also in this case you can revert the message because you can't make use of visibleRows anymore.
Comment 33 Andreea Matei [:AndreeaMatei] 2012-08-17 08:14:40 PDT
Created attachment 652774 [details] [diff] [review]
patch v3.2

Changed the waitFor to compare directly the visible rows through getNumberOfVisibleRows() and reverted the message.
Comment 34 Dave Hunt (:davehunt) 2012-08-17 09:33:05 PDT
Comment on attachment 652774 [details] [diff] [review]
patch v3.2

>Bug 770117 - Test failure 'Number of visible rows should equal max rows - '1' should equal '6'' in /testAwesomeBar/testVisibleItemsMax.js. r=hskupin, r=dhunt

Could you wrap lines with single quotes with double quotes.

>+  controller.waitFor(function () {
>+    return autoCompleteResultsList.getNode().getNumberOfVisibleRows() === parseInt(maxRows);
>+  }, "Number of visible rows in the autocomplete list - expected " + parseInt(maxRows) + 
>+    ", got " + autoCompleteResultsList.getNode().getNumberOfVisibleRows());

There's a chance this message could be misleading, as the value of autoCompleteResultsList.getNode().getNumberOfVisibleRows() could change after the wait has timed out. I would suggest using the original "Number of visible rows should equal max rows", unless Henrik feels the is an acceptable risk.
Comment 35 Andreea Matei [:AndreeaMatei] 2012-08-20 01:39:07 PDT
Created attachment 653307 [details] [diff] [review]
patch v4

Addressed Dave's request regarding the waitFor message.
Comment 36 Henrik Skupin (:whimboo) 2012-08-20 12:46:52 PDT
Comment on attachment 653307 [details] [diff] [review]
patch v4

>+  controller.waitFor(function () {
>     return locationBar.autoCompleteResults.isOpened;
>   }, "Autocomplete list has been opened");

I wonder if we can get rid of this waitFor call given that getNumberOfVisibleRows() will probably return 0 while the popup has not been opened?

>+  controller.waitFor(function () {
>+    return autoCompleteResultsList.getNode().getNumberOfVisibleRows() === parseInt(maxRows);
>+  }, "Number of visible rows should equal max rows");

What Dave mentioned was that you should put the actual maxRows number into the message string. That way we at least know which value we expect to see.
Comment 37 Andreea Matei [:AndreeaMatei] 2012-08-21 01:04:06 PDT
Created attachment 653683 [details] [diff] [review]
patch v4.1

Removed the first waitFor, I've tested with heavy load environment and it passes.
Also updated the other waitFor message.
Comment 38 Henrik Skupin (:whimboo) 2012-08-21 01:50:33 PDT
Comment on attachment 653683 [details] [diff] [review]
patch v4.1

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

Looks fine. Just fix the remaining little thing and we can get it checked in.

::: tests/functional/testAwesomeBar/testVisibleItemsMax.js
@@ +67,4 @@
>    var maxRows = locationBar.urlbar.getNode().getAttribute("maxrows");
> +  controller.waitFor(function () {
> +    return autoCompleteResultsList.getNode().getNumberOfVisibleRows() === parseInt(maxRows);
> +  }, "Number of visible rows should equal max rows: " + maxRows);

Please kill the 'max rows' in the string. This doesn't give us any information.
Comment 39 Andreea Matei [:AndreeaMatei] 2012-08-21 01:55:15 PDT
Created attachment 653690 [details] [diff] [review]
patch v4.2 [checked-in]

Removed 'max rows' from the string.
Comment 40 Henrik Skupin (:whimboo) 2012-08-21 02:24:29 PDT
Landed on default:
http://hg.mozilla.org/qa/mozmill-tests/rev/993744da615f

Lets check later today if we can backport this patch.
Comment 41 Henrik Skupin (:whimboo) 2012-09-03 16:03:32 PDT
Andreea, I miss a follow-up on this bug from your side. We still have to backport this patch, so what's the status? Do we need separate patches?
Comment 42 Andreea Matei [:AndreeaMatei] 2012-09-04 05:47:46 PDT
Created attachment 658053 [details] [diff] [review]
[beta, release] patch v1 [checked-in]

Patch v4.2 works for default and aurora.
Uploading patch for beta and release.
Comment 43 Andreea Matei [:AndreeaMatei] 2012-09-04 05:51:16 PDT
Created attachment 658054 [details] [diff] [review]
[ESR10] patch v1

Patch for esr10.
Comment 44 Henrik Skupin (:whimboo) 2012-09-05 00:32:48 PDT
(In reply to Andreea Matei [:AndreeaMatei] from comment #42)
> Patch v4.2 works for default and aurora.

There is no need for the aurora branch. It got this fix by the last merge:
http://hg.mozilla.org/qa/mozmill-tests/rev/63bdef26047a
Comment 45 Henrik Skupin (:whimboo) 2012-09-05 00:42:01 PDT
Comment on attachment 658054 [details] [diff] [review]
[ESR10] patch v1

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

::: tests/functional/testAwesomeBar/testVisibleItemsMax.js
@@ +67,5 @@
>    var maxRows = locationBar.urlbar.getNode().getAttribute("maxrows");
> +  controller.waitFor(function () {
> +    return autoCompleteResultsList.getNode().getNumberOfVisibleRows() === parseInt(maxRows);
> +  }, "Number of visible rows should equal " + maxRows);
> + 

nit: please correct this whitespace issue.
Comment 47 Andreea Matei [:AndreeaMatei] 2012-09-05 00:52:03 PDT
Created attachment 658406 [details] [diff] [review]
[ESR10] patch v1.1

Removed whitespace.
Comment 48 Henrik Skupin (:whimboo) 2012-09-05 03:20:07 PDT
Landed on esr10:
http://hg.mozilla.org/qa/mozmill-tests/rev/f495c23f0887

Note You need to log in before you can comment on or make changes to this bug.