601069 - Some mochitest-browser-chrome tests in suite/ time out, after bug 543800

Status: VERIFIED FIXED

(SeaMonkey :: Testing Infrastructure, defect)

Description

[Bruno 'Aqualon' Escherl]

The first one is "TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/suite/browser/test/browser_page_style_menu.js | Test timed out" but others follow. See it so far on Linux and OSX, Windows hasn't run it so far.

Example log (OSX): http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1285903268.1285905799.29670.gz

Regression range: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=8dcc5e960d5c&tochange=669eb2d837d7

Could be the results from the changes for mochitest, perhaps we have to port some of the changes that happened since our inital port (which was done 04/2009):

http://hg.mozilla.org/mozilla-central/log/6eb42326798b/browser/base/content/test/browser_page_style_menu.js

The main bug to look at here could be bug 592859.

Comment 1

[Serge Gautherie (:sgautherie)]

(In reply to comment #0)
> http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1285903268.1285905799.29670.gz

OS X 10.5 comm-central-trunk debug test mochitest-other on 2010/09/30 20:21:08
{
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/suite/browser/test/browser_page_style_menu.js | Test timed out
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/suite/browser/test/browser_page_style_menu.js | Found a tab after previous test timed out: jar:file:///var/folders/nt/ntruOvnCGaqeHuQCa+uQrU+++Tw/-Tmp-/tmp3dmKdq/extensions/[e-mail]/chrome/mochikit.jar!/content/browser/suite/browser/test/page_style_sample.html
}

The second error makes obvious what the cause is ;->

Comment 2

[Bruno 'Aqualon' Escherl]

Attachment: remove usage of chrome://mochikit from browser-chrome tests

This won't be fixed by the Makefile.in changes in bug 601113. We need to do the same as bug 592859.

Comment 3

[Serge Gautherie (:sgautherie)]

Comment on attachment 480109 [details] [diff] [review]
remove usage of chrome://mochikit from browser-chrome tests

(In reply to comment #2)

> This won't be fixed by the Makefile.in changes in bug 601113.

Correct, but that change was just the start of that bug...
At least, you should have commented there: no need we do double work :-/

> We need to do the same as bug 592859.

Damn, some m-c tests were fixed in that bug too :-<
Yet, one bug should be enough on our side.


>-  content.location =
>-    "chrome://mochikit/content/browser/suite/browser/test/page_style_sample.html";
>+  let rootDir = getRootDirectory(gTestPath);
>+  content.location = rootDir + "page_style_sample.html";

Do you know why they used that getRootDirectory() instead of "usual" http://mochi.test:8888 ?

Comment 4

[Bruno 'Aqualon' Escherl]

Sorry serge, but I thought your bug should be about our make package-tests issue, which has nothing to do with the mochitest-browser-chrome test failures. And I saw your dupe first when I uploaded the patch here.

> Do you know why they used that getRootDirectory() instead of "usual"
> http://mochi.test:8888 ?
Probably to be prepared for further changes of the url. You don't need to change the tests then.

Comment 5

[Bruno 'Aqualon' Escherl]

(In reply to comment #4)
> > Do you know why they used that getRootDirectory() instead of "usual"
> > http://mochi.test:8888 ?
> Probably to be prepared for further changes of the url. You don't need to
> change the tests then.
http://elvis314.wordpress.com/2010/10/01/mochikit-jar-changes-are-in-mozilla-central/ also contains some information about getRootDirectory().

Comment 6

[Serge Gautherie (:sgautherie)]

(In reply to comment #4)

So I got double-confused:

1) They started to update tests in bug 592859 then completed that in bug 543800.
As I saw test updates in the latter, I didn't looked for the former :-/
And, as often, they didn't let us know about that work beforehand :-(

2) I noticed that the unjarred files are still present in the package.
But I missed to check that http://mochi.test:8888 was working only because it was actually accessing them and not the jarred files :-/

***

A mochitest-chrome test to fix too:
{
7729 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/suite/common/tests/chrome/test_idcheck.xul | [SimpleTest/SimpleTest.js, window.onerror] An error occurred - loadLocalMailAccount is not defined at chrome://mochitests/content/chrome/suite/common/tests/chrome/test_idcheck.xul:214
}
I assume it fails to load mailTestUtils.js .

Comment 7

[Bruno 'Aqualon' Escherl]

Attachment: remove usage of chrome://mochikit from browser-chrome tests v2

This also fixes test_idcheck.xul, thanks for spotting this serge!

Comment 8

[Serge Gautherie (:sgautherie)]

Comment on attachment 480250 [details] [diff] [review]
remove usage of chrome://mochikit from browser-chrome tests v2

I didn't test this patch, but the list of tests is correct and the fixes look fine...


>diff --git a/suite/browser/test/browser/browser_pluginnotification.js b/suite/browser/test/browser/browser_pluginnotification.js
>@@ -1,9 +1,10 @@
>-const gTestRoot = "chrome://mochikit/content/browser/suite/browser/test/";
>+//const gTestRoot = "chrome://mochikit/content/browser/suite/browser/test/";
>+const gTestRoot = getRootDirectory(gTestPath);

Nit: drop the commented out line.

>diff --git a/suite/common/tests/chrome/test_idcheck.xul b/suite/common/tests/chrome/test_idcheck.xul
>@@ -43,24 +43,31 @@
>+
>+    let scriptLoader = Cc["@mozilla.org/moz/jssubscript-loader;1"].
>+                getService(Ci.mozIJSSubScriptLoader);
>+
>+    let rootDir = getRootDirectory(window.location.href);
>+    scriptLoader.loadSubScript(rootDir + "/mailTestUtils.js", this);
>+

Remove the "/", as is bug 543800.

Comment 9

[Justin Wood (:Callek)]

note to self for review assist: http://elvis314.wordpress.com/2010/10/01/mochikit-jar-changes-are-in-mozilla-central/

Comment 10

[Justin Wood (:Callek)]

Comment on attachment 480250 [details] [diff] [review]
remove usage of chrome://mochikit from browser-chrome tests v2

>--- a/suite/browser/test/browser/browser_pluginnotification.js
>+//const gTestRoot = "chrome://mochikit/content/browser/suite/browser/test/";

Nit, don't use this comment.


Everything else looks good to me, I am asking serge to give this an "official" review [though he's not a peer]. Serge what I am looking for from you is:

* Anything obvious in this diff that I missed.
* Any other test files you have noticed that might be missing here.

Thank You!

Comment 11

[Bruno 'Aqualon' Escherl]

I already included the review/feedback comments into my local queue, will upload a new patch after Serge had another look at it.

A grep on all test files in suite/ hasn't found other occurrences of chrome://mochikit except those needed by the test harness itself.

Comment 12

[Justin Wood (:Callek)]

(In reply to comment #10)
> Serge what I am looking for from you is:

Are you able to review this by tomorrow evening?, if not I will probably just land it, since I did give a review, just felt it was worth a second set of eyes.

Comment 13

[Bruno 'Aqualon' Escherl]

Attachment: remove usage of chrome://mochikit from browser-chrome tests v3 [Checked in: See comment 15]

This is the updated version of the patch, I won't have time to update the patch until Sunday evening if Serge finds any other issues with the patch. Feel free to do the update yourself to get it checked-in.

Comment 14

[Serge Gautherie (:sgautherie)]

Comment on attachment 481801 [details] [diff] [review]
remove usage of chrome://mochikit from browser-chrome tests v3
[Checked in: See comment 15]

(In reply to comment #10)
> * Anything obvious in this diff that I missed.
> * Any other test files you have noticed that might be missing here.

My comment 8 feedback already covers (most of) these ;-)


(In reply to comment #13)
> Feel free to do the update yourself to get it checked-in.

I found two (non-code) nits which I'll fix, then I'll push the patch.

NB: This time, I took time to run the tests (as Callek asked for my "review") and noticed issues with 2 of them, but I believe they are unrelated and will file separate bugs.

Comment 15

[Serge Gautherie (:sgautherie)]

Comment on attachment 481801 [details] [diff] [review]
remove usage of chrome://mochikit from browser-chrome tests v3
[Checked in: See comment 15]

http://hg.mozilla.org/comm-central/rev/2994d32dff9c

Comment 16

[Serge Gautherie (:sgautherie)]

(In reply to comment #14)
> NB: ... I ... noticed issues with 2 of them, but I believe they are unrelated
> and will file separate bugs.

I filed bug 603028 and bug 603031.
Bug 602633 fixed DOMi hang in test_idcheck.xul, while I was reviewing ;-)

Comment 17

[Justin Wood (:Callek)]

(In reply to comment #14)
> (In reply to comment #13)
> > Feel free to do the update yourself to get it checked-in.
> 
> I found two (non-code) nits which I'll fix, then I'll push the patch.
> 

I'm curious what these are (without having to compare checked in cset to the diff here)

Thanks though!

Comment 18

[Robert Kaiser]

(In reply to comment #16)
> Bug 602633 fixed DOMi hang in test_idcheck.xul, while I was reviewing ;-)

Yes, been a bit slower than usual with updating this while I was at Mozilla...

Comment 19

[Serge Gautherie (:sgautherie)]

(In reply to comment #17)
> I'm curious what these are (without having to compare checked in cset to the
> diff here)

2-3 whitespaces, and const before var.

*****

V.Fixed per
http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1286608666.1286610806.16791.gz
Linux comm-central-trunk debug test mochitest-other on 2010/10/09 00:17:46