784644 - Test failure 'The page title matches the underlined text - 'grants' should equal 'grant' ' in /testAwesomeBar/testSuggestBookmarks.js

Status: RESOLVED FIXED

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

Description

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

Looks like the failure we already thought to have fixed on bug 742550 is back. Probably it could also be a new regression or a change in Firefox. So we have to investigate this problem.

It only happened on OS X 10.7 for now.

Comment 1

[Maniac Vlad Florin (:vladmaniac)]

Looks like we need another check in this test. 
Seeing the failure message it seems we do not type the last character 's' from 'grants'
I suggest to wait for the string in the location bar to be the expected one. If there is a hang in the test, the timeout error will confirm that. 

Does any of this make sense?

Comment 2

[Maniac Vlad Florin (:vladmaniac)]

Question: do we still need this? 

for each (var entry in entries) {
    expect.equal(LOCAL_TEST_PAGE.string, entry.toLowerCase(),
                 "The page title matches the underlined text");
  }

From what I have seen we are typing the whole string now and there is only one entry ever time in 'entries'. Just make some dumps in the console to see that

Comment 3

[Maniac Vlad Florin (:vladmaniac)]

Also this 
https://bugzilla.mozilla.org/show_bug.cgi?id=742550#c6
was not addressed and we still have the flipped order of expect.equal arguments. 
Will have to land with a patch here then

Comment 4

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

locationBar_type() already checks if the text is contained. But it doesn't wait. So probably that's the cause here. We could remove the .contains() call and let the tests do the check whenever they have. That's more appropriate but it needs a check for all other tests which make use of .type().

Comment 5

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

testCheckItemHighlight fails in a similar manner:
http://mozmill-ci.blargon7.com/#/functional/report/d87d47fd1034f072b9bece6ee6840738

So probably worth fixing on a module level.

Comment 6

[Maniac Vlad Florin (:vladmaniac)]

Attachment: fix patch v1.0

* proposed fix
* fix at the module level, meaning updating locationBar_type with a waitFor needed to wait for the url bar to really contain the typed strings 
* refactored test to make usage of the editBookmarksPanel UI map re 'doneButton' element
* fixed wrong expect call

Comment 7

[Maniac Vlad Florin (:vladmaniac)]

If this does not fix our errors at least we are getting closer by having another error

Comment 8

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

Comment on attachment 654579 [details] [diff] [review]
fix patch v1.0

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

::: lib/toolbars.js
@@ +566,5 @@
>    type : function locationBar_type(text) {
>      this._controller.type(this.urlbar, text);
> +    this._controller.waitFor(function () {
> +      return this.contains(text);
> +    }, "Location bar contains the typed data", undefined, undefined, this);

No, that's not what I wanted. As I have said we should remove any kind of check from the module and do that on the test level.

::: tests/functional/testAwesomeBar/testSuggestBookmarks.js
@@ +71,5 @@
>    // Define the path to the first auto-complete result
>    var richlistItem = locationBar.autoCompleteResults.getResult(0);
>  
>    var entries = locationBar.autoCompleteResults.getUnderlinedText(richlistItem, "title");
>    for each (var entry in entries) {

Change this to forEach because 'for each' can cause failures.

@@ +72,5 @@
>    var richlistItem = locationBar.autoCompleteResults.getResult(0);
>  
>    var entries = locationBar.autoCompleteResults.getUnderlinedText(richlistItem, "title");
>    for each (var entry in entries) {
> +    expect.equal(entry.toLowerCase(), LOCAL_TEST_PAGE.string,

Good catch.

Comment 9

[Maniac Vlad Florin (:vladmaniac)]

Attachment: fix patch v2

* Moved .contains calls in all tests which make usage of locationBar.type
* Updated tests to remove unnecessary sleep calls which were replaced by waitFors
* Updated tests to remove for each with forEach, where applicable. 
* Tested the patch on default branch

Comment 10

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

Comment on attachment 654951 [details] [diff] [review]
fix patch v2

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

::: tests/functional/testAwesomeBar/testCheckItemHighlight.js
@@ +50,5 @@
>    locationBar.clear();
>  
>    // Type the page name into the location bar
>    locationBar.type(LOCAL_TEST_PAGES[0].name);
> +  controller.waitFor(function () {

This looks unnecessary to me. We next wait for the strings to match, so why first wait for one string to contain the other?

::: tests/functional/testAwesomeBar/testEscapeAutocomplete.js
@@ +47,3 @@
>    for (var i = 0; i < TEST_STRING.length; i++) {
>      locationBar.type(TEST_STRING[i]);
> +    controller.waitFor(function () {

We type one character at a time, so this is invalid as soon as we type the same character twice. If the full string was 'foo' then the final character would not wait for the final 'o' because the location bar already contains one.

::: tests/functional/testAwesomeBar/testFaviconInAutocomplete.js
@@ +52,5 @@
>  
>    // Type in each letter of the test string to allow the autocomplete to populate with results
>    for each (var letter in LOCAL_TEST_PAGE.string) {
>      locationBar.type(letter);
> +    controller.waitFor(function () {

As mentioned, this would be invalid for any string containing the same character more than once.

::: tests/functional/testAwesomeBar/testVisibleItemsMax.js
@@ +59,3 @@
>    for each (var letter in testString) {
>      locationBar.type(letter);
> +    controller.waitFor(function () {

Same comment here as above.

@@ +64,4 @@
>    }
>  
> +  // Wait for the autocomplete list to be opened
> +  controller.waitFor(function () {

We recently removed this waitFor. Why are we reintroducing it?

Comment 11

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

Andreea, would you mind to continue here? Would be appreciated.

Comment 12

[Andreea Matei [:AndreeaMatei]]

Attachment: patch v3

Regarding the last concerns:
Because of the autocomplete in the locationBar, while typing, we can't compare the length of the text typed with urlbar length.
I've found an attribute (selectionStart) which returns the index where the autocomplete text starts. If we type "m" we have highlighted "ozilla" as autocomplete.
In the waitFor we now compare the length of the text typed so far, increased with each step, against the index. On some cases is index - 1 because we start with i=0. 

Testrun on Mac OS X 10.7.4 since on Mac was reported the failure:
http://mozmill-crowd.blargon7.com/#/functional/report/d87d47fd1034f072b9bece6ee6de81ae

Comment 13

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

Why do we have to type in each character on its own? We have changed that lately to the whole string in another fixed bug. Why do we revert this too?

Comment 14

[Andreea Matei [:AndreeaMatei]]

Attachment: patch v4

Sorry, I misunderstood the comments.

Have now a waitFor the locationBar value to be equal to the typed string.

Results:
* http://mozmill-crowd.blargon7.com/#/functional/report/d87d47fd1034f072b9bece6ee6e29b22
* http://mozmill-crowd.blargon7.com/#/functional/report/d87d47fd1034f072b9bece6ee6e31159

Comment 15

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

Comment on attachment 656445 [details] [diff] [review]
patch v4

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

This bug is about a failure in testSuggestBookmarks.js so I would like to see that we limit our changes to that single file or if necessary the lib modules. Please update the patch accordingly and file a follow-up bug if you want to see the other tests modified.

::: tests/functional/testAwesomeBar/testSuggestBookmarks.js
@@ +46,5 @@
>    // editBookmarksPanel is loaded lazily. Wait until overlay for StarUI has been loaded, then close the dialog
>    controller.waitFor(function () {
>      return controller.window.top.StarUI._overlayLoaded;
>    }, "Edit This Bookmark doorhanger has been loaded");
> +  var doneButton = locationBar.editBookmarksPanel.getElement({type: "doneButton"});

This change doesn't affect the fix for the failure and should be moved out to the follow-up bug.

@@ +80,2 @@
>                   "The page title matches the underlined text");
> +  });

Similar to this change. Otherwise this file looks good.

Comment 16

[Andreea Matei [:AndreeaMatei]]

Attachment: patch v5

Left only the waitFor in testSuggestBookmarks.js for this patch.

Comment 17

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

Comment on attachment 656818 [details] [diff] [review]
patch v5

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

So this will not work because you left out the removal of 'this.contains(text);' from the toolbars.js module. That way the test will still fail. Please test if this change will affect other tests for the locationbar.

Comment 18

[Andreea Matei [:AndreeaMatei]]

Attachment: patch v5.1

Runned functional testruns on Mac and Ubuntu and no tests are affected by removing .contains from toolbars.js

Comment 19

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

Comment on attachment 656834 [details] [diff] [review]
patch v5.1

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

Looks fine then. Lets see how it sticks before backporting to older branches.

Comment 20

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

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

Comment 21

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

http://hg.mozilla.org/qa/mozmill-tests/rev/b027cfc565c9 (aurora)
http://hg.mozilla.org/qa/mozmill-tests/rev/302f05bc5c51 (beta)
http://hg.mozilla.org/qa/mozmill-tests/rev/84feaad3962d (release)

Andreea, can you please provide a backport for the esr10 branch?

Comment 22

[Andreea Matei [:AndreeaMatei]]

Attachment: [ESR10] patch v1

This patch applies to Esr10. 
The weird thing here is that by just removing this line in lib/toolbars.js
 this.contains(text);

the patch is created with the next function removed and then added back. 
We don't change anything there, tried several times, also in Alex's machine. On default didn't had this issue.

Comment 23

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

Comment on attachment 658451 [details] [diff] [review]
[ESR10] patch v1

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

Landed as:
http://hg.mozilla.org/qa/mozmill-tests/rev/ac03c289d07d (esr10)

Looks like a line endings issue that caused the need for a ESR10 patch.