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

VERIFIED FIXED in seamonkey2.1b2

Status

VERIFIED FIXED
8 years ago
8 years ago

People

(Reporter: bugzilla, Assigned: bugzilla)

Tracking

(Blocks: 1 bug, {regression})

Trunk
seamonkey2.1b2
regression
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

8 years ago
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.
Summary: mochitest-browser-chrome tests in suite/ time out → mochitest-browser-chrome tests in suite/ intermittently times out
(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/mochikit@mozilla.org/chrome/mochikit.jar!/content/browser/suite/browser/test/page_style_sample.html
}

The second error makes obvious what the cause is ;->
No longer blocks: 452942
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → DUPLICATE
Whiteboard: [orange]
Duplicate of bug: 601113
Keywords: regression
Summary: mochitest-browser-chrome tests in suite/ intermittently times out → Some mochitest-browser-chrome tests in suite/ time out
(Assignee)

Comment 2

8 years ago
Created attachment 480109 [details] [diff] [review]
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.
Assignee: nobody → aqualon
Status: RESOLVED → REOPENED
Attachment #480109 - Flags: review?(bugspam.Callek)
Resolution: DUPLICATE → ---
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 ?
Hardware: x86 → All
(Assignee)

Comment 4

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

Comment 5

8 years ago
(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().
(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 .
Blocks: 543800
Status: REOPENED → ASSIGNED
Depends on: 592859
(Assignee)

Comment 7

8 years ago
Created attachment 480250 [details] [diff] [review]
remove usage of chrome://mochikit from browser-chrome tests v2

This also fixes test_idcheck.xul, thanks for spotting this serge!
Attachment #480109 - Attachment is obsolete: true
Attachment #480250 - Flags: review?(bugspam.Callek)
Attachment #480109 - Flags: review?(bugspam.Callek)
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.
Attachment #480250 - Flags: feedback+
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!
Attachment #480250 - Flags: review?(sgautherie.bz)
Attachment #480250 - Flags: review?(bugspam.Callek)
Attachment #480250 - Flags: review+
(Assignee)

Comment 11

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

Comment 13

8 years ago
Created attachment 481801 [details] [diff] [review]
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.
Attachment #480250 - Attachment is obsolete: true
Attachment #481801 - Flags: review?
Attachment #480250 - Flags: review?(sgautherie.bz)
(Assignee)

Updated

8 years ago
Attachment #481801 - Flags: review? → review?(sgautherie.bz)
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.
Attachment #481801 - Flags: review?(sgautherie.bz) → review+
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
Attachment #481801 - Attachment description: remove usage of chrome://mochikit from browser-chrome tests v3 → remove usage of chrome://mochikit from browser-chrome tests v3 [Checked in: See comment 15]
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago8 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.1b2
(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 ;-)
Summary: Some mochitest-browser-chrome tests in suite/ time out → Some mochitest-browser-chrome tests in suite/ time out, after bug 543800
(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

8 years ago
(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...
(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
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.