The default bug view has changed. See this FAQ.

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

RESOLVED FIXED

Status

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

People

(Reporter: ashughes, Assigned: RemusPop)

Tracking

unspecified
All
Mac OS X

Firefox Tracking Flags

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

Details

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

Attachments

(1 attachment, 4 obsolete attachments)

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'
(Assignee)

Comment 2

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

Comment 4

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

Comment 9

5 years ago
Haven't got the chance to load my system, but it will be done tomorrow.
(Assignee)

Comment 10

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

Comment 12

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

Comment 14

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

Comment 16

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

Comment 18

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

Comment 19

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

Comment 21

5 years ago
Created attachment 627277 [details] [diff] [review]
patch v2 (default, aurora)

Now we type directly by using locationBar.type
Attachment #626800 - Attachment is obsolete: true
Attachment #627277 - Flags: review?(hskupin)
(Assignee)

Comment 22

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

Comment 23

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

Comment 24

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

Comment 27

5 years ago
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.
Attachment #627701 - Attachment is obsolete: true
Attachment #627944 - Flags: review?(hskupin)
status-firefox14: --- → affected
status-firefox15: --- → affected
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)
(Assignee)

Updated

5 years ago
Depends on: 760013
(Assignee)

Comment 29

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

Updated

5 years ago
Attachment #627944 - Attachment is obsolete: true
(Assignee)

Comment 30

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

Comment 32

5 years ago
The Linux report for functional testrun:
http://mozmill-crowd.blargon7.com/#/functional/report/fdec829b93b19c73985be1d3884249f6
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
Last Resolved: 5 years ago
status-firefox15: affected → fixed
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
status-firefox14: affected → fixed
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.
status-firefox-esr10: --- → affected
status-firefox13: --- → affected
(Assignee)

Comment 36

5 years ago
Henrik, it applies cleanly on both specified branches.
Thanks Remus.

Pushed:
http://hg.mozilla.org/qa/mozmill-tests/rev/22a13279911b (release)
http://hg.mozilla.org/qa/mozmill-tests/rev/38ee448e48b6 (esr10)
status-firefox-esr10: affected → fixed
status-firefox13: affected → fixed
status-firefox16: --- → unaffected

Updated

5 years ago
Whiteboard: [mozmill-test-failure] → [mozmill-test-failure][qa-]
You need to log in before you can comment on or make changes to this bug.