Test failure "Number of visible rows should equal max rows - '1' should equal '6'" in /testAwesomeBar/testVisibleItemsMax.js

RESOLVED FIXED

Status

Mozilla QA
Mozmill Tests
P2
normal
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: whimboo, Assigned: AndreeaMatei)

Tracking

({regression})

unspecified
regression

Firefox Tracking Flags

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

Details

(Whiteboard: [mozmill-test-failure], URL)

Attachments

(6 attachments, 8 obsolete attachments)

1.35 KB, patch
whimboo
: review+
Details | Diff | Splinter Review
2.88 KB, patch
whimboo
: review+
Details | Diff | Splinter Review
2.99 KB, patch
whimboo
: review+
Details | Diff | Splinter Review
1.76 KB, patch
whimboo
: review+
Details | Diff | Splinter Review
1.78 KB, patch
whimboo
: review+
Details | Diff | Splinter Review
1.78 KB, patch
whimboo
: review+
Details | Diff | Splinter Review
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.
Assignee: nobody → remus.pop
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.
Attachment #638320 - Flags: review?(hskupin)
Attachment #638320 - Flags: review?(dave.hunt)
Blocks: 583683
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.
Attachment #638320 - Flags: review?(hskupin)
Attachment #638320 - Flags: review?(dave.hunt)
Attachment #638320 - Flags: review+
Comment on attachment 638320 [details] [diff] [review]
patch v1(default) [checked-in]

Pushed to default:
http://hg.mozilla.org/qa/mozmill-tests/rev/2394b81e8318
Attachment #638320 - Attachment description: patch v1(default) → patch v1(default) [checked-in]
Status: NEW → ASSIGNED
status-firefox15: --- → unaffected
status-firefox16: --- → affected
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.
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED

Updated

5 years ago
status-firefox16: affected → ---
We should back out this as per the back out in bug 586683 comment 28.
Status: RESOLVED → REOPENED
status-firefox16: --- → affected
Resolution: FIXED → ---
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.
Status: REOPENED → ASSIGNED
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.
Attachment #642575 - Flags: review?(hskupin)
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.
Attachment #642575 - Flags: review?(hskupin) → review+
Pushed to default:
http://hg.mozilla.org/qa/mozmill-tests/rev/23e03fc16f87

Remus, please check if we can backport the patch to older branches.
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago5 years ago
status-firefox16: affected → fixed
Resolution: --- → FIXED
(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.
Assignee: remus.pop → vlad.mozbugs
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.
Assignee: vlad.mozbugs → alex.lakatos
Attachment #643304 - Flags: review?(hskupin)
Created attachment 643305 [details] [diff] [review]
combinedPatch v1.0 [esr]

combined patch for the esr branch
Attachment #643305 - Flags: review?(hskupin)
Attachment #643304 - Flags: review?(hskupin) → review+
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.
Attachment #643305 - Flags: review?(hskupin) → review-
Pushed:
http://hg.mozilla.org/qa/mozmill-tests/rev/fad71ab926c0 (beta)
http://hg.mozilla.org/qa/mozmill-tests/rev/19f2a0690021 (release)
status-firefox-esr10: --- → affected
status-firefox14: --- → fixed
status-firefox15: unaffected → fixed
status-firefox17: --- → fixed
Created attachment 643343 [details] [diff] [review]
combinedPatch v1.1 [esr]

removed pref code
Attachment #643305 - Attachment is obsolete: true
Attachment #643343 - Flags: review?(hskupin)
Attachment #643343 - Flags: review?(dave.hunt)
Attachment #643343 - Flags: review?(hskupin)
Attachment #643343 - Flags: review?(dave.hunt)
Attachment #643343 - Flags: review+
Fixed on esr10 branch:
http://hg.mozilla.org/qa/mozmill-tests/rev/0e2cfac9262b
status-firefox-esr10: affected → fixed
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(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.
(Assignee)

Comment 24

5 years ago
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.
(Assignee)

Comment 25

5 years ago
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.
Priority: -- → P2
(Assignee)

Updated

5 years ago
Assignee: alex.lakatos → andreea.matei
(Assignee)

Comment 29

5 years ago
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.
Attachment #642575 - Attachment is obsolete: true
Attachment #652419 - Flags: review?(hskupin)
Attachment #652419 - Flags: review?(dave.hunt)
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.
Attachment #652419 - Flags: review?(hskupin)
Attachment #652419 - Flags: review?(dave.hunt)
Attachment #652419 - Flags: review-
Summary: Test failure 'Number of visible rows returned should equal 6' in /testAwesomeBar/testVisibleItemsMax.js → Test failure "Number of visible rows should equal max rows - '1' should equal '6'" in /testAwesomeBar/testVisibleItemsMax.js
(Assignee)

Comment 31

5 years ago
Created attachment 652694 [details] [diff] [review]
patch v3.1

Replaced expect with waitFor the visible rows to be equal with max rows.
Attachment #652419 - Attachment is obsolete: true
Attachment #652694 - Flags: review?(hskupin)
Attachment #652694 - Flags: review?(dave.hunt)
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.
Attachment #652694 - Flags: review?(hskupin)
Attachment #652694 - Flags: review?(dave.hunt)
Attachment #652694 - Flags: review-
(Assignee)

Comment 33

5 years ago
Created attachment 652774 [details] [diff] [review]
patch v3.2

Changed the waitFor to compare directly the visible rows through getNumberOfVisibleRows() and reverted the message.
Attachment #652694 - Attachment is obsolete: true
Attachment #652774 - Flags: review?(hskupin)
Attachment #652774 - Flags: review?(dave.hunt)
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.
Attachment #652774 - Flags: review?(hskupin)
Attachment #652774 - Flags: review?(dave.hunt)
Attachment #652774 - Flags: review-
(Assignee)

Comment 35

5 years ago
Created attachment 653307 [details] [diff] [review]
patch v4

Addressed Dave's request regarding the waitFor message.
Attachment #652774 - Attachment is obsolete: true
Attachment #653307 - Flags: review?(hskupin)
Status: REOPENED → ASSIGNED
status-firefox-esr10: fixed → affected
status-firefox14: fixed → affected
status-firefox15: fixed → affected
status-firefox16: fixed → affected
status-firefox17: fixed → affected
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.
Attachment #653307 - Flags: review?(hskupin) → review-
(Assignee)

Comment 37

5 years ago
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.
Attachment #653307 - Attachment is obsolete: true
Attachment #653683 - Flags: review?(hskupin)
Attachment #653683 - Flags: review?(dave.hunt)
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.
Attachment #653683 - Flags: review?(hskupin)
Attachment #653683 - Flags: review?(dave.hunt)
Attachment #653683 - Flags: review-
(Assignee)

Comment 39

5 years ago
Created attachment 653690 [details] [diff] [review]
patch v4.2 [checked-in]

Removed 'max rows' from the string.
Attachment #653683 - Attachment is obsolete: true
Attachment #653690 - Flags: review?(hskupin)
Attachment #653690 - Flags: review?(hskupin) → review+
Landed on default:
http://hg.mozilla.org/qa/mozmill-tests/rev/993744da615f

Lets check later today if we can backport this patch.
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago5 years ago
status-firefox17: affected → fixed
Resolution: --- → FIXED
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?
(Assignee)

Comment 42

5 years ago
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.
Attachment #658053 - Flags: review?(hskupin)
Attachment #658053 - Flags: review?(dave.hunt)
(Assignee)

Comment 43

5 years ago
Created attachment 658054 [details] [diff] [review]
[ESR10] patch v1

Patch for esr10.
Attachment #658054 - Flags: review?(hskupin)
Attachment #658054 - Flags: review?(dave.hunt)
Attachment #653690 - Attachment description: patch v4.2 → patch v4.2 [checked-in]
Attachment #658053 - Flags: review?(hskupin)
Attachment #658053 - Flags: review?(dave.hunt)
Attachment #658053 - Flags: review+
(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
status-firefox18: --- → unaffected
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.
Attachment #658054 - Flags: review?(hskupin)
Attachment #658054 - Flags: review?(dave.hunt)
Attachment #658054 - Flags: review-
http://hg.mozilla.org/qa/mozmill-tests/rev/4026325038f0 (beta)
http://hg.mozilla.org/qa/mozmill-tests/rev/4ff0256c50ac (release)
status-firefox14: affected → ---
status-firefox15: affected → fixed
status-firefox16: affected → fixed
(Assignee)

Comment 47

5 years ago
Created attachment 658406 [details] [diff] [review]
[ESR10] patch v1.1

Removed whitespace.
Attachment #658054 - Attachment is obsolete: true
Attachment #658406 - Flags: review?(hskupin)
Attachment #658406 - Flags: review?(dave.hunt)
Attachment #658406 - Flags: review?(hskupin)
Attachment #658406 - Flags: review?(dave.hunt)
Attachment #658406 - Flags: review+
Attachment #658053 - Attachment description: [beta, release] patch v1 → [beta, release] patch v1 [checked-in]
Landed on esr10:
http://hg.mozilla.org/qa/mozmill-tests/rev/f495c23f0887
status-firefox-esr10: affected → fixed
You need to log in before you can comment on or make changes to this bug.