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.

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 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 on attachment 638320 [details] [diff] [review] patch v1(default) [checked-in] Pushed to default: http://hg.mozilla.org/qa/mozmill-tests/rev/2394b81e8318

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.

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

Strange. That's the second request which is for an older build from yesterday. Not sure what happened here. Thanks Remus.

Looks good so far. Lets close this bug until something will change again.

We should back out this as per the back out in bug 586683 comment 28.

Sorry, this meant to be bug 583683 comment 28.

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.

We now wait for the autocomplete to open, and then compare the maxrows with the visible results. Maxrows is taken from browser.xul.

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.

Pushed to default: http://hg.mozilla.org/qa/mozmill-tests/rev/23e03fc16f87 Remus, please check if we can backport the patch to older branches.

(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.

Taking this. Patch for beta and release branch. The patch seems to already be applied on the aurora branch.

combined patch for the esr branch

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.

Pushed: http://hg.mozilla.org/qa/mozmill-tests/rev/fad71ab926c0 (beta) http://hg.mozilla.org/qa/mozmill-tests/rev/19f2a0690021 (release)

removed pref code

Fixed on esr10 branch: http://hg.mozilla.org/qa/mozmill-tests/rev/0e2cfac9262b

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.

(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.

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.

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.

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.

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?

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.

Andreea, do you have an update for us? 6 days have been passed.

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 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.

Replaced expect with waitFor the visible rows to be equal with max rows.

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.

Changed the waitFor to compare directly the visible rows through getNumberOfVisibleRows() and reverted the message.

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.

Addressed Dave's request regarding the waitFor message.

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.

Removed the first waitFor, I've tested with heavy load environment and it passes. Also updated the other waitFor message.

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.

Removed 'max rows' from the string.

Landed on default: http://hg.mozilla.org/qa/mozmill-tests/rev/993744da615f Lets check later today if we can backport this patch.

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?

Patch v4.2 works for default and aurora. Uploading patch for beta and release.

Patch for esr10.

(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 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.

http://hg.mozilla.org/qa/mozmill-tests/rev/4026325038f0 (beta) http://hg.mozilla.org/qa/mozmill-tests/rev/4ff0256c50ac (release)

Removed whitespace.

Landed on esr10: http://hg.mozilla.org/qa/mozmill-tests/rev/f495c23f0887