Closed Bug 742550 Opened 9 years ago Closed 9 years ago

testSuggestHistoryBookmarks.js fails with 'The page title matches the underlined text - 'grants' should equal 'grant''

Categories

(Mozilla QA Graveyard :: Mozmill Tests, defect)

All
macOS
defect
Not set
normal

Tracking

(firefox13 fixed, firefox14 fixed, firefox15 fixed, firefox16 unaffected, firefox-esr10 fixed)

RESOLVED FIXED
Tracking Status
firefox13 --- fixed
firefox14 --- fixed
firefox15 --- fixed
firefox16 --- unaffected
firefox-esr10 --- fixed

People

(Reporter: u279076, Assigned: remus.pop)

References

()

Details

(Whiteboard: [mozmill-test-failure][qa-])

Attachments

(1 file, 4 obsolete files)

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.
Depends on: 586286
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.
No longer depends on: 586286
Summary: Awesomebar :: testSuggestHistoryBookmarks.js fails in controller.assertJS → testSuggestHistoryBookmarks.js fails with 'subject.enteredTitle == subject.underlinedTitle'
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.
Summary: testSuggestHistoryBookmarks.js fails with 'subject.enteredTitle == subject.underlinedTitle' → testSuggestHistoryBookmarks.js fails with 'The page title matches the underlined text - 'grants' should equal 'grant''
Remus, please try to investigate that so we know what that happens. Looks like we have an additional 's' here.
Unfotunately I cannot reproduce locally.
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.
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");
(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
Remus, any update from your side?
Status: NEW → ASSIGNED
Haven't got the chance to load my system, but it will be done tomorrow.
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.
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'?
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?
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.
I think we will have to wait for the last letter (string.length-1) and the popup open.
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.
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.
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
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.
Attached patch patch v1 (default, aurora) (obsolete) — Splinter Review
Removed the sleep call and defined richlistItem after we wait for the autocomplete to open.
Attachment #626800 - Flags: review?(hskupin)
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?
Attachment #626800 - Flags: review?(hskupin) → review-
Attached patch patch v2 (default, aurora) (obsolete) — Splinter Review
Now we type directly by using locationBar.type
Attachment #626800 - Attachment is obsolete: true
Attachment #627277 - Flags: review?(hskupin)
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 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.
Attachment #627277 - Flags: review?(hskupin) → feedback?(hskupin)
Attached patch patch v3 (default, aurora) (obsolete) — Splinter Review
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.
Attachment #627277 - Attachment is obsolete: true
Attachment #627277 - Flags: feedback?(hskupin)
Attachment #627701 - Flags: feedback?(hskupin)
(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.
Attachment #627701 - Flags: feedback?(hskupin)
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.
Attached patch patch v4 (default, aurora) (obsolete) — Splinter Review
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.
Attachment #627701 - Attachment is obsolete: true
Attachment #627944 - Flags: review?(hskupin)
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.
Attachment #627944 - Flags: review?(hskupin)
Depends on: 760013
Fixed testSuggestBookmarks and testSuggestHistory, formerly known as testSuggestHistoryBookmarks.
No fails in the testrun on Mac:
http://mozmill-crowd.blargon7.com/#/functional/report/fdec829b93b19c73985be1d38841b42a
Attachment #629102 - Flags: review?(hskupin)
Attachment #627944 - Attachment is obsolete: true
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 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.
Attachment #629102 - Flags: review?(hskupin) → review+
Landed as:
http://hg.mozilla.org/qa/mozmill-tests/rev/73b913d2b1f4

If todays results are green we can backport to mozilla-aurora.
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
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
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.
Henrik, it applies cleanly on both specified branches.
Whiteboard: [mozmill-test-failure] → [mozmill-test-failure][qa-]
Product: Mozilla QA → Mozilla QA Graveyard
You need to log in before you can comment on or make changes to this bug.