Closed Bug 592859 Opened 11 years ago Closed 11 years ago

update all browser-chrome tests to not use chrome://mochikit for test data

Categories

(Testing :: Mochitest, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla2.0b7

People

(Reporter: jmaher, Assigned: jmaher)

References

Details

Attachments

(1 file, 1 obsolete file)

here is a patch (that is on try and passed 1 check so far) that will remove the hardcoded chrome://mochikit/content/browser references from the test files.  This will also work if I used another chrome name like chrome://mochitests in place of the test files.
Attachment #471289 - Flags: review?(ctalbert)
Comment on attachment 471289 [details] [diff] [review]
update all browser-chrome tests to remove mochikit

>diff --git a/browser/base/content/test/browser_bug462673.js b/browser/base/content/test/browser_bug462673.js
>--- a/browser/base/content/test/browser_bug462673.js
>+++ b/browser/base/content/test/browser_bug462673.js
>@@ -42,12 +42,12 @@ function runOneTest() {
>         win.close();
>         if (runs.length)
>           runOneTest();
>         else
>           finish();
>       });
>     }, true);
> 
>-    browser.contentWindow.location =
>-      "chrome://mochikit/content/browser/browser/base/content/test/test_bug462673.html";
>+    var rootDir = getRootDirectory(path);
>+    browser.contentWindow.location = rootDir + "test_bug462673.html"
That's way too much magic.  What is 'path'?  I see below that you add it into the global scope, that is such a common variable name, I'm afraid you're going to inadvertantly run into scope collisions with this.  In fact there is one below...

>--- a/browser/base/content/test/browser_bug553455.js
>+++ b/browser/base/content/test/browser_bug553455.js
>@@ -1,17 +1,27 @@
> /* Any copyright is dedicated to the Public Domain.
>  * http://creativecommons.org/publicdomain/zero/1.0/
>  */
> 
> const TESTROOT = "http://example.com/browser/toolkit/mozapps/extensions/test/xpinstall/";
> const TESTROOT2 = "http://example.org/browser/toolkit/mozapps/extensions/test/xpinstall/";
>-const CHROMEROOT = "chrome://mochikit/content/browser/toolkit/mozapps/extensions/test/xpinstall/";
> const XPINSTALL_URL = "chrome://mozapps/content/xpinstall/xpinstallConfirm.xul";
> 
>+var rootDir = getRootDirectory(path);
>+var path = rootDir.split('/');
And there's the scope confusion.  Now the global 'path' is replaced with this 'path'.  None will ever be the wiser.


>diff --git a/testing/mochitest/browser-test.js b/testing/mochitest/browser-test.js
>--- a/testing/mochitest/browser-test.js
>+++ b/testing/mochitest/browser-test.js
>@@ -18,16 +18,17 @@ function testOnLoad() {
> 
>   prefs.setBoolPref("testing.browserTestHarness.running", true);
> 
>   var ww = Cc["@mozilla.org/embedcomp/window-watcher;1"].
>            getService(Ci.nsIWindowWatcher);
>   var sstring = Cc["@mozilla.org/supports-string;1"].
>                 createInstance(Ci.nsISupportsString);
>   sstring.data = location.search;
>+
>   ww.openWindow(window, "chrome://mochikit/content/browser-harness.xul", "browserTest",
>                 "chrome,centerscreen,dialog,resizable,titlebar,toolbar=no,width=800,height=600", sstring);
> }
> 
> function Tester(aTests, aDumper, aCallback) {
>   this.dumper = aDumper;
>   this.tests = aTests;
>   this.callback = aCallback;
>@@ -189,21 +190,28 @@ Tester.prototype = {
>     this.dumper.dump("TEST-START | " + this.currentTest.path + "\n");
> 
>     // Load the tests into a testscope
>     this.currentTest.scope = new testScope(this, this.currentTest);
> 
>     // Import utils in the test scope.
>     this.currentTest.scope.EventUtils = this.EventUtils;
>     this.currentTest.scope.SimpleTest = this.SimpleTest;
>+    this.currentTest.scope.path = this.currentTest.path;
This should really be named something more descriptive.  I'm not even sure if this is the definition of the "path" variable that you are referring to in all these tests.  It should be named something to indicate this variable comes from the harness.
>+
>     // Override SimpleTest methods with ours.
>     ["ok", "is", "isnot", "todo", "todo_is", "todo_isnot"].forEach(function(m) {
>       this.SimpleTest[m] = this[m];
>     }, this.currentTest.scope);
> 
>+    //load the tools to work with chrome .jar and remote
>+    try {
>+      this._scriptLoader.loadSubScript("chrome://mochikit/content/chrome-harness.js", this.currentTest.scope);
>+    } catch (ex) { /* no chrome-harness tools */ }
>+
>     // Import head.js script if it exists.
>     var currentTestDirPath =
>       this.currentTest.path.substr(0, this.currentTest.path.lastIndexOf("/"));
>     var headPath = currentTestDirPath + "/head.js";
>     try {
>       this._scriptLoader.loadSubScript(headPath, this.currentTest.scope);
>     } catch (ex) { /* no head */ }
>
Attachment #471289 - Flags: review?(ctalbert) → review-
updated the global 'path' variable to be gTestPath which is much more obvious what it is and not going to be assigned a new value in a test.
Assignee: nobody → jmaher
Attachment #471289 - Attachment is obsolete: true
Attachment #471861 - Flags: review?(ctalbert)
Comment on attachment 471861 [details] [diff] [review]
update all browser-chrome tests to remove mochikit

Discussed on IRC:

[2:08pm] jmaher: ctalbert: do you mean the chrome-harness.js stuff?
[2:08pm] ctalbert: browser-test.js file
[2:09pm] jmaher: ok, it is intended for a silent fail, just like the other loadSubScript stuff below it
[2:09pm] ctalbert: does the entire harness fail at that point?
[2:10pm] jmaher: I don't know, the other stuff was a silent fail and I decided to go that route
[2:10pm] ctalbert: the other makes more sense because that only fails if there is no head.js file, which is optional, as the head.js file is a recent addtiion to browser-chrome
[2:11pm] ctalbert: I think this error should actually throw and state something about an invalid configuration or some such
[2:11pm] jmaher: yeah, the more I think about it that makes the best sense
[2:11pm] ctalbert: ok, r+ wiht that change
[2:11pm] jmaher: woot!
[2:11pm] • ctalbert copies this into the bug
[2:11pm] jmaher: thanks
Attachment #471861 - Flags: review?(ctalbert) → review+
in the scenario where chrome-harness.js is missing, we will not even get to this part of the code.  We need chrome-harness.js for the core browser pieces (like packed.js and SimpleTest.js), so if it is missing the harness fails early on!
(In reply to comment #4)
> in the scenario where chrome-harness.js is missing, we will not even get to
> this part of the code.  We need chrome-harness.js for the core browser pieces
> (like packed.js and SimpleTest.js), so if it is missing the harness fails early
> on!
Ok, then we can leave it as is.
http://hg.mozilla.org/mozilla-central/rev/f9fcf102493f
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Flags: in-testsuite+
OS: Linux → All
Hardware: x86 → All
Target Milestone: --- → mozilla2.0b7
Version: unspecified → Trunk
Component: General → BrowserTest
QA Contact: general → browsertest
Component: BrowserTest → Mochitest
You need to log in before you can comment on or make changes to this bug.