Last Comment Bug 742550 - testSuggestHistoryBookmarks.js fails with 'The page title matches the underlined text - 'grants' should equal 'grant''
: testSuggestHistoryBookmarks.js fails with 'The page title matches the underli...
Status: RESOLVED FIXED
[mozmill-test-failure][qa-]
:
Product: Mozilla QA
Classification: Other
Component: Mozmill Tests (show other bugs)
: unspecified
: All Mac OS X
: -- normal (vote)
: ---
Assigned To: Remus Pop (:RemusPop)
:
Mentors:
http://mozmill-ci.blargon7.com/#/func...
Depends on: 760013
Blocks:
  Show dependency treegraph
 
Reported: 2012-04-04 14:56 PDT by Anthony Hughes (:ashughes) [GFX][QA][Mentor]
Modified: 2012-08-14 08:16 PDT (History)
4 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---
fixed
fixed
fixed
unaffected
fixed


Attachments
patch v1 (default, aurora) (2.01 KB, patch)
2012-05-24 07:32 PDT, Remus Pop (:RemusPop)
hskupin: review-
Details | Diff | Review
patch v2 (default, aurora) (2.06 KB, patch)
2012-05-25 10:20 PDT, Remus Pop (:RemusPop)
no flags Details | Diff | Review
patch v3 (default, aurora) (3.90 KB, patch)
2012-05-28 08:08 PDT, Remus Pop (:RemusPop)
no flags Details | Diff | Review
patch v4 (default, aurora) (12.54 KB, patch)
2012-05-29 06:53 PDT, Remus Pop (:RemusPop)
no flags Details | Diff | Review
patch v5 (default) (2.54 KB, patch)
2012-06-01 01:15 PDT, Remus Pop (:RemusPop)
hskupin: review+
Details | Diff | Review

Description Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-04-04 14:56:21 PDT
Seems to only happen on Firefox 12 and Mac OSX right now. I suspect this will be fixed by the assertion refactor (bug 586286) but I'm filing anyway so we can track it.
Comment 1 Henrik Skupin (:whimboo) 2012-04-04 16:04:42 PDT
Anthony please always add the failure message to the summary so we can query for. It will not be covered by bug 586286 because that one will not fix broken tests. So this should be tracked in parallel.
Comment 2 Remus Pop (:RemusPop) 2012-05-15 00:39:09 PDT
After the assertJS refactoring we have a new error that is linked to the old message so I updated the summary.
This problem also appears in Nightly and Aurora:
http://mozmill-ci.blargon7.com/#/functional/failure?branch=All&platform=All&from=2012-05-12&to=2012-05-15&test=%2FtestAwesomeBar%2FtestSuggestHistoryBookmarks.js&func=testSuggestHistoryBookmarks.js%3A%3AtestSuggestHistoryAndBookmarks
Still applies only on OSX.
Comment 3 Henrik Skupin (:whimboo) 2012-05-15 01:06:53 PDT
Remus, please try to investigate that so we know what that happens. Looks like we have an additional 's' here.
Comment 4 Remus Pop (:RemusPop) 2012-05-15 07:02:01 PDT
Unfotunately I cannot reproduce locally.
Comment 5 Henrik Skupin (:whimboo) 2012-05-16 17:02:17 PDT
I think that happens if the system is under heavy load. Try to build e.g. Firefox or transcode a video while letting those tests run.
Comment 6 Henrik Skupin (:whimboo) 2012-05-16 17:05:33 PDT
Also this expect.equal call is wrong which has a flipped order of expected and value:

    expect.equal(LOCAL_TEST_PAGE.string, entry.toLowerCase(),
                 "The page title matches the underlined text");
Comment 7 Maniac Vlad Florin (:vladmaniac) 2012-05-21 06:11:26 PDT
(In reply to Henrik Skupin (:whimboo) from comment #6)
> Also this expect.equal call is wrong which has a flipped order of expected
> and value:
> 
>     expect.equal(LOCAL_TEST_PAGE.string, entry.toLowerCase(),
>                  "The page title matches the underlined text");

This should be updated either way
Comment 8 Henrik Skupin (:whimboo) 2012-05-21 06:17:05 PDT
Remus, any update from your side?
Comment 9 Remus Pop (:RemusPop) 2012-05-21 06:40:55 PDT
Haven't got the chance to load my system, but it will be done tomorrow.
Comment 10 Remus Pop (:RemusPop) 2012-05-22 07:17:16 PDT
I tried with transcoding a video with VLC. System was fully loaded, but gave Nightly enough processing power to run the test and no fail was registered.
Comment 11 Henrik Skupin (:whimboo) 2012-05-22 08:21:51 PDT
So what's the next test or a better question. Where does the additional 's' comes from? Do we have any keypress sequence which makes use of 's'?
Comment 12 Remus Pop (:RemusPop) 2012-05-23 01:13:18 PDT
This might have something to do with system load.
I dropped the sleep call when typing 'grants' to 50 ms and the error reproduces 100%.
http://hg.mozilla.org/qa/mozmill-tests/file/800ac3301dca/tests/functional/testAwesomeBar/testSuggestHistoryBookmarks.js#l51
I think we must find a way to make sure we typed what we wanted. Maybe refactor locationBar.type and get rid of the sleep call?
Comment 13 Henrik Skupin (:whimboo) 2012-05-23 01:25:01 PDT
That would be nice but there are at least two open questions:

* What do we have to wait for? Shall we wait until the last letter is visible in the location bar?

* What do we expect? In some cases we have to wait until the popup opens, otherwise for an update inside the popup.
Comment 14 Remus Pop (:RemusPop) 2012-05-23 04:54:23 PDT
I think we will have to wait for the last letter (string.length-1) and the popup open.
Comment 15 Henrik Skupin (:whimboo) 2012-05-23 11:22:46 PDT
In eventutils we have an expect version of keypress. It's also supported by controller.keypress I think. So we probably should make use of it now.
Comment 16 Remus Pop (:RemusPop) 2012-05-24 01:59:16 PDT
I think locationBar.type is broken. It calls the contains(text) method but does not use it's result. We should have a waitFor - contains.
Comment 17 Henrik Skupin (:whimboo) 2012-05-24 02:03:29 PDT
No, please change the call to keypress so we handle the expected event which should be a keypress. That will make us totally safe.

https://github.com/mozautomation/mozmill/blob/hotfix-1.5/mozmill/mozmill/extension/resource/modules/controller.js#L385
Comment 18 Remus Pop (:RemusPop) 2012-05-24 07:05:06 PDT
Forget comment #16 for now.
Found the problem to be when doing:
var richlistItem = locationBar.autoCompleteResults.getResults(0);
Why this? if the controller.sleep call is not high enough it would get incomplete results. So waiting for the autocomplete to open and then doing the getResults passes without having any sleep call.
Comment 19 Remus Pop (:RemusPop) 2012-05-24 07:32:06 PDT
Created attachment 626800 [details] [diff] [review]
patch v1 (default, aurora)

Removed the sleep call and defined richlistItem after we wait for the autocomplete to open.
Comment 20 Henrik Skupin (:whimboo) 2012-05-24 16:47:07 PDT
Comment on attachment 626800 [details] [diff] [review]
patch v1 (default, aurora)

>-  for each (var letter in LOCAL_TEST_PAGE.string) {
>+  for each (var letter in LOCAL_TEST_PAGE.string)
>     locationBar.type(letter);
>-    controller.sleep(200);
>-  }

The reason for the sleep call was that the auto-complete popup has been closed itself while typing the whole string into the location bar too quickly. Only with that delay we were able to ensure that the popup stays open. Has this been changed? Can you show us results across platforms? If that's the case remove the for loop and call locationBar.type with the complete string.

>-  // Define the path to the first auto-complete result
>-  var richlistItem = locationBar.autoCompleteResults.getResult(0);
> 
>   // Get the visible results from the autocomplete list. Verify it is 1
>   controller.waitFor(function () {
>     return locationBar.autoCompleteResults.isOpened;
>   }, "Autocomplete list has been opened");
>-
>+  
>+  // Define the path to the first auto-complete result
>+  var richlistItem = locationBar.autoCompleteResults.getResult(0);
>+  

That's strange. Not sure why that was in this order. But it makes sense to change it.

Does that also fix the other failing test? What about all the other awesome bar tests?
Comment 21 Remus Pop (:RemusPop) 2012-05-25 10:20:36 PDT
Created attachment 627277 [details] [diff] [review]
patch v2 (default, aurora)

Now we type directly by using locationBar.type
Comment 22 Remus Pop (:RemusPop) 2012-05-25 10:27:54 PDT
Henrik, which other test?
As for the other AwesomeBar tests, we'll need a new bug to investigate what is happening. This auotocomplete popup is so sensitive. I'd like to have some strong code on it so we don't fail anymore because of our code.
Comment 23 Remus Pop (:RemusPop) 2012-05-25 11:27:05 PDT
Comment on attachment 627277 [details] [diff] [review]
patch v2 (default, aurora)

Going for feedback as the second test wasn't fixed.
I applied the same fix for the second test but I get a different error:
"message": "The auto-complete result is a bookmark - 'action favicon' should contain 'bookmark'"
We might go with defining the richlistItem down in the test, but I cannot reproduce the failure locally, so I'm not sure if what I'm doing is right.
Comment 24 Remus Pop (:RemusPop) 2012-05-28 08:08:31 PDT
Created attachment 627701 [details] [diff] [review]
patch v3 (default, aurora)

Test 2 now uses the same method to type in the locationBar as first test. The problem adding this is that "Switch to tab" appeared instead of the waited autocomplete result. Comment on that appears in the patch.
richlistItem is now defined after the autocomplete popup is opened, likein the first test.
In theory this should work. I cannot test enough because I cannot reproduce the error in the second test which is the same to the one in first test.
Comment 25 Henrik Skupin (:whimboo) 2012-05-29 03:52:27 PDT
(In reply to Remus Pop (:RemusPop) from comment #22)
> Henrik, which other test?
> As for the other AwesomeBar tests, we'll need a new bug to investigate what
> is happening. This auotocomplete popup is so sensitive. I'd like to have
> some strong code on it so we don't fail anymore because of our code.

This test module (testSuggestHistoryBookmarks.js) contains two test functions:

testSuggestHistoryAndBookmarks
testStarInAutocomplete

Both should be in separate tests and I highly would encourage you to move the second test to its own module now.
Comment 26 Henrik Skupin (:whimboo) 2012-05-29 06:44:21 PDT
Those two failures you can also see here:
http://mozmill-ci.blargon7.com/#/functional/report/fdec829b93b19c73985be1d388189772

If you want to work on the splitting in another bug feel free to do so. It should block this bug.
Comment 27 Remus Pop (:RemusPop) 2012-05-29 06:53:01 PDT
Created attachment 627944 [details] [diff] [review]
patch v4 (default, aurora)

Splitted 2nd test to a new file and updated the change of preferences. For the first test the refactoring bug will change the code which changes prefs.
Comment 28 Henrik Skupin (:whimboo) 2012-05-30 13:17:05 PDT
Comment on attachment 627944 [details] [diff] [review]
patch v4 (default, aurora)

Remus, sorry for proposing to do everything in a single patch. Would you mind to do the split in a separate bug first? In case of the fix for this bug is failing I kinda would like to backout the fix only but not the split.
Comment 29 Remus Pop (:RemusPop) 2012-06-01 01:15:42 PDT
Created attachment 629102 [details] [diff] [review]
patch v5 (default)

Fixed testSuggestBookmarks and testSuggestHistory, formerly known as testSuggestHistoryBookmarks.
No fails in the testrun on Mac:
http://mozmill-crowd.blargon7.com/#/functional/report/fdec829b93b19c73985be1d38841b42a
Comment 30 Remus Pop (:RemusPop) 2012-06-01 01:26:47 PDT
Please ignore the above link to mozmill-crowd. This is the correct one:
http://mozmill-crowd.blargon7.com/#/functional/report/fdec829b93b19c73985be1d388419ffe
1 failure but not related to the patch.
Comment 31 Henrik Skupin (:whimboo) 2012-06-01 02:01:43 PDT
Comment on attachment 629102 [details] [diff] [review]
patch v5 (default)

Looks good to me, but I kinda would like to see at least results from a testrun on Linux. If you have the time Windows would be fine too.
Comment 32 Remus Pop (:RemusPop) 2012-06-01 02:48:26 PDT
The Linux report for functional testrun:
http://mozmill-crowd.blargon7.com/#/functional/report/fdec829b93b19c73985be1d3884249f6
Comment 33 Henrik Skupin (:whimboo) 2012-06-01 02:51:11 PDT
Landed as:
http://hg.mozilla.org/qa/mozmill-tests/rev/73b913d2b1f4

If todays results are green we can backport to mozilla-aurora.
Comment 34 Henrik Skupin (:whimboo) 2012-06-01 07:09:09 PDT
Only landed on aurora for now. If it turns out that older branches are also affected we can transplant.

http://hg.mozilla.org/qa/mozmill-tests/rev/e804a7f5d039
Comment 35 Henrik Skupin (:whimboo) 2012-06-26 05:31:38 PDT
This failed today on ESR10:
http://mozmill-ci.blargon7.com/#/functional/report/a7655636e327552d4750d1013c05ec0f

Remus, please check if the patch applies correctly for 12 and 10. I'm sure we should land this fix even on those branches.
Comment 36 Remus Pop (:RemusPop) 2012-06-26 05:54:20 PDT
Henrik, it applies cleanly on both specified branches.
Comment 37 Henrik Skupin (:whimboo) 2012-06-26 07:04:21 PDT
Thanks Remus.

Pushed:
http://hg.mozilla.org/qa/mozmill-tests/rev/22a13279911b (release)
http://hg.mozilla.org/qa/mozmill-tests/rev/38ee448e48b6 (esr10)

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