Last Comment Bug 784644 - Test failure 'The page title matches the underlined text - 'grants' should equal 'grant' ' in /testAwesomeBar/testSuggestBookmarks.js
: Test failure 'The page title matches the underlined text - 'grants' should eq...
Product: Mozilla QA
Classification: Other
Component: Mozmill Tests (show other bugs)
: unspecified
: All Mac OS X
: P2 normal (vote)
: ---
Assigned To: Andreea Matei [:AndreeaMatei]
: mozmill-tests
Depends on:
Blocks: 787924
  Show dependency treegraph
Reported: 2012-08-22 03:36 PDT by Henrik Skupin (:whimboo)
Modified: 2012-09-07 15:29 PDT (History)
4 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---

fix patch v1.0 (2.89 KB, patch)
2012-08-23 05:20 PDT, Maniac Vlad Florin (:vladmaniac)
hskupin: review-
Details | Diff | Splinter Review
fix patch v2 (10.94 KB, patch)
2012-08-24 00:38 PDT, Maniac Vlad Florin (:vladmaniac)
dave.hunt: review-
Details | Diff | Splinter Review
patch v3 (12.42 KB, patch)
2012-08-29 05:05 PDT, Andreea Matei [:AndreeaMatei]
no flags Details | Diff | Splinter Review
patch v4 (11.01 KB, patch)
2012-08-29 07:44 PDT, Andreea Matei [:AndreeaMatei]
hskupin: review-
Details | Diff | Splinter Review
patch v5 (1.55 KB, patch)
2012-08-30 04:52 PDT, Andreea Matei [:AndreeaMatei]
hskupin: review-
Details | Diff | Splinter Review
patch v5.1 (2.09 KB, patch)
2012-08-30 06:06 PDT, Andreea Matei [:AndreeaMatei]
hskupin: review+
Details | Diff | Splinter Review
[ESR10] patch v1 (3.24 KB, patch)
2012-09-05 05:06 PDT, Andreea Matei [:AndreeaMatei]
dave.hunt: review+
Details | Diff | Splinter Review

Description Henrik Skupin (:whimboo) 2012-08-22 03:36:13 PDT
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) 2012-08-23 02:18:19 PDT
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) 2012-08-23 02:25:20 PDT
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) 2012-08-23 02:26:44 PDT
Also this
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) 2012-08-23 03:50:58 PDT
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) 2012-08-23 03:52:10 PDT
testCheckItemHighlight fails in a similar manner:

So probably worth fixing on a module level.
Comment 6 Maniac Vlad Florin (:vladmaniac) 2012-08-23 05:20:20 PDT
Created attachment 654579 [details] [diff] [review]
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) 2012-08-23 05:21:12 PDT
If this does not fix our errors at least we are getting closer by having another error
Comment 8 Henrik Skupin (:whimboo) 2012-08-23 22:47:56 PDT
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) 2012-08-24 00:38:44 PDT
Created attachment 654951 [details] [diff] [review]
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) 2012-08-25 08:58:09 PDT
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) 2012-08-28 01:56:21 PDT
Andreea, would you mind to continue here? Would be appreciated.
Comment 12 Andreea Matei [:AndreeaMatei] 2012-08-29 05:05:53 PDT
Created attachment 656400 [details] [diff] [review]
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:
Comment 13 Henrik Skupin (:whimboo) 2012-08-29 05:53:43 PDT
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] 2012-08-29 07:44:01 PDT
Created attachment 656445 [details] [diff] [review]
patch v4

Sorry, I misunderstood the comments.

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

Comment 15 Henrik Skupin (:whimboo) 2012-08-30 02:54:54 PDT
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;
>    }, "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] 2012-08-30 04:52:55 PDT
Created attachment 656818 [details] [diff] [review]
patch v5

Left only the waitFor in testSuggestBookmarks.js for this patch.
Comment 17 Henrik Skupin (:whimboo) 2012-08-30 05:04:54 PDT
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] 2012-08-30 06:06:13 PDT
Created attachment 656834 [details] [diff] [review]
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) 2012-09-03 14:06:48 PDT
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) 2012-09-03 14:09:33 PDT (default)
Comment 21 Henrik Skupin (:whimboo) 2012-09-05 00:21:45 PDT (aurora) (beta) (release)

Andreea, can you please provide a backport for the esr10 branch?
Comment 22 Andreea Matei [:AndreeaMatei] 2012-09-05 05:06:36 PDT
Created attachment 658451 [details] [diff] [review]
[ESR10] patch v1

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

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) 2012-09-05 09:47:22 PDT
Comment on attachment 658451 [details] [diff] [review]
[ESR10] patch v1

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

Landed as: (esr10)

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

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