767822 - Failure in testGreyLarry.js | Timeout waiting for page loaded

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

[u279076]

Failure discovered during Firefox 14.0b9 functional tests.

Module: /testSecurity/testGreyLarry.js 	
Test: testGreyLarry.js::testLarryGrey 	
Error: controller.waitForPageLoad(): Timeout waiting for page loaded
Report: http://mozmill-ci.blargon7.com/#/functional/report/44fc451c2e6b66b62172f2e13e16a438

Comment 1

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

This can happen on each branch. Remus, can you please make this test local?

Comment 2

[Remus Pop (:RemusPop)]

Attachment: patch v1 (default, aurora, beta)

Now we use a local test page.

Comment 3

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

Comment on attachment 636632 [details] [diff] [review]
patch v1 (default, aurora, beta)

> /**
>  * Litmus Test 8806: Grey Larry
>  */

Can you please remove that part? We should really get rid of it at the beginning of the file.

>+  var domain = LOCAL_TEST_FOLDER.split("/")[2];
>+  controller.assertValue(webIDDomainLabel, domain);
[..]
>   controller.assertValue(webIDOwnerLabel, securityOwner);

Can you please change those asserts into expect.* calls? For the former I would propose expect.match() with a regular expression which check for the domain and port at the beginning of the URI.

Comment 4

[Remus Pop (:RemusPop)]

I don't see how .match will help us more then equal. We only must check if the accessed domain is the same as in the grey larry info page.

Comment 5

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

With match you wouldn't have to call 'split("/")[2];' and more traceable output would exist in the assertion message.

Comment 6

[Remus Pop (:RemusPop)]

Attachment: patch v2 (default) [checked-in]

Addressed all requests.

Comment 7

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

Comment on attachment 639627 [details] [diff] [review]
patch v2 (default) [checked-in]

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

Looks good. Landed as:
http://hg.mozilla.org/qa/mozmill-tests/rev/d8c84efcec22 (default)

Comment 8

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

Comment on attachment 639627 [details] [diff] [review]
patch v2 (default) [checked-in]

>-  controller.assertValue(webIDDomainLabel, "www.mozilla.org");
>+  expect.match(webIDDomainLabel.getNode().value, /\/\/(.*[^\/])/.exec(LOCAL_TEST_FOLDER)[1],
>+               "The domain label should equal the domain");

Just thought about it and we should better have used 'window.location.host' here. No strange regex would have to be used in this case.

Comment 9

[Remus Pop (:RemusPop)]

Strange enough, controller.window.location.host returns "browser".

Comment 10

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

You probably want 'controller.window.content.location.host' or try to get it via the content document.

Comment 11

[Remus Pop (:RemusPop)]

Attachment: patch v3 (default, aurora, beta)

Henrik thank you for the tip. The problem is I can only send the value from controller.window.content.location.host if using a global variable. Otherwise that is not defined in the window that opens.

Comment 12

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

Comment on attachment 640598 [details] [diff] [review]
patch v3 (default, aurora, beta)

I'd like to see what Henrik thinks of this change. I'm not keen on storing the host as a variable, but other than leaving the regex I'm not sure what else to propose.

Comment 13

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

Comment on attachment 640598 [details] [diff] [review]
patch v3 (default, aurora, beta)

Hm, haven't thought about that this check is in a callback. So keeping the regex would make sense. Lets discard my last idea and get former patch backported to other affected branches. Remus, please check if it applies cleanly.

Comment 14

[Remus Pop (:RemusPop)]

Applies cleanly on aurora and beta. Release and esr10 have to be fixed in regards to the favicon.

Comment 15

[Remus Pop (:RemusPop)]

Attachment: patch v2 (release) [checked-in]

This needed the fix for favicon.

Comment 16

[Remus Pop (:RemusPop)]

Attachment: patch v2 (esr10) [checked-in]

Comment 17

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

Please add a comment with a link when pushing changes.

Landed as:
http://hg.mozilla.org/qa/mozmill-tests/rev/d8c84efcec22 (default)
http://hg.mozilla.org/qa/mozmill-tests/rev/54915147af74 (aurora)
http://hg.mozilla.org/qa/mozmill-tests/rev/e6f59f1872f4 (beta)

Comment 18

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

Comment on attachment 641428 [details] [diff] [review]
patch v2 (release) [checked-in]

Check your commit messages have the correct reviewer details.

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

Comment 19

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

Comment on attachment 641430 [details] [diff] [review]
patch v2 (esr10) [checked-in]

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